| Message ID | 20180912181443.3972-1-jamrial@gmail.com |
|---|---|
| State | Accepted |
| Commit | 87588caf8cff318cd022ad5002304198c6ddbd51 |
| Headers | show |
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
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.
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?
> 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.
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 >
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.
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.
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);
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(-)