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

Submitted by James Almer on Sept. 12, 2018, 6:14 p.m.

Details

Message ID 20180912181443.3972-1-jamrial@gmail.com
State Accepted
Commit 87588caf8cff318cd022ad5002304198c6ddbd51
Headers show

Commit Message

James Almer Sept. 12, 2018, 6:14 p.m.
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

Signed-off-by: James Almer <jamrial@gmail.com>
---
Also reported in
http://ffmpeg.org/pipermail/ffmpeg-devel/2018-August/233584.html

Commit 0c71c6d66f9ae8265158181e5b1cbc5c63525fde may have to be reverted as
well. It is also an API change to require av_malloc() to be used when users
have probably been using other allocators for years now, as was the case with
Firefox.

Suggestions to work around it are very welcome, of course. While no bitstream
filter currently autoinserted by a decoder requires the filtered extradata to
be propagated to work properly, we don't know what may be needed in the future,
and without this the usability of bsfs in decoders is potentially limited.

 libavcodec/decode.c | 4 ----
 1 file changed, 4 deletions(-)

Comments

Hendrik Leppkes Sept. 12, 2018, 9:44 p.m.
On Wed, Sep 12, 2018 at 8:15 PM James Almer <jamrial@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
James Almer Sept. 12, 2018, 10:03 p.m.
On 9/12/2018 6:44 PM, Hendrik Leppkes wrote:
> On Wed, Sep 12, 2018 at 8:15 PM James Almer <jamrial@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.
James Almer Sept. 13, 2018, 1:46 a.m.
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@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?
Timo Rothenpieler Sept. 13, 2018, 9:50 a.m.
> 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?

Documenting the API break that already happened seems like the least bad 
option right now. Firefox already merged code to work around the crash.

It already is freed in multiple places, so existing code probably 
already is compatible.

No solution to this is overly pretty unfortunately.
Specially as this goes along with likely leaking the extradata pointer, 
as unconditionally freeing it will most definitely cause crashes left 
and right.
James Almer Sept. 13, 2018, 3:44 p.m.
On 9/13/2018 6:50 AM, Timo Rothenpieler wrote:
>> 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?
> 
> Documenting the API break that already happened seems like the least bad
> option right now. Firefox already merged code to work around the crash.
> 
> It already is freed in multiple places, so existing code probably
> already is compatible.
> 
> No solution to this is overly pretty unfortunately.
> Specially as this goes along with likely leaking the extradata pointer,
> as unconditionally freeing it will most definitely cause crashes left
> and right.

Commit fd056029f45a9f6d213d9fce8165632042511d4f is what introduced
avcodec_free_context(), including the unconditional av_free call for
extradata. Assuming that's the first case of the extradata pointer being
manipulated by lavc for a decoder, then the doxy has been pretty much
incorrect for at least four years now.

So i guess you're right, and perhaps we should just change the doxy to
reflect the behavior the library has featured all this time: That the
user needs to allocate it with av_malloc(), but after calling
avcodec_open2() ownership is passed to lavc.

> 
> 
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
James Almer Oct. 22, 2018, 9:44 p.m.
On 9/13/2018 6:50 AM, Timo Rothenpieler wrote:
>> 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?
> 
> Documenting the API break that already happened seems like the least bad
> option right now. Firefox already merged code to work around the crash.
> 
> It already is freed in multiple places, so existing code probably
> already is compatible.
> 
> No solution to this is overly pretty unfortunately.
> Specially as this goes along with likely leaking the extradata pointer,
> as unconditionally freeing it will most definitely cause crashes left
> and right.

[18:32:33] <@mateo`> jamrial: hello, it looks like
662558f985f50834eebe82d6b6854c66f33ab320 introduced a regression on the
mediacodec decoders. The decoder ends up receiving the bitstream in the
hvcC form (for hevc) and not annex-b. The underlying bsf seems
initialized twice, first time, everything is fine, second time it
beleives the bitstream is already annex-b because of the newly replaced
extradata (now in annex-b form).
[18:35:00] <@mateo`> jamrial: any ideas on how to fix that ? I haven't
found yet why the bsf is initialized twice (maybe because of
avformat_find_stream_info() which opens a decoder)

I'm going to push this revert commit soon. We can reintroduce it and
adapt the doxy accordingly at some later time. But for now we gain
nothing and it only brought issues for library users.
James Almer Oct. 24, 2018, 7:44 p.m.
On 10/22/2018 6:44 PM, James Almer wrote:
> On 9/13/2018 6:50 AM, Timo Rothenpieler wrote:
>>> 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?
>>
>> Documenting the API break that already happened seems like the least bad
>> option right now. Firefox already merged code to work around the crash.
>>
>> It already is freed in multiple places, so existing code probably
>> already is compatible.
>>
>> No solution to this is overly pretty unfortunately.
>> Specially as this goes along with likely leaking the extradata pointer,
>> as unconditionally freeing it will most definitely cause crashes left
>> and right.
> 
> [18:32:33] <@mateo`> jamrial: hello, it looks like
> 662558f985f50834eebe82d6b6854c66f33ab320 introduced a regression on the
> mediacodec decoders. The decoder ends up receiving the bitstream in the
> hvcC form (for hevc) and not annex-b. The underlying bsf seems
> initialized twice, first time, everything is fine, second time it
> beleives the bitstream is already annex-b because of the newly replaced
> extradata (now in annex-b form).
> [18:35:00] <@mateo`> jamrial: any ideas on how to fix that ? I haven't
> found yet why the bsf is initialized twice (maybe because of
> avformat_find_stream_info() which opens a decoder)
> 
> I'm going to push this revert commit soon. We can reintroduce it and
> adapt the doxy accordingly at some later time. But for now we gain
> nothing and it only brought issues for library users.

Pushed.

Patch hide | download patch | download mbox

diff --git a/libavcodec/decode.c b/libavcodec/decode.c
index 4607e9f318..2e82f6b506 100644
--- a/libavcodec/decode.c
+++ b/libavcodec/decode.c
@@ -281,10 +281,6 @@  int ff_decode_bsfs_init(AVCodecContext *avctx)
             bsfs_str++;
     }
 
-    ret = avcodec_parameters_to_context(avctx, s->bsfs[s->nb_bsfs - 1]->par_out);
-    if (ret < 0)
-        return ret;
-
     return 0;
 fail:
     ff_decode_bsfs_uninit(avctx);