[vlc-devel] [PATCH 5/7] vout/control: keep the lock instead of setting a held flag

[Thomas Guillem]

On Fri, Aug 14, 2020, at 11:25, Steve Lhomme wrote:
> On 2020-08-14 11:02, Thomas Guillem wrote:
> > Why is it not needed anymore?
> 
> I don't know why it was needed before. Maybe I'm missing something.

I would wait for Rémi's review then.

> 
> > vout_control_Push() will now have to wait that controls are processed.
> 
> I don't see how it's different from now. It needs the lock to push a 
> command. The control pop also gets the lock. So in the while loop to pop 
> all the commands there's a chance for the push to get the lock first. 
> There's also a chance to get the lock while the pop is waiting for its 
> deadline or a wake up.
> 
> > Are you really sure we want that? For example, vout_MouseState() will wait until a flush command is processed.
> 
> Why would it need that ? vout_control_Push() gets the lock as described 
> above.

Yes, but before your patch, vout_control_Hold() was releasing the lock, letting all vout_control_Push() to be executed while the control was processed.

> 
> > 
> > On Thu, Aug 13, 2020, at 14:12, Steve Lhomme wrote:
> >> The code calling vout_control_Hold holds the lock until vout_control_Release is
> >> called. vout_control_Pop() doesn't need to wait for it. If it has the lock no
> >> other code is "holding" it.
> >>
> >> Now vout_control_Release() doesn't wake up the vout_control_Pop() directly.
> >> Either commands were added and wait_request was signaled. Or nothing was done
> >> and it doesn't need to wake up vout_control_Pop(). It will be unblocked by new
> >> commands, a vout_control_Wake call or a deadline timeout.
> >> ---
> >>   src/video_output/control.c | 17 -----------------
> >>   src/video_output/control.h |  3 ---
> >>   2 files changed, 20 deletions(-)
> >>
> >> diff --git a/src/video_output/control.c b/src/video_output/control.c
> >> index 675953f2c89..93bf7c3f92a 100644
> >> --- a/src/video_output/control.c
> >> +++ b/src/video_output/control.c
> >> @@ -51,10 +51,7 @@ void vout_control_Init(vout_control_t *ctrl)
> >>   {
> >>       vlc_mutex_init(&ctrl->lock);
> >>       vlc_cond_init(&ctrl->wait_request);
> >> -    vlc_cond_init(&ctrl->wait_available);
> >>   
> >> -    ctrl->is_held = false;
> >> -    ctrl->is_waiting = false;
> >>       ctrl->is_dead = false;
> >>       ctrl->can_sleep = true;
> >>       ARRAY_INIT(ctrl->cmd);
> >> @@ -125,18 +122,10 @@ void vout_control_PushString(vout_control_t
> >> *ctrl, int type, const char *string)
> >>   void vout_control_Hold(vout_control_t *ctrl)
> >>   {
> >>       vlc_mutex_lock(&ctrl->lock);
> >> -    while (ctrl->is_held || !ctrl->is_waiting)
> >> -        vlc_cond_wait(&ctrl->wait_available, &ctrl->lock);
> >> -    ctrl->is_held = true;
> >> -    vlc_mutex_unlock(&ctrl->lock);
> >>   }
> >>   
> >>   void vout_control_Release(vout_control_t *ctrl)
> >>   {
> >> -    vlc_mutex_lock(&ctrl->lock);
> >> -    assert(ctrl->is_held);
> >> -    ctrl->is_held = false;
> >> -    vlc_cond_signal(&ctrl->wait_available);
> >>       vlc_mutex_unlock(&ctrl->lock);
> >>   }
> >>   
> >> @@ -148,16 +137,10 @@ int vout_control_Pop(vout_control_t *ctrl,
> >> vout_control_cmd_t *cmd,
> >>       if (ctrl->cmd.i_size <= 0) {
> >>           /* Spurious wakeups are perfectly fine */
> >>           if (deadline != INVALID_DEADLINE && ctrl->can_sleep) {
> >> -            ctrl->is_waiting = true;
> >> -            vlc_cond_signal(&ctrl->wait_available);
> >>               vlc_cond_timedwait(&ctrl->wait_request, &ctrl->lock,
> >> deadline);
> >> -            ctrl->is_waiting = false;
> >>           }
> >>       }
> >>   
> >> -    while (ctrl->is_held)
> >> -        vlc_cond_wait(&ctrl->wait_available, &ctrl->lock);
> >> -
> >>       int ret;
> >>       if (ctrl->cmd.i_size > 0) {
> >>           ret = VLC_SUCCESS;
> >> diff --git a/src/video_output/control.h b/src/video_output/control.h
> >> index 6161c5ba3db..af13cc8fede 100644
> >> --- a/src/video_output/control.h
> >> +++ b/src/video_output/control.h
> >> @@ -50,13 +50,10 @@ void vout_control_cmd_Clean(vout_control_cmd_t *);
> >>   typedef struct {
> >>       vlc_mutex_t lock;
> >>       vlc_cond_t  wait_request;
> >> -    vlc_cond_t  wait_available;
> >>   
> >>       /* */
> >>       bool is_dead;
> >>       bool can_sleep;
> >> -    bool is_waiting;
> >> -    bool is_held;
> >>       DECL_ARRAY(vout_control_cmd_t) cmd;
> >>   } vout_control_t;
> >>   
> >> -- 
> >> 2.26.2
> >>
> >> _______________________________________________
> >> 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
> > 
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel