1303922 - [EME] Aggregate keystatus changes into a single keystatuseschanged event

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, defect, P3)

Description

[Chris Pearce [:cpearce (Not reading bugmail)]]

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.

Comment 1

[Chris Pearce [:cpearce (Not reading bugmail)]]

Blake: Can someone in Taipei handle this? I this will likely help with Web Platform Tests, so is Kilik available? Thanks!

Comment 2

[Blake Wu [:bwu][:blakewu]]

Cool. He is going to have a lot of fun in it. :-)

Comment 3

[Chris Pearce [:cpearce (Not reading bugmail)]]

This causes https://w3c-test.org/encrypted-media/clearkey-keystatuses-multiple-sessions.html to fail.

Comment 4

[Kilik Kuo [:kikuo] (inactive)]

(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.

Comment 5

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch: Work around

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.

Comment 6

[Kilik Kuo [:kikuo] (inactive)]

Attachment: [WIP] Add web-platform-test (clearkey-keystatus.html) for mutli-keyIds in single session & notify statuses in batch.

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.

Comment 7

[Kilik Kuo [:kikuo] (inactive)]

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 8

[Chris Pearce [:cpearce (Not reading bugmail)]]

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.

Comment 9

[Kilik Kuo [:kikuo] (inactive)]

(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 ?

Comment 10

[Kilik Kuo [:kikuo] (inactive)]

Attachment: Bug 1303922-[Part1] Make EME keystatuschanged information notified in batch.

Review commit: https://reviewboard.mozilla.org/r/82350/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/82350/

Comment 11

[Kilik Kuo [:kikuo] (inactive)]

Attachment: Bug 1303922-[Part2] Remove KeyStatusChanged/ForgetKeyStatus IPDL messages.

Review commit: https://reviewboard.mozilla.org/r/82352/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/82352/

Comment 12

[Chris Pearce [:cpearce (Not reading bugmail)]]

(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.

Comment 13

[Chris Pearce [:cpearce (Not reading bugmail)]]

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) {

Comment 14

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8796502 [details]
Bug 1303922-[Part2] Remove KeyStatusChanged/ForgetKeyStatus IPDL messages.

https://reviewboard.mozilla.org/r/82352/#review81176

Comment 15

[Kilik Kuo [:kikuo] (inactive)]

Comment on attachment 8796501 [details]
Bug 1303922-[Part1] Make EME keystatuschanged information notified in batch.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/82350/diff/1-2/

Comment 16

[Kilik Kuo [:kikuo] (inactive)]

Comment on attachment 8796502 [details]
Bug 1303922-[Part2] Remove KeyStatusChanged/ForgetKeyStatus IPDL messages.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/82352/diff/1-2/

Comment 17

[Kilik Kuo [:kikuo] (inactive)]

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 18

[Kilik Kuo [:kikuo] (inactive)]

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 19

[Kilik Kuo [:kikuo] (inactive)]

Comment on attachment 8796501 [details]
Bug 1303922-[Part1] Make EME keystatuschanged information notified in batch.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/82350/diff/2-3/

Comment 20

[Kilik Kuo [:kikuo] (inactive)]

Comment on attachment 8796502 [details]
Bug 1303922-[Part2] Remove KeyStatusChanged/ForgetKeyStatus IPDL messages.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/82352/diff/2-3/

Comment 21

[Kilik Kuo [:kikuo] (inactive)]

Attachment: Bug 1303922-[Part3] Remove unexpected meta file for web platform test 'encrypted-media/Google/encrypted-media-keystatuses.html'

Review commit: https://reviewboard.mozilla.org/r/83292/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/83292/

Comment 22

[Kilik Kuo [:kikuo] (inactive)]

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.

Comment 23

[Chris Pearce [:cpearce (Not reading bugmail)]]

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

Comment 24

[Kilik Kuo [:kikuo] (inactive)]

(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 !

Comment 25

[Kilik Kuo [:kikuo] (inactive)]

Try run, https://treeherder.mozilla.org/#/jobs?repo=try&revision=620133a562e0 , looks ok.

Comment 26

[Pulsebot]

Pushed by [e-mail]:
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

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e8c434e31c2d
https://hg.mozilla.org/mozilla-central/rev/67708538358d
https://hg.mozilla.org/mozilla-central/rev/58286ce1d3d1