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)

Product:
Core
Component:
Audio/Video: Playback
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla52
Tracking Flags:
Tracking Status
firefox52 --- fixed

People

(Reporter: cpearce, Assigned: kikuo)

References

(Blocks 1 open bug,
URL
)

Details

Attachments

(5 files)

Patch: Work around
1.99 KB, patch
Details | Diff | Splinter Review
[WIP] Add web-platform-test (clearkey-keystatus.html) for mutli-keyIds in single session & notify statuses in batch.
38.11 KB, patch
cpearce
: feedback+
Details | Diff | Splinter Review
Bug 1303922-[Part1] Make EME keystatuschanged information notified in batch.
58 bytes, text/x-review-board-request
cpearce
: review+
Details
Bug 1303922-[Part2] Remove KeyStatusChanged/ForgetKeyStatus IPDL messages.
58 bytes, text/x-review-board-request
cpearce
: review+
Details
Bug 1303922-[Part3] Remove unexpected meta file for web platform test 'encrypted-media/Google/encrypted-media-keystatuses.html'
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.
Blake: Can someone in Taipei handle this? I this will likely help with Web Platform Tests, so is Kilik available? Thanks!
Flags: needinfo?(bwu)
Cool. He is going to have a lot of fun in it. :-)
Assignee: nobody → kikuo
Flags: needinfo?(bwu)
(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.
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.
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)
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
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+
(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)
Attachment #8796501 - Flags: review?(cpearce)
(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)
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+
Comment on attachment 8796502 [details]
Bug 1303922-[Part2] Remove KeyStatusChanged/ForgetKeyStatus IPDL messages.

https://reviewboard.mozilla.org/r/82352/#review81176
Attachment #8796502 - Flags: review+
Blocks: 1306572
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 :)
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 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.
Attachment #8797637 - Flags: review?(cpearce)
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+
(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 !
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
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.

Attachment

General

Creator:
Created:
Updated:
Size: