1300654 - Remove MOZ_EME build option and make EME disabled by preferences.

Status: RESOLVED FIXED

(Core :: Audio/Video: GMP, defect, P3)

Description

[James Cheng[:JamesCheng]]

Refer to bug 1299627 comment 8, 

We should remove MOZ_EME build option and make EME disabled by preferences in current codebase.

I think this bug is against to bug 1038031. 

If we can remove MOZ_EME than we could stop suffering from conditional build errors.

Comment 1

[James Cheng[:JamesCheng]]

Attachment: Bug 1300654 Part1-Remove MOZ_EME from code base.

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

Comment 2

[James Cheng[:JamesCheng]]

Attachment: Bug 1300654 Part2-Remove MOZ_EME build config.

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

Comment 3

[James Cheng[:JamesCheng]]

Hi Chris,
Please review these patches and please tell me if anything I ignored to do.

Do you know whom I should tell that stop building --disable-eme once this bug landed?

Thank you!

Comment 4

[James Cheng[:JamesCheng]]

attach try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3cba145d7121

Comment 5

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

Comment on attachment 8789322 [details]
Bug 1300654 Part1-Remove MOZ_EME from code base.

https://reviewboard.mozilla.org/r/77598/#review76054

::: dom/webidl/HTMLMediaElement.webidl
(Diff revision 1)
>  
>    [Pref="media.useAudioChannelAPI"]
>    attribute EventHandler onmozinterruptend;
>  };
>  
> -#ifdef MOZ_EME

I expect you'll find commit hooks will prevent you landing this patch without an r+ from a DOM peer, because you've touched a WebIDL file.

Try asking :smaug or :bz to r+ this change.

Comment 6

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

Comment on attachment 8789323 [details]
Bug 1300654 Part2-Remove MOZ_EME build config.

Even though this change is pretty trivial, you should get r+ from a build peer for configure changes.

Comment 7

[James Cheng[:JamesCheng]]

Hi smaug,

Since we need to remove MOZ_EME, I've modified the webidl files.

Please help to review this patch.

Thanks.

Comment 8

[James Cheng[:JamesCheng]]

Hi glandium,

Is this build error related to my patch?

I have no idea what happened.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3cba145d7121&selectedJob=27160416

Thank you.

Comment 9

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

(In reply to James Cheng[:JamesCheng] from comment #3)
> Do you know whom I should tell that stop building --disable-eme once this
> bug landed?

We don't have a --disable-eme configure option; EME must be turned on or off via --enable-eme=+widevine,+adobe.

So you don't need to worry about telling people to stop using --disable-eme.

Comment 10

[James Cheng[:JamesCheng]]

I am not sure about that.

By my experience,

I used 

ac_add_options --disable-eme in my mozconfig and that takes effect to disable MOZ_EME effect.

So I don't know what and how the build condition the build team used to build without EME stuff.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8789322 [details]
Bug 1300654 Part1-Remove MOZ_EME from code base.

https://reviewboard.mozilla.org/r/77598/#review76260

r+ for the .webidl

Comment 12

[Mike Hommey [:glandium]]

(In reply to PTO until Sep 19 NZT Chris Pearce (:cpearce) from comment #9)
> (In reply to James Cheng[:JamesCheng] from comment #3)
> > Do you know whom I should tell that stop building --disable-eme once this
> > bug landed?
> 
> We don't have a --disable-eme configure option; EME must be turned on or off
> via --enable-eme=+widevine,+adobe.

There automatically is a --disable-eme by the existence of --enable-eme. If you want --disable-eme to do nothing, you need to explicitly make it error out in configure.

Comment 13

[Mike Hommey [:glandium]]

(In reply to James Cheng[:JamesCheng] from comment #8)
> Hi glandium,
> 
> Is this build error related to my patch?
> 
> I have no idea what happened.
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=3cba145d7121&selectedJob=27160416
> 
> Thank you.

No, that's related to bug 1297585

Comment 14

[Mike Hommey [:glandium]]

Comment on attachment 8789323 [details]
Bug 1300654 Part2-Remove MOZ_EME build config.

https://reviewboard.mozilla.org/r/77600/#review76884

::: toolkit/moz.configure
(Diff revision 1)
>  
>  @depends(enable_eme)
>  def eme_modules(value):
>      return value
>  
> -set_config('MOZ_EME', eme)

Shouldn't the default pref value be influenced by --enable/disable-eme, though? (especially considering the default is --disable on non tier-1, essentially).

Comment 15

[James Cheng[:JamesCheng]]

(In reply to Mike Hommey [:glandium] from comment #14)
> Comment on attachment 8789323 [details]
> Bug 1300654 Part2-Remove MOZ_EME build config.
> 
> https://reviewboard.mozilla.org/r/77600/#review76884
> 
> ::: toolkit/moz.configure
> (Diff revision 1)
> >  
> >  @depends(enable_eme)
> >  def eme_modules(value):
> >      return value
> >  
> > -set_config('MOZ_EME', eme)
> 
> Shouldn't the default pref value be influenced by --enable/disable-eme,
> though? (especially considering the default is --disable on non tier-1,
> essentially).

Hi glandium,

Could you please tell me what is the relationship between MOZ_EME and --disable-eme?

By experiment, when I declare --disable-eme and MOZ_EME will be undefined. They must have something related but I cannot easily find out in [1] .

And I saw some pref is determined by [2].

Is MOZ_ADOBE_EME or MOZ_WIDEVINE_EME related to [1]?

I don't really understand the magic when I config  --enable-eme or --disable-eme...

Please give me some suggestion that can make me more understand the build config magic.

Thank you very much.

[1]
https://dxr.mozilla.org/mozilla-central/rev/1851b78b5a9673ee422f189b92e5f1e86b82a01c/toolkit/moz.configure#353-388


[2]
https://dxr.mozilla.org/mozilla-central/rev/1851b78b5a9673ee422f189b92e5f1e86b82a01c/browser/app/profile/firefox.js#178

Comment 16

[James Cheng[:JamesCheng]]

(In reply to Mike Hommey [:glandium] from comment #14)
> Comment on attachment 8789323 [details]
> Bug 1300654 Part2-Remove MOZ_EME build config.
> 
> https://reviewboard.mozilla.org/r/77600/#review76884
> 
> ::: toolkit/moz.configure
> (Diff revision 1)
> >  
> >  @depends(enable_eme)
> >  def eme_modules(value):
> >      return value
> >  
> > -set_config('MOZ_EME', eme)
> 
> Shouldn't the default pref value be influenced by --enable/disable-eme,
> though? (especially considering the default is --disable on non tier-1,
> essentially).


May I consider the question as "Should the pref value be influenced by MOZ_EME on or off?"?

IIUC, there is no preference checked by MOZ_EME(my patch did not meet that).

I would like to forward the question to Chris. He knows the effect on --enable/disable-eme.

Thanks

Comment 17

[Mike Hommey [:glandium]]

I'll leave it to Chris to answer.

Comment 18

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

(In reply to James Cheng[:JamesCheng] from comment #15)
> Could you please tell me what is the relationship between MOZ_EME and
> --disable-eme?

Currently, if --disable-eme is present in the mozconfig, MOZ_EME is defined by the configure script. If MOZ_EME is defined, the preferences to enable the EME keysystems are set to true.


> By experiment, when I declare --disable-eme and MOZ_EME will be undefined.
> They must have something related but I cannot easily find out in [1] .

The purpose of this bug is to remove the MOZ_EME C++ define.

When --disable-eme is present in the mozconfig, we should not set the MOZ_WIDEVINE_EME or MOZ_ADOBE_EME defines.


> And I saw some pref is determined by [2].
> 
> Is MOZ_ADOBE_EME or MOZ_WIDEVINE_EME related to [1]?

The MOZ_EME define acts as a global "is EME enabled" build flag. We want to remove that build flag, and just let preferences control turning EME on or off.


> I don't really understand the magic when I config  --enable-eme or
> --disable-eme...

I don't either.



(In reply to James Cheng[:JamesCheng] from comment #16)
> (In reply to Mike Hommey [:glandium] from comment #14)
> > Comment on attachment 8789323 [details]
> > Bug 1300654 Part2-Remove MOZ_EME build config.
> > 
> > https://reviewboard.mozilla.org/r/77600/#review76884
> > 
> > ::: toolkit/moz.configure
> > (Diff revision 1)
> > >  
> > >  @depends(enable_eme)
> > >  def eme_modules(value):
> > >      return value
> > >  
> > > -set_config('MOZ_EME', eme)
> > 
> > Shouldn't the default pref value be influenced by --enable/disable-eme,
> > though? (especially considering the default is --disable on non tier-1,
> > essentially).
> 
> 
> May I consider the question as "Should the pref value be influenced by
> MOZ_EME on or off?"?


The MOZ_EME define should be removed. i.e. you need to delete the line "set_define('MOZ_EME', eme)".

You can probably also delete the line "set_config('MOZ_EME', eme)".

> IIUC, there is no preference checked by MOZ_EME(my patch did not meet that).

The preferences are set based on the value of MOZ_{WIDEVINE,ADOBE}_EME defines, in firefox.js as you discovered. This I think we need to keep.

> I would like to forward the question to Chris. He knows the effect on
> --enable/disable-eme.

--disable-eme should result in Widevine and Adobe DRM not being enabled in preferences by default, but the users should be prompted the first time a site uses EME wants to use the keysystem, so the user can opt-in to enable EME.

Comment 19

[Mike Hommey [:glandium]]

(In reply to Chris Pearce (:cpearce) from comment #18)
> --disable-eme should result in Widevine and Adobe DRM not being enabled in
> preferences by default, but the users should be prompted the first time a
> site uses EME wants to use the keysystem, so the user can opt-in to enable
> EME.

I don't think that's a good idea. There are people out there that expect --disable-eme does that: disable EME. If we go forward with EME never being disabled through configure, then we should rename the flag to something like --enable/disable-eme-modules.

Comment 20

[James Cheng[:JamesCheng]]

Attachment: Bug 1300654 Part3-Rename --enable-eme to --enable-eme-modules and enable_eme to enable_eme_modules.

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

Comment 21

[James Cheng[:JamesCheng]]

Comment on attachment 8789322 [details]
Bug 1300654 Part1-Remove MOZ_EME from code base.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77598/diff/1-2/

Comment 22

[James Cheng[:JamesCheng]]

Comment on attachment 8789323 [details]
Bug 1300654 Part2-Remove MOZ_EME build config.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77600/diff/1-2/

Comment 23

[James Cheng[:JamesCheng]]

(In reply to Mike Hommey [:glandium] from comment #19)
> (In reply to Chris Pearce (:cpearce) from comment #18)
> > --disable-eme should result in Widevine and Adobe DRM not being enabled in
> > preferences by default, but the users should be prompted the first time a
> > site uses EME wants to use the keysystem, so the user can opt-in to enable
> > EME.
> 
> I don't think that's a good idea. There are people out there that expect
> --disable-eme does that: disable EME. If we go forward with EME never being
> disabled through configure, then we should rename the flag to something like
> --enable/disable-eme-modules.

Thanks for your feedback,

According to the comment, I create part3 for renaming the enable-eme to enable-eme-modules.

Rebase part1.

Please help to review.

Thank you.

Comment 24

[James Cheng[:JamesCheng]]

Comment on attachment 8789323 [details]
Bug 1300654 Part2-Remove MOZ_EME build config.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77600/diff/2-3/

Comment 25

[James Cheng[:JamesCheng]]

Comment on attachment 8792407 [details]
Bug 1300654 Part3-Rename --enable-eme to --enable-eme-modules and enable_eme to enable_eme_modules.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/79492/diff/1-2/

Comment 26

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

(In reply to Mike Hommey [:glandium] from comment #19)
> (In reply to Chris Pearce (:cpearce) from comment #18)
> > --disable-eme should result in Widevine and Adobe DRM not being enabled in
> > preferences by default, but the users should be prompted the first time a
> > site uses EME wants to use the keysystem, so the user can opt-in to enable
> > EME.
> 
> I don't think that's a good idea. There are people out there that expect
> --disable-eme does that: disable EME. If we go forward with EME never being
> disabled through configure, then we should rename the flag to something like
> --enable/disable-eme-modules.

This is the behaviour that we want for the EME-Free repacks; we don't want users to be tricked into downloading these builds and then finding out that they're stuck not being able to make DRM protected video work; there should be a path to make EME work, and that's what these builds have.

That said, I was mistakenly under the impression that the EME free repacks use --disable-eme, but nthomas on IRC tells me that the EME free repacks are "the same as the regular release build, but with https://github.com/mozilla-partners/mozilla-EME-free/blob/master/desktop/mozilla-EME-free/distribution/distribution.ini packaged in afterwards".

So I'm not opposed to having --disable-eme turn off all key systems.

To achieve that, we can remove MOZ_EME from all C++ code and mozbuild files, but leave it in moz.configure, and have the MOZ_EME #define only used in firefox.js to set the media.gmp-* prefs to false if MOZ_EME isn't defined.

i.e. we should be able to leave the build system as is, and only use MOZ_EME in firefox.js.

Comment 27

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

Comment on attachment 8792407 [details]
Bug 1300654 Part3-Rename --enable-eme to --enable-eme-modules and enable_eme to enable_eme_modules.

https://reviewboard.mozilla.org/r/79492/#review78302

We should be able to get away without this, see my comment in the bug. Thanks, and sorry for giving you the run-around on this.

Comment 28

[James Cheng[:JamesCheng]]

Thank you!

I will r cancel all review requests and modify the patch.

Thanks

Comment 29

[James Cheng[:JamesCheng]]

Comment on attachment 8789322 [details]
Bug 1300654 Part1-Remove MOZ_EME from code base.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77598/diff/2-3/

Comment 30

[James Cheng[:JamesCheng]]

(In reply to Chris Pearce (:cpearce) from comment #26)
> (In reply to Mike Hommey [:glandium] from comment #19)
> > (In reply to Chris Pearce (:cpearce) from comment #18)
> > > --disable-eme should result in Widevine and Adobe DRM not being enabled in
> > > preferences by default, but the users should be prompted the first time a
> > > site uses EME wants to use the keysystem, so the user can opt-in to enable
> > > EME.
> > 
> > I don't think that's a good idea. There are people out there that expect
> > --disable-eme does that: disable EME. If we go forward with EME never being
> > disabled through configure, then we should rename the flag to something like
> > --enable/disable-eme-modules.
> 
> This is the behaviour that we want for the EME-Free repacks; we don't want
> users to be tricked into downloading these builds and then finding out that
> they're stuck not being able to make DRM protected video work; there should
> be a path to make EME work, and that's what these builds have.
> 
> That said, I was mistakenly under the impression that the EME free repacks
> use --disable-eme, but nthomas on IRC tells me that the EME free repacks are
> "the same as the regular release build, but with
> https://github.com/mozilla-partners/mozilla-EME-free/blob/master/desktop/
> mozilla-EME-free/distribution/distribution.ini packaged in afterwards".
> 
> So I'm not opposed to having --disable-eme turn off all key systems.
> 
> To achieve that, we can remove MOZ_EME from all C++ code and mozbuild files,
> but leave it in moz.configure, and have the MOZ_EME #define only used in
> firefox.js to set the media.gmp-* prefs to false if MOZ_EME isn't defined.
> 
I removed all MOZ_EME in current code base and I think media.gmp-* has already guarded by MOZ_ADOBE_EME and MOZ_WIDEVINE_EME which are defined by --enable-eme+=XXX and not defined when --disable-eme
So I will not modify the firefox.js.

> i.e. we should be able to leave the build system as is, and only use MOZ_EME
> in firefox.js.

Please help to review again,

Thank you very much.

Comment 31

[James Cheng[:JamesCheng]]

Comment on attachment 8789322 [details]
Bug 1300654 Part1-Remove MOZ_EME from code base.

Use bugzilla |edit detail| to do r? again since it seems review board cannot do this.

Comment 32

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

Comment on attachment 8789322 [details]
Bug 1300654 Part1-Remove MOZ_EME from code base.

https://reviewboard.mozilla.org/r/77598/#review78634

Comment 33

[James Cheng[:JamesCheng]]

Attachment: Bug 1300654 Part2-Remove MOZ_EME define from moz.configure.

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

Comment 34

[James Cheng[:JamesCheng]]

Comment on attachment 8789322 [details]
Bug 1300654 Part1-Remove MOZ_EME from code base.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77598/diff/3-4/

Comment 35

[James Cheng[:JamesCheng]]

Hi glandium,
Based on comment 26,

We could get rid of MOZ_EME macro anymore .
Please review it again thank you

Comment 36

[Mike Hommey [:glandium]]

Comment on attachment 8793119 [details]
Bug 1300654 Part2-Remove MOZ_EME define from moz.configure.

https://reviewboard.mozilla.org/r/79898/#review79880

Comment 37

[James Cheng[:JamesCheng]]

Attachment: build with ac_add_options --disable-eme

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

Comment 38

[James Cheng[:JamesCheng]]

Comment on attachment 8789322 [details]
Bug 1300654 Part1-Remove MOZ_EME from code base.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77598/diff/4-5/

Comment 39

[James Cheng[:JamesCheng]]

Comment on attachment 8793119 [details]
Bug 1300654 Part2-Remove MOZ_EME define from moz.configure.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/79898/diff/1-2/

Comment 40

[James Cheng[:JamesCheng]]

Comment on attachment 8795209 [details]
build with ac_add_options --disable-eme

Thanks for the reviewing.

Comment 41

[Ryan VanderMeulen [:RyanVM]]

Please clean up your patch queue so that autoland doesn't think that "build with ac_add_options --disable-eme" needs to be landed here.

Comment 42

[James Cheng[:JamesCheng]]

Comment on attachment 8789322 [details]
Bug 1300654 Part1-Remove MOZ_EME from code base.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77598/diff/5-6/

Comment 43

[James Cheng[:JamesCheng]]

Comment on attachment 8793119 [details]
Bug 1300654 Part2-Remove MOZ_EME define from moz.configure.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/79898/diff/2-3/

Comment 44

[James Cheng[:JamesCheng]]

Thanks for reminding. I cleaned up my local queue and re-mozreview push again.

So you mean that setting the patch to "obsolete" in bugzilla is useless for autoland?

I noticed that some bugs(check in needed) the Pulsebot will push to inbound but not autoland. Is it random or having some rules to tell?

Thanks.

Comment 45

[Ryan VanderMeulen [:RyanVM]]

(In reply to James Cheng[:JamesCheng] from comment #44)
> So you mean that setting the patch to "obsolete" in bugzilla is useless for
> autoland?

Autoland is going to try to push whatever it sees in MozReview for that push.

> I noticed that some bugs(check in needed) the Pulsebot will push to inbound
> but not autoland. Is it random or having some rules to tell?

Pulsebot just comments in bugs, it doesn't do any landing. If your patch went to inbound, it's because someone pushed it there themselves.

Comment 46

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8eef5d2cc850
Part1-Remove MOZ_EME from code base. r=cpearce,smaug
https://hg.mozilla.org/integration/autoland/rev/67109e311e4c
Part2-Remove MOZ_EME define from moz.configure. r=glandium

Comment 47

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8eef5d2cc850
https://hg.mozilla.org/mozilla-central/rev/67109e311e4c

Comment 48

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

Note this 8eef5d2cc850 had an incorrect merge conflict resolution which effectively backed out bug 1280829.

https://hg.mozilla.org/mozilla-central/rev/8eef5d2cc850#l8.108

Tracking fixing that in bug 1307595:

Comment 49

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

Comment on attachment 8789322 [details]
Bug 1300654 Part1-Remove MOZ_EME from code base.

Approval Request Comment
[Feature/regressing bug #]: The landing of bug 1299694 means that we properly don't enable Widevine for Android builds. However, the build is broken on Android without Widevine enabled, so we need this patch to fix the build so that we can build on Android with Widevine disabled.
[User impact if declined]: Firefox for Android won't build.
[Describe test coverage new/current, TreeHerder]: This change affects EME, and we've got lots of EME test coverage.
[Risks and why]: Low; this is mostly a configuration change.
[String/UUID change made/needed]: None.

Comment 50

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

Comment on attachment 8793119 [details]
Bug 1300654 Part2-Remove MOZ_EME define from moz.configure.

Approval Request Comment
[Feature/regressing bug #]: The landing of bug 1299694 means that we properly don't enable Widevine for Android builds. However, the build is broken on Android without Widevine enabled, so we need this patch to fix the build so that we can build on Android with Widevine disabled.
[User impact if declined]: Firefox for Android won't build.
[Describe test coverage new/current, TreeHerder]: This change affects EME, and we've got lots of EME test coverage.
[Risks and why]: Low; this is mostly a configuration change.
[String/UUID change made/needed]: None.

Comment 51

[Gerry Chang [:gchang]]

Comment on attachment 8789322 [details]
Bug 1300654 Part1-Remove MOZ_EME from code base.

Fix a build issue related to Widevine on android. Take it in 51 aurora.

Comment 52

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/afc827378c1075c2d849f07f64a4021235a51586
https://hg.mozilla.org/releases/mozilla-aurora/rev/31b12c04afc5d54f8f75d37547907532db4e872b