Closed Bug 1160101 Opened 9 years ago Closed 9 years ago

[EME] Pref off Adobe EME in non-official builds

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox39 --- fixed
firefox40 --- fixed
firefox41 --- fixed

People

(Reporter: cpearce, Assigned: eflores)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 4 obsolete files)

If some pulls mozilla-central and does a build, Adobe EME is not going to work as that build won't have a plugin-container.exe signed by Mozilla. We should pref off Adobe EME, so that we don't report that Adobe EME is available, don't show UI for it, and don't even download the Adobe EME plugin.
Only enable Adobe EME in official builds. ClearKey EME will still work in non-official builds, but Adobe EME won't.

I'm sure glandium knows whether this is the right #define to check...
Attachment #8599800 - Flags: review?(mh+mozilla)
Comment on attachment 8599800 [details] [diff] [review]
Patch: Only enable EME in official builds

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

::: browser/app/profile/firefox.js
@@ +1822,5 @@
>  pref("media.eme.enabled", true);
>  pref("media.eme.apiVisible", true);
>  
>  #ifdef XP_WIN
> +#ifdef MOZILLA_OFFICIAL

MOZILLA_OFFICIAL is probably not the right flag for this, as it's being used by e.g. Linux distros, which won't have a signed plugin-container either.
Attachment #8599800 - Flags: review?(mh+mozilla) → feedback-
So, we should add a new ac_add_option to enable Adobe EME, off by default, and have our build configs enable that? Do downstream use our build-configs too? Any other ideas?
Flags: needinfo?(mh+mozilla)
(In reply to Mike Hommey [:glandium] from comment #2)
> MOZILLA_OFFICIAL is probably not the right flag for this, as it's being used
> by e.g. Linux distros, which won't have a signed plugin-container either.

The idea is that downstream could ship our vouched plugin-container binary along their self-built browser. We should probably have another bug about enabling that.
(In reply to Chris Pearce (:cpearce) from comment #3)
> So, we should add a new ac_add_option to enable Adobe EME, off by default,
> and have our build configs enable that?

Sounds about right.

> Do downstream use our build-configs too?

I don't think they do. It's not something they should be doing anyways.

(In reply to Henri Sivonen (:hsivonen) from comment #4)
> (In reply to Mike Hommey [:glandium] from comment #2)
> > MOZILLA_OFFICIAL is probably not the right flag for this, as it's being used
> > by e.g. Linux distros, which won't have a signed plugin-container either.
> 
> The idea is that downstream could ship our vouched plugin-container binary
> along their self-built browser. We should probably have another bug about
> enabling that.

With my downstream hat, this is not going to happen on many distros because packages can't contain randomly downloaded binaries. The best you can do for them is, as k17e mentioned, downloading mozilla's plugin-container, which, arguably, is not much different, but is the same situation as openh264.
Flags: needinfo?(mh+mozilla)
Attached patch 1160101.patch (obsolete) — Splinter Review
Attachment #8607299 - Flags: review?(mh+mozilla)
Comment on attachment 8607299 [details] [diff] [review]
1160101.patch

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

::: browser/app/profile/firefox.js
@@ +1834,2 @@
>  pref("media.gmp-eme-adobe.enabled", true);
>  pref("browser.eme.ui.enabled", true);

browser.eme.ui.enabled isn't technically limited to Adobe EME. Not sure if this matters at the moment, but wanted to call it out.
Comment on attachment 8607299 [details] [diff] [review]
1160101.patch

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

::: browser/app/profile/firefox.js
@@ +1834,2 @@
>  pref("media.gmp-eme-adobe.enabled", true);
>  pref("browser.eme.ui.enabled", true);

We should leave browser.eme.ui.enabled even if Adobe EME is not enabled, so that we can show the "your keysystem is not supported" UI when users try to use Adobe (or some other) EME.
Assignee: cpearce → edwin
Comment on attachment 8607299 [details] [diff] [review]
1160101.patch

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

::: configure.in
@@ +5348,5 @@
> +    if test -z "$MOZ_EME"; then
> +        AC_MSG_ERROR([Adobe Primetime support requires EME support])
> +    fi
> +    AC_DEFINE(MOZ_ADOBE_EME)
> +fi;

My guess is that in the near future, there will be other EMEs with the same need for an option. Why not make this
--enable-eme=list,of,emes, which, for now, would be --enable-eme=abobe (see what we do for --enable-necko-protocols ; the important part is splitting on the comma and using AC_SUBST_SET)

Then in browser/app/moz.build you can do:

if 'adobe' in CONFIG['MOZ_EME']:
    DEFINES['MOZ_ADOBE_EME'] = True

or

for eme in CONFIG['MOZ_EME']:
    DEFINES['MOZ_%s_EME' % eme.upper()] = True
Attachment #8607299 - Flags: review?(mh+mozilla) → feedback+
Attached patch 1160101.patch (obsolete) — Splinter Review
Attachment #8599800 - Attachment is obsolete: true
Attachment #8607299 - Attachment is obsolete: true
Attachment #8610834 - Flags: review?(mh+mozilla)
Comment on attachment 8610834 [details] [diff] [review]
1160101.patch

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

::: configure.in
@@ +5324,5 @@
> +        dnl Split on commas, convert to uppercase, remove duplicates.
> +        MOZ_EME_ARGS=`echo $MOZ_EME_ARGS | sed -e 's/,/ /g' | tr [a-z] [A-Z]`
> +        MOZ_EME_ARGS=`$PYTHON ${srcdir}/build/unix/uniq.py ${MOZ_EME_ARGS}`
> +        for cdm in $MOZ_EME_ARGS; do
> +            AC_DEFINE_UNQUOTED(MOZ_EME_$cdm)

I'd rather see this defined in browser/app/moz.build, as per comment 9.

@@ +5329,5 @@
> +        done
> +    fi
> +fi
> +
> +AC_SUBST_SET(MOZ_EME)

MOZ_EME has the value "" or "1", that's not a set.
Attachment #8610834 - Flags: review?(mh+mozilla) → feedback+
Attached patch 1160101.patch v3 (obsolete) — Splinter Review
Attachment #8610834 - Attachment is obsolete: true
Attachment #8613859 - Flags: review?(mh+mozilla)
Comment on attachment 8613859 [details] [diff] [review]
1160101.patch v3

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

::: browser/app/moz.build
@@ +60,5 @@
>      RCINCLUDE = 'splash.rc'
>      DEFINES['MOZ_PHOENIX'] = True
>  
> +for cdm in CONFIG['MOZ_EME']:
> +    if cdm != 'yes':

It would be better if "yes" didn't end up here. I'd say the clearest way to do this would be to make the CONFIG key 'MOZ_EME_MODULES', and only set it when a list is given.
Attachment #8613859 - Flags: review?(mh+mozilla) → feedback+
Attached patch 1160101.patch v4Splinter Review
Attachment #8613859 - Attachment is obsolete: true
Attachment #8614516 - Flags: review?(mh+mozilla)
Attachment #8614516 - Flags: review?(mh+mozilla) → review+
https://hg.mozilla.org/mozilla-central/rev/5d8c22617d0d
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Comment on attachment 8614516 [details] [diff] [review]
1160101.patch v4

Approval Request Comment
[Feature/regressing bug #]: EME

[User impact if declined]: With this patch, Adobe EME is off by default, and must be explicitly turned on in the builder's mozconfig. Without this patch, Windows builds of mozilla-central by downstream users (i.e. Palemoon, other Firefox forks), and developers building Firefox locally, will download and install the Adobe EME Plugin. However EME won't work in those builds, as EME requires a plugin-container.exe built and signed by Mozilla's private key to work. Downstream/developer build will advertise to web pages that they support EME, but then EME will fail to when the web page tries to use EME.

[Describe test coverage new/current, TreeHerder]: We have lots of EME mochitests.

[Risks and why]: Low; it's a build config change, and we're building with both the on and off option in our official builds.

[String/UUID change made/needed]: None.

ni?cpearce so I don't forget to drive this to uplift.
Flags: needinfo?(cpearce)
Attachment #8614516 - Flags: approval-mozilla-beta?
Attachment #8614516 - Flags: approval-mozilla-aurora?
Comment on attachment 8614516 [details] [diff] [review]
1160101.patch v4

Approved for uplift to aurora and beta. Build config change.
Attachment #8614516 - Flags: approval-mozilla-beta?
Attachment #8614516 - Flags: approval-mozilla-beta+
Attachment #8614516 - Flags: approval-mozilla-aurora?
Attachment #8614516 - Flags: approval-mozilla-aurora+
Flags: needinfo?(cpearce)
Comment on attachment 8614516 [details] [diff] [review]
1160101.patch v4

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

::: browser/app/profile/firefox.js
@@ +1841,5 @@
>  
>  // Encrypted media extensions.
>  pref("media.eme.enabled", true);
>  pref("media.eme.apiVisible", true);
> +pref("browser.eme.ui.enabled", true);

Gah! I realized while testing this patch rebased on Beta, that making browser.eme.ui.enabled=true is actually not a good idea; if the user tries to use an Adobe-EME site in a non official build, they'll get UI with an "enable DRM" button, but that button won't make the page work. I'll remove that change from Beta and revert it in Inbound and Aurora.
Revert browser.eme.ui.enable change in Beta 39:
https://hg.mozilla.org/releases/mozilla-beta/rev/51f5d060b146

Revert browser.eme.ui.enable change in Aurora 40:
https://hg.mozilla.org/releases/mozilla-aurora/rev/5f30a6ae5279

Revert browser.eme.ui.enable change in Inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ce3823ec9b72
You need to log in before you can comment on or make changes to this bug.