Closed Bug 1276132 Opened 3 years ago Closed 3 years ago

[EME] Compile in EME code by default, behind hidden prefs

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox49 --- fixed
firefox50 --- fixed

People

(Reporter: cpearce, Assigned: cpearce)

References

Details

Attachments

(4 files)

We want to make it easier for CDM developers (Adobe, Google) to test their CDMs without having to build Firefox themselves with their keysystem configured on in their build config. So we should compile in the Widevine and Adobe specific code by default, but make them hidden behind a hidden pref, so that the keysystems aren't visible by default, but can be preffed on.

The build config flags then will control turning on this pref.

That means, users who want to test Widevine don't need an official Mozilla Firefox build, they can just pref it on.

We need separate "visibility" and "enabled" flags, because for builds that don't have EME turned on in their mozconfig, we don't want the "click here to enable EME" UI to show.
Repurpose the media.gmp-*.forcevisible pref to control whether the
corresponding GMP is visible in the addons manager UI. The pref has to be true
for the GMP to be usable.

The pref is enabled and not hidden when the corresponding EME keysystem is
enabled in the mozconfig.

This means users can turn on EME without needing to recompile their build; they
just need to create a hidden pref. This will be useful for CDM developers, and
users on platforms where we've not enabled EME yet but users want to test it
(Linux).

Review commit: https://reviewboard.mozilla.org/r/56448/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/56448/
Attachment #8758073 - Flags: review?(spohl.mozilla.bugs)
Attachment #8758074 - Flags: review?(jwwang)
Attachment #8758075 - Flags: review?(gijskruitbosch+bugs)
Attachment #8758076 - Flags: review?(jwwang)
Instead of controlling visibility of EME keysystems by build config, do it by
preference. This means keysystems can be turned on easier.

Review commit: https://reviewboard.mozilla.org/r/56450/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/56450/
This means we'll only show the EME UI for keysystems that are explicitly turned
on in the build config, or those that are enabled after the build via prefs.

Review commit: https://reviewboard.mozilla.org/r/56452/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/56452/
Attachment #8758073 - Flags: review?(spohl.mozilla.bugs) → review+
Comment on attachment 8758073 [details]
MozReview Request: Bug 1276132 - Rename media.gmp-*.forcevisible to media.gmp-*.visible, and set it when keysystems are enabled. r?spohl

https://reviewboard.mozilla.org/r/56448/#review53008

::: toolkit/modules/GMPUtils.jsm:95
(Diff revision 1)
>  
>      return true;
>    },
>  
>    /**
>     * Checks whether or not a given plugin is forced visible. This can be used

This function comment should be updated as well.
Comment on attachment 8758074 [details]
MozReview Request: Bug 1276132 - Remove use of #ifdef MOZ_{KEYSYSTEM}_EME in dom/media code. r=jwwang

https://reviewboard.mozilla.org/r/56450/#review53032
Attachment #8758074 - Flags: review?(jwwang) → review+
Comment on attachment 8758076 [details]
MozReview Request: Bug 1276132 - Make more Widevine class constructors explicit to keep gcc happy. r=jwwang

https://reviewboard.mozilla.org/r/56454/#review53034
Attachment #8758076 - Flags: review?(jwwang) → review+
Comment on attachment 8758075 [details]
MozReview Request: Bug 1276132 - Don't show EME 'enable' UI for non-visible keysystems. r=gijs

https://reviewboard.mozilla.org/r/56452/#review53092

::: browser/base/content/browser-media.js:39
(Diff revision 1)
> +        Services.prefs.getPrefType("media.gmp-eme-adobe.visible") &&
> +        !Services.prefs.getBoolPref("media.gmp-eme-adobe.visible")) {
> +      return false;
> +    }
> +    if (keySystem == "com.widevine.alpha" &&
> +        Services.prefs.getPrefType("media.gmp-widevinecdm.visible") &&
> +        !Services.prefs.getBoolPref("media.gmp-widevinecdm.visible")) {

I don't like being annoying, but if we're changing all these prefs now, can we change them to be consistent/predictable? We're using "cdm" in one, and "eme" in the other, in different parts of the pref name... it's a bit inconsistent.

::: browser/base/content/browser-media.js:41
(Diff revision 1)
> +      return false;
> +    }
> +    if (keySystem.startsWith("com.adobe") &&
> +        Services.prefs.getPrefType("media.gmp-eme-adobe.visible") &&
> +        !Services.prefs.getBoolPref("media.gmp-eme-adobe.visible")) {
> +      return false;

Could simplify this and the next if statement to just:

```
if (keySystem.startsWith("com.adobe") &&
    Services.prefs.getPrefType("media.gmp-eme-adobe.visible")) {
  return Services.prefs.getBoolPref("media.gmp-eme-adobe.visible");
```
Attachment #8758075 - Flags: review?(gijskruitbosch+bugs) → review+
(In reply to :Gijs Kruitbosch from comment #10)
> Comment on attachment 8758075 [details]
> MozReview Request: Bug 1276132 - Don't show EME 'enable' UI for non-visible
> keysystems. r?gijs
> 
> https://reviewboard.mozilla.org/r/56452/#review53092
> 
> ::: browser/base/content/browser-media.js:39
> (Diff revision 1)
> > +        Services.prefs.getPrefType("media.gmp-eme-adobe.visible") &&
> > +        !Services.prefs.getBoolPref("media.gmp-eme-adobe.visible")) {
> > +      return false;
> > +    }
> > +    if (keySystem == "com.widevine.alpha" &&
> > +        Services.prefs.getPrefType("media.gmp-widevinecdm.visible") &&
> > +        !Services.prefs.getBoolPref("media.gmp-widevinecdm.visible")) {
> 
> I don't like being annoying, but if we're changing all these prefs now, can
> we change them to be consistent/predictable? We're using "cdm" in one, and
> "eme" in the other, in different parts of the pref name... it's a bit
> inconsistent.

Unfortunately no. Due to a quirk of the original GMP implementation, the name matters and is used in the path to the GMP. So we can't change it without rewriting a bunch of stuff relating to how the GMPs are stored on disk and loaded.
Are we going to stop offering EME-free builds?
Comment on attachment 8758073 [details]
MozReview Request: Bug 1276132 - Rename media.gmp-*.forcevisible to media.gmp-*.visible, and set it when keysystems are enabled. r?spohl

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56448/diff/1-2/
Attachment #8758074 - Attachment description: MozReview Request: Bug 1276132 - Remove use of #ifdef MOZ_{KEYSYSTEM}_EME in dom/media code. r?jwwang → MozReview Request: Bug 1276132 - Remove use of #ifdef MOZ_{KEYSYSTEM}_EME in dom/media code. r=jwwang
Attachment #8758075 - Attachment description: MozReview Request: Bug 1276132 - Don't show EME 'enable' UI for non-visible keysystems. r?gijs → MozReview Request: Bug 1276132 - Don't show EME 'enable' UI for non-visible keysystems. r=gijs
Attachment #8758076 - Attachment description: MozReview Request: Bug 1276132 - Make more Widevine class constructors explicit to keep gcc happy. r?jwwang → MozReview Request: Bug 1276132 - Make more Widevine class constructors explicit to keep gcc happy. r=jwwang
Comment on attachment 8758074 [details]
MozReview Request: Bug 1276132 - Remove use of #ifdef MOZ_{KEYSYSTEM}_EME in dom/media code. r=jwwang

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56450/diff/1-2/
Comment on attachment 8758075 [details]
MozReview Request: Bug 1276132 - Don't show EME 'enable' UI for non-visible keysystems. r=gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56452/diff/1-2/
Comment on attachment 8758076 [details]
MozReview Request: Bug 1276132 - Make more Widevine class constructors explicit to keep gcc happy. r=jwwang

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56454/diff/1-2/
(In reply to Masatoshi Kimura [:emk] from comment #12)
> Are we going to stop offering EME-free builds?

No. Behaviour in the EME-free repacks EME will not change. EME will still be off by default and the user will not be prompted to enable it should they visit a site that tries to use EME.
Comment on attachment 8758073 [details]
MozReview Request: Bug 1276132 - Rename media.gmp-*.forcevisible to media.gmp-*.visible, and set it when keysystems are enabled. r?spohl

Re-requesting review on this, since I've changed it enough that I think it warrants another look.

(I thought mozreview would have requested review again... must have gotten lost in the post somehow?)
Attachment #8758073 - Flags: review+ → review?(spohl.mozilla.bugs)
Comment on attachment 8758073 [details]
MozReview Request: Bug 1276132 - Rename media.gmp-*.forcevisible to media.gmp-*.visible, and set it when keysystems are enabled. r?spohl

https://reviewboard.mozilla.org/r/56448/#review54456
Attachment #8758073 - Flags: review?(spohl.mozilla.bugs) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/69052d4e3dbb457219dad902a1ff9f1d98300c57
Bug 1276132 - Rename media.gmp-*.forcevisible to media.gmp-*.visible, and set it when keysystems are enabled. r=spohl

https://hg.mozilla.org/integration/mozilla-inbound/rev/09b9972e36f9fb309a1f86918beeae26d1888aa6
Bug 1276132 - Remove use of #ifdef MOZ_{KEYSYSTEM}_EME in dom/media code. r=jwwang

https://hg.mozilla.org/integration/mozilla-inbound/rev/91b3cdd0640a29eefc8619bf06e431dafe22c6ed
Bug 1276132 - Don't show EME 'enable' UI for non-visible keysystems. r=gijs

https://hg.mozilla.org/integration/mozilla-inbound/rev/511a2389ca48a403386aeb8f0025f26bab1e882e
Bug 1276132 - Make more Widevine class constructors explicit to keep gcc happy. r=jwwang
(In reply to Phil Ringnalda (:philor) from comment #21)
> Backed out in
> https://hg.mozilla.org/integration/mozilla-inbound/rev/
> 75f65d79283ffe8fbfa797d62e64d7b797d20f6c for
> https://treeherder.mozilla.org/logviewer.html#?job_id=29841174&repo=mozilla-
> inbound etc.

 01:37:38     INFO -  563 INFO TEST-UNEXPECTED-FAIL | toolkit/mozapps/extensions/test/browser/browser_gmpProvider.js | Got add-on element:gmp-eme-adobe - null == true - JS frame :: chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser/browser_gmpProvider.js :: testNotInstalledDisabled :: line 141


This is because I forgot to add KEY_PLUGIN_FORCE_SUPPORTED pref sets in the places where FORCE_VISIBLE used to be in this test.
https://hg.mozilla.org/integration/mozilla-inbound/rev/a92a5799ecbfd9088472607c07626831de6996b0
Bug 1276132 - Rename media.gmp-*.forcevisible to media.gmp-*.visible, and set it when keysystems are enabled. r=spohl

https://hg.mozilla.org/integration/mozilla-inbound/rev/066879c46800ff0aae008d2fbf3981f1e2fe309c
Bug 1276132 - Remove use of #ifdef MOZ_{KEYSYSTEM}_EME in dom/media code. r=jwwang

https://hg.mozilla.org/integration/mozilla-inbound/rev/ea6fcc76216c86e1d7a5fa2f53ca5d37bcb647c8
Bug 1276132 - Don't show EME 'enable' UI for non-visible keysystems. r=gijs

https://hg.mozilla.org/integration/mozilla-inbound/rev/dabfb32450b9602b1fb9df53c33f9eddb26e0d60
Bug 1276132 - Make more Widevine class constructors explicit to keep gcc happy. r=jwwang
Depends on: 1283630
Comment on attachment 8758073 [details]
MozReview Request: Bug 1276132 - Rename media.gmp-*.forcevisible to media.gmp-*.visible, and set it when keysystems are enabled. r?spohl

Approval Request Comment
[Feature/regressing bug #]: Widevine EME on Linux
[User impact if declined]: This patch changes how our build config options an prefs affect EME. Without this, it's not easy for users and Linux distros to turn off/on Widevein.
[Describe test coverage new/current, TreeHerder]: We have plenty of EME mochitests, which run on Linux.
[Risks and why]: Low; this is tweaking how we build EME code, and it's been riding the trains for a while.
[String/UUID change made/needed]: None
Attachment #8758073 - Flags: approval-mozilla-beta?
Comment on attachment 8758074 [details]
MozReview Request: Bug 1276132 - Remove use of #ifdef MOZ_{KEYSYSTEM}_EME in dom/media code. r=jwwang

Approval Request Comment
[Feature/regressing bug #]: Widevine EME on Linux
[User impact if declined]: This patch changes how our build config options an prefs affect EME. Without this, it's not easy for users and Linux distros to turn off/on Widevein.
[Describe test coverage new/current, TreeHerder]: We have plenty of EME mochitests, which run on Linux.
[Risks and why]: Low; this is tweaking how we build EME code, and it's been riding the trains for a while.
[String/UUID change made/needed]: None
Attachment #8758074 - Flags: approval-mozilla-beta?
Comment on attachment 8758075 [details]
MozReview Request: Bug 1276132 - Don't show EME 'enable' UI for non-visible keysystems. r=gijs

Approval Request Comment
[Feature/regressing bug #]: Widevine EME on Linux
[User impact if declined]: This patch changes how our build config options an prefs affect EME. Without this, it's not easy for users and Linux distros to turn off/on Widevein.
[Describe test coverage new/current, TreeHerder]: We have plenty of EME mochitests, which run on Linux.
[Risks and why]: Low; this is tweaking how we build EME code, and it's been riding the trains for a while.
[String/UUID change made/needed]: None
Attachment #8758075 - Flags: approval-mozilla-beta?
Comment on attachment 8758076 [details]
MozReview Request: Bug 1276132 - Make more Widevine class constructors explicit to keep gcc happy. r=jwwang

Approval Request Comment
[Feature/regressing bug #]: Widevine EME on Linux
[User impact if declined]: This patch changes how our build config options an prefs affect EME. Without this, it's not easy for users and Linux distros to turn off/on Widevein.
[Describe test coverage new/current, TreeHerder]: We have plenty of EME mochitests, which run on Linux.
[Risks and why]: Low; this is tweaking how we build EME code, and it's been riding the trains for a while.
[String/UUID change made/needed]: None
Attachment #8758076 - Flags: approval-mozilla-beta?
Comment on attachment 8758073 [details]
MozReview Request: Bug 1276132 - Rename media.gmp-*.forcevisible to media.gmp-*.visible, and set it when keysystems are enabled. r?spohl

Tweaks for the build for Widevine support. OK for beta uplift.
Chris, do you have extra plans to test the build options?
Flags: needinfo?(cpearce)
Attachment #8758073 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8758074 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8758075 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #29)
> Comment on attachment 8758073 [details]
> MozReview Request: Bug 1276132 - Rename media.gmp-*.forcevisible to
> media.gmp-*.visible, and set it when keysystems are enabled. r?spohl
> 
> Tweaks for the build for Widevine support. OK for beta uplift.
> Chris, do you have extra plans to test the build options?

I'm going to wait for a try push with these patches in to ensure they work before landing. These patches are in 50, so I'm pretty sure they'll work. I just want to be sure before landing.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5a6015957ee3&selectedJob=25428221
Flags: needinfo?(cpearce)
Attachment #8758076 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Depends on: 1296627
You need to log in before you can comment on or make changes to this bug.