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

[Thomas Guillem]

On Thu, Jun 13, 2019, at 15:05, Steve Lhomme wrote:
> 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.

I don't see any other way to achieve locking of window and vd.
Rémi's solution looks fine to me, but yes, it could use an extra comment.

> 
> > 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
> > 
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel