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)
Core
Audio/Video
Tracking
()
RESOLVED
FIXED
mozilla38
People
(Reporter: kinetik, Assigned: kinetik)
References
(Depends on 1 open bug)
Details
Attachments
(2 files, 2 obsolete files)
38.91 KB,
patch
|
cpearce
:
review+
|
Details | Diff | Splinter Review |
39.15 KB,
patch
|
lmandel
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•9 years ago
|
||
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
Assignee | ||
Comment 2•9 years ago
|
||
Attachment #8548529 -
Attachment is obsolete: true
Assignee | ||
Comment 3•9 years ago
|
||
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.
Assignee | ||
Comment 4•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=838bd9bc4b35
Assignee | ||
Comment 5•9 years ago
|
||
OpenH264 GMP changes submitted upstream after cleaning them up and rebasing: https://github.com/cisco/openh264/pull/1746
Assignee | ||
Updated•9 years ago
|
Attachment #8552164 -
Flags: review?(cpearce)
Comment 6•9 years ago
|
||
Sorry for the review delay, I'm prioritizing MSE blockers. I should get to this next week.
Comment 7•9 years ago
|
||
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 8•9 years ago
|
||
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-
Assignee | ||
Comment 9•9 years ago
|
||
(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.
Assignee | ||
Comment 10•9 years ago
|
||
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
Assignee | ||
Updated•9 years ago
|
Attachment #8558329 -
Flags: review?(cpearce)
Assignee | ||
Comment 11•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d30c6d816455
Updated•9 years ago
|
Attachment #8558329 -
Flags: review?(cpearce) → review+
Assignee | ||
Comment 12•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/06e956a91995
Comment 13•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/06e956a91995
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox38:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Comment 14•9 years ago
|
||
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?
Comment 15•9 years ago
|
||
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
Comment 16•9 years ago
|
||
my gcc version was 4.6.4 The following might be related. http://forums.codeguru.com/showthread.php?521615-Using-constructor-in-constructor
Comment 17•9 years ago
|
||
From the following, Delegated constructors seems not permitted yet in gecko. https://developer.mozilla.org/en-US/docs/Using_CXX_in_Mozilla_code
Assignee | ||
Comment 18•9 years ago
|
||
(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.
Updated•9 years ago
|
Blocks: eme-platform-uplift
Comment 19•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-beta/rev/f2e35a9f30a7
status-firefox37:
--- → fixed
Comment 20•9 years ago
|
||
Patch for beta branch as part of EME platform uplift.
Comment 21•9 years ago
|
||
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 22•9 years ago
|
||
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+
Comment 23•9 years ago
|
||
(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)
Comment 24•9 years ago
|
||
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.
Comment 25•9 years ago
|
||
I'm happy to land the patch in bug 1134487 and have it uplifted... Can you take point on that?
Flags: needinfo?(cpearce)
Updated•9 years ago
|
Flags: needinfo?(bobowen.code)
Comment 26•9 years ago
|
||
(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.
Description
•