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

[Steve Lhomme]

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.

> 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
>