PlatformDecoderModule for Linux

RESOLVED FIXED in mozilla31

Status

()

Core
Audio/Video
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: cpearce, Assigned: eflores)

Tracking

(Depends on: 1 bug)

unspecified
mozilla31
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 8 obsolete attachments)

38.98 KB, patch
cpearce
: review+
Details | Diff | Splinter Review
528.08 KB, patch
eflores
: review+
Details | Diff | Splinter Review
6.34 KB, patch
eflores
: review+
Details | Diff | Splinter Review
3.08 KB, patch
glandium
: review+
Details | Diff | Splinter Review
(Reporter)

Description

4 years ago
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.
(Reporter)

Updated

4 years ago
Depends on: 962385
Created attachment 8377298 [details] [diff] [review]
GStreamer PlatformDecoderModule for Linux
Assignee: nobody → edwin
Status: NEW → ASSIGNED
Created attachment 8377299 [details] [diff] [review]
GStreamer PlatformDecoderModule for Linux
Attachment #8377298 - Attachment is obsolete: true
Created attachment 8377300 [details] [diff] [review]
Fix errors in content/media/fmp4 on Linux with clang and --enable-warnings-as-errors
Created attachment 8377301 [details] [diff] [review]
Build changes for FFmpeg PlatformDecoderModule
Created attachment 8377302 [details] [diff] [review]
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.
Attachment #8377299 - Flags: review?(cpearce)
Attachment #8377299 - Flags: review?(chris.double)
Attachment #8377300 - Flags: review?(cpearce)
Attachment #8377301 - Flags: review?(gps)
Attachment #8377302 - Flags: review?(chris.double)
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!)
(Reporter)

Comment 7

4 years ago
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.
Attachment #8377300 - Flags: review?(cpearce) → review+

Comment 8

4 years ago
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).
Attachment #8377302 - Flags: review?(chris.double) → review+
(Reporter)

Comment 9

4 years ago
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.
Attachment #8377299 - Flags: review?(cpearce) → review-
(Reporter)

Comment 10

4 years ago
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

4 years ago
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 = ...;
Attachment #8377299 - Flags: review?(chris.double) → review+
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?
Attachment #8377302 - Flags: feedback?(gerv)
(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.
Created attachment 8377991 [details] [diff] [review]
FFmpeg PlatformDecoderModule for Linux
Attachment #8377299 - Attachment is obsolete: true
Created attachment 8377992 [details] [diff] [review]
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.
Attachment #8377302 - Attachment is obsolete: true
Attachment #8377302 - Flags: feedback?(gerv)
Created attachment 8377993 [details] [diff] [review]
Fix errors in content/media/fmp4 on Linux with clang and --enable-warnings-as-errors
Attachment #8377300 - Attachment is obsolete: true
Created attachment 8377994 [details] [diff] [review]
Build changes for FFmpeg PlatformDecoderModule
Attachment #8377301 - Attachment is obsolete: true
Attachment #8377301 - Flags: review?(gps)
Comment on attachment 8377991 [details] [diff] [review]
FFmpeg PlatformDecoderModule for Linux

Carry over r=doublec.
Attachment #8377991 - Flags: review?(cpearce)
Comment on attachment 8377992 [details] [diff] [review]
FFmpeg headers for FFmpeg PlatformDecoderModule

Added README and an entry in about:license.

Carry over r=doublec.
Attachment #8377992 - Flags: review+
Attachment #8377992 - Flags: feedback?(gerv)
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.
Attachment #8377993 - Flags: review+
Attachment #8377994 - Flags: review?(gps)
Created attachment 8378032 [details] [diff] [review]
Build changes for FFmpeg PlatformDecoderModule
Attachment #8377994 - Attachment is obsolete: true
Attachment #8377994 - Flags: review?(gps)
Attachment #8378032 - Flags: review?(gps)
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
(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.
(Reporter)

Comment 24

4 years ago
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.
Attachment #8377991 - Flags: review?(cpearce) → review+
I'm looking into this further; please don't commit it until I've finished. Thanks :-)

Gerv
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.
Attachment #8378032 - Flags: review?(gps)
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.
(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.
(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?
Flags: needinfo?(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
Flags: needinfo?(gerv)
(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.
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
(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!
Attachment #8377992 - Flags: feedback?(gerv)
Created attachment 8391968 [details] [diff] [review]
Build changes for FFmpeg PlatformDecoderModule
Attachment #8378032 - Attachment is obsolete: true
Attachment #8391968 - Flags: review?(mh+mozilla)
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?
Attachment #8391968 - Flags: review?(mh+mozilla)
Created attachment 8393922 [details] [diff] [review]
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).
Attachment #8391968 - Attachment is obsolete: true
Attachment #8393922 - Flags: review?(mh+mozilla)
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.
Attachment #8393922 - Flags: review?(mh+mozilla) → review+
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
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
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Depends on: 986786
Depends on: 986820

Updated

4 years ago
Blocks: 987773
You need to log in before you can comment on or make changes to this bug.