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

[Steve Lhomme]

On 2019-09-03 10:22, Thomas Guillem wrote:
> 
> 
> On Tue, Sep 3, 2019, at 09:53, Steve Lhomme wrote:
>> 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.
> 
> I already proposed the same kind of patches and  we already had this discussion with Rémi in the past.
> This part is really tricky, I should write a comment.

If some functions are allowed to read some (lcoked) variables without 
lock because they are only called by a particular thread, they should be 
marked as such. That would also potential errors when updating the code.

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