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

[Steve Lhomme]

On 20/02/2019 11:19, Thomas Guillem wrote:
> On Wed, Feb 20, 2019, at 11:16, Steve Lhomme wrote:
>> 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.
> When I said decoder implementation, I was talking about decoder modules (avocec/MediaCodec/Videotoolbox are using synchronization mechanism), so it doesn't depend on input/decoder.c

Ah, I tought the decooder owner->lock was used for that. But it turns 
out it's not. lavc uses a semaphore for example (wait_mt/post_mt) to 
protect the calls. Looking at dav1d or aom (first picks which I know use 
multithread) there's no such thing. Luckily the calls seem to be done in 
the decoder loop.