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

[Hendrik Leppkes]

On Wed, Oct 17, 2018 at 10:14 AM Steve Lhomme <robux4 at ycbcr.xyz> wrote:
>
> 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.
>

Unloading DLLs, at least system DLLs on Windows, is mostly pointless anyway.
For dav1d, it would be linked by the linker to avcodec, and thus you
wouldn't unload it unless you unload avcodec, which you really
shouldn't do unless you destroyed all objects from it anyway.
Basically, DLL unloading is not really a factor to worry about much.

- Hendrik