[FFmpeg-devel] [PATCH] ffmpeg: release the last_frame before the decoders are closed

[Steve Lhomme]

On 17/10/2018 09:43, Steve Lhomme wrote:
> On 16/10/2018 18:04, James Almer wrote:
>> On 10/16/2018 12:34 PM, Steve Lhomme wrote:
>>> On 16/10/2018 16:59, Hendrik Leppkes wrote:
>>>> On Tue, Oct 16, 2018 at 4:02 PM James Almer <jamrial at gmail.com> wrote:
>>>>> On 10/16/2018 9:16 AM, Steve Lhomme wrote:
>>>>>> If the decoder provides its own buffers it might not be able to
>>>>>> release its
>>>>>> buffers once it has been closed. (this is the case with dav1d).
>>>>>> ---
>>>>>>    fftools/ffmpeg.c | 1 +
>>>>>>    1 file changed, 1 insertion(+)
>>>>>>
>>>>>> diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
>>>>>> index da4259a9a8..faf62475a2 100644
>>>>>> --- a/fftools/ffmpeg.c
>>>>>> +++ b/fftools/ffmpeg.c
>>>>>> @@ -4738,6 +4738,7 @@ static int transcode(void)
>>>>>>            if (ost->encoding_needed) {
>>>>>> av_freep(&ost->enc_ctx->stats_in);
>>>>>>            }
>>>>>> +        av_frame_unref(ost->last_frame);
>>>>>>            total_packets_written += ost->packets_written;
>>>>>>        }
>>>>> I'm not against this change, but this issue should be solved within
>>>>> dav1d as well. Either the caller fully owns the picture returned by
>>>>> dav1d, or it doesn't. "Partially owns it while some other context is
>>>>> still valid" is not really acceptable.
>>> You can't ask any library to own content even after you have closed it
>>> (and potentially unloaded the DLL).
>> As i said in dav1d's merge request, with libavcodec i can open an
>> encoder, generate an AVPacket, close the encoder, open a bitstream
>> filter, filter the packet i got from the encoder, close the bitstream
>> filter, and still fully own the packet without having in it any dangling
>> pointer to some callback stored in a long dead context. Same thing with
>> AVFrames.
>
> It generally works because it's usually used statically and with no 
> external decoder, loaded dynamically.
>
>> This patch prevents ffmpeg.c closing the decoder (and thus the
>> Dav1dContext) before a reference to one AVFrame, owned by ffmpeg.c and
>> supposedly standalone, was freed. That's ok, but ffmpeg.c is just one
>> libavcodec user, and we have no control whatsoever of what other
>> libavcodec users will do with a returned AVFrame from
>> avcodec_receive_frame().
>> And as Hendrik said, they are guaranteed to remain valid regardless of
>> the component they came out of, so this is not acceptable.
>
> I'd be curious to see how it works with DXVA2/D3D11VA where a DLL 
> needs to be loaded and the buffers come for that DLL. If last_frame 
> holds a DXVA2/D3D11 texture and tries to free it after the DLL has 
> been unloaded it shouldn't work well.

I had a quick look. It turns out that the DLLs are loaded but never 
unloaded. So of course it will never execute code in unloaded DLLs.