Closed Bug 1121258 Opened 9 years ago Closed 9 years ago

Add a GMP PDM to allow MP4 playback via OpenH264

Categories

(Core :: Audio/Video, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox37 --- fixed
firefox38 --- fixed

People

(Reporter: kinetik, Assigned: kinetik)

References

(Depends on 1 open bug)

Details

Attachments

(2 files, 2 obsolete files)

Adding a PDM that uses the GMP API for video decoding allows the use of OpenH264 to decode at least baseline H.264 content on devices that have no other H.264 decoding option.  Much of the code to support this already exists in the EME backend, so there's some refactoring work to be done to share the code that makes sense to share.
Attached patch wip v0 (obsolete) — Splinter Review
This works as is, but I've got a clean up pass to do before requesting review.

To test, you'll need a build of the OpenH264 GMP with several bugs fixed, my fork is here: https://github.com/kinetiknz/openh264/tree/gmp-fixes.  I'll submit these fixes upstream after I've cleaned them up slightly.

If you want to test with AAC, I wrote a demo GMP based on FAAD2 to learn the GMP API, that's available here: https://github.com/kinetiknz/gmpfaad2aac
Attached patch patch v0 (obsolete) — Splinter Review
Attachment #8548529 - Attachment is obsolete: true
Comment on attachment 8552164 [details] [diff] [review]
patch v0

This ends up duplicating a lot of code with the EME decoders, so I've split it out in such a way that it can easily be shared with the EME code.  I'll spin off another bug for the EME deduplication.

Aside from that, this adds a new method to the decoder callback, FlushComplete (with a dummy default implementation), since there was no existing way to signal a completed async flush and the EME decoder had constructed an ad-hoc way to handle it.

Also adds a simple VideoInfo(width, height) constructor to make some code slightly nicer.
OpenH264 GMP changes submitted upstream after cleaning them up and rebasing: https://github.com/cisco/openh264/pull/1746
Attachment #8552164 - Flags: review?(cpearce)
Depends on: 1124542
Sorry for the review delay, I'm prioritizing MSE blockers. I should get to this next week.
Comment on attachment 8552164 [details] [diff] [review]
patch v0

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

Overall very good, but I don't like adding something internal to the GMP PDM to the public MediaDataDecoderCallback interface (FlushComplete).

::: dom/media/fmp4/PlatformDecoderModule.h
@@ +170,5 @@
>    virtual void NotifyResourcesStatusChanged() {};
>  
>    virtual void ReleaseMediaResources() {};
> +
> +  virtual void FlushComplete() {};

PlatformDecoderModule.h defines the interface for MP4Reader to interact with decoders. But MP4Reader doesn't use FlushComplete().

Adding a public FlushComplete() method gives the impression that the FlushComplete() is required.

Making MediaDataDecoder::Flush() async, with a FlushComplete() callback isn't a bad idea, we use that pattern for other operations like Drain(). So please either remove FlushComplete() from the interface here, or change MP4Reader to rely on FlushComplete(), and make the other MediaDataDecoders call it.

::: dom/media/fmp4/gmp/GMPDecoderModule.cpp
@@ +123,5 @@
> +  virtual void Output(MediaData* aData) {
> +    mProxyCallback->Output(aData);
> +  }
> +
> +  virtual void Error();

MOZ_OVERRIDE on virtual methods according to style guide...

@@ +403,5 @@
> +{
> +  MOZ_ASSERT(IsOnGMPThread());
> +
> +  NS_WARNING("H.264 GMP decoder terminated.");
> +  mCallback->Error();

You still need to call Close() on your GMPVideoDecoder in your Terminated() handler, else you'll leak the GMPVideoDecoder. Ditto for the GMPAudio* path.
Attachment #8552164 - Flags: review?(cpearce) → review-
Comment on attachment 8552164 [details] [diff] [review]
patch v0

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

::: dom/media/fmp4/gmp/GMPDecoderModule.cpp
@@ +333,5 @@
> +    if (i == kGMPYPlane) {
> +      b.mPlanes[i].mWidth = decodedFrame->Width();
> +      b.mPlanes[i].mHeight = decodedFrame->Height();
> +    } else {
> +      b.mPlanes[i].mWidth = decodedFrame->Width() / 2;

(width+1)/2 !  Ditto for height
Attachment #8552164 - Flags: review-
Blocks: 1128794
(In reply to Chris Pearce (:cpearce) from comment #7)
> Overall very good, but I don't like adding something internal to the GMP PDM
> to the public MediaDataDecoderCallback interface (FlushComplete).

I thought that change might be contentious.  My argument is that we've got callbacks for completion of other async events, but not this one and it just happens that this is the first PDM to need an async flush. It could be that it's the only one that'll ever need it, in which case it might be a mistake to add it...

> PlatformDecoderModule.h defines the interface for MP4Reader to interact with
> decoders. But MP4Reader doesn't use FlushComplete().
> 
> Adding a public FlushComplete() method gives the impression that the
> FlushComplete() is required.
> 
> Making MediaDataDecoder::Flush() async, with a FlushComplete() callback
> isn't a bad idea, we use that pattern for other operations like Drain(). So
> please either remove FlushComplete() from the interface here, or change
> MP4Reader to rely on FlushComplete(), and make the other MediaDataDecoders
> call it.

Having briefly looked at what's involved, I'll leave this to another bug.  The required changes are large enough to warrant a new bug and careful testing.  I'll make the necessary changes to this patch to make FlushComplete internal for now.  Filed bug 1128827 for this.

> MOZ_OVERRIDE on virtual methods according to style guide...

Fixed, thanks.

> You still need to call Close() on your GMPVideoDecoder in your Terminated()
> handler, else you'll leak the GMPVideoDecoder. Ditto for the GMPAudio* path.

It already does, via:

{Audio,Video}CallbackAdapter::Terminated forwards to MediaDataDecoderCallback::Error, the callback passed in to GMP{Audio,Video}Decoder is wrapped with the MediaDataDecoderProxy's MediaDataDecoderCallbackProxy, in which the Error implementation forwards the error to the wrapped callback and then calls mProxyDecoder->Shutdown().

Note that this is slightly different to the EME code, which calls GmpShutdown directly.  Here we call Shutdown, which posts an event to the GMP thread to call shutdown on the GMP{Audio,Video}Decoder.

(In reply to Randell Jesup [:jesup] from comment #8)
> (width+1)/2 !  Ditto for height

Good catch, thanks.  That bug exists in the current EMEH264VIdeoDecoder too, but they'll share this code after this bug and bug 1128794 lands.
Aside from review changes, this adds the mIsShutdown assert back that I missed from the EME -> GMP code movement and fixes an A/V sync bug caused by a lost reset of mMustRecaptureAudioPosition during the refactoring.  It also splits the classes in GMPDecoderModule.cpp out into separate files to make the EME refactoring patch easier to understand.
Attachment #8552164 - Attachment is obsolete: true
Attachment #8558329 - Flags: review?(cpearce)
Attachment #8558329 - Flags: review?(cpearce) → review+
https://hg.mozilla.org/mozilla-central/rev/06e956a91995
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
This patch causes this build error for me:

1:45.69 In file included from ../../dist/include/AbstractMediaDecoder.h:11:0,
 1:45.69                  from ../../dist/include/MediaDecoderOwner.h:8,
 1:45.69                  from ../../dist/include/mozilla/dom/HTMLMediaElement.h:11,
 1:45.69                  from ../../dist/include/mozilla/dom/HTMLVideoElement.h:11,
 1:45.69                  from /home/jgriffin/mozilla-inbound/src/layout/generic/nsVideoFrame.cpp:14,
 1:45.69                  from /home/jgriffin/mozilla-inbound/src/obj-x86_64-unknown-linux-gnu/layout/generic/Unified_cpp_layout_generic3.cpp:29:
 1:45.69 ../../dist/include/MediaInfo.h: In constructor ‘mozilla::VideoInfo::VideoInfo()’:
 1:45.69 ../../dist/include/MediaInfo.h:50:7: error: type ‘mozilla::VideoInfo’ is not a direct base of ‘mozilla::VideoInfo’

Any ideas why?
I also see similar errors.


 0:08.33                  from /tmp/mozilla-central/obj-x86_64-unknown-linux-gnu/dom/media/webm/Unified_cpp_dom_media_webm0.cpp:11:
 0:08.33 ../../../dist/include/MediaInfo.h: In constructor ‘mozilla::VideoInfo::VideoInfo()’:
 0:08.33 ../../../dist/include/MediaInfo.h:50:7: error: type ‘mozilla::VideoInfo’ is not a direct base of ‘mozilla::VideoInfo’
 0:08.33 ../../../dist/include/MediaInfo.h: In constructor ‘mozilla::VideoInfo::VideoInfo(int32_t, int32_t)’:
 0:08.33 ../../../dist/include/MediaInfo.h:59:7: error: type ‘mozilla::VideoInfo’ is not a direct base of ‘mozilla::VideoInfo’
 0:10.42 libdom_time.a.desc
 0:10.55 Voicemail.o
 0:10.56 VoicemailIPCService.o
 0:12.48 In the directory  /tmp/mozilla-central/obj-x86_64-unknown-linux-gnu/dom/media/webm

------------------------------------------

 0:12.99 In file included from ../../dist/include/AbstractMediaDecoder.h:11:0,
 0:12.99                  from ../../dist/include/MediaDecoderOwner.h:8,
 0:12.99                  from ../../dist/include/mozilla/dom/HTMLMediaElement.h:11,
 0:12.99                  from ../../dist/include/mozilla/dom/HTMLVideoElement.h:11,
 0:12.99                  from /tmp/mozilla-central/layout/generic/nsVideoFrame.cpp:14,
 0:12.99                  from /tmp/mozilla-central/obj-x86_64-unknown-linux-gnu/layout/generic/Unified_cpp_layout_generic3.cpp:29:
 0:12.99 ../../dist/include/MediaInfo.h: In constructor ‘mozilla::VideoInfo::VideoInfo()’:
 0:12.99 ../../dist/include/MediaInfo.h:50:7: error: type ‘mozilla::VideoInfo’ is not a direct base of ‘mozilla::VideoInfo’
 0:12.99 ../../dist/include/MediaInfo.h: In constructor ‘mozilla::VideoInfo::VideoInfo(int32_t, int32_t)’:
 0:12.99 ../../dist/include/MediaInfo.h:59:7: error: type ‘mozilla::VideoInfo’ is not a direct base of ‘mozilla::VideoInfo’
 0:14.11 libdom_animation.a.desc
 0:14.15 Unified_cpp_dom_voicemail0.o
 0:15.16 In the directory  /tmp/mozilla-central/obj-x86_64-unknown-linux-gnu/layout/generic
From the following, Delegated constructors seems not permitted yet in gecko.

https://developer.mozilla.org/en-US/docs/Using_CXX_in_Mozilla_code
(In reply to Sotaro Ikeda [:sotaro] from comment #17)
> From the following, Delegated constructors seems not permitted yet in gecko.

Will fix in bug 1131340.
Depends on: 1131340
Depends on: 1132797
Attached patch Beta patchSplinter Review
Patch for beta branch as part of EME platform uplift.
Comment on attachment 8572346 [details] [diff] [review]
Beta patch

Requesting retroactive approval for Beta landing as part of EME platform uplift.
Attachment #8572346 - Flags: approval-mozilla-beta?
Comment on attachment 8572346 [details] [diff] [review]
Beta patch

Approved for Beta as part of EME platform uplift.
Attachment #8572346 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(In reply to Chris Pearce (:cpearce) from comment #20)
> Created attachment 8572346 [details] [diff] [review]
> Beta patch
> 
> Patch for beta branch as part of EME platform uplift.

I think this uplift will have broken building with gcc-4.6 on Beta.
I just realised this after posting to dev-platform over dropping support for 4.6 for Fx38+.
We might want to land and uplift the patch on bug 1134487, so as to not break this so close to release.
Flags: needinfo?(cpearce)
Um... So we don't support delegated constructors in gecko 37 but we do in gecko 38, is that right?

I'd like to keep living in the present if possible, W.R.T. compiler features. So I guess we can we land the patch in bug 1134487, uplift, and then back it out on m-c some time later? I assume landing only on beta is not going to fly.
I'm happy to land the patch in bug 1134487 and have it uplifted... Can you take point on that?
Flags: needinfo?(cpearce)
Flags: needinfo?(bobowen.code)
(In reply to Chris Pearce (:cpearce) from comment #25)
> I'm happy to land the patch in bug 1134487 and have it uplifted... Can you
> take point on that?

Assuming that "take point" means landing and anything up to getting metaphorically shot at if something goes wrong, no problem.
Literally shot at, maybe not.
Flags: needinfo?(bobowen.code)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: