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.

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.

WIP patches

WIP patches

WIP patches

WIP patches

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.

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.

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

moar review spam!

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

four.

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

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.

Fixes linux m3 e10s crashes.

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

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

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

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

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.

Comment on attachment 8572338 [details] [diff] [review] Patch 4 - Beta patch Requesting retroactive approval for Beta landing as part of EME platform uplift.

Comment on attachment 8572339 [details] [diff] [review] Patch 3 - Beta patch Requesting retroactive approval for Beta landing as part of EME platform uplift.

Comment on attachment 8572340 [details] [diff] [review] Patch 2 - Beta patch Requesting retroactive approval for Beta landing as part of EME platform uplift.

Comment on attachment 8572341 [details] [diff] [review] Patch 1 - Beta patch Requesting retroactive approval for Beta landing as part of EME platform uplift.

Comment on attachment 8572341 [details] [diff] [review] Patch 1 - Beta patch Previously approved as part of the EME platform landing on Beta.