<html><head></head><body>Hi,<br><br>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().<br><br><div class="gmail_quote">Le 4 février 2020 13:01:22 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 11:10, 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;">I don't change the ordering. Previously, the window was destroyed "at <br>the same time" as the thread exited, with no predictable order. Now it's <br>always thread then window.<br></blockquote><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><blockquote class="gmail_quote" style="margin: 0pt 0pt 1ex 0.8ex; border-left: 1px solid #729fcf; padding-left: 1ex;">If you want to destroy the window while the thread exists then do it in <br>the thread. But that's a separate problem from this patch.<br></blockquote><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><blockquote class="gmail_quote" style="margin: 0pt 0pt 1ex 0.8ex; border-left: 1px solid #729fcf; padding-left: 1ex;">Le 4 février 2020 10:19:22 GMT+02:00, Steve Lhomme <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 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 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 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>