Closed
Bug 1124031
Opened 9 years ago
Closed 9 years ago
[EME] Make CDM version accessible in Navigator.RequestMediaKeysystemAccess() keysystemId
Categories
(Core :: Audio/Video, defect)
Tracking
()
RESOLVED
FIXED
mozilla38
People
(Reporter: cpearce, Assigned: cpearce)
References
(Blocks 1 open bug)
Details
Attachments
(8 files)
6.78 KB,
patch
|
jesup
:
review+
|
Details | Diff | Splinter Review |
5.14 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
7.32 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
9.32 KB,
patch
|
bzbarsky
:
review+
Gijs
:
feedback+
|
Details | Diff | Splinter Review |
9.51 KB,
patch
|
lmandel
:
approval-mozilla-aurora+
lmandel
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
7.55 KB,
patch
|
lmandel
:
approval-mozilla-aurora+
lmandel
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
5.26 KB,
patch
|
lmandel
:
approval-mozilla-aurora+
lmandel
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
7.43 KB,
patch
|
lmandel
:
approval-mozilla-aurora+
lmandel
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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•9 years ago
|
Assignee: nobody → cpearce
Assignee | ||
Comment 1•9 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•9 years ago
|
||
Make the "Version" field from GMPs ".info" file accessible outside of GMPParent.
Attachment #8566343 -
Flags: review?(rjesup)
Assignee | ||
Comment 3•9 years ago
|
||
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•9 years ago
|
||
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•9 years ago
|
||
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 6•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6d3d1278b1da
Assignee | ||
Comment 7•9 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 8•9 years ago
|
||
Comment on attachment 8566344 [details] [diff] [review] Patch 2: Rename EMELog.h to EMEUtils.h r=me
Attachment #8566344 -
Flags: review?(bzbarsky) → review+
Comment 9•9 years ago
|
||
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 10•9 years ago
|
||
Comment on attachment 8566347 [details] [diff] [review] Patch 4: Enforce CDM minimum version r=me
Attachment #8566347 -
Flags: review?(bzbarsky) → review+
Comment 11•9 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•9 years ago
|
Attachment #8566343 -
Flags: review?(rjesup) → review+
Assignee | ||
Comment 12•9 years ago
|
||
Thanks for quick reviews! https://hg.mozilla.org/integration/mozilla-inbound/rev/0c200f94c3cf https://hg.mozilla.org/integration/mozilla-inbound/rev/f0e5c16136ca https://hg.mozilla.org/integration/mozilla-inbound/rev/b37b1cc5163f https://hg.mozilla.org/integration/mozilla-inbound/rev/138f7daa8b62
Comment 13•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/0c200f94c3cf https://hg.mozilla.org/mozilla-central/rev/f0e5c16136ca https://hg.mozilla.org/mozilla-central/rev/b37b1cc5163f https://hg.mozilla.org/mozilla-central/rev/138f7daa8b62
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox38:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Assignee | ||
Updated•9 years ago
|
Blocks: eme-platform-uplift
Comment 14•9 years ago
|
||
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)
Comment 15•9 years ago
|
||
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)
Comment 16•9 years ago
|
||
It seems pretty unlikely that binary addons would be using this interface, fwiw.
Assignee | ||
Comment 17•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-beta/rev/f0b35fc2bfbf https://hg.mozilla.org/releases/mozilla-beta/rev/cee66f9d30e7 https://hg.mozilla.org/releases/mozilla-beta/rev/16dddf827464 https://hg.mozilla.org/releases/mozilla-beta/rev/6437b406a0fa
status-firefox37:
--- → fixed
Assignee | ||
Comment 18•9 years ago
|
||
Patch for beta branch as part of EME platform uplift.
Assignee | ||
Comment 19•9 years ago
|
||
Patch for beta branch as part of EME platform uplift.
Assignee | ||
Comment 20•9 years ago
|
||
Patch for beta branch as part of EME platform uplift.
Assignee | ||
Comment 21•9 years ago
|
||
Patch for beta branch as part of EME platform uplift.
Assignee | ||
Comment 22•9 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•9 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•9 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•9 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•9 years ago
|
Flags: needinfo?(rjesup)
Assignee | ||
Comment 26•9 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•9 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•9 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•9 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 30•9 years ago
|
||
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+
Updated•9 years ago
|
Attachment #8572381 -
Flags: approval-mozilla-beta?
Attachment #8572381 -
Flags: approval-mozilla-beta+
Attachment #8572381 -
Flags: approval-mozilla-aurora?
Attachment #8572381 -
Flags: approval-mozilla-aurora+
Updated•9 years ago
|
Attachment #8572380 -
Flags: approval-mozilla-beta?
Attachment #8572380 -
Flags: approval-mozilla-beta+
Attachment #8572380 -
Flags: approval-mozilla-aurora?
Attachment #8572380 -
Flags: approval-mozilla-aurora+
Updated•9 years ago
|
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.
Description
•