[vlc-devel] [PATCH] win32/drawable: fix deadlock

[Steve Lhomme]

On 2021-02-19 14:41, Thomas Guillem wrote:
> The WinVoutEventProc() callback can either be called from the
> WindowLoopThread() when a new message need to be processed or from
> Windows "vout display" plugins when the plugin change the state of the
> hwnd.

This is not what I have seen. The vout display HWND also calls 
WinVoutEventProc() from the WindowLoopThread() loop. And it is done in a 
recursive way:

> There was a deadlock (or an assert in vlc_mutex_trylock()) when
> processing the WM_SIZE message. vout_window_ReportSize() was called from
> WinVoutEventProc() causing the "vout display" plugin to call
> SetWindowPos() (from CommonControl() in win32/common.c) and causing
> WinVoutEventProc() to be called again. The second call to
> vout_window_ReportSize() would deadlock (or assert) when trying to lock
> the display_lock from video_output.c.

WindowLoopThread gets a WM_SIZE and report the size. This causes the 
vout display HWND to be resized, in the same stack/call (good). Which 
then ends up generating a WM_ERASEBKGND on its parent (the drawable 
HWND). This does a WM_WINDOWPOSCHANGED and a WM_SIZE. But we're already 
in a WM_SIZE in WindowLoopThread.

The vout_window_ReportSize() is called twice in the same thread. This 
ends up locking the display_lock twice. (what you wrote with other words).

It might be possible to counter that by making the WM_SIZE handling in 
drawable asynchronous. We can PostMessage an internal message to tell we 
need to report the size. This should never cause reentrant WM_SIZE calls 
as PostMessage will not process the message right away.

> This problem occurs now (and not in VLC 3.0) since Windows mutexes are
> not recursives anymore, cf. 72acfccd5311e8f051843bbd947fd321c7916766.
> ---
>   modules/video_output/win32/drawable.c | 20 ++++++++++++++++++--
>   1 file changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/modules/video_output/win32/drawable.c b/modules/video_output/win32/drawable.c
> index 90106558848..167edcdae5a 100644
> --- a/modules/video_output/win32/drawable.c
> +++ b/modules/video_output/win32/drawable.c
> @@ -74,6 +74,12 @@ struct drawable_sys
>       vlc_sem_t hwnd_set;
>   
>       vout_window_t *wnd;
> +
> +    /* Prevent calling vout_window_Report*() callbacks from an event already
> +     * triggered by a previous vout_window_Report*() call from the same thread.
> +     * */
> +    bool reporting_window;
> +
>       HWND hWnd;
>       HWND embed_hwnd;
>       RECT rect_parent;
> @@ -121,7 +127,12 @@ static LRESULT CALLBACK WinVoutEventProc(HWND hwnd, UINT message,
>           break;
>   
>       case WM_CLOSE:
> -        vout_window_ReportClose(wnd);
> +        if (!sys->reporting_window)
> +        {
> +            sys->reporting_window = true;
> +            vout_window_ReportClose(wnd);
> +            sys->reporting_window = false;
> +        }
>           return 0;
>   
>       /* the window has been closed so shut down everything now */
> @@ -131,7 +142,12 @@ static LRESULT CALLBACK WinVoutEventProc(HWND hwnd, UINT message,
>           return 0;
>   
>       case WM_SIZE:
> -        vout_window_ReportSize(wnd, LOWORD(lParam), HIWORD(lParam));
> +        if (!sys->reporting_window)
> +        {
> +            sys->reporting_window = true;
> +            vout_window_ReportSize(wnd, LOWORD(lParam), HIWORD(lParam));
> +            sys->reporting_window = false;
> +        }
>           return 0;
>   
>       default:
> -- 
> 2.30.0
> 
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel
>