[vlc-devel] [PATCH 4/7] win32/window: remove useless done boolean

[Rémi Denis-Courmont]

I don't change the ordering. Previously, the window was destroyed "at the same time" as the thread exited, with no predictable order. Now it's always thread then window.

If you want to destroy the window while the thread exists then do it in the thread. But that's a separate problem from this patch.

Le 4 février 2020 10:19:22 GMT+02:00, Steve Lhomme <robux4 at ycbcr.xyz> a écrit :
>On 2020-02-03 22:13, Rémi Denis-Courmont wrote:
>> It was written (and signaled) without proper locking, and it
>duplicated
>> the thread-join anyway since it was the very last observable action.
>> ---
>>   modules/video_output/win32/window.c | 17 +++--------------
>>   1 file changed, 3 insertions(+), 14 deletions(-)
>> 
>> diff --git a/modules/video_output/win32/window.c
>b/modules/video_output/win32/window.c
>> index 7a91c5dc05..3dc9cf32d2 100644
>> --- a/modules/video_output/win32/window.c
>> +++ b/modules/video_output/win32/window.c
>> @@ -52,7 +52,6 @@ typedef struct vout_window_sys_t
>>       vlc_mutex_t  lock;
>>       vlc_cond_t   wait;
>>       bool         b_ready;
>> -    bool         b_done;
>>   
>>       HWND hwnd;
>>   
>> @@ -484,22 +483,15 @@ static void Close(vout_window_t *wnd)
>>   
>>       free( sys->psz_title );
>>       if (sys->hwnd)
>> -    {
>>           PostMessage( sys->hwnd, WM_CLOSE, 0, 0 );
>> -        /* wait until the thread is done */
>> -        vlc_mutex_lock( &sys->lock );
>> -        while( !sys->b_done )
>> -        {
>> -            vlc_cond_wait( &sys->wait, &sys->lock );
>> -        }
>> -        vlc_mutex_unlock( &sys->lock );
>>   
>> -        DestroyWindow( sys->hwnd );
>> -    }
>>       vlc_join(sys->thread, NULL);
>>       vlc_mutex_destroy( &sys->lock );
>>       vlc_cond_destroy( &sys->wait );
>>   
>> +    if (sys->hwnd)
>> +        DestroyWindow( sys->hwnd );
>
>You destroy the window after the event loop thread has died. It may
>have 
>so odd side effects. Like keyboard focus issues:
>https://docs.microsoft.com/en-us/windows/win32/api/winuser/nf-winuser-destroywindow
>
>The WM_DESTROY we don't handle anymore doesn't seem to be an issue, 
>since we're just signaling the event loop thread is dying. (and we 
>should handle the WM_QUIT in the thread, which we currently don't)
>
>I haven't seen any issue with this patch applied, but I can't guarantee
>
>it is clean.
>
>> +
>>       HINSTANCE hInstance = GetModuleHandle(NULL);
>>       UnregisterClass( sys->class_main, hInstance );
>>       DestroyCursor( sys->cursor_empty );
>> @@ -691,8 +683,6 @@ static void *EventThread( void *p_this )
>>           TranslateMessage(&msg);
>>           DispatchMessage(&msg);
>>       }
>> -    sys->b_done = true;
>> -    vlc_cond_signal( &sys->wait );
>>       vlc_restorecancel(canc);
>>       return NULL;
>>   }
>> @@ -750,7 +740,6 @@ static int Open(vout_window_t *wnd)
>>       vlc_mutex_init( &sys->lock );
>>       vlc_cond_init( &sys->wait );
>>       sys->b_ready = false;
>> -    sys->b_done = false;
>>   
>>       wnd->sys = sys;
>>       if( vlc_clone( &sys->thread, EventThread, wnd,
>VLC_THREAD_PRIORITY_LOW ) )
>> -- 
>> 2.25.0
>> 
>> _______________________________________________
>> vlc-devel mailing list
>> To unsubscribe or modify your subscription options:
>> https://mailman.videolan.org/listinfo/vlc-devel
>> 
>_______________________________________________
>vlc-devel mailing list
>To unsubscribe or modify your subscription options:
>https://mailman.videolan.org/listinfo/vlc-devel

-- 
Envoyé de mon appareil Android avec Courriel K-9 Mail. Veuillez excuser ma brièveté.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20200204/62eedb9f/attachment.html>