1075199 - [EME] Make a ClearKey GMP for mochitests that exercises whitelisted APIs

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

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

We need to make a clone of Edwin's ClearKey GMP, and run mochitests over this one too. This need to exercise the video decoding APIs using WMF on Windows, and a BlankDecoder on other platforms. This is to test that the GMP{Video,Audio}Decoder implementations work correctly and don't regress.

We still want to run tests on the ClearKey GMP that we're shipping, so we'll need to run our tests twice; once with the shippable ClearKey GMP, and once more with the decoding GMP.

Comment 1

[Chris Peterson [:cpeterson]]

This bug is necessary for basic playback of EME video on Windows.

Comment 2

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

I think what we should actually do here is make the in-tree gmp-clearkey plugin use WMF to decode video data on Windows. This means we'll test the DECRYPT_AND_DECODE path on Windows, which is what our partner's CDMs will use. This also means we won't need to run our tests twice.

Originally I thought we'd need to have a separate GMP so that we could exercise the Output Protection APIs, but we ended up doing this in gtests in the gmp-fake plugin.

For now, we can not worry about Mac and Linux. In theory we could use the Mac system decoders inside gmp-clearkey in future, but on Linux I don't think we can rely on that. Let's just handle the Windows case for now.

Comment 3

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 1: Import gmp-task-utils* from Chris' gmp-clearkey implementation

WIP patches

Comment 4

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 2: WIP - Small refactor of gmp-clearkey to separate decryption and session management

WIP patches

Comment 5

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 3: Import WMF decoding support from Chris' gmp-clearkey implementation

WIP patches

Comment 6

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 4: WIP - WMF decoding in gmp-clearkey

WIP patches

Comment 7

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: clearkey-decrypt-refactor.patch

Still have to track down some gecko-side timing issues with the main patch here, but want to get this refactor landed so it stops bitrotting all the time.

This patch separates decryption and session management in the ClearKey CDM. Decryption state becomes global as there's no nice way to figure out which decoder should have which decryptor.

Comment 8

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Refactor green, apart from red static analysis build; fixed in patch.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a9bfa3fd42b9

Comment 9

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

Comment on attachment 8545086 [details] [diff] [review]
clearkey-decrypt-refactor.patch

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

A couple of issues to work out...

::: media/gmp-clearkey/0.1/ClearKeyDecryptionManager.cpp
@@ +12,2 @@
>  
>  class ClearKeyDecryptor

Now that the Release() doesn't do the thread dispatch, we should make this class extend RefCounted and make its destructor private.

@@ +66,5 @@
> +  }
> +  mDecryptors.clear();
> +}
> +
> +bool 

Nit trailing whitespace here and below.

::: media/gmp-clearkey/0.1/ClearKeyDecryptionManager.h
@@ +24,2 @@
>  
> +  bool SeenKeyId(KeyId aKeyId);

bool HasSeenKeyId(const KeyId& aKeyId) const;
bool IsExpectingKeyForKeyId(const KeyId& aKeyId) const;
bool HasKeyForKeyId(const KeyId& aKeyId) const;

It should be obvious which things are mutating state and which are not, and what things do.

Only HasSeenKeyId needs to be public, the other two can be private.

@@ +30,3 @@
>  
> +  // Create a decryptor for the given KeyId if one does not already exist.
> +  // May create a new decryptor or return an existing one.

The comment " May create a new decryptor or return an existing one." doesn't make any sense; the function returns void, not a decryptor.

::: media/gmp-clearkey/0.1/ClearKeySessionManager.cpp
@@ +85,5 @@
>  
>    const vector<KeyId>& sessionKeys = session->GetKeyIds();
>    vector<KeyId> neededKeys;
>    for (auto it = sessionKeys.begin(); it != sessionKeys.end(); it++) {
> +    if (!GetDecryptionManager()->SeenKeyId(*it)) {

I think you should *always* request the key when creating the session.

Consider this case:

1. session A is created, requests key X.
2. session B is created, also needs key X, but sees it's expected so does not request it.
3. session A closes before key X can be delivered in an update.
4. session B will never receive key X, as it was dependent on the other session to receive it.

However, if you always request they key, you need to handle the case where the new a second key comes in for the same keyId and the key is *different* than the one you have stored. I think you should just reject the promise in that case; it's an edge case anyway.

We also may end up with mochitests running concurrently which expect to always receive a key request, but won't without this change.

@@ +387,5 @@
>      delete it->second;
>    }
>    mSessions.clear();
>  
> +  GetDecryptionManager()->Shutdown();

Won't calling GetDecryptionManager()->Shutdown(); here cause the singleton DecryptorManager to shutdown, taking the keys with it? If there are other ClearKeySessionManagers open that could be bad news.

Perhaps the decryption manager should be refcounted, and the ClearKeySessionManagers take a ref to the singleton in their constructor, and release in their destructor?

@@ +398,3 @@
>  {
> +  CK_LOGD("ClearKeySessionManager::DecryptingComplete");
> +

If we make the DecryptorManager refcounted, we should hold a reference here while we post the Shutdown() task, so that we deref the DecryptorManager here on the main thread, not on the other thread, since RefCounted isn't (yet) threadsafe.

::: media/gmp-clearkey/0.1/ClearKeySessionManager.h
@@ +17,5 @@
> +#include "RefCounted.h"
> +
> +class ClearKeyDecryptor;
> +class ClearKeySessionManager MOZ_FINAL : public GMPDecryptor
> +                                          , public RefCounted

Nit: indentation of by 1.

Comment 10

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: clearkey-decrypt-refactor.patch v2

Comment 11

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

Comment on attachment 8545659 [details] [diff] [review]
clearkey-decrypt-refactor.patch v2

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

::: media/gmp-clearkey/0.1/ClearKeyDecryptionManager.h
@@ +25,2 @@
>  
> +  bool HasSeenKeyId(KeyId aKeyId);

bool HasSeenKeyId(const KeyId& aKeyId) const;
bool HasKeyForKeyId(const KeyId& aKeyId) const;

Is there a reason why these can't be const? Is it because you're using mDecryptors[] instead of mDecryptors->find()? The latter is safer IMHO, since it doesn't modify mDecryptors.

Seriously, I've been bitten by using std::map operator[] in a multithreaded program before, and it's really subtle and hard to track down when it bites. I don't want us to get bitten by this in future. Make these const.

::: media/gmp-clearkey/0.1/ClearKeySessionManager.cpp
@@ +80,5 @@
>  
>    const vector<KeyId>& sessionKeys = session->GetKeyIds();
>    vector<KeyId> neededKeys;
>    for (auto it = sessionKeys.begin(); it != sessionKeys.end(); it++) {
> +    // Need to request this key ID from the client.

We should probably add a comment here saying why we're always requesting, regardless of whether we have already requested the key in another session.

@@ +387,5 @@
>  {
> +  CK_LOGD("ClearKeySessionManager::DecryptingComplete");
> +
> +  RefPtr<ClearKeyDecryptionManager> kungFuDeathGrip(mDecryptionManager);
> +

err, I guess instead of the deathgrip, we could just mDecryptionManager=nullptr here after Join() returns, rather than doing it inside Shutdown().

Comment 12

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: clearkey-decrypt-refactor.patch VEE THREE

moar review spam!

Comment 13

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

Comment on attachment 8545731 [details] [diff] [review]
clearkey-decrypt-refactor.patch VEE THREE

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

Do you actually read review comments?

::: media/gmp-clearkey/0.1/ClearKeySessionManager.cpp
@@ +379,5 @@
>      delete it->second;
>    }
>    mSessions.clear();
>  
> +  mDecryptionManager = nullptr;

Do this only in DecryptingComplete() after the Join().

Comment 14

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: clearkey-decrypt-refactor.patch v4. four.

four.

Comment 15

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b096ec537e81

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b096ec537e81

Comment 17

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Import WMF decoding support from Chris' gmp-clearkey implementation

Comment 18

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Make WMF decoding build and work

Comment 19

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Logging!

Comment 20

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Fix crash deallocating buffers after VideoHost shutdown

Comment 21

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

try push looks green so far: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cc2dc1393196

Comment 22

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

Comment on attachment 8552224 [details] [diff] [review]
Make WMF decoding build and work

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

r+ with nits. Looks good, except for the AnnexB code which needs some bounds checks.

::: media/gmp-clearkey/0.1/AnnexB.cpp
@@ +12,5 @@
> +/* static */ void
> +AnnexB::ConvertFrameInPlace(std::vector<uint8_t>& aBuffer)
> +{
> +  for (size_t i = 0; i < aBuffer.size(); ) {
> +    uint32_t nalLen = BigEndian::readUint32(&aBuffer[i]);

readUint32 could read out of bounds. You need to ensure that i < aBuffer.size - ArrayLength(kAnnexBDelimiter). Maybe change the loop condition?

@@ +33,5 @@
> +  }
> +}
> +
> +/* static */ void
> +AnnexB::ConvertConfig(std::vector<uint8_t>& aBuffer,

Can aBuffer be const?

@@ +37,5 @@
> +AnnexB::ConvertConfig(std::vector<uint8_t>& aBuffer,
> +                      std::vector<uint8_t>& aOutAnnexB)
> +{
> +  // Skip past irrelevant headers
> +  auto& it = aBuffer.begin() + 5;

Ensure that it < aBuffer.end(), else you could read out of bounds here.

@@ +39,5 @@
> +{
> +  // Skip past irrelevant headers
> +  auto& it = aBuffer.begin() + 5;
> +
> +  ConvertParamSetToAnnexB(it, *(it++) & 31, aOutAnnexB);

How do you know that the result of (it++) is in bounds here? Does it get bounds checked in release builds? I suspect you need to explicitly check its bounds before derefing the iterator, and passing the result to ConvertParamSetToAnnexB. Ditto for the next call too.

::: media/gmp-clearkey/0.1/AudioDecoder.cpp
@@ +48,5 @@
>    HRESULT hr = mDecoder->Init(aConfig.mChannelCount,
>                                aConfig.mSamplesPerSecond,
>                                (BYTE*)aConfig.mExtraData,
>                                aConfig.mExtraDataLen);
> +  LOG("[%p] WMFDecodingModule::InitializeAudioDecoder() hr=0x%x\n", this, hr);

s/WMFDecodingModule/AudioDecoder/g

::: media/gmp-clearkey/0.1/WMFUtils.cpp
@@ +55,5 @@
> +  static bool sInitOk = false;
> +  if (!sInitDone) {
> +    sInitDone = true;
> +    auto handle = GetModuleHandle("mfplat.dll");
> +#define MFPLAT_FUNC(_func) \

This is nicer than how I did the same thing in the WMFReader. Well done.

Comment 23

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Make clearkey.info match actual functionality per platform

Fixes linux m3 e10s crashes.

Comment 24

[Mike Hommey [:glandium]]

Comment on attachment 8556197 [details] [diff] [review]
Make clearkey.info match actual functionality per platform

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

::: media/gmp-clearkey/0.1/Makefile.in
@@ +3,5 @@
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
> +libs::
> +	$(call py_action,preprocessor,-Fsubstitution $(DEFINES) $(ACDEFINES) \
> +		$(srcdir)/clearkey.info.in -o $(DIST)/bin/gmp-clearkey/0.1/clearkey.info)

Use PP_TARGETS. See config/rules.mk around line 1455.

Comment 25

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Make clearkey.info match actual functionality per platform, v2

Comment 26

[Mike Hommey [:glandium]]

Comment on attachment 8557676 [details] [diff] [review]
Make clearkey.info match actual functionality per platform, v2

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

::: media/gmp-clearkey/0.1/clearkey.info.in
@@ +6,5 @@
> +Libraries: dxva2.dll, d3d9.dll, msmpeg2vdec.dll, msmpeg2adec.dll, MSAudDecMFT.dll
> +#else
> +APIs: eme-decrypt-v4[org.w3.clearkey]
> +Libraries:
> +#endif

The patch is missing the deletion of clearkey.info (or better, its renaming)

Comment 27

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d983e7eb2ec0
https://hg.mozilla.org/integration/mozilla-inbound/rev/a483e217bbaa
https://hg.mozilla.org/integration/mozilla-inbound/rev/2297934585ab
https://hg.mozilla.org/integration/mozilla-inbound/rev/841453f78c39
https://hg.mozilla.org/integration/mozilla-inbound/rev/70855643209a

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d983e7eb2ec0
https://hg.mozilla.org/mozilla-central/rev/a483e217bbaa
https://hg.mozilla.org/mozilla-central/rev/2297934585ab
https://hg.mozilla.org/mozilla-central/rev/841453f78c39
https://hg.mozilla.org/mozilla-central/rev/70855643209a

Comment 29

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

https://hg.mozilla.org/releases/mozilla-beta/rev/9910b5a6a99f
https://hg.mozilla.org/releases/mozilla-beta/rev/6cb6bddb9b9d
https://hg.mozilla.org/releases/mozilla-beta/rev/8fb0193c1399
https://hg.mozilla.org/releases/mozilla-beta/rev/ed78f124783d
https://hg.mozilla.org/releases/mozilla-beta/rev/c197f7371955

Comment 30

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

Attachment: Patch 5 - Beta patch

Patch for beta branch as part of EME platform uplift.

Comment 31

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

Attachment: Patch 4 - Beta patch

Patch for beta branch as part of EME platform uplift.

Comment 32

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

Attachment: Patch 3 - Beta patch

Patch for beta branch as part of EME platform uplift.

Comment 33

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

Attachment: Patch 2 - Beta patch

Patch for beta branch as part of EME platform uplift.

Comment 34

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

Attachment: Patch 1 - Beta patch

Patch for beta branch as part of EME platform uplift.

Comment 35

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

Comment on attachment 8572337 [details] [diff] [review]
Patch 5 - Beta patch

Requesting retroactive approval for Beta landing as part of EME platform uplift.

Comment 36

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

Comment on attachment 8572338 [details] [diff] [review]
Patch 4 - Beta patch

Requesting retroactive approval for Beta landing as part of EME platform uplift.

Comment 37

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

Comment on attachment 8572339 [details] [diff] [review]
Patch 3 - Beta patch

Requesting retroactive approval for Beta landing as part of EME platform uplift.

Comment 38

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

Comment on attachment 8572340 [details] [diff] [review]
Patch 2 - Beta patch

Requesting retroactive approval for Beta landing as part of EME platform uplift.

Comment 39

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

Comment on attachment 8572341 [details] [diff] [review]
Patch 1 - Beta patch

Requesting retroactive approval for Beta landing as part of EME platform uplift.

Comment 40

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8572341 [details] [diff] [review]
Patch 1 - Beta patch

Previously approved as part of the EME platform landing on Beta.