[vlc-devel] [PATCH] decoder: thumbnailer: Fix unprotected access to b_first

[Hugo Beauzée-Luyssen]

On Wed, Oct 2, 2019, at 8:30 AM, Rémi Denis-Courmont wrote:
> +1
> 
> This probably be separated better from normal decoding.
> 
> Le 2 octobre 2019 08:53:24 GMT+03:00, Steve Lhomme <robux4 at ycbcr.xyz> a 
> écrit : Maybe an atomic bool would be more appropriate  ?
> > 
> > On 2019-10-01 18:33, Hugo Beauzée-Luyssen wrote:
> >>   src/input/decoder.c | 17 +++++++++++++----
> >>   1 file changed, 13 insertions(+), 4 deletions(-)
> >> 
> >> diff --git a/src/input/decoder.c b/src/input/decoder.c
> >> index 57c9de6a1e..bb1beca38d 100644
> >> --- a/src/input/decoder.c
> >> +++ b/src/input/decoder.c
> >> @@ -1101,19 +1101,28 @@ static picture_t *thumbnailer_buffer_new( decoder_t *p_dec )
> >>       struct decoder_owner *p_owner = dec_get_owner( p_dec );
> >>       /* Avoid decoding more than one frame when a thumbnail was
> >>        * already generated */
> >> +    vlc_mutex_lock( &p_owner->lock );
> >>       if( !p_owner->b_first )
> >> +    {
> >> +        vlc_mutex_unlock( &p_owner->lock );
> >>           return NULL;
> >> +    }
> >> +    vlc_mutex_unlock( &p_owner->lock );
> >>       return picture_NewFromFormat( &p_dec->fmt_out.video );
> >>   }
> >>   
> >>   static void ModuleThread_QueueThumbnail( decoder_t *p_dec, picture_t *p_pic )
> >>   {
> >>       struct decoder_owner *p_owner = dec_get_owner( p_dec );
> >> -    if( p_owner->b_first )
> >> -    {
> >> +    bool b_first;
> >> +
> >> +    vlc_mutex_lock( &p_owner->lock );
> >> +    b_first = p_owner->b_first;
> >> +    p_owner->b_first = false;
> >> +    vlc_mutex_unlock( &p_owner->lock );
> >> +
> >> +    if( b_first )
> >>           decoder_Notify(p_owner, on_thumbnail_ready, p_pic);
> >> -        p_owner->b_first = false;
> >> -    }
> >>       picture_Release( p_pic );
> >>   
> >>   }
> >> -- 
> >> 2.20.1vlc-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
> 

Hi,

Do you mean not using b_first in the case of thumbnailing? I'm fine with that but I'm not sure it's worth adding yet another member in the decoder_owner structure.
Otherwise I agree with Thomas that in other cases this variable is better of not being atomic.

Regards,

-- 
  Hugo Beauzée-Luyssen
  hugo at beauzee.fr