Closed Bug 1211812 Opened 4 years ago Closed 4 years ago

Use MozPromise to init mozilla::GMPVideoDecoder

Categories

(Core :: Audio/Video: Playback, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: cpearce, Assigned: cpearce)

References

Details

Attachments

(3 files)

We have a dispatch and spin on the GMP thread in mozilla::GMP{Audio,Video}Decoder::Init(). Since MediaDataDecoder::Init() now returns a promise, we don't need this to be synchronous and spin the event loop anymore.
Use MozPromise to init mozilla::GMPVideoDecoder.
Attachment #8670136 - Flags: review?(jwwang)
Prefs to toggle between gmp-clearkey and gmp-eme-adobe for unencrypted <video> decoding. To use OpenH264, don't set a pref for h.264, as it's added to the GMPService first.

(We don't have a way to distinguish OpenH264, as it doesn't include a distinctive tag in its .info file)

Note this is based on top of bug 1206977.
Attachment #8670138 - Flags: review?(jwwang)
Test that gmp-clearkey can decode on Windows Vista and later. We don't have a GMP that can decode on other platforms.
Attachment #8670139 - Flags: review?(jwwang)
Attachment #8670136 - Flags: review?(jwwang) → review+
Comment on attachment 8670138 [details] [diff] [review]
Patch 2: Add prefs to toggle which GMP to use for unencrypted decoding

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

::: browser/app/profile/firefox.js
@@ +1826,5 @@
>  pref("media.eme.apiVisible", true);
>  
> +// If decoding-via-gmp is turned on for <video>, default to using
> +// Adobe's GMP for decoding.
> +pref("media.fragmented-mp4.gmp.aac", 2);

The magic number is hard to comprehend. Can we use strings like "org.w3.clearkey" or "com.adobe.primetime"?

::: dom/media/platforms/agnostic/gmp/GMPDecoderModule.cpp
@@ +93,5 @@
> +/* static */
> +void
> +GMPDecoderModule::Init()
> +{
> +  Preferences::AddUintVarCache(&sPreferredAacGmp,

Can it simply be |Preferences::GetUint|? Or do we want it to take effect as soon as the pref value changes?
Attachment #8670138 - Flags: review?(jwwang) → review+
Attachment #8670139 - Flags: review?(jwwang) → review+
(In reply to JW Wang [:jwwang] from comment #4)
> Comment on attachment 8670138 [details] [diff] [review]
> Patch 2: Add prefs to toggle which GMP to use for unencrypted decoding
> 
> Review of attachment 8670138 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/app/profile/firefox.js
> @@ +1826,5 @@
> >  pref("media.eme.apiVisible", true);
> >  
> > +// If decoding-via-gmp is turned on for <video>, default to using
> > +// Adobe's GMP for decoding.
> > +pref("media.fragmented-mp4.gmp.aac", 2);
> 
> The magic number is hard to comprehend. Can we use strings like
> "org.w3.clearkey" or "com.adobe.primetime"?

I would prefer to avoid making it easy to use GMPs as an extension point for media; we want to closely control what codecs are used, to avoid proliferation. And numbers are harder to misspell.

 
> ::: dom/media/platforms/agnostic/gmp/GMPDecoderModule.cpp
> @@ +93,5 @@
> > +/* static */
> > +void
> > +GMPDecoderModule::Init()
> > +{
> > +  Preferences::AddUintVarCache(&sPreferredAacGmp,
> 
> Can it simply be |Preferences::GetUint|? Or do we want it to take effect as
> soon as the pref value changes?

The pref has to be readable off the main thread, so we can't call Preferences::GetUint on use. Pref caches are cheap, so I don't see why not use them. They're more convenient for testing, that's for sure!
(In reply to Chris Pearce (:cpearce) from comment #5)
> The pref has to be readable off the main thread, so we can't call
> Preferences::GetUint on use. Pref caches are cheap, so I don't see why not
> use them. They're more convenient for testing, that's for sure!

See bug 1212220 for the concern about data race. We can store the pref value as a member or a static variable so it is safe to read it off the main thread. However, it is less convenient for you have to reboot for the pref changes to take effect.
Tests fail:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=030d0f2a84b6

Problem here is that GMP{Audio,Video}Decoder::InitTags() is called by EME{Audio,Video}Decoder::InitTags(), and the base InitTags() is specifying Adobe's GMP by default, breaking the clearkey EME*Decoder. I'll need to change EME{Audio,Video}Decoder::InitTags() to not call the base.
You need to log in before you can comment on or make changes to this bug.