1136707 - Unchecking "Play DRM Content" disables Clear Key

Status: RESOLVED FIXED

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

Description

[Henri Sivonen (:hsivonen) (not reading bugmail until 2024-08-29)]

Steps to reproduce:
 1) Use Nightly on Windows Vista or later or Mac (I tried Windows 8.1)
 2) Open a new non-e10s window.
 3) Load http://media.junglecode.net/test/dash-eme/sintel.html
 4) Observe the video play.
 5) Flip use browser.eme.ui.enabled to true in about:config
 6) Open Preferences
 7) Open the Content pane
 8) Uncheck Play DRM Content
 9) Reload http://media.junglecode.net/test/dash-eme/sintel.html

Actual results:
An infobar says I must enable DRM for the video to play and the video does not play.

Expected results:
Expected the Play DRM Content pref not to affect the Clear Key key system, since Clear Key is mere encryption and not DRM. (There's no Robustness&Compliance regime for Clear Key.)

Comment 1

[Chris Peterson [:cpeterson]]

We should fix this soon, but I don't think it needs to block Adobe EME MVP.

Comment 2

[Henri Sivonen (:hsivonen) (not reading bugmail until 2024-08-29)]

Moving to P1 per discussion with ajones and cpearce in London.

Comment 3

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

Basically, we want media.eme.enabled to only disable Widevine and Primetime, and not ClearKey. So if media.eme.enabled==false, ClearKey will still work, but Widevine and Primetime won't.

Comment 4

[Kilik Kuo [:kikuo] (inactive)]

It's reproducible on Linux, I'd like to take this bug.

Comment 5

[Kilik Kuo [:kikuo] (inactive)]

Attachment: Bug 1136707 - [Part1] Make pref 'media.eme.enabled' only affect practical DRMs.

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

Comment 6

[Kilik Kuo [:kikuo] (inactive)]

https://reviewboard.mozilla.org/r/65594/#review62582

::: dom/media/eme/MediaKeySystemAccess.cpp:263
(Diff revision 1)
>  MediaKeySystemAccess::GetKeySystemStatus(const nsAString& aKeySystem,
>                                           int32_t aMinCdmVersion,
>                                           nsACString& aOutMessage,
>                                           nsACString& aOutCdmVersion)
>  {
> -  MOZ_ASSERT(MediaPrefs::EMEEnabled());
> +  MOZ_ASSERT(MediaPrefs::EMEEnabled() || IsClearkeyKeySystem(aKeySystem));

I'm thinking to centralize these 2 conditions into one single helper function. e.g. IsEMEKeySystemAllowed(nsAString& aKeySystem).
Or just leave it as-is ?

Comment 7

[Kilik Kuo [:kikuo] (inactive)]

https://reviewboard.mozilla.org/r/65594/#review62584

::: dom/media/eme/EMEUtils.cpp:154
(Diff revision 1)
>  }
>  
> +bool
> +IsClearkeyKeySystem(const nsAString& aKeySystem)
> +{
> +  return aKeySystem.EqualsLiteral("org.w3.clearkey");

Also, I'm wondering if it's worth filing a bug to make these KeySystem string as specific definitions (e.g. KEYSYSTEM_WIDEVINE, KEYSYSTEM_PLAYREADY...) instead of putting "some.keysystem" in many places. Though I'm ok with the code now.
Any thoughts ?

Comment 8

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

Comment on attachment 8772898 [details]
Bug 1136707 - [Part1] Make pref 'media.eme.enabled' only affect practical DRMs.

https://reviewboard.mozilla.org/r/65594/#review62686

::: dom/media/eme/MediaKeySystemAccessManager.cpp:100
(Diff revision 1)
>      diagnostics.StoreMediaKeySystemAccess(mWindow->GetExtantDoc(),
>                                            aKeySystem, false, __func__);
>      return;
>    }
>  
> -  if (!MediaPrefs::EMEEnabled()) {
> +  if (!MediaPrefs::EMEEnabled() && !IsClearkeyKeySystem(aKeySystem)) {

Let's remove the "media.eme.clearkey.enabled" pref entirely, and just assume it's always enabled and cannot be disabled.

We'll need to remove it in a number of places:
https://dxr.mozilla.org/mozilla-central/search?q=%22media.eme.clearkey.enabled%3A%22&redirect=false

Comment 9

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

https://reviewboard.mozilla.org/r/65594/#review62582

> I'm thinking to centralize these 2 conditions into one single helper function. e.g. IsEMEKeySystemAllowed(nsAString& aKeySystem).
> Or just leave it as-is ?

I'm not sure what you mean, can you elaborate?

Comment 10

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

https://reviewboard.mozilla.org/r/65594/#review62584

> Also, I'm wondering if it's worth filing a bug to make these KeySystem string as specific definitions (e.g. KEYSYSTEM_WIDEVINE, KEYSYSTEM_PLAYREADY...) instead of putting "some.keysystem" in many places. Though I'm ok with the code now.
> Any thoughts ?

I think this is a good idea.

Comment 11

[Kilik Kuo [:kikuo] (inactive)]

https://reviewboard.mozilla.org/r/65594/#review62584

> I think this is a good idea.

Bug 1288320 - Define each keysystem string as specific identifier instead of placing "some.keysystem" everywhere.

Comment 12

[Kilik Kuo [:kikuo] (inactive)]

https://reviewboard.mozilla.org/r/65594/#review62582

> I'm not sure what you mean, can you elaborate?

There're only 2 placese which checks |MediaPrefs::EMEEnabled()| for further operations, now we'd like to allow clearkey in any case. So I was thinkg if it's better to modify e.g. 
from

MOZ_ASSERT(MediaPrefs::EMEEnabled() || IsClearkeyKeySystem(aKeySystem))

to

bool IsEMEKeySystemAllowed(const nsString& aKeySystem)
{   // Leave the idea here.
    return MediaPrefs::EMEEnabled() || IsClearkeyKeySystem(aKeySystem);
}
MOZ_ASSERT(IsEMEKeySystemAllowed(aKeySystem));
...

Although now the condition combination is not too complicated.

Comment 13

[Kilik Kuo [:kikuo] (inactive)]

https://reviewboard.mozilla.org/r/65594/#review62582

> There're only 2 placese which checks |MediaPrefs::EMEEnabled()| for further operations, now we'd like to allow clearkey in any case. So I was thinkg if it's better to modify e.g. 
> from
> 
> MOZ_ASSERT(MediaPrefs::EMEEnabled() || IsClearkeyKeySystem(aKeySystem))
> 
> to
> 
> bool IsEMEKeySystemAllowed(const nsString& aKeySystem)
> {   // Leave the idea here.
>     return MediaPrefs::EMEEnabled() || IsClearkeyKeySystem(aKeySystem);
> }
> MOZ_ASSERT(IsEMEKeySystemAllowed(aKeySystem));
> ...
> 
> Although now the condition combination is not too complicated.

After thinking it over, I don't think there's obvious benefit by doing this, so leave it as-is

Comment 14

[Kilik Kuo [:kikuo] (inactive)]

Attachment: Bug 1136707 - [Part2] Remove the pref 'media.eme.clearkey.enabled' in browser.

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

Comment 15

[Kilik Kuo [:kikuo] (inactive)]

Comment on attachment 8772898 [details]
Bug 1136707 - [Part1] Make pref 'media.eme.enabled' only affect practical DRMs.

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

Comment 16

[:Gijs (he/him)]

Comment on attachment 8773586 [details]
Bug 1136707 - [Part2] Remove the pref 'media.eme.clearkey.enabled' in browser.

https://reviewboard.mozilla.org/r/66296/#review63146

Why get rid of this code when the other patch does not remove the actual pref, which is still present in the codebase?

Comment 17

[:Gijs (he/him)]

Comment on attachment 8773586 [details]
Bug 1136707 - [Part2] Remove the pref 'media.eme.clearkey.enabled' in browser.

https://reviewboard.mozilla.org/r/66296/#review63148

Ah, so I misread - I don't really understand why this pref was never in all.js or similar, but at this stage it doesn't matter anymore. r=me if the pref is just going away. This means users can no longer disable clearkey at all. Is that intentional? If the rationale is that clearkey isn't "real" drm/eme, don't we need to update other UI and availability (e.g. should users be able to save videos that are using clearkey) ?

Comment 18

[Kilik Kuo [:kikuo] (inactive)]

(In reply to :Gijs Kruitbosch from comment #17)
> Comment on attachment 8773586 [details]
> Bug 1136707 - [Part2] Remove the pref 'media.eme.clearkey.enabled' in
> browser. +bugs
> 
> https://reviewboard.mozilla.org/r/66296/#review63148
> 
> Ah, so I misread - I don't really understand why this pref was never in
> all.js or similar, but at this stage it doesn't matter anymore. r=me if the
> pref is just going away. This means users can no longer disable clearkey at
> all. Is that intentional? If the rationale is that clearkey isn't "real"
> drm/eme, don't we need to update other UI and availability (e.g. should
> users be able to save videos that are using clearkey) ?

Thanks for the review. Yes, that's intentional.
According to spec, browser supporting EME should implement Clearkey KeySystem and it's handy for testing without the need of requesting keys from license server.
IMHO, though Clearkey is not a "real" DRM, we should keep its behavior for content protection.

Comment 19

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

Comment on attachment 8772898 [details]
Bug 1136707 - [Part1] Make pref 'media.eme.enabled' only affect practical DRMs.

https://reviewboard.mozilla.org/r/65594/#review63526

::: dom/media/eme/MediaKeySystemAccessManager.cpp:102
(Diff revision 2)
>      return;
>    }
>  
> -  if (!MediaPrefs::EMEEnabled()) {
> +  if (!MediaPrefs::EMEEnabled() && !IsClearkeyKeySystem(aKeySystem)) {
>      // EME disabled by user, send notification to chrome so UI can inform user.
> +    // Clearkey is allowed even EME is disabled because we want the pref

"Clearkey is allowed even *when* EME is disabled because we want the pref "media.eme.enabled" only taking effect on *proprietary* DRMs."

Comment 20

[Kilik Kuo [:kikuo] (inactive)]

Comment on attachment 8772898 [details]
Bug 1136707 - [Part1] Make pref 'media.eme.enabled' only affect practical DRMs.

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

Comment 21

[Kilik Kuo [:kikuo] (inactive)]

Comment on attachment 8773586 [details]
Bug 1136707 - [Part2] Remove the pref 'media.eme.clearkey.enabled' in browser.

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

Comment 22

[Kilik Kuo [:kikuo] (inactive)]

try run, looks ok.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9ee003b781ef
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f417a861f2a6

Comment 23

[Kilik Kuo [:kikuo] (inactive)]

https://reviewboard.mozilla.org/r/65594/#review63526

> "Clearkey is allowed even *when* EME is disabled because we want the pref "media.eme.enabled" only taking effect on *proprietary* DRMs."

Thanks for the review : )

Comment 24

[Carsten Book [:Tomcat]]

has problems to apply like pplying 3296c3fb0801
patching file dom/media/test/test_eme_request_notifications.html
Hunk #1 FAILED at 58
1 out of 1 hunks FAILED -- saving rejects to file dom/media/test/test_eme_request_notifications.html.rej
patch failed to apply
abort: fix up the working directory and run hg transplant --continue

Comment 25

[Kilik Kuo [:kikuo] (inactive)]

Comment on attachment 8772898 [details]
Bug 1136707 - [Part1] Make pref 'media.eme.enabled' only affect practical DRMs.

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

Comment 26

[Kilik Kuo [:kikuo] (inactive)]

Comment on attachment 8773586 [details]
Bug 1136707 - [Part2] Remove the pref 'media.eme.clearkey.enabled' in browser.

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

Comment 27

[Kilik Kuo [:kikuo] (inactive)]

(In reply to Kilik Kuo [:kikuo] from comment #25)
> Comment on attachment 8772898 [details]
> Bug 1136707 - [Part1] Make pref 'media.eme.enabled' only affect practical
> DRMs.
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/65594/diff/3-4/

Rebased, make "dom/media/test/test_eme_request_notifications.html" update-to-date.
Hey Tomcat, thanks for the notification : )

Comment 28

[Kilik Kuo [:kikuo] (inactive)]

(In reply to Kilik Kuo [:kikuo] from comment #27)
> (In reply to Kilik Kuo [:kikuo] from comment #25)
> > Comment on attachment 8772898 [details]
> > Bug 1136707 - [Part1] Make pref 'media.eme.enabled' only affect practical
> > DRMs.
> > 
> > Review request updated; see interdiff:
> > https://reviewboard.mozilla.org/r/65594/diff/3-4/
> 
> Rebased, make "dom/media/test/test_eme_request_notifications.html"

Just a definition replacement, I don't think that requires another try.

> update-to-date.
> Hey Tomcat, thanks for the notification : )

Comment 29

[Ryan VanderMeulen [:RyanVM]]

Still doesn't apply cleanly to inbound.

Comment 30

[Kilik Kuo [:kikuo] (inactive)]

Comment on attachment 8772898 [details]
Bug 1136707 - [Part1] Make pref 'media.eme.enabled' only affect practical DRMs.

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

Comment 31

[Kilik Kuo [:kikuo] (inactive)]

Comment on attachment 8773586 [details]
Bug 1136707 - [Part2] Remove the pref 'media.eme.clearkey.enabled' in browser.

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

Comment 32

[Kilik Kuo [:kikuo] (inactive)]

Sorry for inconvenience, I rebased to a wrong changeset.
Now should be ok !

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d8c5236bbb9d
[Part1] Make pref 'media.eme.enabled' only affect practical DRMs. r=cpearce
https://hg.mozilla.org/integration/mozilla-inbound/rev/48be91b20210
[Part2] Remove the pref 'media.eme.clearkey.enabled' in browser. r=Gijs

Comment 34

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d8c5236bbb9d
https://hg.mozilla.org/mozilla-central/rev/48be91b20210