1041950 - FFmpegDataDecoder incompatible with libav >= 54

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Matthew Gregan [:kinetik]]

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.

Comment 1

[Matthew Gregan [:kinetik]]

Attachment: Allocate AVCodecContext using avcodec_alloc_context3 to avoid requiring FFmpegDataDecoder to know AVCodecContext's size.

Comment 2

[Matthew Gregan [:kinetik]]

(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.

Comment 3

[Matthew Gregan [:kinetik]]

Attachment: Allocate AVCodecContext using avcodec_alloc_context3 to avoid requiring FFmpegDataDecoder to know AVCodecContext's size. v2

Comment 4

[Matthew Gregan [:kinetik]]

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.

Comment 5

[Matthew Gregan [:kinetik]]

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.

Comment 6

[Jean-Yves Avenard [:jya]]

Attachment: Fix codec context allocation

Comment 7

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

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.

Comment 8

[Jean-Yves Avenard [:jya]]

Attachment: Fix codec context allocation

Carrying r+

Comment 9

[Jean-Yves Avenard [:jya]]

(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...

Comment 10

[Jean-Yves Avenard [:jya]]

Attachment: Fix usage with FFmpeg version 54 and above

Comment 11

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

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 { ... }

Comment 12

[Jean-Yves Avenard [:jya]]

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

Comment 13

[Jean-Yves Avenard [:jya]]

Attachment: Fix usage with FFmpeg version 54 and above

Carrying r+

Comment 14

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

https://tbpl.mozilla.org/?tree=Try&rev=108c6b0a52f7

Comment 15

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/98581d0168bc
https://hg.mozilla.org/integration/mozilla-inbound/rev/99126eb54dff

Comment 16

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8ebcdb3dc2a1

Comment 17

[Jean-Yves Avenard [:jya]]

Attachment: Create image copy instead of reference

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/98581d0168bc
https://hg.mozilla.org/mozilla-central/rev/99126eb54dff
https://hg.mozilla.org/mozilla-central/rev/8ebcdb3dc2a1

Comment 19

[Jean-Yves Avenard [:jya]]

Re-opening until last patch is committed. As it is required for <= libav54

Comment 20

[Jean-Yves Avenard [:jya]]

https://tbpl.mozilla.org/?tree=Try&rev=9b70633b6884

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3e4236a5d8ed

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/3e4236a5d8ed