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