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

[Steve Lhomme]

OK, I see b_first is modified under lock in input_DecoderStartWait() so 
it's better/easier to use the lock.

On 2019-10-02 10:23, Hugo Beauzée-Luyssen wrote:
> 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
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel
>