Closed
Bug 1303922
Opened 8 years ago
Closed 8 years ago
[EME] Aggregate keystatus changes into a single keystatuseschanged event
Categories
(Core :: Audio/Video: Playback, defect, P3)
Core
Audio/Video: Playback
Tracking
()
RESOLVED
FIXED
mozilla52
Tracking | Status | |
---|---|---|
firefox52 | --- | fixed |
People
(Reporter: cpearce, Assigned: kikuo)
References
(Blocks 1 open bug, )
Details
Attachments
(5 files)
1.99 KB,
patch
|
Details | Diff | Splinter Review | |
38.11 KB,
patch
|
cpearce
:
feedback+
|
Details | Diff | Splinter Review |
58 bytes,
text/x-review-board-request
|
cpearce
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
cpearce
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
cpearce
:
review+
|
Details |
ddorwin pointed out that Firefox is firing extra keystatuseschanged events: https://github.com/w3c/web-platform-tests/pull/3746 The Update Key Statuses algorithm requires us to fire a single keystatuseschanged event when key statuses change: https://w3c.github.io/encrypted-media/#update-key-statuses However we're currently firing one keystatuseschanged event for every key that changes. This is resulting in a behaviour difference from Widevine in Chrome. The GMP API handles key statuses changes on a per key basis: http://searchfox.org/mozilla-central/source/dom/media/gmp/gmp-api/gmp-decryption.h#183 Whereas the Chromium CDM API handles key status changes in aggregate: https://cs.chromium.org/chromium/src/media/cdm/api/content_decryption_module.h?q=content_decr&sq=package:chromium&l=742 And we adapt the aggregate and un-aggregate it here: http://searchfox.org/mozilla-central/source/dom/media/gmp/widevine-adapter/WidevineDecryptor.cpp#412 We need to add a function to the end of the GMPDecryptorCallback interface (so the v-table layout of earlier functions doesn't change, so we don't break the Adobe CDM) that accepts key statuses changes in aggregate, and ensures we don't fire multiple keystatuseschanged events.
Reporter | ||
Comment 1•8 years ago
|
||
Blake: Can someone in Taipei handle this? I this will likely help with Web Platform Tests, so is Kilik available? Thanks!
Flags: needinfo?(bwu)
Comment 2•8 years ago
|
||
Cool. He is going to have a lot of fun in it. :-)
Assignee: nobody → kikuo
Flags: needinfo?(bwu)
Reporter | ||
Comment 3•8 years ago
|
||
This causes https://w3c-test.org/encrypted-media/clearkey-keystatuses-multiple-sessions.html to fail.
Assignee | ||
Comment 4•8 years ago
|
||
(In reply to Chris Pearce (:cpearce) from comment #3) > This causes > https://w3c-test.org/encrypted-media/clearkey-keystatuses-multiple-sessions. > html to fail. Weird, I've tested nightly with & without modification in Bug 1289968, all pass ! I saw there's only single KeyStatusChanged during |ClearKeySessionManager::UpdateSession| per session id. Despite of the above, I'll implement a new API as Chris said in Comment 0.
Reporter | ||
Comment 5•8 years ago
|
||
You're right, clearkey-keystatuses-multiple-sessions.html passes. I must have pasted the wrong link, as the following test fails: https://w3c-test.org/encrypted-media/drm-keystatuses.html This test passes if I apply the following hacky patch. This patch is merely designed to prove that we need to make this change for the test to pass; it's not a proper solution! We should fix the issue properly, rather than doing something like this hack. It's useful to diagnose the problem however.
Assignee | ||
Comment 6•8 years ago
|
||
This WIP can pass wpt clearkey-keystatuses.html and each single EME mochitest run. But GMP child crashed during a full EME mochitest run, I'm still investigating. By this WIP, our ClearKey/Widevine implementation shall only invoke new interface |BatchedKeyStatusChanged| in the future and won't invoke |ForgetKeyStatus|/|KeyStatusChanged| anymore. These interfaces will be kept for backward-capability. I'll make this patch separated in smaller ones for review once I find the cause of crash and test Clearkey & Widevine on Windows. Hi Chris, it would be appreciated if you could give me some feedback regarding this patch. or is there any concern about the data structure used in GMP IPDL I need to know ? Thanks.
Attachment #8795798 -
Flags: feedback?(cpearce)
Assignee | ||
Comment 7•8 years ago
|
||
Comment on attachment 8795798 [details] [diff] [review] [WIP] Add web-platform-test (clearkey-keystatus.html) for mutli-keyIds in single session & notify statuses in batch. Review of attachment 8795798 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/gmp/widevine-adapter/WidevineDecryptor.cpp @@ +409,5 @@ > return; > } > Log("Decryptor::OnSessionKeysChange()"); > + > + GMPMediaKeyInfo* key_infos = new GMPMediaKeyInfo[aKeysInfoCount]; Oh! Should check aKeysInfoCount > 0, and assert for null here. ::: media/gmp-clearkey/0.1/ClearKeySessionManager.cpp @@ +170,5 @@ > uint32_t numKeys = aKeyDataSize / (2 * CLEARKEY_KEY_LEN); > + > + // GMPMediaKeyInfo shall be copied in GMPDecryptorChild and > + // be deleted after the invocation of BatchedKeyStatusChanged. > + GMPMediaKeyInfo* key_infos = new GMPMediaKeyInfo[numKeys]; ditto. @@ +228,5 @@ > } > > + // GMPMediaKeyInfo shall be copied in GMPDecryptorChild and > + // be deleted after the invocation of BatchedKeyStatusChanged. > + GMPMediaKeyInfo* key_infos = new GMPMediaKeyInfo[keyPairs.size()]; ditto
Reporter | ||
Comment 8•8 years ago
|
||
Comment on attachment 8795798 [details] [diff] [review] [WIP] Add web-platform-test (clearkey-keystatus.html) for mutli-keyIds in single session & notify statuses in batch. Review of attachment 8795798 [details] [diff] [review]: ----------------------------------------------------------------- Looks good! ::: dom/media/gmp/GMPDecryptorChild.cpp @@ +183,5 @@ > + for (uint32_t i = 0; i < aKeyInfosLength; i++) { > + GMPKeyInformation gmpKeyInfo; > + gmpKeyInfo.keyId().AppendElements(aKeyInfos[i].keyid, aKeyInfos[i].keyid_size); > + gmpKeyInfo.status() = aKeyInfos[i].status; > + keyInfos.AppendElement(gmpKeyInfo); I would have expected IPDL to generate a GMPKeyInformation constructor that you could use more conveniently here. ::: dom/media/gmp/PGMPDecryptor.ipdl @@ +81,1 @@ > async KeyStatusChanged(nsCString aSessionId, uint8_t[] aKey, You could remove the (unbatched) KeyStatusChanged IPDL message here, and have the implementation of a the (unbatched) GMPDecryptorCallback::KeyStatusChanged() dispatched the batched IPDL message with only a single status change. That is, you can express a single key status change as a batch of size 1. We still need the unbatched KeyStatusChanged for as long as we need to maintain compatibility with the Adobe CDM, but the IPDL code can at least get by with only one thing called .*KeyStatusChanged. ::: media/gmp-clearkey/0.1/ClearKeySession.cpp @@ +44,5 @@ > + uint32_t numKeys = mKeyIds.size(); > + if (numKeys > 0) { > + // GMPMediaKeyInfo shall be copied in GMPDecryptorChild and > + // be deleted after the invocation of BatchedKeyStatusChanged. > + GMPMediaKeyInfo* key_infos = new GMPMediaKeyInfo[numKeys]; It's safer to use a std::vector<GMPMediaKeyInfo> here, so you don't have to worry about deleting the new'd memory. ::: media/gmp-clearkey/0.1/ClearKeySessionManager.cpp @@ +170,5 @@ > uint32_t numKeys = aKeyDataSize / (2 * CLEARKEY_KEY_LEN); > + > + // GMPMediaKeyInfo shall be copied in GMPDecryptorChild and > + // be deleted after the invocation of BatchedKeyStatusChanged. > + GMPMediaKeyInfo* key_infos = new GMPMediaKeyInfo[numKeys]; Safer to use std::vector over raw arrays here. @@ +228,5 @@ > } > > + // GMPMediaKeyInfo shall be copied in GMPDecryptorChild and > + // be deleted after the invocation of BatchedKeyStatusChanged. > + GMPMediaKeyInfo* key_infos = new GMPMediaKeyInfo[keyPairs.size()]; Safer to use std::vector over raw arrays here. ::: testing/web-platform/meta/MANIFEST.json @@ +37385,5 @@ > "path": "editing/other/restoration.html", > "url": "/editing/other/restoration.html" > } > ], > + "encrypted-media/clearkey-keystatuses.html": [ This test should be pulled in naturally; James Graham pulls in upstream WPT every few weeks. That is, you shouldn't need to add this; it'll come in soon.
Attachment #8795798 -
Flags: feedback?(cpearce) → feedback+
Assignee | ||
Comment 9•8 years ago
|
||
(In reply to Chris Pearce (:cpearce) from comment #8) > @@ +81,1 @@ > > async KeyStatusChanged(nsCString aSessionId, uint8_t[] aKey, > > You could remove the (unbatched) KeyStatusChanged IPDL message here, and > have the implementation of a the (unbatched) > GMPDecryptorCallback::KeyStatusChanged() dispatched the batched IPDL message > with only a single status change. > > That is, you can express a single key status change as a batch of size 1. We > still need the unbatched KeyStatusChanged for as long as we need to maintain > compatibility with the Adobe CDM, but the IPDL code can at least get by with > only one thing called .*KeyStatusChanged. > Great thanks for the feedback, IIUC, we can also achieve all keystatus notifications by "async BatchedKeyStatusChange" only, that is, both "async KeyStatusChanged(...)" and "async ForgetKeyStatus(...)" IPDL messages can be removed. Here's the reason, we're going to remove(ForgetKeyStatus) the MediaKey when receiving kGMPUnknown status in GMPDecryptorChild, however, other statuses will be delivered by |KeyStatusChagned| IPDL message. I think we can let the decision-making code happening in GMPDecryptorParent. Of course, we should keep |KeyStatusChanged| in gmp-decryption.h, because this interface was exposed to Adobe CDM already, but |ForgetKeyStatus| wasn't. In short, in my opinion, it would be simpler to send just GMPMediaKeyStatus from Child to Parent, and the following diverged actions (forget/update) are performed in Parent. For Adobe CDMs, they know the key will be removed by returning the status kGMPUnknown [1]. [1] http://searchfox.org/mozilla-central/source/dom/media/gmp/gmp-api/gmp-decryption.h#99 Does it make sense to you ?
Flags: needinfo?(cpearce)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Attachment #8796501 -
Flags: review?(cpearce)
Reporter | ||
Comment 12•8 years ago
|
||
(In reply to Kilik Kuo [:kikuo] from comment #9) > (In reply to Chris Pearce (:cpearce) from comment #8) > > @@ +81,1 @@ > > > async KeyStatusChanged(nsCString aSessionId, uint8_t[] aKey, > > > > You could remove the (unbatched) KeyStatusChanged IPDL message here, and > > have the implementation of a the (unbatched) > > GMPDecryptorCallback::KeyStatusChanged() dispatched the batched IPDL message > > with only a single status change. > > > > That is, you can express a single key status change as a batch of size 1. We > > still need the unbatched KeyStatusChanged for as long as we need to maintain > > compatibility with the Adobe CDM, but the IPDL code can at least get by with > > only one thing called .*KeyStatusChanged. > > > > Great thanks for the feedback, IIUC, we can also achieve all keystatus > notifications by "async BatchedKeyStatusChange" only, that is, both "async > KeyStatusChanged(...)" and "async ForgetKeyStatus(...)" IPDL messages can be > removed. > > Here's the reason, > we're going to remove(ForgetKeyStatus) the MediaKey when receiving > kGMPUnknown status in GMPDecryptorChild, however, other statuses will be > delivered by |KeyStatusChagned| IPDL message. > I think we can let the decision-making code happening in GMPDecryptorParent. > Of course, we should keep |KeyStatusChanged| in gmp-decryption.h, because > this interface was exposed to Adobe CDM already, but |ForgetKeyStatus| > wasn't. > > In short, in my opinion, it would be simpler to send just GMPMediaKeyStatus > from Child to Parent, and the following diverged actions (forget/update) are > performed in Parent. > For Adobe CDMs, they know the key will be removed by returning the status > kGMPUnknown [1]. > > [1] > http://searchfox.org/mozilla-central/source/dom/media/gmp/gmp-api/gmp- > decryption.h#99 > > Does it make sense to you ? Yes, sounds good.
Flags: needinfo?(cpearce)
Reporter | ||
Comment 13•8 years ago
|
||
mozreview-review |
Comment on attachment 8796501 [details] Bug 1303922-[Part1] Make EME keystatuschanged information notified in batch. https://reviewboard.mozilla.org/r/82350/#review81174 r+ with comments addressed. ::: dom/media/eme/CDMProxy.h:44 (Diff revision 1) > + : mKeyId(aKeyId) > + , mStatus(aStatus) > + , mShouldForget(aForget) > + {} > + nsTArray<uint8_t> mKeyId; > + dom::MediaKeyStatus mStatus; Can you instead make mStatus a dom::Optional<>, and pass the status in the case where we don't remove the status, and not pass the status in the case where we do? Then you don't need the mShouldForget field here, and you can simplify the logic a bit I think. ::: media/gmp-clearkey/0.1/ClearKeySession.cpp:45 (Diff revision 1) > ClearKeySession::~ClearKeySession() > { > CK_LOGD("ClearKeySession dtor %p", this); > > - auto& keyIds = GetKeyIds(); > - for (auto it = keyIds.begin(); it != keyIds.end(); it++) { > + std::vector<GMPMediaKeyInfo> key_infos; > + for (size_t i = 0; i < mKeyIds.size(); i++) { for (const KeyId& keyId : mKeyIds) {
Attachment #8796501 -
Flags: review?(cpearce) → review+
Reporter | ||
Comment 14•8 years ago
|
||
mozreview-review |
Comment on attachment 8796502 [details] Bug 1303922-[Part2] Remove KeyStatusChanged/ForgetKeyStatus IPDL messages. https://reviewboard.mozilla.org/r/82352/#review81176
Attachment #8796502 -
Flags: review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 17•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8796501 [details] Bug 1303922-[Part1] Make EME keystatuschanged information notified in batch. https://reviewboard.mozilla.org/r/82350/#review81174 Thanks for the reivew ! > Can you instead make mStatus a dom::Optional<>, and pass the status in the case where we don't remove the status, and not pass the status in the case where we do? Then you don't need the mShouldForget field here, and you can simplify the logic a bit I think. I've done the modification and added a copy constructor for CDMKeyInfo in order to behave correctly during nsTArray's operation. Took me some time to understand what's going on :)
Assignee | ||
Comment 18•8 years ago
|
||
mozreview-review |
Comment on attachment 8796501 [details] Bug 1303922-[Part1] Make EME keystatuschanged information notified in batch. https://reviewboard.mozilla.org/r/82348/#review81682 https://treeherder.mozilla.org/#/jobs?repo=try&revision=8d3230550dca Hmm, I should add keyword "explicit" in front of the constructor. do it again !
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 22•8 years ago
|
||
mozreview-review |
Comment on attachment 8797637 [details] Bug 1303922-[Part3] Remove unexpected meta file for web platform test 'encrypted-media/Google/encrypted-media-keystatuses.html' https://reviewboard.mozilla.org/r/83292/#review81774 This web-platform-test is enabled on Windows, and the result will be PASS after applying the patch Part1. Should also remove the meta file in this bug to avoid try run failure.
Assignee | ||
Updated•8 years ago
|
Attachment #8797637 -
Flags: review?(cpearce)
Reporter | ||
Comment 23•8 years ago
|
||
mozreview-review |
Comment on attachment 8797637 [details] Bug 1303922-[Part3] Remove unexpected meta file for web platform test 'encrypted-media/Google/encrypted-media-keystatuses.html' https://reviewboard.mozilla.org/r/83292/#review81964
Attachment #8797637 -
Flags: review?(cpearce) → review+
Assignee | ||
Comment 24•8 years ago
|
||
(In reply to Chris Pearce (:cpearce) from comment #23) > Comment on attachment 8797637 [details] > Bug 1303922-[Part3] Remove unexpected meta file for web platform test > 'encrypted-media/Google/encrypted-media-keystatuses.html' > > https://reviewboard.mozilla.org/r/83292/#review81964 Thanks !
Assignee | ||
Comment 25•8 years ago
|
||
Try run, https://treeherder.mozilla.org/#/jobs?repo=try&revision=620133a562e0 , looks ok.
Keywords: checkin-needed
Comment 26•8 years ago
|
||
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/e8c434e31c2d Part 1: Make EME keystatuschanged information notified in batch. r=cpearce https://hg.mozilla.org/integration/mozilla-inbound/rev/67708538358d Part 2: Remove KeyStatusChanged/ForgetKeyStatus IPDL messages. r=cpearce https://hg.mozilla.org/integration/mozilla-inbound/rev/58286ce1d3d1 Part 3: Remove unexpected meta file for web platform test 'encrypted-media/Google/encrypted-media-keystatuses.html'. r=cpearce
Keywords: checkin-needed
Comment 27•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/e8c434e31c2d https://hg.mozilla.org/mozilla-central/rev/67708538358d https://hg.mozilla.org/mozilla-central/rev/58286ce1d3d1
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox52:
--- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in
before you can comment on or make changes to this bug.
Description
•