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

[Steve Lhomme]

On 2020-08-14 11:30, Thomas Guillem wrote:
> 
> 
> 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.

Yes, this patchset is tricky (but also making the code easier to 
understand in the end) and sensitive.

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