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

Status: RESOLVED FIXED

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

Description

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

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.

Comment 1

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

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

Comment 2

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

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

Comment 3

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

Attachment: MozReview Request: Bug 1276132 - Rename media.gmp-*.forcevisible to media.gmp-*.visible, and set it when keysystems are enabled. r?spohl

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/

Comment 4

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

Attachment: MozReview Request: Bug 1276132 - Remove use of #ifdef MOZ_{KEYSYSTEM}_EME in dom/media code. r=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/

Comment 5

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

Attachment: MozReview Request: Bug 1276132 - Don't show EME 'enable' UI for non-visible keysystems. r=gijs

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/

Comment 6

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

Attachment: MozReview Request: Bug 1276132 - Make more Widevine class constructors explicit to keep gcc happy. r=jwwang

Fixes compile errors on Linux when Widevine is compiled.

Review commit: https://reviewboard.mozilla.org/r/56454/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/56454/

Comment 7

[Stephen A Pohl [:spohl]]

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 8

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

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

Comment 9

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

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

Comment 10

[:Gijs (he/him)]

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");
```

Comment 11

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

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

Comment 12

[Masatoshi Kimura [:emk]]

Are we going to stop offering EME-free builds?

Comment 13

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

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/

Comment 14

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

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 15

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

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 16

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

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/

Comment 17

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

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

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

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?)

Comment 19

[Stephen A Pohl [:spohl]]

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

Comment 20

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

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

Comment 21

[Phil Ringnalda (:philor)]

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.

Comment 22

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

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

Comment 23

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

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

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a92a5799ecbf
https://hg.mozilla.org/mozilla-central/rev/066879c46800
https://hg.mozilla.org/mozilla-central/rev/ea6fcc76216c
https://hg.mozilla.org/mozilla-central/rev/dabfb32450b9

Comment 25

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

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

Comment 26

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

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

Comment 27

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

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

Comment 28

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

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

Comment 29

[Liz Henry (:lizzard) (relman/hg->git project)]

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?

Comment 30

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

(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

Comment 31

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

https://hg.mozilla.org/releases/mozilla-beta/rev/c2222ca05ad1
https://hg.mozilla.org/releases/mozilla-beta/rev/7b8879ab0e02
https://hg.mozilla.org/releases/mozilla-beta/rev/11b64780a7f5
https://hg.mozilla.org/releases/mozilla-beta/rev/1ac47043ac28

Comment 32

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

https://hg.mozilla.org/releases/mozilla-beta/rev/c2222ca05ad1
https://hg.mozilla.org/releases/mozilla-beta/rev/7b8879ab0e02
https://hg.mozilla.org/releases/mozilla-beta/rev/11b64780a7f5
https://hg.mozilla.org/releases/mozilla-beta/rev/1ac47043ac28