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

[Steve Lhomme]

On 2020-02-04 12:01, Steve Lhomme wrote:
> On 2020-02-04 11:10, Rémi Denis-Courmont wrote:
>> 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.
> 
> We send WM_CLOSE, which in turn is going to call DestroyWindow().
> https://docs.microsoft.com/en-us/windows/win32/winmsg/wm-close
> 
> It will send WM_DESTROY (and WM_NCDESTROY). Then a PostQuitMessage() is 
> sent which sends a WM_QUIT, which will make the event-loop break and 
> finish the event-loop thread.
> 
> When the vlc_join returns, I think the HWND is already gone. That's at 
> least what it was expecting. It should be the case even without the 
> current DestroyWindow() call we have (I tested, it works the same).
> 
>> 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.
> 
> You can't do that:
> "A thread cannot use DestroyWindow to destroy a window created by a 
> different thread."
> 
> The HWND is created in the event-loop so it should be "destroyed" in the 
> event loop as well. That's what is happening now with the WM_CLOSE call. 
> There should not be a DestroyWindow at all in the Close.

In other word, the patch should work even if you remove the 
DestroyWindow part. Doing it after the join would lead to even more 
undefined behaviour than what we have now.

There are places in Open() where we call Close(). Either we shouldn't do 
that or we should DestroyWindow in the rare case the event-loop failed 
to initialize properly but it left a hwnd to destroy. In practice, it 
can't happen. None of the Close() calls in Open can happen with 
sys->hwnd non NULL.

>> 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é.
>>
>> _______________________________________________
>> 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