941298 - PlatformDecoderModule for Linux

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Chris Pearce [:cpearce (Not reading bugmail)]]

We need a PlatformDecoderModule created for Linux before we can playback fragmented MP4 in MSE on Linux. Unless GStreamer already supports FMP4, then it might just work. But we want to switch to using a PlatformDecoderModule for every Platform anyway, so we have more control over threading/lifetimes etc.

Comment 1

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

Attachment: GStreamer PlatformDecoderModule for Linux

Comment 2

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

Attachment: GStreamer PlatformDecoderModule for Linux

Comment 3

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

Attachment: Fix errors in content/media/fmp4 on Linux with clang and --enable-warnings-as-errors

Comment 4

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

Attachment: Build changes for FFmpeg PlatformDecoderModule

Comment 5

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

Attachment: FFmpeg headers for FFmpeg PlatformDecoderModule

While FFmpeg function signatures tend not to change between versions of FFmpeg,
class layouts can change dramatically. We include libavcodec, libavformat,
and libavutil headers here so that we don't accidentally build against the wrong
binary interface.

Comment 6

[Ralph Giles (:rillian)]

Comment on attachment 8377300 [details] [diff] [review]
Fix errors in content/media/fmp4 on Linux with clang and --enable-warnings-as-errors

Review of attachment 8377300 [details] [diff] [review]:
-----------------------------------------------------------------

r=me fwiw.

::: content/media/fmp4/BlankDecoderModule.cpp
@@ +167,1 @@
>    {

Do you not need to cast way an unused argument warning here?

Oh, I see we pass /Wno-unused or -Qunused-arguments (twice!)

Comment 7

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8377300 [details] [diff] [review]
Fix errors in content/media/fmp4 on Linux with clang and --enable-warnings-as-errors

Review of attachment 8377300 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/media/fmp4/MP4Reader.cpp
@@ -370,5 @@
>    data.mMonitor.Unlock();
>    return true;
>  }
>  
> -static const char*

This function is used when LOG_SAMPLE_DECODE is enabled. Put this function definition inside an #ifdef LOG_SAMPLE_DECODE guard please.

Comment 8

[cajbir (:cajbir)]

Comment on attachment 8377302 [details] [diff] [review]
FFmpeg headers for FFmpeg PlatformDecoderModule

Review of attachment 8377302 [details] [diff] [review]:
-----------------------------------------------------------------

Have you checked with legal to make sure that it's ok to use this code? Might want to get feedback from gerv if not. Add a README somewhere explaining where the code is from, and what version (or source code control commit id).

Comment 9

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8377299 [details] [diff] [review]
GStreamer PlatformDecoderModule for Linux

Review of attachment 8377299 [details] [diff] [review]:
-----------------------------------------------------------------

In general it looks pretty good. I'd like to see it again before r+ing though.

Please add MOZ_COUNT_CTOR and DTOR to your ctors and dtors.

::: content/media/fmp4/ffmpeg/FFmpegAACDecoder.cpp
@@ +31,5 @@
> +  return NS_OK;
> +}
> +
> +static
> +AudioDataValue *

Asterisks on pointers hugging left. Everywhere.

@@ +39,5 @@
> +    audio(new AudioDataValue[aNumChannels * aNumSamples]);
> +
> +  AudioDataValue **data = reinterpret_cast<AudioDataValue **>(aFrame->data);
> +
> +  if(aFrame->format == AV_SAMPLE_FMT_FLT) {

if (..) {
} else if (..) {
}

@@ +89,5 @@
> +  nsAutoArrayPtr<AudioDataValue> audio(CopyAndPackAudio(frame.get(),
> +                                                        numChannels,
> +                                                        frame->nb_samples));
> +
> +  nsAutoPtr<AudioData> data(new AudioData(packet.pos, 

Nit: Trailing space.

It would be nice if we used the AudioCompactor here...

::: content/media/fmp4/ffmpeg/FFmpegDataDecoder.cpp
@@ +38,5 @@
> +ChoosePixelFormat(AVCodecContext *aCodecContext,
> +                  const PixelFormat *aFormats)
> +{
> +  FFMPEG_LOG("Choosing FFmpeg pixel format for video decoding.");
> +  for (; *aFormats > -1; aFormats++) { 

Nit: trailing space.

::: content/media/fmp4/ffmpeg/FFmpegDecoderModule.cpp
@@ +11,5 @@
> +#include "FFmpegDecoderModule.h"
> +
> +namespace mozilla {
> +
> +PRLogModuleInfo *gFFmpegDecoderLog;

static PRLogModuleInfo *gFFmpegDecoderLog = nullptr;

You're not initializing this. Bad things could happen, right?


Here I would prefer:

PRLogModuleInfo* GetFFmpegDecoderLog() {
  static PRLogModuleInfo *gFFmpegDecoderLog = nullptr;
  if (!gFFmpegDecoderLog) {
    gFFmpegDecoderLog = PR_NewLogModule("FFmpegDecoderModule");
  }
  return gFFmpegDecoderLog;
}

and then in FFmpegDecoderModule.h:

#ifdef PR_LOGGING
extern PRLogModuleInfo* GetFFmpegDecoderLog();
#define FFMPEG_LOG(...) PR_LOG(GetFFmpegDecoderLog(), PR_LOG_DEBUG, (__VA_ARGS__))
#else
#define FFMPEG_LOG(...)
#endif

Then you don't need to worry about an decoder module ctor being run before the log function will work without crashing.

@@ +33,5 @@
> +  if (sFFmpegLinkDone) {
> +    return true;
> +  }
> +
> +  if (!FFmpegRuntimeLinker::Link()) {

Do you need to do something to ensure these are unlinked? At least an unlink in nsLayoutStatics::Shutdown()?

::: content/media/fmp4/ffmpeg/FFmpegH264Decoder.cpp
@@ +237,5 @@
> +    nsAutoPtr<MP4Sample> empty;
> +
> +    // An empty frame tells FFmpeg to decode the next delayed frame it has in
> +    // its queue, if it has any.
> +    empty = new MP4Sample(0 /* dts */,

ewwww.....

@@ +250,5 @@
> +    nsresult rv = Input(empty.forget());
> +    NS_ENSURE_SUCCESS(rv, rv);
> +  }
> +
> +  while (!mDelayedFrames.IsEmpty()) {

You're racing here with the runnable dispatched by Input(). That runnable will also be popping mDelayedFrames. If they run at the same time, bad things could happen. I suggest you dispatch a task to the queue that calls DecodeFrame() with a null sample and then pops/delivers everything from mDelayedFrames. Then you'll only be accessing mDelayedFrames on the decode task queue, which is synchronized for you.

@@ +258,5 @@
> +  return NS_OK;
> +}
> +
> +FFmpegH264Decoder::~FFmpegH264Decoder()
> +{

Please assert that mDelayedFrames is empty here. We don't want to leak.

Also you need to add a Flush() override that calls FFmpegDataDecoder::Flush() and then purges mDelayedFrames.

::: content/media/fmp4/ffmpeg/FFmpegH264Decoder.h
@@ +6,5 @@
> +
> +#ifndef __FFmpegH264Decoder_h__
> +#define __FFmpegH264Decoder_h__
> +
> +#include <queue>

You're not using std::queue here, so you don't need to #include it.

@@ +45,5 @@
> +  static int AllocateBufferCb(AVCodecContext *aCodecContext, AVFrame *aFrame);
> +
> +  mp4_demuxer::VideoDecoderConfig mConfig;
> +  MediaDataDecoderCallback *mCallback;
> +  ImageContainer *mImageContainer;

RefPtr<layers::ImageContainer>

Or nsRefPtr since you're mostly using that here.

Comment 10

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8377301 [details] [diff] [review]
Build changes for FFmpeg PlatformDecoderModule

Review of attachment 8377301 [details] [diff] [review]:
-----------------------------------------------------------------

::: configure.in
@@ +5563,5 @@
> +[  --enable-ffmpeg         Enable FFmpeg for fragmented H264/AAC decoding],
> +    MOZ_FFMPEG=1,
> +    MOZ_FFMPEG=
> +)
> +

I think you should turn this on by default, and make this a disable option.

You should also add to the conditional that enables MOZ_FMP4 when MOZ_WMF is enabled (line 5181), so that we'll enable MOZ_FMP4 if MOZ_FFMPEG is enabled, since we'll have a decoder that can handle FMP4 then too.

Comment 11

[cajbir (:cajbir)]

Comment on attachment 8377299 [details] [diff] [review]
GStreamer PlatformDecoderModule for Linux

Review of attachment 8377299 [details] [diff] [review]:
-----------------------------------------------------------------

I think you should get some form of security review to cast their eye over the loading of the dynamic libraries and make sure that is fine. r+ for the bits I looked at. No need to ask me for re-review when you've addressed cpearce's stuff.

::: content/media/fmp4/ffmpeg/FFmpegAACDecoder.cpp
@@ +32,5 @@
> +}
> +
> +static
> +AudioDataValue *
> +CopyAndPackAudio(AVFrame *aFrame, uint32_t aNumChannels, uint32_t aNumSamples)

Are the aNumChannels and aNumSamples arguments checked for valid ranges anywhere? They are multipled and used in memory copying so some sanity checking to prevent overflow should be done somewhere.

::: content/media/fmp4/ffmpeg/FFmpegH264Decoder.cpp
@@ +237,5 @@
> +    nsAutoPtr<MP4Sample> empty;
> +
> +    // An empty frame tells FFmpeg to decode the next delayed frame it has in
> +    // its queue, if it has any.
> +    empty = new MP4Sample(0 /* dts */,

Initialize on construction:

  nsAutoPtr<MP4Sample> empty = ...;

Comment 12

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

Comment on attachment 8377302 [details] [diff] [review]
FFmpeg headers for FFmpeg PlatformDecoderModule

(In reply to Chris Double (:doublec) from comment #8)
> Comment on attachment 8377302 [details] [diff] [review]
> FFmpeg headers for FFmpeg PlatformDecoderModule
> 
> Review of attachment 8377302 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Have you checked with legal to make sure that it's ok to use this code?
> Might want to get feedback from gerv if not. Add a README somewhere
> explaining where the code is from, and what version (or source code control
> commit id).

We already have LGPL code in media/libsoundtouch. And as we already distribute copies of the GPL and LGPL, it looks like we might only have to list FFmpeg under about:license according to §3 "Object Code Incorporating Material from Library Header Files":

>The object code form of an Application may incorporate material from a header file that is part of the Library. You may convey such object code under terms of your choice, provided that, if the incorporated material is not limited to numerical parameters, data structure layouts and accessors, or small macros, inline functions and templates (ten or fewer lines in length), you do both of the following:
>
>    a) Give prominent notice with each copy of the object code that the Library is used in it and that the Library and its use are covered by this License.
>    b) Accompany the object code with a copy of the GNU GPL and this license document.

Gerv, thoughts?

Comment 13

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

(In reply to Chris Pearce (:cpearce) from comment #9)
> It would be nice if we used the AudioCompactor here...

Not worth it. We won't be generating much slop, as we're dealing with one audio packet at a time, which means we're allocating close to (often exactly) whole page-size chunks of data at a time here.

Comment 14

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

Attachment: FFmpeg PlatformDecoderModule for Linux

Comment 15

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

Attachment: FFmpeg headers for FFmpeg PlatformDecoderModule

While FFmpeg function signatures tend not to change between versions of FFmpeg,
class layouts can change dramatically. We include libavcodec, libavformat,
and libavutil headers here so that we don't accidentally build against the wrong
binary interface.

Comment 16

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

Attachment: Fix errors in content/media/fmp4 on Linux with clang and --enable-warnings-as-errors

Comment 17

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

Attachment: Build changes for FFmpeg PlatformDecoderModule

Comment 18

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

Comment on attachment 8377991 [details] [diff] [review]
FFmpeg PlatformDecoderModule for Linux

Carry over r=doublec.

Comment 19

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

Comment on attachment 8377992 [details] [diff] [review]
FFmpeg headers for FFmpeg PlatformDecoderModule

Added README and an entry in about:license.

Carry over r=doublec.

Comment 20

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

Comment on attachment 8377993 [details] [diff] [review]
Fix errors in content/media/fmp4 on Linux with clang and --enable-warnings-as-errors

Addressed review comments. Carry over r=cpearce.

Comment 21

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

Attachment: Build changes for FFmpeg PlatformDecoderModule

Comment 22

[Gervase Markham [:gerv]]

Can someone summarise for me exactly what we are doing here, technically?

We can't ship GPLed code. We can ship LGPLed code if we:

A) Keep it in a shared library on its own, or only with other LGPLed code
B) Add the necessary incantations to about:license

Are we doing A)?

If we are just building with LGPLed headers, but the library itself is expected to be present on the Linux system, I'd have to do more investigation.

Gerv

Comment 23

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

(In reply to Gervase Markham [:gerv] from comment #22)
> If we are just building with LGPLed headers, but the library itself is
> expected to be present on the Linux system, I'd have to do more
> investigation.

This is the case here.

Comment 24

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8377991 [details] [diff] [review]
FFmpeg PlatformDecoderModule for Linux

Review of attachment 8377991 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good!

::: content/media/fmp4/ffmpeg/FFmpegH264Decoder.cpp
@@ +238,5 @@
> +  for (int32_t i = 0; i <= mCodecContext.max_b_frames; i++) {
> +    // An empty frame tells FFmpeg to decode the next delayed frame it has in
> +    // its queue, if it has any.
> +    nsAutoPtr<MP4Sample> empty(new MP4Sample(0 /* dts */, 0 /* cts */,
> +                                              0 /* duration */, 0 /* offset */,

Looks like the indentation is off by 1 space here.

Comment 25

[Gervase Markham [:gerv]]

I'm looking into this further; please don't commit it until I've finished. Thanks :-)

Gerv

Comment 26

[Gregory Szorc [:gps]]

Comment on attachment 8378032 [details] [diff] [review]
Build changes for FFmpeg PlatformDecoderModule

Review of attachment 8378032 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/media/fmp4/Makefile.in
@@ +9,5 @@
>  endif
> +
> +ifdef MOZ_FFMPEG
> +CFLAGS += -I$(srcdir)/ffmpeg/include
> +CXXFLAGS += -I$(srcdir)/ffmpeg/include

You should be using LOCAL_INCLUDES in a moz.build file.

Comment 27

[Mike Hommey [:glandium]]

Comment on attachment 8377991 [details] [diff] [review]
FFmpeg PlatformDecoderModule for Linux

Review of attachment 8377991 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/media/fmp4/ffmpeg/FFmpegRuntimeLinker.cpp
@@ +22,5 @@
> +FFmpegRuntimeLinker::LinkStatus FFmpegRuntimeLinker::sLinkStatus =
> +  LinkStatus_INIT;
> +
> +static const char * const sLibNames[] = {
> +  "libavcodec.so.53", "libavformat.so.53", "libavutil.so.51",

Current Debian testing has libavcodec.so.54, libavformat.so.54 and libavutil.so.52. Debian experimental has libavcodec.so.55, libavformat.so.55 and libavutil.so.53. Upcoming Ubuntu Trusty has the same as Debian testing. IOW, this is going to be outdated before even reaching beta.

Comment 28

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

(In reply to Mike Hommey [:glandium] from comment #27)
> Current Debian testing has libavcodec.so.54, libavformat.so.54 and
> libavutil.so.52. Debian experimental has libavcodec.so.55, libavformat.so.55
> and libavutil.so.53. Upcoming Ubuntu Trusty has the same as Debian testing.
> IOW, this is going to be outdated before even reaching beta.

I'm aware. I'll be working on more compatibility probably next week.

Comment 29

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

(In reply to Gervase Markham [:gerv] from comment #25)
> I'm looking into this further; please don't commit it until I've finished.
> Thanks :-)
> 
> Gerv

Any news here?

Comment 30

[Gervase Markham [:gerv]]

Our obligations depend on how much material from the headers we incorporate into our code. Is the following true of our use?

"[We use] only numerical parameters, data structure layouts and accessors, and small macros and small inline functions (ten lines or less in length)" from the headers."

If it is, things are simpler.

Gerv

Comment 31

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

(In reply to Gervase Markham [:gerv] from comment #30)
> Our obligations depend on how much material from the headers we incorporate
> into our code. Is the following true of our use?
> 
> "[We use] only numerical parameters, data structure layouts and accessors,
> and small macros and small inline functions (ten lines or less in length)"
> from the headers."

Yes.

Comment 32

[Gervase Markham [:gerv]]

If we are just building with LGPLed headers, but the library itself is expected to be present on the Linux system (so we are not shipping it), and if we use only numerical parameters, data structure layouts and accessors, and small macros and small inline functions (ten lines or less in length) from the headers, then we have no licensing obligations under the LGPL 2.1.

If we are including some headers in our tree for the build process, then you should put them in a separate directory, and include a COPYING file in the directory with a copy of the LGPL (whichever LGPL upstream uses).

Does that answer all remaining questions?

Gerv

Comment 33

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

(In reply to Gervase Markham [:gerv] from comment #32)
> If we are including some headers in our tree for the build process, then you
> should put them in a separate directory, and include a COPYING file in the
> directory with a copy of the LGPL (whichever LGPL upstream uses).

Awesome, will do.

> Does that answer all remaining questions?

Yep; thanks!

Comment 34

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

Attachment: Build changes for FFmpeg PlatformDecoderModule

Comment 35

[Mike Hommey [:glandium]]

Comment on attachment 8391968 [details] [diff] [review]
Build changes for FFmpeg PlatformDecoderModule

Review of attachment 8391968 [details] [diff] [review]:
-----------------------------------------------------------------

Please re-flag me when you answer the questions below.

::: configure.in
@@ +5235,5 @@
>  dnl ========================================================
> +dnl FFmpeg H264/AAC Decoding Support
> +dnl ========================================================
> +if test "$OS_ARCH" = "Linux"; then
> +  MOZ_FFMPEG=1

This is going to enable it on android and b2g. Is that expected?

@@ +5242,5 @@
> +MOZ_ARG_DISABLE_BOOL(ffmpeg,
> +[  --disable-ffmpeg         Disable FFmpeg for fragmented H264/AAC decoding],
> +    MOZ_FFMPEG=,
> +    MOZ_FFMPEG=1
> +)

Is a configure flag really wanted?

Comment 36

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

Attachment: ffmpeg-pdm-build.patch

Addressed $OS_ARCH vs $OS_TARGET

(In reply to Mike Hommey [:glandium] from comment #35)
> @@ +5242,5 @@
> > +MOZ_ARG_DISABLE_BOOL(ffmpeg,
> > +[  --disable-ffmpeg         Disable FFmpeg for fragmented H264/AAC decoding],
> > +    MOZ_FFMPEG=,
> > +    MOZ_FFMPEG=1
> > +)
> 
> Is a configure flag really wanted?

We may as well; even if we consider it okay to land enabled as is, a distro or something may disagree (especially given the lack of compatibility you commented on above).

Comment 37

[Mike Hommey [:glandium]]

Comment on attachment 8393922 [details] [diff] [review]
ffmpeg-pdm-build.patch

Review of attachment 8393922 [details] [diff] [review]:
-----------------------------------------------------------------

r+ on a purely technical point of view, but I'm still questioning the usefulness of enabling this feature in mozilla.org builds when most users won't have the libraries we're looking for installed.

Comment 38

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/56244a819a29
https://hg.mozilla.org/integration/mozilla-inbound/rev/ca38dab56641
https://hg.mozilla.org/integration/mozilla-inbound/rev/2df0b05cb911
https://hg.mozilla.org/integration/mozilla-inbound/rev/4cedbdf21ddd

Comment 39

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/56244a819a29
https://hg.mozilla.org/mozilla-central/rev/ca38dab56641
https://hg.mozilla.org/mozilla-central/rev/2df0b05cb911
https://hg.mozilla.org/mozilla-central/rev/4cedbdf21ddd