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

[Steve Lhomme]

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.

> 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.

> 
> 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
>