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

[James Almer]

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.

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 agree. AVFrames guarantee to remain valid independently of which
>> component they ever came out of. So if dav1d cannot provide that,
>> either we need to solve that in the dav1d wrapper (ie. by keeping  an
>> instance to the dav1d context tied to the frame like we do for some
>> hardware frames), or dav1d fixes that.
> 
> If we provide a picture allocator to dav1d that wraps AVFrame we
> allocate it should not be an issue. We would own them no matter what.

Yes, but dav1d will be used by more projects than just ffmpeg. The
internal allocator is going to be the most used one, and until this
merge, returned pictures were not constrained by the lifetime of the
decoder context that produced them. This is effectively a regression.