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

RESOLVED FIXED in Firefox 37

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: cpearce, Assigned: eflores)

Tracking

(Blocks 1 bug)

Trunk
mozilla38
x86_64
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox37 fixed, firefox38 fixed)

Details

Attachments

(11 attachments, 8 obsolete attachments)

60.52 KB, patch
cpearce
: review+
Details | Diff | Splinter Review
55.17 KB, patch
cpearce
: review+
Details | Diff | Splinter Review
52.64 KB, patch
cpearce
: review+
Details | Diff | Splinter Review
5.98 KB, patch
cpearce
: review+
Details | Diff | Splinter Review
1.37 KB, patch
jesup
: review+
Details | Diff | Splinter Review
1.38 KB, patch
glandium
: review+
Details | Diff | Splinter Review
1.61 KB, patch
Details | Diff | Splinter Review
1.34 KB, patch
Details | Diff | Splinter Review
6.10 KB, patch
Details | Diff | Splinter Review
53.22 KB, patch
Details | Diff | Splinter Review
55.21 KB, patch
Details | Diff | Splinter Review
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.
This bug is necessary for basic playback of EME video on Windows.
Blocks: eme-m1
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.
Assignee: nobody → edwin
Status: NEW → ASSIGNED
No longer blocks: eme-m1
Attachment #8530006 - Attachment description: Patch 4: WMF decoding in gmp-clearkey → Patch 4: WIP - WMF decoding in gmp-clearkey
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.
Attachment #8530000 - Attachment is obsolete: true
Attachment #8545086 - Flags: review?(cpearce)
Refactor green, apart from red static analysis build; fixed in patch.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a9bfa3fd42b9
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.
Attachment #8545086 - Flags: review?(cpearce) → review-
Attachment #8545086 - Attachment is obsolete: true
Attachment #8545659 - Flags: review?(cpearce)
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().
Attachment #8545659 - Flags: review?(cpearce) → review-
moar review spam!
Attachment #8545659 - Attachment is obsolete: true
Attachment #8545731 - Flags: review?(cpearce)
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().
Attachment #8545731 - Flags: review?(cpearce) → review-
Attachment #8545744 - Flags: review?(cpearce) → review+
Attachment #8529999 - Attachment is obsolete: true
Attachment #8552226 - Flags: review?(rjesup) → review+
Attachment #8552223 - Flags: review?(cpearce) → review+
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.
Attachment #8552224 - Flags: review?(cpearce) → review+
Attachment #8552225 - Flags: review?(cpearce) → review+
Fixes linux m3 e10s crashes.
Attachment #8556197 - Flags: review?(gps)
Attachment #8556197 - Flags: review?(cpearce)
Attachment #8556197 - Flags: review?(cpearce) → review+
Attachment #8556197 - Flags: review?(gps) → review?(mh+mozilla)
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.
Attachment #8556197 - Flags: review?(mh+mozilla) → review-
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)
Attachment #8557676 - Flags: review?(mh+mozilla) → review+
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Patch for beta branch as part of EME platform uplift.
Patch for beta branch as part of EME platform uplift.
Patch for beta branch as part of EME platform uplift.
Patch for beta branch as part of EME platform uplift.
Patch for beta branch as part of EME platform uplift.
Comment on attachment 8572337 [details] [diff] [review]
Patch 5 - Beta patch

Requesting retroactive approval for Beta landing as part of EME platform uplift.
Attachment #8572337 - Flags: approval-mozilla-beta?
Comment on attachment 8572338 [details] [diff] [review]
Patch 4 - Beta patch

Requesting retroactive approval for Beta landing as part of EME platform uplift.
Attachment #8572338 - Flags: approval-mozilla-beta?
Comment on attachment 8572339 [details] [diff] [review]
Patch 3 - Beta patch

Requesting retroactive approval for Beta landing as part of EME platform uplift.
Attachment #8572339 - Flags: approval-mozilla-beta?
Comment on attachment 8572340 [details] [diff] [review]
Patch 2 - Beta patch

Requesting retroactive approval for Beta landing as part of EME platform uplift.
Attachment #8572340 - Flags: approval-mozilla-beta?
Comment on attachment 8572341 [details] [diff] [review]
Patch 1 - Beta patch

Requesting retroactive approval for Beta landing as part of EME platform uplift.
Attachment #8572341 - Flags: approval-mozilla-beta?
Comment on attachment 8572341 [details] [diff] [review]
Patch 1 - Beta patch

Previously approved as part of the EME platform landing on Beta.
Attachment #8572341 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8572340 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8572339 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8572338 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8572337 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.