Closed Bug 1136707 Opened 5 years ago Closed 4 years ago

Unchecking "Play DRM Content" disables Clear Key

Categories

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

x86_64
All
defect

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: hsivonen, Assigned: kikuo)

References

(Blocks 1 open bug, )

Details

Attachments

(2 files)

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.)
Blocks: 1144903
We should fix this soon, but I don't think it needs to block Adobe EME MVP.
Priority: -- → P2
Component: Audio/Video → Audio/Video: Playback
Moving to P1 per discussion with ajones and cpearce in London.
Priority: P2 → P1
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.
It's reproducible on Linux, I'd like to take this bug.
Assignee: nobody → kikuo
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 ?
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 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
Attachment #8772898 - Flags: review?(cpearce) → review-
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?
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.
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.
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.
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
Review commit: https://reviewboard.mozilla.org/r/66296/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/66296/
Attachment #8772898 - Attachment description: Bug 1136707 - Make pref 'media.eme.enabled' only affect practical DRMs. → Bug 1136707 - [Part1] Make pref 'media.eme.enabled' only affect practical DRMs.
Attachment #8773586 - Flags: review?(gijskruitbosch+bugs)
Attachment #8772898 - Flags: review- → review?(cpearce)
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 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?
Attachment #8773586 - Flags: review?(gijskruitbosch+bugs)
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) ?
Attachment #8773586 - Flags: review+
(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 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."
Attachment #8772898 - Flags: review?(cpearce) → review+
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/
Attachment #8773586 - Attachment description: Bug 1136707 - [Part2] Remove the pref 'media.eme.clearkey.enabled' in browser. +bugs → Bug 1136707 - [Part2] Remove the pref 'media.eme.clearkey.enabled' in browser.
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/
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 : )
Keywords: checkin-needed
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
Flags: needinfo?(kikuo)
Keywords: checkin-needed
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 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/
(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 : )
Flags: needinfo?(kikuo)
(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 : )
Keywords: checkin-needed
Still doesn't apply cleanly to inbound.
Flags: needinfo?(kikuo)
Keywords: checkin-needed
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 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/
Sorry for inconvenience, I rebased to a wrong changeset.
Now should be ok !
Flags: needinfo?(kikuo)
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/d8c5236bbb9d
https://hg.mozilla.org/mozilla-central/rev/48be91b20210
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.