[vlc-devel] [PATCH 4/7] win32/window: remove useless done boolean
Rémi Denis-Courmont
remi at remlab.net
Tue Feb 4 14:52:42 CET 2020
Hi,
I don't see how it's any worse. The current code is already calling DestroyWindow() on the owner thread and potentially after/while the event thread exits. I'm all for fixing bugs in general, but I am against multi-purpose patches. And this patch is neither adding nor removing that bug. This is not about having it both ways; it's just not in scope of this patch.
Le 4 février 2020 15:41:33 GMT+02:00, Steve Lhomme <robux4 at ycbcr.xyz> a écrit :
>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
>>
>_______________________________________________
>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/6b439f74/attachment.html>
More information about the vlc-devel
mailing list