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

[Thomas Guillem]

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.

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