Closed Bug 1038031 Opened 10 years ago Closed 10 years ago

[EME] Configure flag to remove EME

Categories

(Core :: Audio/Video, defect)

29 Branch
x86_64
Windows 8.1
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: cpearce, Assigned: eflores)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

We should have a configure flag to prevent EME related code being built.
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 :)
(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.
Attachment #8455121 - Flags: review?(gps)
Attachment #8455121 - Flags: review?(cpearce)
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.
Attachment #8455121 - Flags: review?(cpearce) → review+
Attachment #8455121 - Flags: review?(gps) → review+
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.
Attachment #8455121 - Flags: review?(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.
Attachment #8455121 - Flags: review?(bzbarsky) → review+
https://hg.mozilla.org/mozilla-central/rev/17fad150b6b0
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
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?
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.
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.
We will retain prefs to disable individual EME CDMs.
(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.)
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.
(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.
(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.
Flags: needinfo?(cbook)
(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 ?
Flags: needinfo?(cbook) → needinfo?(matt.woodrow)
Oh, sorry, looks like the patch description was wrong (both times it landed!). Bug 1032633.
Flags: needinfo?(matt.woodrow)
No longer depends on: 1103648
Depends on: 1106763
No longer depends on: 1106763
You need to log in before you can comment on or make changes to this bug.