Closed
Bug 1041950
Opened 10 years ago
Closed 10 years ago
FFmpegDataDecoder incompatible with libav >= 54
Categories
(Core :: Audio/Video, defect)
Tracking
()
RESOLVED
FIXED
mozilla34
People
(Reporter: kinetik, Assigned: jya)
References
Details
Attachments
(3 files, 4 obsolete files)
9.13 KB,
patch
|
Details | Diff | Splinter Review | |
16.85 KB,
patch
|
Details | Diff | Splinter Review | |
6.58 KB,
patch
|
eflores
:
review+
|
Details | Diff | Splinter Review |
FFmpegDataDecoder includes an AVCodecContext as a member variable, but this requires knowing sizeof(AVCodecContext), which is explicitly prohibited per http://dxr.mozilla.org/mozilla-central/source/content/media/fmp4/ffmpeg/libav55/include/libavcodec/avcodec.h#1052 Indeed, they don't match on my system: From within Gecko: (gdb) p sizeof(AVCodecContext) $1 = 1000 From within libavcodec: gdb) p sizeof(AVCodecContext) $2 = 1072 Okay, so, easy fix: make mCodecContext a pointer and allocate it using avcodec_alloc_context3. But how do we free it? avcodec_free_context has only been added very recently. Older examples use av_free. Next problem: avcodec_alloc_context3 ends up allocating this memory via posix_memalign, which our malloc does not interpose, but av_free calls regular free which our malloc *does* interpose.
Reporter | ||
Comment 1•10 years ago
|
||
Reporter | ||
Comment 2•10 years ago
|
||
(In reply to Matthew Gregan [:kinetik] from comment #0) > Next problem: > avcodec_alloc_context3 ends up allocating this memory via posix_memalign, > which our malloc does not interpose, but av_free calls regular free which > our malloc *does* interpose. This seems to be incorrect, stepping through the code I end up inside je_posix_memalign. Gah, stupid me. It turns out when I converted the code from trying to use avcodec_free_context (with takes an AVCodecContext**) to av_free (which takes a void*), I forgot to remove the address-operator from the call.
Reporter | ||
Comment 3•10 years ago
|
||
Attachment #8460071 -
Flags: review?(edwin)
Reporter | ||
Updated•10 years ago
|
Attachment #8460069 -
Attachment is obsolete: true
Reporter | ||
Comment 4•10 years ago
|
||
Comment on attachment 8460071 [details] [diff] [review] Allocate AVCodecContext using avcodec_alloc_context3 to avoid requiring FFmpegDataDecoder to know AVCodecContext's size. v2 Something still broken, the second or so call to avcodec_decode_video2 crashes with heap corruption in je_free.
Attachment #8460071 -
Flags: review?(edwin)
Reporter | ||
Comment 5•10 years ago
|
||
jya knows FFmpeg much better than I do and was working on the same bug in parallel, so he's going to grab this bug.
Assignee: kinetik → jyavenard
Assignee | ||
Comment 6•10 years ago
|
||
Attachment #8460707 -
Flags: review?(edwin)
Assignee | ||
Updated•10 years ago
|
Attachment #8460071 -
Attachment is obsolete: true
Comment on attachment 8460707 [details] [diff] [review] Fix codec context allocation Review of attachment 8460707 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/fmp4/ffmpeg/FFmpegFunctionList.h @@ +14,5 @@ > AV_FUNC(avcodec_default_get_buffer) > AV_FUNC(avcodec_default_release_buffer) > AV_FUNC(avcodec_find_decoder) > AV_FUNC(avcodec_flush_buffers) > AV_FUNC(avcodec_get_context_defaults3) Remove avcodec_get_context_defaults3 from this list.
Attachment #8460707 -
Flags: review?(edwin) → review+
Assignee | ||
Comment 8•10 years ago
|
||
Carrying r+
Assignee | ||
Updated•10 years ago
|
Attachment #8460707 -
Attachment is obsolete: true
Assignee | ||
Comment 9•10 years ago
|
||
(In reply to Matthew Gregan [:kinetik] from comment #0) > Next problem: > avcodec_alloc_context3 ends up allocating this memory via posix_memalign, > which our malloc does not interpose, but av_free calls regular free which > our malloc *does* interpose. avcodec_free_context is in FFmpeg 2.x only. In FFmpeg 1.x, which still works in 2.2 but marked as deprecated you do avcodec_close, followed by av_free/av_freep There are other issues in the FFmpeg code with the allocation of AVFrame, they are allocated via malloc, but detroyed in the shared pointer using delete operator. The other issue is in the H264 decoder, the buffer of the AVFrame aren't allocated, only the AVFrame container itself is. In the AAC decode, the buffer for the AVFrame is allocated, but never freed. So this will cause a leak for every call to DecodePacket And finally, AVCodecContext::extradata is used to store internal data, used in the H264 decoder. You can't do that. If FFmpeg sees that extradata is != data, then it will attempt to free it once he's done with the context. Which would later cause a crash as extradata is really a Vector<uint8_t>, so double free in its destructor this is mostly a note_to_self...
Assignee | ||
Comment 10•10 years ago
|
||
Attachment #8461317 -
Flags: review?(edwin)
Assignee | ||
Updated•10 years ago
|
Summary: FFmpegDataDecoder allocates AVCodecContext in an unsafe way → FFmpegDataDecoder incompatible with libav >= 54
Comment 11•10 years ago
|
||
Comment on attachment 8461317 [details] [diff] [review] Fix usage with FFmpeg version 54 and above Review of attachment 8461317 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/fmp4/ffmpeg/FFmpegDataDecoder.cpp @@ +21,5 @@ > > FFmpegDataDecoder<LIBAV_VER>::FFmpegDataDecoder(MediaTaskQueue* aTaskQueue, > AVCodecID aCodecID) > + : mTaskQueue(aTaskQueue), > + mCodecContext(nullptr), mFrame(NULL), mCodecID(aCodecID) nit: this line wrapping isn't quite right. @@ +89,5 @@ > mCodecContext->thread_type = FF_THREAD_SLICE | FF_THREAD_FRAME; > mCodecContext->thread_safe_callbacks = false; > > mCodecContext->extradata_size = mExtraData.length(); > + for (int i = mExtraData.length(); i < FF_INPUT_BUFFER_PADDING_SIZE; i++) { This seems odd but you're the expert. @@ +161,5 @@ > +#endif > + } > + return mFrame; > +} > + It would be clearer to duplicate the whole if statement rather than have two sets of #ifdefs. I'd also swap the if clauses so we have: if (mFrame) { ... } else { ... }
Attachment #8461317 -
Flags: review?(edwin) → review+
Assignee | ||
Comment 12•10 years ago
|
||
I couldn't find a reference on how initializer should be formed, other than one by line https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Classes seeing that none of this ffmpeg code followed that official style, I decided to group them according to their category. So all AVCodec objects on the same line. there should be no need to even pad the extradata, but the padding is such that it's "at least" FF_INPUT_BUFFER_PADDING_SIZE, which makes the size always a modulo of 64. Only kept it to keep the changes to a minimum
Assignee | ||
Comment 13•10 years ago
|
||
Carrying r+
Assignee | ||
Updated•10 years ago
|
Attachment #8461317 -
Attachment is obsolete: true
Comment 14•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=108c6b0a52f7
Comment 15•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/98581d0168bc https://hg.mozilla.org/integration/mozilla-inbound/rev/99126eb54dff
Assignee | ||
Comment 17•10 years ago
|
||
Attachment #8463212 -
Flags: review?(edwin)
Attachment #8463212 -
Flags: review?(edwin) → review+
Comment 18•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/98581d0168bc https://hg.mozilla.org/mozilla-central/rev/99126eb54dff https://hg.mozilla.org/mozilla-central/rev/8ebcdb3dc2a1
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Assignee | ||
Comment 19•10 years ago
|
||
Re-opening until last patch is committed. As it is required for <= libav54
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 20•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=9b70633b6884
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 21•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/3e4236a5d8ed
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/3e4236a5d8ed
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•