1038031 - [EME] Configure flag to remove EME

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

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

We should have a configure flag to prevent EME related code being built.

Comment 1

[Gregory Szorc [:gps]]

Do we need a separate bug for this? configure flags are standard procedure for compiled features (like EME). The build peers know to enforce this. It's highly unlikely EME lands without a configure flag if it gets proper build peer review. Even if it does, I'm pretty sure glandium would have it fixed in <24 hours given his Debian hat wearing :)

Comment 2

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Add flag to disable EME for great justice, restoration of freedom, and 180 degree rotation of puppy frowns

(In reply to Gregory Szorc [:gps] from comment #1)
> Do we need a separate bug for this? configure flags are standard procedure
> for compiled features (like EME). The build peers know to enforce this. It's
> highly unlikely EME lands without a configure flag if it gets proper build
> peer review. Even if it does, I'm pretty sure glandium would have it fixed
> in <24 hours given his Debian hat wearing :)

There already are bits of EME in m-c.

Comment 3

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

Comment on attachment 8455121 [details] [diff] [review]
Add flag to disable EME for great justice, restoration of freedom, and 180 degree rotation of puppy frowns

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

r=cpearce with nits.

::: content/html/content/public/HTMLMediaElement.h
@@ +1076,5 @@
>    nsCOMPtr<nsIContent> mSourceLoadCandidate;
>  
>    // Range of time played.
>    nsRefPtr<TimeRanges> mPlayed;
>  

We also should have exclusion guards around GetMediaKeys() to IsEventAttributeName() inclusive.

They're excluded in the cpp file, but let's exclude them here to its clear.

Comment 4

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Comment on attachment 8455121 [details] [diff] [review]
Add flag to disable EME for great justice, restoration of freedom, and 180 degree rotation of puppy frowns

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

Pre-push hook tells me I forgot DOM peer review.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8455121 [details] [diff] [review]
Add flag to disable EME for great justice, restoration of freedom, and 180 degree rotation of puppy frowns

r=me, but watch out for possible dep build issues with changing an IDL file to being preprocessed.  They shouldn't be there, but have been known to happen.  :(  If so, you'll need to clobber.

Comment 6

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/17fad150b6b0

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/17fad150b6b0

Comment 9

[kevink9876543]

Thank you so much to the people who worked on this!  When I saw this landed in m-c, it made my day and certainly did result in a 180-degree rotation of my puppy frown :D

Given that some parts of EME landed in Gecko 32 (at least bug 1016162, maybe others but I don't have time to check all resolved EME-related bugs in detail), would it be possible and reasonable to backport this patch to Aurora?

Comment 10

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

The EME code in Firefox 32 is the JS interfaces only, they aren't connected to anything, so there's not much to gain from uplifting this.

Comment 11

[Henri Sivonen (:hsivonen)]

What's the use case for this? I understand that there are downstream users of the code base who'd want to disable *DRM*, but it seems like a bad idea to bait them to disable Clear Key as collateral damage.

There seems to be a non-zero chance of Clear Key getting used make MSE segments available encrypted on a CDN that doesn't have a login mechanism and the MSE player page and the Clear Key keys getting supplied behind login. It would be a shame if the desire to "disable DRM" broke such potential non-DRM cases as collateral damage.

Comment 12

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

We will retain prefs to disable individual EME CDMs.

Comment 13

[Henri Sivonen (:hsivonen)]

(In reply to Chris Pearce (:cpearce) from comment #12)
> We will retain prefs to disable individual EME CDMs.

Is there a use case for disabling Clear Key and EME itself that's better than disabling some other Gecko feature that we don't have flags for? (Again, I expect that there are folks who'll want to disable the download of the Adobe CDM in downstream builds.)

Comment 14

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

This pref is for people who hate DRM and want a nuclear option to remove it from Firefox. That's its use case.

If a clear demand for ClearKey develops, I'm happy to revisit this.

Comment 15

[kevink9876543]

(In reply to Henri Sivonen (:hsivonen) from comment #11)
> What's the use case for this? I understand that there are downstream users
> of the code base who'd want to disable *DRM*, but it seems like a bad idea
> to bait them to disable Clear Key as collateral damage.
Speaking as one such user, I'm not sure I would care about losing the use case you mentioned.  I prefer that the browser doesn't even have the *ability* to support CDMs.

(In reply to Chris Pearce (:cpearce) from comment #14)
> If a clear demand for ClearKey develops, I'm happy to revisit this.
I understand from hsivonen's comments that EME potentially has valid non-DRM use cases, and that in some cases (maybe e.g. bug 1025421) it might thus be best to zap only the actual DRM stuff at build time, but I like the current effect of the --disable-eme option for my custom SeaMonkey builds.  If the subject comes up again, please create a separate configure option for disabling only DRM.

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/deb0b389efa7

Comment 17

[Henri Sivonen (:hsivonen)]

(In reply to Carsten Book [:Tomcat] from comment #16)
> https://hg.mozilla.org/mozilla-central/rev/deb0b389efa7

Looks like this changeset landed with the wrong bug number and the landing comment belongs elsewhere.

Comment 18

[Carsten Book [:Tomcat]]

(In reply to Henri Sivonen (:hsivonen) from comment #17)
> (In reply to Carsten Book [:Tomcat] from comment #16)
> > https://hg.mozilla.org/mozilla-central/rev/deb0b389efa7
> 
> Looks like this changeset landed with the wrong bug number and the landing
> comment belongs elsewhere.

hi Matt, seems you checked this in https://treeherder.mozilla.org/ui/#/jobs?repo=mozilla-inbound&revision=d2828e139d25 - do you know the right bug number ?

Comment 19

[Matt Woodrow (:mattwoodrow)]

Oh, sorry, looks like the patch description was wrong (both times it landed!). Bug 1032633.