[EME] Make CDM version accessible in Navigator.RequestMediaKeysystemAccess() keysystemId

RESOLVED FIXED in Firefox 37

Status

()

Core
Audio/Video
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: cpearce, Assigned: cpearce)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla38
x86_64
Windows 8.1
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox37 fixed, firefox38 fixed)

Details

Attachments

(8 attachments)

(Assignee)

Description

3 years ago
Adobe need to be able to query in navigator.requestMediaKeysystemAccess() for the version of CDM that is installed. We can do this with a version number in the keysystemId string.

For example, if script knows that only version 7 of Adobes CDM supports the desired capabilities (HDCP for example, or a critical fix, etc), then script can query for keysystemId com.adobe.access.7.

This should be backwards compatible; if the installed CDM is version 9, but we query for version 7, then we'll report that version 7 is supported, but we'll instantiate version 9 (we'll probably only have the latest version of the CDM installed on disk anyway).

The inverse should fail however; if we have version 7 but script queries for version 9, requestMediaKeysystemAccess() should fail.
(Assignee)

Updated

3 years ago
Assignee: nobody → cpearce
(Assignee)

Comment 1

3 years ago
(In reply to Chris Pearce (:cpearce) from comment #0)
> This should be backwards compatible; if the installed CDM is version 9, but
> we query for version 7, then we'll report that version 7 is supported, but
> we'll instantiate version 9 (we'll probably only have the latest version of
> the CDM installed on disk anyway).

A clearer way to describe this, is to say that the version in the keySystem string is the "minimum acceptable" version.
(Assignee)

Comment 2

3 years ago
Created attachment 8566343 [details] [diff] [review]
Patch 1: Expose the version of available GMPs

Make the "Version" field from GMPs ".info" file accessible outside of GMPParent.
Attachment #8566343 - Flags: review?(rjesup)
(Assignee)

Comment 3

3 years ago
Created attachment 8566344 [details] [diff] [review]
Patch 2: Rename EMELog.h to EMEUtils.h

Rename EMELog.h to EMEUtils.h.

I'm going to stick a utility function in there in my next patch...
Attachment #8566344 - Flags: review?(bzbarsky)
(Assignee)

Comment 4

3 years ago
Created attachment 8566345 [details] [diff] [review]
Patch 3: Utility function to parse the CDM version from keySystem string

Add a utility function to parse the keySystem string handed to us by JavaScript, and extract the CDM version from said string. We'll use this in navigator.requestKeySystemAccess to validate the keySystem string JavaScript passes to us.
Attachment #8566345 - Flags: review?(bzbarsky)
(Assignee)

Comment 5

3 years ago
Created attachment 8566347 [details] [diff] [review]
Patch 4: Enforce CDM minimum version

Parse the keySystem string to extract the minimum CDM version and enforce that we're using at least hat.

Send a notification to chrome when script requests a version that we don't support. We should initiate a check-for-updates in the CDM downloader, if we've not done recently.
Attachment #8566347 - Flags: review?(bzbarsky)
Attachment #8566347 - Flags: feedback?(gijskruitbosch+bugs)
(Assignee)

Comment 7

3 years ago
Jesup: Note here I'm enforcing that the version numbers for EME GMPs are whole integers, whereas OpenH264 is using a major.minor (e.g. 2.1) format. I didn't not do anything to enforce that in GMPs in general, as I didn't want to break OpenH264. If using a different versioning number format for EME GMPs is an issue, let me know...
Flags: needinfo?(rjesup)
Comment on attachment 8566344 [details] [diff] [review]
Patch 2: Rename EMELog.h to EMEUtils.h

r=me
Attachment #8566344 - Flags: review?(bzbarsky) → review+
Comment on attachment 8566345 [details] [diff] [review]
Patch 3: Utility function to parse the CDM version from keySystem string

Worth documenting what aOutKeySystem is going to be, as well as what the return value of the function means and when aOutMinCDMVersion will be set to NO_CDM_VERSION.

And maybe use a pointer, not reference, for the int outparam?  Not sure what style is in this part of the code.

>+  nsAString::const_iterator iter, end;

Why not just work with char16_t*?

>+    NS_WARNING("Input KeySystem including was suspiciously long");

So this is only allowing 4 bytes for primetime versions before it starts producing warnings on clearkey, right?  Worse yet, if we add more keysystems this will be even mor likely to warn. Why not do this check after doing the prefix check?

>+  if (!FindInReadable(aExpectedKeySystem, iterStart, iterEnd) ||
>+      iterStart != start) {

Please don't reinvent StringBeginsWith.  Just use it here.  That won't give you the iterEnd bit, bit that's easy: if StringBeginsWith passes, it's aInputKeySystem.BeginReading() + aExpectedKeySystem.Length().  Which you can then compare to aInputKeySystem.EndReading() as needed, working with char16_t* the whole time instead of string iterators.

r=me modulo the above.
Attachment #8566345 - Flags: review?(bzbarsky) → review+
Comment on attachment 8566347 [details] [diff] [review]
Patch 4: Enforce CDM minimum version

r=me
Attachment #8566347 - Flags: review?(bzbarsky) → review+

Comment 11

3 years ago
Comment on attachment 8566347 [details] [diff] [review]
Patch 4: Enforce CDM minimum version

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

If we need this, sure... but there is no UI provision for this, it seems. :-(

I'll ask in the notification bar bug.
Attachment #8566347 - Flags: feedback?(gijskruitbosch+bugs) → feedback+

Updated

3 years ago
Attachment #8566343 - Flags: review?(rjesup) → review+
(Assignee)

Updated

3 years ago
Blocks: 1137045
Jorge - There is one UUID change to dom/media/gmp/mozIGeckoMediaPluginService.idl in this set of patches. If we were to uplift this set of patches in Beta 2 or Beta 3, do you have any AMO data that can tell us of expected add-on breakage as a result?
Flags: needinfo?(jorge)
The main concern with beta uplifts is binary compatibility, and that's not something that normally affects AMO add-ons or can be easily predicted by us. I don't know if this interface change will break any binary add-ons. If we need this change, I suggest we uplift it as early as possible to mitigate its impact on people building on beta.
Flags: needinfo?(jorge)
It seems pretty unlikely that binary addons would be using this interface, fwiw.
(Assignee)

Comment 18

3 years ago
Created attachment 8572379 [details] [diff] [review]
Patch 4 - Beta patch

Patch for beta branch as part of EME platform uplift.
(Assignee)

Comment 19

3 years ago
Created attachment 8572380 [details] [diff] [review]
Patch 3 - Beta patch

Patch for beta branch as part of EME platform uplift.
(Assignee)

Comment 20

3 years ago
Created attachment 8572381 [details] [diff] [review]
Patch 2 - Beta patch

Patch for beta branch as part of EME platform uplift.
(Assignee)

Comment 21

3 years ago
Created attachment 8572382 [details] [diff] [review]
Patch 1 - Beta patch

Patch for beta branch as part of EME platform uplift.
(Assignee)

Comment 22

3 years ago
Comment on attachment 8572379 [details] [diff] [review]
Patch 4 - Beta patch

Requesting retroactive approval for Beta landing as part of EME platform uplift.
Attachment #8572379 - Attachment description: Patch 1 - Beta patch → Patch 4 - Beta patch
Attachment #8572379 - Flags: approval-mozilla-beta?
(Assignee)

Comment 23

3 years ago
Comment on attachment 8572380 [details] [diff] [review]
Patch 3 - Beta patch

Requesting retroactive approval for Beta landing as part of EME platform uplift.
Attachment #8572380 - Attachment description: Patch 1 - Beta patch → Patch 3 - Beta patch
Attachment #8572380 - Flags: approval-mozilla-beta?
(Assignee)

Comment 24

3 years ago
Comment on attachment 8572381 [details] [diff] [review]
Patch 2 - Beta patch

Requesting retroactive approval for Beta landing as part of EME platform uplift.
Attachment #8572381 - Attachment description: Patch 1 - Beta patch → Patch 2 - Beta patch
Attachment #8572381 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 25

3 years ago
Comment on attachment 8572382 [details] [diff] [review]
Patch 1 - Beta patch

Requesting retroactive approval for Beta landing as part of EME platform uplift.
Attachment #8572382 - Flags: approval-mozilla-beta?
(Assignee)

Updated

3 years ago
Flags: needinfo?(rjesup)
(Assignee)

Comment 26

3 years ago
Comment on attachment 8572379 [details] [diff] [review]
Patch 4 - Beta patch

Also need approval for landing on Aurora.
Attachment #8572379 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 27

3 years ago
Comment on attachment 8572380 [details] [diff] [review]
Patch 3 - Beta patch

Also need approval for landing on Aurora.
Attachment #8572380 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 28

3 years ago
Comment on attachment 8572381 [details] [diff] [review]
Patch 2 - Beta patch

Requesting retroactive approval for Beta landing as part of EME platform uplift.
Attachment #8572381 - Flags: approval-mozilla-beta?
(Assignee)

Comment 29

3 years ago
Comment on attachment 8572382 [details] [diff] [review]
Patch 1 - Beta patch

Also need approval for landing on Aurora.
Attachment #8572382 - Flags: approval-mozilla-aurora?
Comment on attachment 8572382 [details] [diff] [review]
Patch 1 - Beta patch

Approved for Beta and Aurora as part of EME platform uplift.
Attachment #8572382 - Flags: approval-mozilla-beta?
Attachment #8572382 - Flags: approval-mozilla-beta+
Attachment #8572382 - Flags: approval-mozilla-aurora?
Attachment #8572382 - Flags: approval-mozilla-aurora+
Attachment #8572381 - Flags: approval-mozilla-beta?
Attachment #8572381 - Flags: approval-mozilla-beta+
Attachment #8572381 - Flags: approval-mozilla-aurora?
Attachment #8572381 - Flags: approval-mozilla-aurora+
Attachment #8572380 - Flags: approval-mozilla-beta?
Attachment #8572380 - Flags: approval-mozilla-beta+
Attachment #8572380 - Flags: approval-mozilla-aurora?
Attachment #8572380 - Flags: approval-mozilla-aurora+
Attachment #8572379 - Flags: approval-mozilla-beta?
Attachment #8572379 - Flags: approval-mozilla-beta+
Attachment #8572379 - Flags: approval-mozilla-aurora?
Attachment #8572379 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.