[vlc-devel] [PATCH 05/13] vout: set display filled synchronously

[Steve Lhomme]

On 2019-06-13 14:39, Rémi Denis-Courmont wrote:
> Hi,
> 
> T1 sets, say, A/R, which updates the window size. Then T2 does the same. 
> Then T2 sends the A/R control to vout thread, then T1 sends the same.
> 
> -> window uses T2 A/R, display uses T1 A/R.

OK, so about those intertwined locks. It doesn't seem to be a good 
practice when using locks. It may work if always done the same way (lock 
A while B is locked, then unlock B). But that's very risky IMO. It's 
very easy to make a mistake. For example what if you lock A, do a lot of 
things, including locking B inside a function you didn't think of ? At 
the very least this behavior should be documented as such.

> Le 13 juin 2019 19:15:20 GMT+08:00, Steve Lhomme <robux4 at ycbcr.xyz> a 
> écrit :
> 
>     On 2019-06-12 19:48, Rémi Denis-Courmont wrote:
> 
>         This fixes an ABBA race if two threads set the flag simultaneously.
> 
> 
>     What is an ABBA race ?
>     Did you mean this ?https://en.wikipedia.org/wiki/ABA_problem
> 
>         ------------------------------------------------------------------------
>         src/video_output/control.h | 1 -
>         src/video_output/video_output.c | 12 +++++-------
>         2 files changed, 5 insertions(+), 8 deletions(-)
> 
>         diff --git a/src/video_output/control.h b/src/video_output/control.h
>         index b189b6478a..a384afa2c6 100644
>         --- a/src/video_output/control.h
>         +++ b/src/video_output/control.h
>         @@ -32,7 +32,6 @@ enum {
> 
>         VOUT_CONTROL_MOUSE_STATE, /* vlc_mouse_t */
>         VOUT_CONTROL_DISPLAY_SIZE, /* window */
>         - VOUT_CONTROL_DISPLAY_FILLED, /* bool */
>         VOUT_CONTROL_ZOOM, /* pair */
> 
>         VOUT_CONTROL_ASPECT_RATIO, /* pair */
>         diff --git a/src/video_output/video_output.c
>         b/src/video_output/video_output.c
>         index ca9831e086..af2c0ecf8d 100644
>         --- a/src/video_output/video_output.c
>         +++ b/src/video_output/video_output.c
>         @@ -433,10 +433,13 @@ void
>         vout_ChangeDisplayFilled(vout_thread_t *vout, bool is_filled)
>         vlc_mutex_lock(&sys->window_lock);
>         sys->display_cfg.is_display_filled = is_filled;
>         /* no window size update here */
>         +
>         + vlc_mutex_lock(&sys->display_lock);
>         vlc_mutex_unlock(&sys->window_lock);
> 
> 
>     That looks like the kind of thing that may end up in a deadlock if not
>     done consistently.
> 
>         - vout_control_PushBool(&vout->p->control,
>         VOUT_CONTROL_DISPLAY_FILLED,
>         - is_filled);
>         + if (sys->display != NULL)
>         + vout_SetDisplayFilled(sys->display, is_filled);
>         + vlc_mutex_unlock(&sys->display_lock);
>         }
> 
>         void vout_ChangeZoom(vout_thread_t *vout, unsigned num, unsigned
>         den)
>         @@ -1573,11 +1576,6 @@ static void ThreadControl(vout_thread_t
>         *vout, vout_control_cmd_t cmd)
>         cmd.window.width, cmd.window.height);
>         vlc_mutex_unlock(&vout->p->display_lock);
>         break;
>         - case VOUT_CONTROL_DISPLAY_FILLED:
>         - vlc_mutex_lock(&vout->p->display_lock);
>         - vout_SetDisplayFilled(vout->p->display, cmd.boolean);
>         - vlc_mutex_unlock(&vout->p->display_lock);
>         - break;
>         case VOUT_CONTROL_ZOOM:
>         vlc_mutex_lock(&vout->p->display_lock);
>         vout_SetDisplayZoom(vout->p->display, cmd.pair.a, cmd.pair.b);
>         -- 
>         2.20.1
>         ------------------------------------------------------------------------
>         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
>