[FFmpeg-devel] [PATCH] Revert "avcodec/decode: copy the output parameters from the last bsf in the chain back to the AVCodecContext"

[James Almer]

On 9/12/2018 7:03 PM, James Almer wrote:
> On 9/12/2018 6:44 PM, Hendrik Leppkes wrote:
>> On Wed, Sep 12, 2018 at 8:15 PM James Almer <jamrial at gmail.com> wrote:
>>>
>>> This reverts commit f631c328e680a3dd491936b92f69970c20cdcfc7.
>>>
>>> The avcodec_parameters_to_context() call was freeing and reallocating
>>> AVCodecContext->extradata, essentially taking ownership of it, which according
>>> to the doxy is user owned. This is an API break and has produces crashes in
>>> some library users like Firefox[1].
>>> Revert until a better solution is found to internally propagate the filtered
>>> extradata back into the decoder context.
>>>
>>> [1] https://bugzilla.mozilla.org/show_bug.cgi?id=1486080
>>>
>>
>> This is not the only place where extradata is being
>> free'ed/re-allocated by avcodec during decoding, which is why I
>> recommended the documentation change when it came up.
>>
>> At least this one place is one I know of, maybe there are more:
>> http://git.videolan.org/?p=ffmpeg.git;a=blob;f=libavcodec/aacdec.c;h=d394700cdc857fa9386fc60ed2509fd869461f7f;hb=HEAD#l331
>>
>> - Hendrik
> 
> Sounds like it should use an internal buffer in AACContext instead.
> 
> The addition to require av_malloc() does not change the implications of
> what the doxy said before its addition, and in fact it only limits its
> versatility. If the user is meant to set, allocate and free said
> extradata, then that means they own it and could just set a pointer to
> some buffer they are also using elsewhere or plan to use long after
> closing the decoder, be it allocated by av_malloc() or otherwise.
> Libavcodec can't just free it and allocate its own replacement
> internally, as it would mean virtually taking ownership of it.
> 

> Hell, even avcodec_free_context() just frees it without checking if it's
> a decoder first, which is probably why Firefox and other cases I've seen
> do avcodec_close(avctx) followed by av_freep(&avctx) instead to prevent
> it being freed.

avcodec_close() frees extradata only in case it's an encoder, which is
the correct behavior, but then avcodec_free_context() goes and does it
unconditionally right after having called avcodec_close().
The proper thing to do would of course be removing the bogus free call
in avcodec_free_context(), but doing so will probably result in quite a
few library users finding new memleaks in their decoding applications if
they use that function and expect it to clean everything. ffmpeg.c,
ffprobe.c, and ffplay.c are among those users.

So, what do we do now? Honor the doxy and stop trying to manipulate
what's meant to be an user owned pointer/buffer, officially break the
API and declare it's meant to be allocated by the user but then
ownership is passed to the library during or after the avcodec_open2()
call, or just revert this commit and pretend nothing happened?