[vlc-devel] [PATCH 4/7] win32/window: remove useless done boolean
Steve Lhomme
robux4 at ycbcr.xyz
Tue Feb 4 14:41:33 CET 2020
On 2020-02-04 14:33, Rémi Denis-Courmont wrote:
> Hi,
>
> As I understand, you claim that the DestroyWindow() call is bogus. I
> don't know, but it's already there in Close() now and this patch is not
> moving it. It's just changing code that happens close to that
> DestroyWindow().
I just explained how we had an undefined behaviour and now you're making
it worse calling DestroyWindow when the HWND is definitely destroyed.
Also you can't have it both ways:
- I don't change the ordering.
- Now it's always thread then window.
I'll propose a patch to remove the questionable DestroyWindow.
> Le 4 février 2020 13:01:22 GMT+02:00, Steve Lhomme <robux4 at ycbcr.xyz> a
> écrit :
>
> 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
>
> ------------------------------------------------------------------------
> 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
>
More information about the vlc-devel
mailing list