[vlc-devel] [PATCH 10/13] decoder: avoid unlock/lock when we don't need to

[Steve Lhomme]

On 2019-09-02 16:57, Thomas Guillem wrote:
> 
> 
> On Mon, Sep 2, 2019, at 16:20, Steve Lhomme wrote:
>> The p_vout/p_aout are flushed under lock in other places and they are modified
>> under lock so need to be read under lock.
> 
> vout and aout have their own locks.

The owner->lock does serve a purpose aside from these locks. It protects 
the access to multiple variables that may change. p_aout and p_vout are 
among them. It's even documented:

     /* -- These variables need locking on write(only) -- */

It seems rather odd that only one way would be locked and not the other. 
Either the variables need to be protected because they are accessed from 
multiple threads or are not.

> The thread calling DecoderPlayVideo() is the thread that created the vout (via decoder_UpdateVideoFormat()), so it does not need to lock to get the vout variable.

If p_vout needs to be locked for writing from multiple threads I don't 
see how you can read it safely without a lock from one of those threads.

>> ---
>>   src/input/decoder.c | 14 ++++----------
>>   1 file changed, 4 insertions(+), 10 deletions(-)
>>
>> diff --git a/src/input/decoder.c b/src/input/decoder.c
>> index f82e608ad4..24061a6618 100644
>> --- a/src/input/decoder.c
>> +++ b/src/input/decoder.c
>> @@ -976,7 +976,6 @@ static void DecoderQueueCc( decoder_t *p_videodec,
>> block_t *p_cc,
>>   static int DecoderPlayVideo_internal( struct decoder_owner *p_owner,
>> picture_t *p_picture )
>>   {
>>       decoder_t *p_dec = &p_owner->dec;
>> -    vout_thread_t  *p_vout = p_owner->p_vout;
>>   
>>       if( p_picture->date == VLC_TICK_INVALID )
>>           /* FIXME: VLC_TICK_INVALID -- verify video_output */
>> @@ -996,19 +995,16 @@ static int DecoderPlayVideo_internal( struct
>> decoder_owner *p_owner, picture_t *
>>       }
>>   
>>       p_owner->i_preroll_end = (vlc_tick_t)INT64_MIN;
>> -    vlc_mutex_unlock( &p_owner->lock );
>>   
>>       if( unlikely(prerolled) )
>>       {
>>           msg_Dbg( p_dec, "end of video preroll" );
>>   
>> -        if( p_vout )
>> -            vout_FlushAll( p_vout );
>> +        if( p_owner->p_vout )
>> +            vout_FlushAll( p_owner->p_vout );
>>       }
>>   
>>       /* */
>> -    vlc_mutex_lock( &p_owner->lock );
>> -
>>       if( p_owner->b_waiting && !p_owner->b_first )
>>       {
>>           p_owner->b_has_data = true;
>> @@ -1025,6 +1021,7 @@ static int DecoderPlayVideo_internal( struct
>> decoder_owner *p_owner, picture_t *
>>           p_picture->b_force = true;
>>       }
>>   
>> +    vout_thread_t  *p_vout = p_owner->p_vout;
>>       vlc_mutex_unlock( &p_owner->lock );
>>   
>>       /* FIXME: The *input* FIFO should not be locked here. This will
>> not work
>> @@ -1128,7 +1125,6 @@ static int DecoderPlayAudio_internal( struct
>> decoder_owner *p_owner, block_t *p_
>>       }
>>   
>>       p_owner->i_preroll_end = (vlc_tick_t)INT64_MIN;
>> -    vlc_mutex_unlock( &p_owner->lock );
>>   
>>       if( unlikely(prerolled) )
>>       {
>> @@ -1139,8 +1135,6 @@ static int DecoderPlayAudio_internal( struct
>> decoder_owner *p_owner, block_t *p_
>>       }
>>   
>>       /* */
>> -    /* */
>> -    vlc_mutex_lock( &p_owner->lock );
>>       if( p_owner->b_waiting )
>>       {
>>           p_owner->b_has_data = true;
>> @@ -1149,9 +1143,9 @@ static int DecoderPlayAudio_internal( struct
>> decoder_owner *p_owner, block_t *p_
>>   
>>       /* */
>>       DecoderWaitUnblock_internal( p_owner );
>> -    vlc_mutex_unlock( &p_owner->lock );
>>   
>>       audio_output_t *p_aout = p_owner->p_aout;
>> +    vlc_mutex_unlock( &p_owner->lock );
>>   
>>       if( p_aout != NULL )
>>       {
>> -- 
>> 2.17.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
>