<html><head></head><body>Hi,<br><br>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.<br><br><div class="gmail_quote">Le 4 février 2020 15:41:33 GMT+02:00, Steve Lhomme <robux4@ycbcr.xyz> a écrit :<blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
<pre class="k9mail">On 2020-02-04 14:33, Rémi Denis-Courmont wrote:<br><blockquote class="gmail_quote" style="margin: 0pt 0pt 1ex 0.8ex; border-left: 1px solid #729fcf; padding-left: 1ex;">Hi,<br><br>As I understand, you claim that the DestroyWindow() call is bogus. I <br>don't know, but it's already there in Close() now and this patch is not <br>moving it. It's just changing code that happens close to that <br>DestroyWindow().<br></blockquote><br>I just explained how we had an undefined behaviour and now you're making <br>it worse calling DestroyWindow when the HWND is definitely destroyed.<br><br>Also you can't have it both ways:<br>- I don't change the ordering.<br>- Now it's always thread then window.<br><br>I'll propose a patch to remove the questionable DestroyWindow.<br><br><blockquote class="gmail_quote" style="margin: 0pt 0pt 1ex 0.8ex; border-left: 1px solid #729fcf; padding-left: 1ex;">Le 4 février 2020 13:01:22 GMT+02:00, Steve Lhomme <robux4@ycbcr.xyz> a <br>écrit :<br><br>    On 2020-02-04 11:10, Rémi Denis-Courmont wrote:<br><br>        I don't change the ordering. Previously, the window was<br>        destroyed "at<br>        the same time" as the thread exited, with no predictable order.<br>        Now it's<br>        always thread then window.<br><br><br>    We send WM_CLOSE, which in turn is going to call DestroyWindow().<br>    <a href="https://docs.microsoft.com/en-us/windows/win32/winmsg/wm-close">https://docs.microsoft.com/en-us/windows/win32/winmsg/wm-close</a><br><br>    It will send WM_DESTROY (and WM_NCDESTROY). Then a PostQuitMessage() is<br>    sent which sends a WM_QUIT, which will make the event-loop break and<br>    finish the event-loop thread.<br><br>    When the vlc_join returns, I think the HWND is already gone. That's at<br>    least what it was expecting. It should be the case even without the<br>    current DestroyWindow() call we have (I tested, it works the same).<br><br>        If you want to destroy the window while the thread exists then<br>        do it in<br>        the thread. But that's a separate problem from this patch.<br><br><br>    You can't do that:<br>    "A thread cannot use DestroyWindow to destroy a window created by a<br>    different thread."<br><br>    The HWND is created in the event-loop so it should be "destroyed" in the<br>    event loop as well. That's what is happening now with the WM_CLOSE call.<br>    There should not be a DestroyWindow at all in the Close.<br><br>        Le 4 février 2020 10:19:22 GMT+02:00, Steve Lhomme<br>        <robux4@ycbcr.xyz> a<br>        écrit :<br><br>        On 2020-02-03 22:13, Rémi Denis-Courmont wrote:<br><br>        It was written (and signaled) without proper locking, and it<br>        duplicated<br>        the thread-join anyway since it was the very last observable action.<hr>        modules/video_output/win32/window.c | 17 +++--------------<br>        1 file changed, 3 insertions(+), 14 deletions(-)<br><br>        diff --git a/modules/video_output/win32/window.c<br>        b/modules/video_output/win32/window.c<br>        index 7a91c5dc05..3dc9cf32d2 100644<br>        --- a/modules/video_output/win32/window.c<br>        +++ b/modules/video_output/win32/window.c<br>        @@ -52,7 +52,6 @@ typedef struct vout_window_sys_t<br>        vlc_mutex_t lock;<br>        vlc_cond_t wait;<br>        bool b_ready;<br>        - bool b_done;<br><br>        HWND hwnd;<br><br>        @@ -484,22 +483,15 @@ static void Close(vout_window_t *wnd)<br><br>        free( sys->psz_title );<br>        if (sys->hwnd)<br>        - {<br>        PostMessage( sys->hwnd, WM_CLOSE, 0, 0 );<br>        - /* wait until the thread is done */<br>        - vlc_mutex_lock( &sys->lock );<br>        - while( !sys->b_done )<br>        - {<br>        - vlc_cond_wait( &sys->wait, &sys->lock );<br>        - }<br>        - vlc_mutex_unlock( &sys->lock );<br><br>        - DestroyWindow( sys->hwnd );<br>        - }<br>        vlc_join(sys->thread, NULL);<br>        vlc_mutex_destroy( &sys->lock );<br>        vlc_cond_destroy( &sys->wait );<br><br>        + if (sys->hwnd)<br>        + DestroyWindow( sys->hwnd );<br><br><br>        You destroy the window after the event loop thread has died. It<br>        may have<br>        so odd side effects. Like keyboard focus issues:<br>        <a href="https://docs.microsoft.com/en-us/windows/win32/api/winuser/nf-winuser-destroywindow">https://docs.microsoft.com/en-us/windows/win32/api/winuser/nf-winuser-destroywindow</a><br><br>        The WM_DESTROY we don't handle anymore doesn't seem to be an issue,<br>        since we're just signaling the event loop thread is dying. (and we<br>        should handle the WM_QUIT in the thread, which we currently don't)<br><br>        I haven't seen any issue with this patch applied, but I can't<br>        guarantee<br>        it is clean.<br><br>        +<br>        HINSTANCE hInstance = GetModuleHandle(NULL);<br>        UnregisterClass( sys->class_main, hInstance );<br>        DestroyCursor( sys->cursor_empty );<br>        @@ -691,8 +683,6 @@ static void *EventThread( void *p_this )<br>        TranslateMessage(&msg);<br>        DispatchMessage(&msg);<br>        }<br>        - sys->b_done = true;<br>        - vlc_cond_signal( &sys->wait );<br>        vlc_restorecancel(canc);<br>        return NULL;<br>        }<br>        @@ -750,7 +740,6 @@ static int Open(vout_window_t *wnd)<br>        vlc_mutex_init( &sys->lock );<br>        vlc_cond_init( &sys->wait );<br>        sys->b_ready = false;<br>        - sys->b_done = false;<br><br>        wnd->sys = sys;<br>        if( vlc_clone( &sys->thread, EventThread, wnd,<br>        VLC_THREAD_PRIORITY_LOW ) )<br>        -- <br>        2.25.0<hr>        vlc-devel mailing list<br>        To unsubscribe or modify your subscription options:<br>        <a href="https://mailman.videolan.org/listinfo/vlc-devel">https://mailman.videolan.org/listinfo/vlc-devel</a><hr>        vlc-devel mailing list<br>        To unsubscribe or modify your subscription options:<br>        <a href="https://mailman.videolan.org/listinfo/vlc-devel">https://mailman.videolan.org/listinfo/vlc-devel</a><br><br><br>        -- <br>        Envoyé de mon appareil Android avec Courriel K-9 Mail. Veuillez<br>        excuser<br>        ma brièveté.<hr>        vlc-devel mailing list<br>        To unsubscribe or modify your subscription options:<br>        <a href="https://mailman.videolan.org/listinfo/vlc-devel">https://mailman.videolan.org/listinfo/vlc-devel</a><hr>    vlc-devel mailing list<br>    To unsubscribe or modify your subscription options:<br>    <a href="https://mailman.videolan.org/listinfo/vlc-devel">https://mailman.videolan.org/listinfo/vlc-devel</a><br><br><br>-- <br>Envoyé de mon appareil Android avec Courriel K-9 Mail. Veuillez excuser <br>ma brièveté.<hr>vlc-devel mailing list<br>To unsubscribe or modify your subscription options:<br><a href="https://mailman.videolan.org/listinfo/vlc-devel">https://mailman.videolan.org/listinfo/vlc-devel</a><br><br></blockquote><hr>vlc-devel mailing list<br>To unsubscribe or modify your subscription options:<br><a href="https://mailman.videolan.org/listinfo/vlc-devel">https://mailman.videolan.org/listinfo/vlc-devel</a></pre></blockquote></div><br>-- <br>Envoyé de mon appareil Android avec Courriel K-9 Mail. Veuillez excuser ma brièveté.</body></html>