Our gmp-clearkey CDM uses the GMP decryptor and GMP video decoder API. However our most popular CDM, the Widevine CDM, does not use that API. So that means we don't have automated test coverage covering our most popular CDM. So we should convert our clearkey CDM from using the GMPAPI to the Chromium CDM API. Our clearkey CDM: http://searchfox.org/mozilla-central/source/media/gmp-clearkey/0.1 GMP API: http://searchfox.org/mozilla-central/source/dom/media/gmp/gmp-api http://searchfox.org/mozilla-central/source/dom/media/gmp/gmp-api/gmp-video-decode.h http://searchfox.org/mozilla-central/source/dom/media/gmp/gmp-api/gmp-decryption.h Chromium CDM API to which gmp-clearkey must be ported: http://searchfox.org/mozilla-central/source/dom/media/gmp/widevine-adapter/content_decryption_module.h

Firstly, we'll need to make it possible for plugins other than the Widevine CDM to use the Widevine Adapter. I'll attach a patch to do that. Then you will need to implement the DLL entry points as defined in content_decryption_module.h, i.e. CreateCdmInstance, DeinitializeCdmModule, and InitializeCdmModule_4. You can put these alongside the other exports in gmp-clearkey.cpp; http://searchfox.org/mozilla-central/source/media/gmp-clearkey/0.1/gmp-clearkey.cpp Make sure you annotate your exported functions with GMP_EXPORT, otherwise they won't have the right linkage. Your CreateCdmInstance needs to allocate and return new ContentDecryptionModule8 instance. An easy first step would be to make your ContentDecryptionModule8 instance just create one instance of the existing ClearKeySessionManager interface and, if on Windows, one instance of the VideoDecoder interface. Then you can have your ContentDecryptionModule8 interface call the appropriate functions on ClearKeySessionManager/VideoDecoder as appropriate. We can them commit and land those changes. Once we've done that, you can work on moving the implementations out of ClearKeySessionManager directly into your ContentDecryptionModule8 instance, and removing all GMP specific stuff from gmp-clearkey.

Comment on attachment 8820865 [details] Bug 1318965 - Converts gmp-clearkey to use Chromium ContentDecryptionModule8 interface used by widevine https://reviewboard.mozilla.org/r/98218/#review100792 ::: media/gmp-clearkey/0.1/ClearKeyCDM.h:29 (Diff revision 1) > + cdm::Host_8* mHost; > + > +public: > + explicit ClearKeyCDM(cdm::Host_8* mHost); > + > + virtual void Initialize(bool aAllowDistinctiveIdentifier, As per the style guide, for functions that override, you should have only the override keyword, and not the 'virtual' keyword. https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Methods ::: media/gmp-clearkey/0.1/ClearKeyCDM.cpp:12 (Diff revision 1) > +ClearKeyCDM::ClearKeyCDM(Host_8 * aHost) > +{ > + mHost = aHost; > +} > + > +void ClearKeyCDM::Initialize(bool aAllowDistinctiveIdentifier, ReturnType ClassName::FunctionName(/*args*/) { // body } That is, in this function definition you don't have the '\{' on its own line. Same with other definitions here. I won't list the other instances of this nits, please address them. ::: media/gmp-clearkey/0.1/ClearKeyCDM.cpp:119 (Diff revision 1) > +#endif > +} > + > +void > +ClearKeyCDM::DeinitializeDecoder(StreamType aDecoderType) { > + if (aDecoderType == StreamType::kStreamTypeVideo) { May as well put the aDecoderType check inside the #ifdef. No point doing it if WMF is not enabled. ::: media/gmp-clearkey/0.1/ClearKeyCDM.cpp:148 (Diff revision 1) > +} > + > +Status > +ClearKeyCDM::DecryptAndDecodeSamples(const InputBuffer& aEncryptedBuffer, > + AudioFrames* aAudioFrame) { > + // Audio decoding is not supported by Clearkey Good comments explain things the code can't and/or doesn't express. Motivations normally. So I think this comments should say that we don't support audio decoding because Widevine doesn't support it either, and the purpose of ClearKey is to provide test coverage for the code paths that Widevine exercises in the wild. ::: media/gmp-clearkey/0.1/ClearKeyCDM.cpp:155 (Diff revision 1) > +} > + > +void > +ClearKeyCDM::OnPlatformChallengeResponse( > + const PlatformChallengeResponse& aResponse) { > + // This function should never be called and is not supported As a general nit, comments should be grammatically correct. So full stops at the end of sentences please. Here and other comments. ::: media/gmp-clearkey/0.1/ClearKeyCDM.cpp:175 (Diff revision 1) > +#ifdef ENABLE_WMF > + mVideoDecoder->DecodingComplete(); > +#endif > +} > + > + Nit: You have two new lines at the end of the file. That's one more than I'd expect. ::: media/gmp-clearkey/0.1/ClearKeyDecryptionManager.cpp:19 (Diff revision 1) > -#include <string.h> > -#include <vector> > - > #include "ClearKeyDecryptionManager.h" > -#include "psshparser/PsshParser.h" > + > #include "gmp-api/gmp-decryption.h" You should be able to remove all includes from gmp-api/. And remove including gmp-api in the include path from the moz.build. Might have to happen in a later patch, but it should happen somewhere. ::: media/gmp-clearkey/0.1/ClearKeyDecryptionManager.cpp:112 (Diff revision 1) > } > > void > ClearKeyDecryptionManager::InitKey(KeyId aKeyId, Key aKey) > { > - CK_LOGD("ClearKeyDecryptionManager::InitKey %08x...", *(uint32_t*)&aKeyId[0]); > + CK_LOGD("ClearKeyDecryptionManager::InitKey %08x...", You can't actually assume that keyIds are always a fixed size; for WebM they can be variable length. You should convert the keyId to a (potentially variable length) hex string here, and print that. Or use your array printing macro. Ditto elsewhere you print out the keyId. Yes, this was a bug that existed in the code you copied, but it's one that we've discovered since the code was originally written, and since you're here you may as well fix it. ::: media/gmp-clearkey/0.1/ClearKeyDecryptionManager.cpp:117 (Diff revision 1) > - CK_LOGD("ClearKeyDecryptionManager::InitKey %08x...", *(uint32_t*)&aKeyId[0]); > + CK_LOGD("ClearKeyDecryptionManager::InitKey %08x...", > + *(uint32_t*)&aKeyId[0]); > if (IsExpectingKeyForKeyId(aKeyId)) { > + CK_LOGARRAY("Initialized Key ", aKeyId.data(), aKeyId.size()); > mDecryptors[aKeyId]->InitKey(aKey); > } } else { https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Control_Structures Visual Studio formats's else's like you've got it here by default. You can change how VS formats else statements in its settings somewhere. ::: media/gmp-clearkey/0.1/ClearKeyDecryptionManager.cpp:159 (Diff revision 1) > ClearKeyDecryptionManager::Decrypt(uint8_t* aBuffer, uint32_t aBufferSize, > const CryptoMetaData& aMetadata) > { > CK_LOGD("ClearKeyDecryptionManager::Decrypt"); > if (!HasKeyForKeyId(aMetadata.mKeyId)) { > - return GMPNoKeyErr; > + CK_LOGARRAY("Unable to find decryptor for: ", I think your logging should read "Unable to find decryptor for keyId:". So that it's clear what the decryptor is indexed by. Ditto in the logging for the found case. ::: media/gmp-clearkey/0.1/ClearKeyPersistence.h:20 (Diff revision 1) > */ > > #ifndef __ClearKeyPersistence_h__ > #define __ClearKeyPersistence_h__ > > -#include <string> > +// This include is required in order for content_decryption_module to work You should really clean up the persistence code before we land this. Either stub out the implementation (i.e. delete the obsolete implementation, leaving the functions empty), or just backport the changes to make it work up front. It would be easier to review if you stubbed the implementation. I think it makes sense to include the new implementation in this patch if there's lots of overlap; i.e. if the code is just a modification of the old code. However if the new code is a rewrite, then you should split it up into a new changeset. ::: media/gmp-clearkey/0.1/ClearKeySession.cpp:58 (Diff revision 1) > - key_infos.data(), key_infos.size()); > + keyInfo.key_id_size = keyId.size(); > + keyInfo.status = KeyStatus::kReleased; > + keyInfos.push_back(keyInfo); > + } > + > + mHost->OnSessionKeysChange(mSessionId.data(), You should format function invocation which are longer than the 80 char line limit: func(arg1, arg2, arg3); The exception to this rule is if the subsequent lines are also too long, whereupon you may format subsequent lines as one more level of indent. i.e. func(arg1 arg2, arg3) ::: media/gmp-clearkey/0.1/ClearKeySession.cpp:75 (Diff revision 1) > > - if (aInitDataType == "cenc") { > + if (aInitDataType == InitDataType::kCenc) { > ParseCENCInitData(aInitData, aInitDataSize, mKeyIds); > - } else if (aInitDataType == "keyids") { > + } else if (aInitDataType == InitDataType::kKeyIds) { > ClearKeyUtils::ParseKeyIdsInitData(aInitData, aInitDataSize, mKeyIds); > - } else if (aInitDataType == "webm" && aInitDataSize <= kMaxWebmInitDataSize) { > + } else if (aInitDataType == InitDataType::kWebM } else if (aInitDataType == InitDataType::kWebM && aInitDataSize <= kMaxWebmInitDataSize) { That is, line up line-broken conditionals with the openening parenthesis of the conditional. ::: media/gmp-clearkey/0.1/ClearKeySessionManager.cpp:125 (Diff revision 1) > > // Send a request for needed key data. > string request; > ClearKeyUtils::MakeKeyRequest(neededKeys, request, aSessionType); > - mCallback->SessionMessage(&sessionId[0], sessionId.length(), > - kGMPLicenseRequest, > + > + //Resolve the promise with the new session information // Resolve... i.e. space between '//' and the comment. ::: media/gmp-clearkey/0.1/ClearKeySessionManager.cpp:158 (Diff revision 1) > - mCallback->ResolveLoadSessionPromise(aPromiseId, false); > + mHost->OnResolveNewSessionPromise(aPromiseId, nullptr, 0); > return; > } > > - // Callsback PersistentSessionDataLoaded with results... > - ClearKeyPersistence::LoadSessionData(this, sid, aPromiseId); > + function<void(const uint8_t*, uint32_t)> success = > + [this, sid, aPromiseId] ClearKeySessionManager is refcounted. And these lambdas are callbacks. So potentially something else could drop the last ref to the ClearKeySessionManager while we're waiting on a callback to fire, and then the callback fires and we end up with a use-after-free. So you need to include a RefPtr<ClearKeySessionManager> in the capture list. Convention is to do: RefPtr<ClearKeySessionManager> self(this); and then capture self rather than 'this', and call functions on the object via the 'self' pointer. ::: media/gmp-clearkey/0.1/ClearKeySessionManager.cpp:169 (Diff revision 1) > + size); > + }; > + > + function<void()> failure = [this, sid, aPromiseId]() -> void { > + // As per the API described in ContentDecryptionModule_8 > + mHost->OnResolveNewSessionPromise(aPromiseId, nullptr, 0); You should be null-checking mHost before calling it, as presumably it can be set to null if we're shutdown before the callback has a chance to run. This applies to all other lambda callbacks and the comment about keeping a refcounted pointer to self probably also applies to most of them too. ::: media/gmp-clearkey/0.1/ClearKeySessionManager.cpp:179 (Diff revision 1) > + mHost->OnResolveNewSessionPromise(aPromiseId, aSessionId, aSessionIdLength); > } > > void > -ClearKeySessionManager::PersistentSessionDataLoaded(GMPErr aStatus, > +ClearKeySessionManager::PersistentSessionDataLoaded( > + FileIOClient::Status aStatus, Indentation of function arguments should be: ReturnType ClassName::FunctionName(arg1, arg2, .... argN) ::: media/gmp-clearkey/0.1/ClearKeySessionManager.cpp:231 (Diff revision 1) > + keyInfo.status = KeyStatus::kUsable; > + > + keyInfos.push_back(keyInfo); > + } > + > + // TODO confirm what the has_additional_usable_keys param should be As discussed, has_additional_usable_keys is unused by Gecko, so you can remove this TODO. ::: media/gmp-clearkey/0.1/ClearKeySessionManager.cpp:285 (Diff revision 1) > } > > // Parse the response for any (key ID, key) pairs. > vector<KeyIdPair> keyPairs; > - if (!ClearKeyUtils::ParseJWK(aResponse, aResponseSize, keyPairs, session->Type())) { > + if (!ClearKeyUtils::ParseJWK(aResponse, > + aResponseSize, Indentation. ::: media/gmp-clearkey/0.1/ClearKeySessionManager.cpp:291 (Diff revision 1) > + keyPairs, > + session->Type())) { > CK_LOGW("ClearKey CDM failed to parse JSON Web Key."); > - mCallback->RejectPromise(aPromiseId, kGMPTypeError, nullptr, 0); > + > + mHost->OnRejectPromise(aPromiseId, > + Error::kInvalidStateError, Try to not change the error types that we reject the promises with. The web platform tests test the behaviour of clearkey in various error conditions, and you're liable to regress them if you change the types. The web platform tests have several tests that check for TypeError in the case of bad input for example. You can also run the subset that we test in our integration: ./mach test testing/web-platform/tests/encrypted-media/ That subset should pass locally. The tests for features that we don't try to support are disabled. You can also test the live upstream version of the tests if you point your build at: http://w3c-test.org/tools/runner/index.html and run the tests in the encrypted-media subdir. We're known to not pass the .\*persistent.\* tests on w3c-test.org, as our persistent license support is non-compliant, and disabled. ::: media/gmp-clearkey/0.1/ClearKeySessionManager.cpp:313 (Diff revision 1) > + keyInfos.push_back(keyInfo); > } > - mCallback->BatchedKeyStatusChanged(aSessionId, aSessionIdLength, > - key_infos.data(), key_infos.size()); > > - if (session->Type() != kGMPPersistentSession) { > + // TODO work out what the 'has_additional_usable_key' param should be set as Obsolete comment. ::: media/gmp-clearkey/0.1/ClearKeySessionManager.cpp:315 (Diff revision 1) > - mCallback->BatchedKeyStatusChanged(aSessionId, aSessionIdLength, > - key_infos.data(), key_infos.size()); > > - if (session->Type() != kGMPPersistentSession) { > - mCallback->ResolvePromise(aPromiseId); > + // TODO work out what the 'has_additional_usable_key' param should be set as > + mHost->OnSessionKeysChange(aSessionId, aSessionIdLength, true, > + keyInfos.data(), keyInfos.size()); Indentation. ::: media/gmp-clearkey/0.1/ClearKeySessionManager.cpp:329 (Diff revision 1) > // and simply append each keyId followed by its key. > vector<uint8_t> keydata; > Serialize(session, keydata); > - GMPTask* resolve = WrapTask(mCallback, &GMPDecryptorCallback::ResolvePromise, aPromiseId); > + > + function<void()> resolve = [this, aPromiseId]() -> void { > + mHost->OnResolvePromise(aPromiseId); Hold refcount to self. ::: media/gmp-clearkey/0.1/ClearKeySessionManager.cpp:334 (Diff revision 1) > + mHost->OnResolvePromise(aPromiseId); > + }; > + > + function<void()> reject = [this, aPromiseId]() -> void { > - static const char* message = "Couldn't store cenc key init data"; > + static const char* message = "Couldn't store cenc key init data"; > - GMPTask* reject = WrapTask(mCallback, > + mHost->OnRejectPromise(aPromiseId, Hold refcount to self and null check mHost. ::: media/gmp-clearkey/0.1/ClearKeySessionManager.cpp:432 (Diff revision 1) > > ClearKeyPersistence::PersistentSessionRemoved(sid); > > - // Overwrite the record storing the sessionId's key data with a zero > - // length record to delete it. > + // TODO Overwrite the record storing the sessionId's key data with a zero > + // length record to delete it. The API suggests writing null but this > + // should *hopefully* work. This should be converted to nullptr. *Hopefully* you tested that it works so we don't need to hope this should work. i.e. don't leave a "TODO I hope this works", figure out whether it works! ::: media/gmp-clearkey/0.1/ClearKeySessionManager.cpp:437 (Diff revision 1) > + // should *hopefully* work. This should be converted to nullptr. > + // It should also be noted the persistence path is only used in a few tests > + // and is not enabled in the browser. > vector<uint8_t> emptyKeydata; > - GMPTask* resolve = WrapTask(mCallback, &GMPDecryptorCallback::ResolvePromise, aPromiseId); > + > + // TODO double check it's all right to access mHost here... You now know how to resolve these two TODOs. ::: media/gmp-clearkey/0.1/ClearKeySessionManager.cpp:453 (Diff revision 1) > - kGMPInvalidAccessError, > - message, > + message, > - strlen(message)); > + strlen(message)); > - StoreData(sessionId, emptyKeydata, resolve, reject); > + }; > + > + WriteData(mHost, sessionId, emptyKeydata, resolve, reject); Optional: I'd be inclined to move() the resolve/reject continuations into the function call, but since it's not a performance critical path, it's not a big deal. ::: media/gmp-clearkey/0.1/ClearKeySessionManager.cpp:472 (Diff revision 1) > + 0 /* messageLen */); > } > > -void > -ClearKeySessionManager::Decrypt(GMPBuffer* aBuffer, > - GMPEncryptedBufferMetadata* aMetadata) > +Status > +ClearKeySessionManager::Decrypt(const InputBuffer& aBuffer, > + DecryptedBlock* aMetadata) aMetadata is no longer an suitable name for this parameter; this parameter holds the decrypted output, not metadata. ::: media/gmp-clearkey/0.1/ClearKeySessionManager.cpp:476 (Diff revision 1) > -ClearKeySessionManager::Decrypt(GMPBuffer* aBuffer, > - GMPEncryptedBufferMetadata* aMetadata) > +ClearKeySessionManager::Decrypt(const InputBuffer& aBuffer, > + DecryptedBlock* aMetadata) > { > CK_LOGD("ClearKeySessionManager::Decrypt"); > > - if (!mThread) { > + assert(aBuffer.key_id_size <= 16); WebM doesn't specify a length on its keyIds. So you can't assert that the keyId is <= 16, as that may not be the case. ::: media/gmp-clearkey/0.1/ClearKeySessionManager.cpp:481 (Diff revision 1) > - &ClearKeySessionManager::DoDecrypt, > - aBuffer, aMetadata)); > -} > > -void > -ClearKeySessionManager::DoDecrypt(GMPBuffer* aBuffer, > + Buffer* buffer = mHost->Allocate(aBuffer.data_size); > + memcpy(buffer->Data(), aBuffer.data, aBuffer.data_size); For safety, I think you should null-check the return value of Allocate(). ::: media/gmp-clearkey/0.1/ClearKeySessionManager.cpp:508 (Diff revision 1) > > void > ClearKeySessionManager::DecryptingComplete() > { > CK_LOGD("ClearKeySessionManager::DecryptingComplete %p", this); > One of these shutdown functions should be setting mHost to nullptr, so that you don't try to call the host back after you've been told to stop. Do you need two functions that do shutdown; one called Shutdown(), the other DecryptingComplete()? ::: media/gmp-clearkey/0.1/ClearKeyStorage.cpp:42 (Diff revision 1) > * delete them when done. > */ > - static void Write(const std::string& aRecordName, > + static void Write(Host_8* aHost, > + std::string& aRecordName, > const std::vector<uint8_t>& aData, > - GMPTask* aOnSuccess, > + const function<void()>& aOnSuccess, You're storing a const reference to the std::function<> objects that are on the stack in the calling function! So there's no guarantee they'll stay valid when the callback get called. You need to either be making a copy of the std::function being passed in (i.e. don't make it const&, pass it by value), or pass it by value by move() it into the function call. ::: media/gmp-clearkey/0.1/ClearKeyStorage.cpp:61 (Diff revision 1) > + mFileIO->Write(&mData[0], mData.size()); > } > } > > - virtual void ReadComplete(GMPErr aStatus, > + virtual void OnReadComplete(Status aStatus, > - const uint8_t* aData, > + const uint8_t* aData, Indentation. ::: media/gmp-clearkey/0.1/ClearKeyStorage.cpp:111 (Diff revision 1) > delete this; > } > > - GMPRecord* mRecord; > - GMPTask* mOnSuccess; > - GMPTask* mOnFailure; > + FileIO* mFileIO; > + > + const function<void()>& mOnSuccess; As I said above, these need to be copies, not a const reference. ::: media/gmp-clearkey/0.1/ClearKeyStorage.cpp:191 (Diff revision 1) > - mRecord->Close(); > + mFileIO->Close(); > } > - mContinuation->ReadComplete(err, aData, aDataSize); > - delete mContinuation; > + > + if (IO_SUCCEEDED(err)) { > + mOnSuccess(aData, aDataSize); > + } } else { ::: media/gmp-clearkey/0.1/ClearKeyStorage.cpp:199 (Diff revision 1) > + } > + > delete this; > } > > - GMPRecord* mRecord; > + function<void(const uint8_t*, uint32_t)> mOnSuccess; This one is making a copy, so it's safe. ::: media/gmp-clearkey/0.1/ClearKeyUtils.h:36 (Diff revision 1) > #if 0 > void CK_Log(const char* aFmt, ...); > #define CK_LOGE(...) CK_Log(__VA_ARGS__) > #define CK_LOGD(...) CK_Log(__VA_ARGS__) > #define CK_LOGW(...) CK_Log(__VA_ARGS__) > +#define CK_LOGARRAY(APREPEND, ADATA, ADATA_SIZE) CK_LogArray(APREPEND, Merge the fix for this macro into this changeset. ::: media/gmp-clearkey/0.1/ClearKeyUtils.cpp:62 (Diff revision 1) > va_end(ap); > > - printf("\n"); > - fflush(stdout); > + fprintf(out, "%s\n", buf); > + fflush(out); > + > + if (usingFile) { if (out != stdout) { fclose(out); } Don't store state that you can infer, as then you need to worry about keeping your copy of the state up to date. ::: media/gmp-clearkey/0.1/ClearKeyUtils.cpp:66 (Diff revision 1) > + > + if (usingFile) { > + fclose(out); > + } > + > + out = nullptr; This assignment is unnecessary; it has no effect. ::: media/gmp-clearkey/0.1/ClearKeyUtils.cpp:69 (Diff revision 1) > + } > + > + out = nullptr; > +} > + > +void CK_LogArray(const char* prepend, Merge the fix for this function into this changeset. ::: media/gmp-clearkey/0.1/VideoDecoder.h:40 (Diff revision 1) > - const uint8_t* aCodecSpecific, > - uint32_t aCodecSpecificLength, > - int64_t aRenderTimeMs = -1); > > - virtual void Reset() override; > + cdm::Status Decode(const cdm::InputBuffer& aEncryptedBuffer, > + cdm::VideoFrame* aVideoFrame); Indentation. ::: media/gmp-clearkey/0.1/VideoDecoder.h:55 (Diff revision 1) > - void EnsureWorker(); > + cdm::Status Drain(cdm::VideoFrame* aVideoFrame); > - > - void DrainTask(); > > struct DecodeData { > DecodeData() You could declare: uint64_t mTimestamp = 0; And then the constructor is unnecessary. ::: media/gmp-clearkey/0.1/VideoDecoder.cpp:42 (Diff revision 1) > - return; > - } > > - // The first byte is mPacketizationMode, which is only relevant for > - // WebRTC/OpenH264 usecase. > - const uint8_t* avcc = aCodecSpecific + 1; > + // If the method is unable to determine the number of cores it returns zero > + if (cores == 0) { > + cores = 1; Indentation. ::: media/gmp-clearkey/0.1/gmp-clearkey.cpp:46 (Diff revision 1) > } > > extern "C" { > > -GMP_EXPORT GMPErr > -GMPInit(GMPPlatformAPI* aPlatformAPI) > + CDM_EXPORT > + void INITIALIZE_CDM_MODULE() { You don't need to indent extern "C" blocks.

Comment on attachment 8820866 [details] Bug 1318965 - Removes the custom AtomicRefCount https://reviewboard.mozilla.org/r/98962/#review100826 ::: media/gmp-clearkey/0.1/ClearKeyUtils.cpp:21 (Diff revision 1) > > #include "ClearKeyUtils.h" > > #include <algorithm> > #include <assert.h> > +#include <stdlib.h> This change seems unrelated to what this changeset is trying to achieve. Would it make more sense for it to belong in another changeset? ::: media/gmp-clearkey/0.1/RefCounted.h:25 (Diff revision 1) > #include <assert.h> > #include "ClearKeyUtils.h" > > -#if defined(_MSC_VER) > #include <atomic> > typedef std::atomic<uint32_t> AtomicRefCount; Maybe you can just do a global s/AtomicRefCount/std::atomic<uint32_t>/g

Comment on attachment 8820869 [details] Bug 1318965 - Fixes a bug in the WideVineAdapter wherein session load failures were not adapted correctly https://reviewboard.mozilla.org/r/99636/#review100830 ::: dom/media/gmp/widevine-adapter/WidevineDecryptor.cpp:321 (Diff revision 1) > + > + // This is laid out in the API. If we fail to load a session we should > + // call OnResolveNewSessionPromise with nullptr as the sessionId. > + // We can safely assume this means that we have failed to load a session > + // as the other methods specify calling 'OnRejectPromise' when they fail. > + if (aSessionId == nullptr) { Style guide says you should null check pointers like so: if (!aSessionId) { ... }

Comment on attachment 8820868 [details] Bug 1318965 - Adds code for deferring the initialization of sessions until persistent sessions have been loaded https://reviewboard.mozilla.org/r/99886/#review100832 I'll re-review when the persistence storage code is put in the right changesets. ::: media/gmp-clearkey/0.1/ClearKeyPersistence.h:44 (Diff revision 1) > LOADED > }; > > class ClearKeyPersistence : public RefCounted { > public: > - ClearKeyPersistence(cdm::Host_8* aHost); > + explicit ClearKeyPersistence(cdm::Host_8* aHost); I'll skip reviewing the persistent stuff until you've sorted out the storage stuff in the first patch in the series. ::: media/gmp-clearkey/0.1/ClearKeySessionManager.cpp:60 (Diff revision 1) > mPersistence = new ClearKeyPersistence(mHost); > > - if (aPersistentStateAllowed) { > - mPersistence->EnsureInitialized(); > + function<void()> onPersistentStateLoaded = > + [this] > + () -> void { > + for (uint32_t i = 0; i < mDefferedInitialize.size(); ++i) { You could use a "range based for loop" here; for (std::function<void()>& func : mDefferedInitialize) { func(); } If you get in the habit of using range based for loops, you're less likely to make mistakes using the indexes. ::: media/gmp-clearkey/0.1/ClearKeySessionManager.cpp:81 (Diff revision 1) > { > + // Copy the init data so it is correctly captured by the lambda > + vector<uint8_t> initData(aInitData, aInitData + aInitDataSize); > + function<void()> deferrer = > + [this, aPromiseId, aInitDataType, initData, aSessionType] > + () -> void { Remember to hold a refcount in the capture. ::: media/gmp-clearkey/0.1/ClearKeySessionManager.cpp:286 (Diff revision 1) > { > - CK_LOGD("ClearKeySessionManager::UpdateSession"); > + // Copy the method arguments so we can capture them in the lambda > string sessionId(aSessionId, aSessionId + aSessionIdLength); > + vector<uint8_t> response(aResponse, aResponse + aResponseSize); > + function<void()> deferrer = > + [this, aPromiseId, sessionId, response] I'd indent that as: [this, aPromiseId, sessionId, response] () { // ... } Note: the '-> void' is optional. Ditto for other lambdas.

Comment on attachment 8820870 [details] Bug 1318965 - Improves the logging functions provided in ClearKeyUtils https://reviewboard.mozilla.org/r/100020/#review100836 r+ with PrintableAsString() not exposed in ClearKeyUtils.h. ::: media/gmp-clearkey/0.1/ClearKeyUtils.h:95 (Diff revision 1) > static bool IsValidSessionId(const char* aBuff, uint32_t aLength); > + > + static std::string ToHexString(const uint8_t * aBytes, uint32_t aLength); > + > + // Determines whether an array of bytes can be printed as a string > + static bool PrintableAsString(const uint8_t* aBytes, uint32_t aLength); PrintableAsString is not used outside of this class, so you may as well just declare it as static inside the .cpp file; it is an implementation detail, and doesn't need to be exposed to the client. ::: media/gmp-clearkey/0.1/ClearKeyUtils.cpp:588 (Diff revision 1) > + return ss.str(); > +} > + > +bool ClearKeyUtils::PrintableAsString(const uint8_t* aBytes, uint32_t aLength) > +{ > + for (uint32_t i = 0; i < aLength; ++i) { Optional: Maybe you could #include <algorithm> up top, and then PrintableAsString() could become: return all_off(aBytes, aBytes + aLength, isprint); Then you eliminate another loop. Loops are a source of coding errors. ;)

Comment on attachment 8820867 [details] Bug 1318965 - Converts gmp-clearkey to use Chromium ContentDecryptionModule8 interface used by widevine https://reviewboard.mozilla.org/r/99634/#review100838 I will review more throughly once the changeset have been reworked to have the persistent storage changes together. ::: media/gmp-clearkey/0.1/ClearKeyPersistence.cpp:33 (Diff revision 1) > > using namespace std; > using namespace cdm; > > -// TODO load persistent session ids from GMP storage > -// Whether we've loaded the persistent session ids from GMPStorage yet. > +void ClearKeyPersistence::ReadAllRecordsFromIndex() { > + //Clear what we think the index file contains, we're about to read it again // Clear (Space between '//' and comment.) ::: media/gmp-clearkey/0.1/ClearKeyPersistence.cpp:41 (Diff revision 1) > - LOADING, > - LOADED > + function<void(const uint8_t*, uint32_t)> onIndexSuccess = > + [this] > + (const uint8_t* data, uint32_t size) -> void { > + CK_LOGD("ClearKeyPersistence: Loaded index file!"); > + const char* charData = (const char*)data; > + Loops are a source of coding error, so simplify or avoid them where possible. So how about: stringstream ss(string(data, data+size)); string name; while (getline(ss, name)) { if (ClearKeyUtils::IsValidSessionId(name)) { mPersistentSessionIds.insert(strtol(name))); } } Then you're not writing such a low level loop. And it's shorter. And it's roughly the inverse of you index-writing loop. ::: media/gmp-clearkey/0.1/ClearKeyPersistence.cpp:48 (Diff revision 1) > + for (uint32_t end = 0; end < size; ++end) { > + if (start == end) { > + continue; > + } > + > + if (charData[end] == '\n') { Code is easier to read if you reduce nesting. So ideal conditionals like this should be written as: if (charData\[end\] \!= '\\n') \{ continue; \} Then you'd reduce the indent, and the cognitive overhead required to understand the code. Of course, you won't need to do this if you replace the loop with the stringstream loop, but it's worth thinking about. ::: media/gmp-clearkey/0.1/ClearKeyPersistence.cpp:90 (Diff revision 1) > -// sTasksBlockedOnSessionIdLoad[i]->Destroy(); > -// } > -// sTasksBlockedOnSessionIdLoad.clear(); > -//} > > -/* static */ void > + function <void()> onIndexFail = Hold a refcount on this, here and elsewhere. ::: media/gmp-clearkey/0.1/ClearKeyPersistence.cpp:99 (Diff revision 1) > + }; > + > + stringstream ss; > + string dataString; > + > + for (auto sessionId : mPersistentSessionIds) { I recommend you refrain from using auto in for loops as it's harder for people reading the code to determine the type of the thing they're looking at. So you should make this: for (const string& : mPersistentSessionIds) Also, note the const reference there; to avoid copying. ::: media/gmp-clearkey/0.1/ClearKeyPersistence.cpp:104 (Diff revision 1) > + for (auto sessionId : mPersistentSessionIds) { > + ss << sessionId; > + ss << '\n'; > + } > + > + dataString = ss.str(); Declare variables as close to where they're used as possible. So declare dataString here, rather than on line 97.

Passing on try (depends on the patch in 1319159) https://treeherder.mozilla.org/#/jobs?repo=try&revision=4676732d8a4940da70f4933ebf0ad13854b6eee4

Comment on attachment 8820866 [details] Bug 1318965 - Removes the custom AtomicRefCount https://reviewboard.mozilla.org/r/98962/#review104062

Comment on attachment 8820867 [details] Bug 1318965 - Converts gmp-clearkey to use Chromium ContentDecryptionModule8 interface used by widevine https://reviewboard.mozilla.org/r/99634/#review104066 ::: media/gmp-clearkey/0.1/ClearKeyCDM.cpp:31 (Diff revision 2) > +void > +ClearKeyCDM::SetServerCertificate(uint32_t aPromiseId, > + const uint8_t* aServerCertificateData, > + uint32_t aServerCertificateDataSize) > +{ > + mSessionManager->SetServerCertificate(aPromiseId, mSessionManager could be null if any of these functions are called before Initialize() is called. So how about constructing mSessionManager in the constructor or at its declaration site? ::: media/gmp-clearkey/0.1/ClearKeyDecryptionManager.h:51 (Diff revision 2) > } > - Assign(mKeyId, aCrypto->KeyId(), aCrypto->KeyIdSize()); > - Assign(mIV, aCrypto->IV(), aCrypto->IVSize()); > - Assign(mClearBytes, aCrypto->ClearBytes(), aCrypto->NumSubsamples()); > - Assign(mCipherBytes, aCrypto->CipherBytes(), aCrypto->NumSubsamples()); > + > + Assign(mKeyId, aInputBuffer->key_id, aInputBuffer->key_id_size); > + Assign(mIV, aInputBuffer->iv, aInputBuffer->iv_size); > + > + for (uint32_t i = 0; i < aInputBuffer->num_subsamples; ++i) { This makes a copy of the SubsampleEntry. You're better to use a const reference, to avoid the overhead of the copy. ::: media/gmp-clearkey/0.1/ClearKeyDecryptionManager.cpp:88 (Diff revision 2) > > bool > ClearKeyDecryptionManager::IsExpectingKeyForKeyId(const KeyId& aKeyId) const > { > - CK_LOGD("ClearKeyDecryptionManager::IsExpectingKeyForId %08x...", *(uint32_t*)&aKeyId[0]); > + CK_LOGARRAY("ClearKeyDecryptionManager::IsExpectingKeyForId ", > + aKeyId.data(), Indentation of hanging args doesn't line up beneath '('. ::: media/gmp-clearkey/0.1/ClearKeyDecryptionManager.cpp:182 (Diff revision 2) > } > > ClearKeyDecryptor::~ClearKeyDecryptor() > { > if (HasKey()) { > - CK_LOGD("ClearKeyDecryptor dtor; key = %08x...", *(uint32_t*)&mKey[0]); > + CK_LOGARRAY("ClearKeyDecryptor dtor; key = %08x...", Why do you have a %08x... in this CK_LOGARRAY call, but not others? Did you mean to have this here? That means your printf will try to print an arugment that's not passed. ::: media/gmp-clearkey/0.1/ClearKeyDecryptionManager.cpp:215 (Diff revision 2) > for (size_t i = 0; i < aMetadata.NumSubsamples(); i++) { > data += aMetadata.mClearBytes[i]; > uint32_t cipherBytes = aMetadata.mCipherBytes[i]; > if (data + cipherBytes > aBuffer + aBufferSize) { > // Trying to read past the end of the buffer! > - return GMPCryptoErr; > + return Status::kNeedMoreData; I think this should be kDecryptError. Firefox doesn't handle kNeedMoreData for decrypt operations. ::: media/gmp-clearkey/0.1/ClearKeyPersistence.h:49 (Diff revision 2) > - static void EnsureInitialized(); > + explicit ClearKeyPersistence(cdm::Host_8* aHost); > + > + void EnsureInitialized(bool aPersistentStateAllowed, > + std::function<void()>&& aOnInitialized); > + > + bool IsLoaded(); Can this function be const? It shouldn't need to change the object. ::: media/gmp-clearkey/0.1/ClearKeySession.cpp:63 (Diff revision 2) > vector<uint8_t> keyId; > keyId.assign(aInitData, aInitData+aInitDataSize); > mKeyIds.push_back(keyId); > } > > + // If there was an initialization error. This comment doesn't seem to be meaningful. ::: media/gmp-clearkey/0.1/ClearKeySessionManager.cpp:61 (Diff revision 2) > + > + RefPtr<ClearKeySessionManager> self(this); > + function<void()> onPersistentStateLoaded = > + [self] () > + { > + for (function<void()>& func : self->mDefferedInitialize) { What if something in the funcs changes what's in mDeferredInitialize? Then the iteration may fail? Typically what you'd do is either a while(!empty){pop front, exectute), or you'd swap everything else out of the vector, and work on the swapped out values. ::: media/gmp-clearkey/0.1/ClearKeySessionManager.cpp:98 (Diff revision 2) > + if (MaybeDeferTillInitialized(deferrer)) { > return; > } > > - if (ClearKeyPersistence::DeferCreateSessionIfNotReady(this, > - aCreateSessionToken, > + CK_LOGD("ClearKeySessionManager::CreateSession type:%s", aInitDataType); > + CK_LOGARRAY("ClearKeySessionManager::CreateSession initdata: %s", This passes a %s. That means sprintf will be trying to print a parameter that is not passed? ::: media/gmp-clearkey/0.1/ClearKeySessionManager.cpp:290 (Diff revision 2) > - } > - mCallback->BatchedKeyStatusChanged(&aSessionId[0], aSessionId.size(), > - key_infos.data(), key_infos.size()); > > - mCallback->ResolveLoadSessionPromise(aPromiseId, true); > + KeyInformation keyInfo = KeyInformation(); > + keyInfo.key_id = &keyPair.mKeyId[0]; Here you're storing a reference to a key that's stored on the stack. By the time this gets dereferenced by the host, it could be invalid, or overwritten with something else. You probably want to pass a pointer to the memory that you put in keypairs, i.e. &keyPairs.back(). ::: media/gmp-clearkey/0.1/ClearKeySessionManager.cpp:298 (Diff revision 2) > + > + keyInfos.push_back(keyInfo); > + } > + > + mHost->OnSessionKeysChange(&aSessionId[0], > + aSessionId.size(), Indentation of arguments. ::: media/gmp-clearkey/0.1/ClearKeySessionManager.cpp:377 (Diff revision 2) > } > > // Parse the response for any (key ID, key) pairs. > vector<KeyIdPair> keyPairs; > - if (!ClearKeyUtils::ParseJWK(aResponse, aResponseSize, keyPairs, session->Type())) { > + if (!ClearKeyUtils::ParseJWK(aResponse, > + aResponseSize, Indentation of args. ::: media/gmp-clearkey/0.1/ClearKeySessionManager.cpp:410 (Diff revision 2) > - if (session->Type() != kGMPPersistentSession) { > - mCallback->ResolvePromise(aPromiseId); > + mHost->OnSessionKeysChange(aSessionId, > + aSessionIdLength, > + true, > + keyInfos.data(), > + keyInfos.size()); > + You've got 2 blank lines here. That's 1 too many. ::: media/gmp-clearkey/0.1/ClearKeySessionManager.cpp:570 (Diff revision 2) > assert(session); > string sid = session->Id(); > - bool isPersistent = session->Type() == kGMPPersistentSession; > + bool isPersistent = session->Type() == SessionType::kPersistentLicense; > ClearInMemorySessionData(session); > > + mHost->OnSessionClosed(aSessionId, aSessionIdLength); content_decryption_module.h doesn't indicate that you're supposed to call OnSessionClosed() when the session is removed? I don't think you're supposed to do this? ::: media/gmp-clearkey/0.1/ClearKeyStorage.cpp:68 (Diff revision 2) > - const uint8_t* aData, > + const uint8_t* aData, > - uint32_t aDataSize) override { > - assert(false); // Should not reach here. > + uint32_t aDataSize) override > + { > + // This function should never be called, we only ever write data with this > + // client. > + assert(false); Looks like the body of this function isn't indented correctly. ::: media/gmp-clearkey/0.1/VideoDecoder.cpp:23 (Diff revision 2) > > -#include "AnnexB.h" > #include "BigEndian.h" > #include "ClearKeyDecryptionManager.h" > #include "ClearKeyUtils.h" > -#include "gmp-task-utils.h" > +#include "unicode\std_string.h" Why do you need this? ::: media/gmp-clearkey/0.1/VideoDecoder.cpp:42 (Diff revision 2) > - return; > - } > > - // The first byte is mPacketizationMode, which is only relevant for > - // WebRTC/OpenH264 usecase. > - const uint8_t* avcc = aCodecSpecific + 1; > + // If the method is unable to determine the number of cores it returns zero, > + // so make sure we tell the decoder we have at least one. > + if (cores == 0) { You could make that cores = max(1, std::thread::hardware_concurrency()) ::: media/gmp-clearkey/0.1/VideoDecoder.cpp:161 (Diff revision 2) > -{ > + } > - CK_LOGD("[%p] VideoDecoder::ReturnOutput()\n", this); > - assert(aSample); > > - HRESULT hr; > + CComPtr<IMFSample> result = mOutputQueue.front(); > I think you should pop here right after the 'front' call, as otherwise you won't change the queue in the failure case. ::: media/gmp-clearkey/0.1/VideoDecoder.cpp:187 (Diff revision 2) > - GMPVideoi420Frame* aVideoFrame) > + VideoFrame* aVideoFrame) > { > + CK_LOGD("[%p] VideoDecoder::SampleToVideoFrame()\n", this); > + assert(aSample); > + > + // WideVine doesn't have support for negative strides, though they are Lowercase 'v' in Widevine. ::: media/gmp-clearkey/0.1/VideoDecoder.cpp:189 (Diff revision 2) > + CK_LOGD("[%p] VideoDecoder::SampleToVideoFrame()\n", this); > + assert(aSample); > + > + // WideVine doesn't have support for negative strides, though they are > + // theoretically possible in real world data. > + assert(aStride > 0); You should error on negative stride; there's nothing stopping the caller passing a negative stride value in, violating the assertion. You shouldn't assert on things that are outside your control. ::: media/gmp-clearkey/0.1/VideoDecoder.cpp:232 (Diff revision 2) > - int32_t y_size = stride * (aHeight + padding); > - int32_t v_size = stride * (aHeight + padding) / 4; > - int32_t halfStride = (stride + 1) / 2; > - int32_t halfHeight = (aHeight + 1) / 2; > - > - auto err = aVideoFrame->CreateEmptyFrame(stride, aHeight, stride, halfStride, halfStride); > + uint32_t ySize = stride * (aHeight + padding); > + uint32_t uSize = stride * (aHeight + padding) / 4; > + uint32_t halfStride = (stride + 1) / 2; > + uint32_t halfHeight = (aHeight + 1) / 2; > + > + uint32_t uOffset = stride*aHeight; Space on either side of operands like '*'. ::: media/gmp-clearkey/0.1/VideoDecoder.cpp:243 (Diff revision 2) > - ENSURE(GMP_SUCCEEDED(err), E_FAIL); > - > - uint8_t* outBuffer = aVideoFrame->Buffer(kGMPYPlane); > - ENSURE(outBuffer != nullptr, E_FAIL); > - assert(aVideoFrame->AllocatedSize(kGMPYPlane) >= stride*aHeight); > + > + aVideoFrame->SetSize(Size(aWidth, aHeight)); > + > + uint64_t bufferSize = static_cast<uint64_t>(stride) > + * aHeight > + + 2* halfStride*halfHeight; spaces around operands. ::: media/gmp-clearkey/0.1/VideoDecoder.cpp:256 (Diff revision 2) > + // Get the buffer from the host. > + Buffer* buffer = mHost->Allocate(bufferSize); > + aVideoFrame->SetFrameBuffer(buffer); > + > + // Make sure the buffer is non-null and of sufficent size. > + assert(buffer != nullptr); This shouldn't be an assert, as it could actually not be true if the memory allocation fails. Note that previously we checked the memory allocation. ::: media/gmp-clearkey/0.1/VideoDecoder.cpp:258 (Diff revision 2) > + aVideoFrame->SetFrameBuffer(buffer); > + > + // Make sure the buffer is non-null and of sufficent size. > + assert(buffer != nullptr); > + assert(buffer->Capacity() >= bufferSize); > + Why can't you just memcpy(data, buffer, bufferSize), rather than copying each plane individually? The GMP API required separate buffers for each plane, but the CDM API doesn't require that, and indeed we don't want it to so that we can optimize the shared memory surfaces we need to transmit frames across processes. So we should just make this easier and use one memcpy. ::: media/gmp-clearkey/0.1/VideoDecoder.cpp:303 (Diff revision 2) > if (mDecoder) { > mDecoder->Reset(); > } > > - // Schedule ResetComplete callback to run after existing frames have been > - // flushed out of the task queue. > + // Remove all the frames from the output queue. > + while (mOutputQueue.size() > 0) { use empty() instead of size() > 0 ::: media/gmp-clearkey/0.1/gmp-clearkey.cpp:45 (Diff revision 2) > -GMPGetAPI(const char* aApiName, void* aHostAPI, void** aPluginAPI) > -{ > - CK_LOGD("ClearKey GMPGetAPI |%s|", aApiName); > - assert(!*aPluginAPI); > +void* CreateCdmInstance(int cdm_interface_version, > + const char* key_system, > + uint32_t key_system_size, > + GetCdmHostFunc get_cdm_host_func, > + void* user_data) { > Opening braces for functions should be on the next line. Return types for functions should be on their own line. So: ReturnType FunctionName(Arg arg) { //... }

Comment on attachment 8820867 [details] Bug 1318965 - Converts gmp-clearkey to use Chromium ContentDecryptionModule8 interface used by widevine https://reviewboard.mozilla.org/r/99634/#review104066 > Why can't you just memcpy(data, buffer, bufferSize), rather than copying each plane individually? > > The GMP API required separate buffers for each plane, but the CDM API doesn't require that, and indeed we don't want it to so that we can optimize the shared memory surfaces we need to transmit frames across processes. > > So we should just make this easier and use one memcpy. I don't think we can can we? Isn't there potentially padding on the data in the `data` variable which we don't want to copy into our decoded frame? The `y_size` refers to the size of the padded y data (`stride * (aHeight + padding)`), whereas we only copy `stride * aHeight` elements into our output buffer. It's similar for `u` and `v`

Comment on attachment 8820867 [details] Bug 1318965 - Converts gmp-clearkey to use Chromium ContentDecryptionModule8 interface used by widevine https://reviewboard.mozilla.org/r/99634/#review104434 ::: media/gmp-clearkey/0.1/ClearKeyPersistence.cpp:122 (Diff revisions 2 - 3) > } else { > mPersistentKeyState = PersistentKeyState::LOADED; > aOnInitialized(); > } > } > You've got two blank lines here. Please use at most one at a time. ::: media/gmp-clearkey/0.1/ClearKeySessionManager.cpp:61 (Diff revisions 2 - 3) > > RefPtr<ClearKeySessionManager> self(this); > function<void()> onPersistentStateLoaded = > [self] () > { > - for (function<void()>& func : self->mDefferedInitialize) { > + while (!self->mDefferedInitialize.empty()) { And Deferred is spelt with one 'f'. ::: media/gmp-clearkey/0.1/ClearKeySessionManager.cpp:62 (Diff revisions 2 - 3) > RefPtr<ClearKeySessionManager> self(this); > function<void()> onPersistentStateLoaded = > [self] () > { > - for (function<void()>& func : self->mDefferedInitialize) { > - func(); > + while (!self->mDefferedInitialize.empty()) { > + function<void()> func = self->mDefferedInitialize.back(); This means you'll be processing the funcs in the reverse order in which they were added to the list. This may be confusing to the calller. You're best to process them in the order they were added. So you may which to change mDefferedInitialize to a queue, and pop the and dispatch the front element until empty. ::: media/gmp-clearkey/0.1/ClearKeySessionManager.cpp:98 (Diff revisions 2 - 3) > if (MaybeDeferTillInitialized(deferrer)) { > return; > } > > CK_LOGD("ClearKeySessionManager::CreateSession type:%s", aInitDataType); > - CK_LOGARRAY("ClearKeySessionManager::CreateSession initdata: %s", > + You've added trailing space here. ::: media/gmp-clearkey/0.1/ClearKeySessionManager.cpp:294 (Diff revisions 2 - 3) > > KeyInformation keyInfo = KeyInformation(); > - keyInfo.key_id = &keyPair.mKeyId[0]; > + keyInfo.key_id = &keyPairs.back().mKeyId[0]; > keyInfo.key_id_size = keyPair.mKeyId.size(); > keyInfo.status = KeyStatus::kUsable; > - > + Trailing whitespace. ::: media/gmp-clearkey/0.1/VideoDecoder.cpp:156 (Diff revisions 2 - 3) > } > > CComPtr<IMFSample> result = mOutputQueue.front(); > + mOutputQueue.pop(); > + > + // Widevine doesn't have support for negative strides, though they are I think it would be clearer if you said "the Chromium CDM API doesn't have support..."

Comment on attachment 8820867 [details] Bug 1318965 - Converts gmp-clearkey to use Chromium ContentDecryptionModule8 interface used by widevine https://reviewboard.mozilla.org/r/99634/#review104780

Comment on attachment 8820869 [details] Bug 1318965 - Fixes a bug in the WideVineAdapter wherein session load failures were not adapted correctly https://reviewboard.mozilla.org/r/99636/#review104794 ::: dom/media/gmp/widevine-adapter/WidevineDecryptor.cpp:323 (Diff revision 4) > + // call OnResolveNewSessionPromise with nullptr as the sessionId. > + // We can safely assume this means that we have failed to load a session > + // as the other methods specify calling 'OnRejectPromise' when they fail. > + if (!aSessionId) { > + Log("Decryptor::OnResolveNewSessionPromise(aPromiseId=0x%d) Failed to load session", aPromiseId); > + mCallback->ResolveLoadSessionPromise(aPromiseId, false); Line 323 resolves the promise, and then on line 333 we'll try to resolve the promise again. It looks like mPromiseIdToNewSessionTokens won't hold the promiseId for loads, so you're probably logging an error and bailing out on line 329. So you may as well just return here after line 323 anyway.

Comment on attachment 8820869 [details] Bug 1318965 - Fixes a bug in the WideVineAdapter wherein session load failures were not adapted correctly https://reviewboard.mozilla.org/r/99636/#review104834

Pushed by cpearce@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/c1e2b6c14a7f Converts gmp-clearkey to use Chromium ContentDecryptionModule8 interface used by widevine r=cpearce https://hg.mozilla.org/integration/mozilla-inbound/rev/4bdf65d60c9e Removes the custom AtomicRefCount r=cpearce https://hg.mozilla.org/integration/mozilla-inbound/rev/3f756d8ee4cf Fixes a bug in the WideVineAdapter wherein session load failures were not adapted correctly r=cpearce

I had to back this and bug 1319159 out for frequent failures like https://treeherder.mozilla.org/logviewer.html#?job_id=68903985&repo=mozilla-inbound You'd probably need to retrigger the jobs on your try runs several times to catch it. https://hg.mozilla.org/integration/mozilla-inbound/rev/babe3f8a0258

Backout by kwierso@gmail.com: https://hg.mozilla.org/mozilla-central/rev/ee205312f370 Backed out 3 changesets for frequent media test failures a=backout

It looks like this issue is caused by the WidevineAdapter. We have a race condition there where the decryptor is being destroyed before we create a decoder, causing the CDM to crash. Hopefully I've got a fix on the way.

Comment on attachment 8827308 [details] Bug 1318965 - Fixes some bugs in the Widevine logging code and improves existing logging https://reviewboard.mozilla.org/r/105032/#review105804 ::: dom/media/gmp/widevine-adapter/WidevineDecryptor.cpp:36 (Diff revision 1) > > > WidevineDecryptor::WidevineDecryptor() > : mCallback(nullptr) > { > - Log("WidevineDecryptor created this=%p", this); > + Log("WidevineDecryptor created this=%p, instanceId=%d", this, mInstanceId); You should use %u for mInstanceId, as it's unsigned rather than signed. Ditto elsewhere.

Comment on attachment 8827309 [details] Bug 1318965 - Adds a dummy WidevineDecoder so the CDM doesn't crash if the Decryptor has been destroyed https://reviewboard.mozilla.org/r/105034/#review105806 ::: dom/media/gmp/widevine-adapter/WidevineAdapter.cpp:136 (Diff revision 1) > + // decoder to avoid crashing. > if (!wrapper) { > - Log("WidevineAdapter::GMPGetAPI(%s, 0x%p, 0x%p, %u) this=0x%p No cdm for video decoder", > + Log("WidevineAdapter::GMPGetAPI(%s, 0x%p, 0x%p, %u) this=0x%p No cdm for video decoder. Using a DummyDecoder", > aAPIName, aHostAPI, aPluginAPI, aDecryptorId, this); > - return GMPGenericErr; > - } > + > + *aPluginAPI = new WidevineDummyDecoder(static_cast<GMPVideoHost*>(aHostAPI), No need to pass a null CDMWrapper in here. ::: dom/media/gmp/widevine-adapter/WidevineDummyDecoder.h:16 (Diff revision 1) > + > +namespace mozilla { > + > +class WidevineDummyDecoder : public GMPVideoDecoder { > +public: > + NS_INLINE_DECL_THREADSAFE_REFCOUNTING(WidevineDummyDecoder) You don't need to make this refcounted, as you're not holding onto any pointers to this instance directly. Just "delete this" in DecodingComplete().

Comment on attachment 8827308 [details] Bug 1318965 - Fixes some bugs in the Widevine logging code and improves existing logging https://reviewboard.mozilla.org/r/105032/#review105816

Comment on attachment 8827309 [details] Bug 1318965 - Adds a dummy WidevineDecoder so the CDM doesn't crash if the Decryptor has been destroyed https://reviewboard.mozilla.org/r/105034/#review105818

Comment on attachment 8827711 [details] Bug 1318965 - Fixes a bug in the video decoder causing a crash if the video decoder wash shutdown while in the middle of a drain or a reset https://reviewboard.mozilla.org/r/105338/#review106162 You have a way with words.

Comment on attachment 8827712 [details] Bug 1318965 - Changes the way the the 'DeinitializeDecoder' method in the ClearkeyCDM works https://reviewboard.mozilla.org/r/105340/#review106164

Comment on attachment 8827713 [details] Bug 1318965 - Improves the logging in Clearkey https://reviewboard.mozilla.org/r/105342/#review106166 r+ with comments addressed. ::: media/gmp-clearkey/0.1/VideoDecoder.cpp:72 (Diff revision 2) > // If the input buffer we have been passed has a null buffer, it means we > // should drain. > if (!aInputBuffer.data) { > // This will drain the decoder until there are no frames left to drain, > // whereupon it will return 'NeedsMoreData'. > - CK_LOGD("Input buffer null: Draining"); > + CK_LOGD("VideoDecoder::Decoder Input buffer null: Draining"); s/Decoder/Decode/ ::: media/gmp-clearkey/0.1/VideoDecoder.cpp:107 (Diff revision 2) > > hr = mDecoder->Input(buffer.data(), > buffer.size(), > data->mTimestamp); > > - CK_LOGD("VideoDecoder::DecodeTask() Input ret hr=0x%x\n", hr); > + CK_LOGD("VideoDecoder::DecodeTask() Input ret hr=0x%x", hr); s/DecodeTask/Decode/ ::: media/gmp-clearkey/0.1/VideoDecoder.cpp:113 (Diff revision 2) > > > if (FAILED(hr)) { > assert(hr != MF_E_TRANSFORM_NEED_MORE_INPUT); > > - CK_LOGE("VideoDecoder::DecodeTask() decode failed ret=0x%x%s\n", > + CK_LOGE("VideoDecoder::DecodeTask() decode failed ret=0x%x%s", s/DecodeTask/Decode/

https://hg.mozilla.org/integration/mozilla-inbound/rev/fe0662dd0b839b00710eae1a7a4452625027a03a Bug 1318965 - Converts gmp-clearkey to use Chromium ContentDecryptionModule8 interface used by widevine r=cpearce https://hg.mozilla.org/integration/mozilla-inbound/rev/268335551186dd3c9bc9d3bdd50e133b9d7bea7b Bug 1318965 - Removes the custom AtomicRefCount r=cpearce https://hg.mozilla.org/integration/mozilla-inbound/rev/6f863c0059eb81d790f8c12e3ddc0a51f49dcc5e Bug 1318965 - Fixes a bug in the WidevineAdapter wherein session load failures were not adapted correctly r=cpearce https://hg.mozilla.org/integration/mozilla-inbound/rev/3aa851369053cf869703c864230eaba2557132da Bug 1318965 - Fixes some bugs in the Widevine logging code and improves existing logging r=cpearce https://hg.mozilla.org/integration/mozilla-inbound/rev/bb8e9c8daa60b20b4f825482522e799e625b7224 Bug 1318965 - Adds a dummy WidevineDecoder so the CDM doesn't crash if the Decryptor has been destroyed r=cpearce https://hg.mozilla.org/integration/mozilla-inbound/rev/019a392cb8dd7e77256c92e1cedd03ea3b480fb7 Bug 1318965 - Fixes a bug in the video decoder causing a crash if the video decoder was shutdown while in the middle of a drain or a reset r=cpearce https://hg.mozilla.org/integration/mozilla-inbound/rev/cc2329247ee15c6adbbbb55d99bdc677ec6b7c67 Bug 1318965 - Changes the way the the 'DeinitializeDecoder' method in the ClearkeyCDM works r=cpearce https://hg.mozilla.org/integration/mozilla-inbound/rev/ffbbd9fe609b6e386e7e5600308171f1d92f0078 Bug 1318965 - Improves the logging in Clearkey r=cpearce

https://hg.mozilla.org/mozilla-central/rev/fe0662dd0b83 https://hg.mozilla.org/mozilla-central/rev/268335551186 https://hg.mozilla.org/mozilla-central/rev/6f863c0059eb https://hg.mozilla.org/mozilla-central/rev/3aa851369053 https://hg.mozilla.org/mozilla-central/rev/bb8e9c8daa60 https://hg.mozilla.org/mozilla-central/rev/019a392cb8dd https://hg.mozilla.org/mozilla-central/rev/cc2329247ee1 https://hg.mozilla.org/mozilla-central/rev/ffbbd9fe609b