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

[Steve Lhomme]

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.