Closed Bug 1041950 Opened 5 years ago Closed 5 years ago

FFmpegDataDecoder incompatible with libav >= 54

Categories

(Core :: Audio/Video, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: kinetik, Assigned: jya)

References

Details

Attachments

(3 files, 4 obsolete files)

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.
(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.
Attachment #8460069 - Attachment is obsolete: true
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)
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
Attached patch Fix codec context allocation (obsolete) — Splinter Review
Attachment #8460707 - Flags: review?(edwin)
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+
Carrying r+
Attachment #8460707 - Attachment is obsolete: true
(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...
Attachment #8461317 - Flags: review?(edwin)
Summary: FFmpegDataDecoder allocates AVCodecContext in an unsafe way → FFmpegDataDecoder incompatible with libav >= 54
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+
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
Attachment #8461317 - Attachment is obsolete: true
Blocks: 1044703
Re-opening until last patch is committed. As it is required for <= libav54
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/3e4236a5d8ed
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.