1211812 - Use MozPromise to init mozilla::GMPVideoDecoder

Status: RESOLVED FIXED

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

Description

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

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.

Comment 1

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

Attachment: Patch 1: Use MozPromise to init mozilla::GMPVideoDecoder

Use MozPromise to init mozilla::GMPVideoDecoder.

Comment 2

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

Attachment: Patch 2: Add prefs to toggle which GMP to use for unencrypted decoding

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.

Comment 3

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

Attachment: Patch 3: Test that gmp-clearkey can decode unencrypted media on Windows NT

Test that gmp-clearkey can decode on Windows Vista and later. We don't have a GMP that can decode on other platforms.

Comment 4

[JW Wang [:jwwang] [:jw_wang]]

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?

Comment 5

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

(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!

Comment 6

[JW Wang [:jwwang] [:jw_wang]]

(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.

Comment 7

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

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.

Comment 8

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=60b72c6ebf46

Comment 9

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

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

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/670449ecfd35
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b67cbe9ae75
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad252d40df2c

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/670449ecfd35
https://hg.mozilla.org/mozilla-central/rev/9b67cbe9ae75
https://hg.mozilla.org/mozilla-central/rev/ad252d40df2c