[vlc-devel] [PATCH] decoder_helpers: fix clean order

[Steve Lhomme]

On 20/02/2019 10:27, Thomas Guillem wrote:
> On Wed, Feb 20, 2019, at 10:09, Steve Lhomme wrote:
>> On 19/02/2019 13:24, Thomas Guillem wrote:
>>> The module need to be unloaded before the dec struct is cleaned.
>>>
>>> This fixes an undefined behavior with threaded decoders like avcodec when they
>>> called decoder_UpdateVideoFormat()/dec_NewPicture() from an other thread just
>>> before they were unloaded.
>> Are there cases where the decoder is still calling
>> decoder_UpdateVideoFormat() and decoder_NewPicture() while being about
>> to be unloaded ?
> It is the case with avcodec with multi thread or every asynchronous decoders (MediaCodec/VideoToolBox).

OK, after double checking it seems they are called in the decoder loop 
and in the decoder threads.

>
>> I don't see how this can work if one thread of the
>> decoder is calling decoder_UpdateVideoFormat() while another thread is
>> unloading the decoder DLLs. If that can happen I think the undefined
>> behavior is the least of our problem.
> This won't happen since decoder implementations use mutex or semaphores to prevent that.

While this is the case for our favorite input decoder, it doesn't exist 
in mosaic_bridge, SDIStream or the image decoder. That's typically the 
kind of thing that should be factorized outside of the decoder.

That means changing the dec->fmt_out in decoder_UpdateVideoFormat() is 
not possible since it's not protected by a lock. I did add a lock to 
protect the pool/context, it should be used for that too.

>
>> AFAIK lavc always call decoder_UpdateVideoFormat()/decoder_NewPicture()
>> from the same (decoder?) thread.
>>
>> This is an important thing because the decoder pool will be released
>> there as well as the video context reference count.
>>
>>> ---
>>>    src/input/decoder_helpers.c | 11 ++++++-----
>>>    1 file changed, 6 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/src/input/decoder_helpers.c b/src/input/decoder_helpers.c
>>> index 3cc02de8d6..b0a7c1890a 100644
>>> --- a/src/input/decoder_helpers.c
>>> +++ b/src/input/decoder_helpers.c
>>> @@ -48,6 +48,12 @@ void decoder_Init( decoder_t *p_dec, const es_format_t *restrict p_fmt )
>>>    
>>>    void decoder_Clean( decoder_t *p_dec )
>>>    {
>>> +    if ( p_dec->p_module != NULL )
>>> +    {
>>> +        module_unneed(p_dec, p_dec->p_module);
>>> +        p_dec->p_module = NULL;
>>> +    }
>>> +
>>>        es_format_Clean( &p_dec->fmt_in );
>>>        es_format_Clean( &p_dec->fmt_out );
>>>    
>>> @@ -56,11 +62,6 @@ void decoder_Clean( decoder_t *p_dec )
>>>            vlc_meta_Delete(p_dec->p_description);
>>>            p_dec->p_description = NULL;
>>>        }
>>> -    if ( p_dec->p_module != NULL )
>>> -    {
>>> -        module_unneed(p_dec, p_dec->p_module);
>>> -        p_dec->p_module = NULL;
>>> -    }
>>>    }
>>>    
>>>    void decoder_Destroy( decoder_t *p_dec )
>>> -- 
>>> 2.20.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