We need a ClearKey GMP that can decrypt and decrypt-and-decode. We should use this for testing, and we can expose it in decrypt mode on the web. First step is to get it working in decrypt mode, and get mochitests running with it.

MP4s with multiple tracks used to be broken if any of the tracks were encrypted. After this patch, combinations of {plain, encrypted} * {audio, video} work.

Green on Try: https://tbpl.mozilla.org/?tree=Try&rev=444164eee44f

Comment on attachment 8488415 [details] [diff] [review] Make ClearKey CDM accessible in mochitests Review of attachment 8488415 [details] [diff] [review]: ----------------------------------------------------------------- You need to find the right build peer for this review. Probably Ted.

Comment on attachment 8488411 [details] [diff] [review] OpenSSL AES-CTR implementation needed for ClearKey CDM Review of attachment 8488411 [details] [diff] [review]: ----------------------------------------------------------------- Gerv: Are we OK to use this OpenSSL code in Gecko? ::: media/gmp-clearkey/0.1/openssl/LICENSE @@ +41,5 @@ > + * permission of the OpenSSL Project. > + * > + * 6. Redistributions of any form whatsoever must retain the following > + * acknowledgment: > + * "This product includes software developed by the OpenSSL Project This means you need to include a blurb in toolkit/content/license.html to acknowledge the OpenSSL project.

Comment on attachment 8488412 [details] [diff] [review] ClearKey CDM for testing EME Review of attachment 8488412 [details] [diff] [review]: ----------------------------------------------------------------- Looks pretty good, but you need to address the issue about ensuring keysessions are closed properly. ::: media/gmp-clearkey/0.1/ClearKeyDecryptionManager.cpp @@ +9,5 @@ > + > +#include "ClearKeyDecryptionManager.h" > +#include "ClearKeyUtils.h" > + > +#define MAX_SESSION_ID_LEN 256 MAX_SESSION_ID_LEN is unused. @@ +36,5 @@ > + { > + mTarget->Decrypt(mBuffer, mMetadata); > + } > + > + virtual void Destroy() MOZ_OVERRIDE {} virtual void Destroy() MOZ_OVERRIDE { delete this; } Otherwise your GMPTask will leak. @@ +82,5 @@ > +{ > + static uint32_t sNextSessionId = 0; > + > + string sessionId; > + stringstream ss; Shame we can't use std::to_string()... @@ +192,5 @@ > + const char* aSessionId, > + uint32_t aSessionIdLength) > +{ > + CK_LOGD("ClearKeyDecryptionManager::CloseSession"); > + mSessions.erase(string(aSessionId, aSessionId + aSessionIdLength)); This doesn't delete the ClearKeySession, so it'll leak right? You should also delete the ClearKeyDecryptor in mDecryptors corresponding to the keys associated with that sessionId, otherwise you'll still be able to decrypt those with the keys from the closed MediaKeySession on the MediaKeys object. However the spec says "The CDM may close a session at any point, such as in response to a close() call, when the session is no longer needed, or when system resources are lost. Keys in other sessions should be unaffected, even if they have overlapping key IDs." So if two MediaKeySessions on the same MediaKeys object have the same keyId usable, and close() is called on one MediaKeySession, the remaining MediaKeySession should still be able to decode with that key. So you can't just erase all ClearKeyDecryptors with the same keyIds as is in the closed session from mDecryptors here, you need to take into account that there may be multiple key sessions requiring each ClearKeyDecryptor to remain alive. @@ +273,5 @@ > + if (aMetadata->NumSubsamples()) { > + // Take all encrypted parts of subsamples and stitch them into one > + // continuous encrypted buffer. > + unsigned char* data = aBuffer->Data(); > + unsigned char* iter = tmp.data(); This may need to be &tmp.front() in order to work on some older compilers, I'm not sure if we're allowed to use std::vector.data() yet. ::: media/gmp-clearkey/0.1/ClearKeyDecryptionManager.h @@ +5,5 @@ > +#ifndef __ClearKeyDecryptor_h__ > +#define __ClearKeyDecryptor_h_ > + > +#include <map> > +#include <memory> You don't need <memory> here right? Include it in the .cpp file where it's used. @@ +63,5 @@ > +private: > + GMPDecryptorCallback* mCallback; > + GMPDecryptorHost* mHost; > + > + std::map<std::vector<uint8_t>, ClearKeyDecryptor*> mDecryptors; std::map<KeyId, ClearKeyDecryptor*> mDecryptors; Right? ::: media/gmp-clearkey/0.1/ClearKeyUtils.cpp @@ +76,5 @@ > + using mozilla::BigEndian; > + > + uint32_t size = 0; > + for (uint32_t offset = 0; offset < aInitDataSize; offset += size) { > + const uint8_t* data = aInitData + offset; Even if (offset < aInitDataSize) is true, there's no guarantee that (offset + sizeof(uint32)) is within the bounds of (aInitData + aInitDataSize). So maybe your for loop condition should be (offset + sizeof(uint32_t) < aInitDataSize)? @@ +107,5 @@ > + continue; > + } > + > + bool systemIdMatches = true; > + for (size_t i = 0; i < sizeof(kSystemID); i++) { Can you use memcmp() instead? @@ +359,5 @@ > + // The number of bytes we haven't yet filled in the current byte, mod 8. > + int shift = 0; > + > + aOutDecoded.resize(aEncoded.length() * 6 / 8); > + aOutDecoded.reserve(aEncoded.length() * 6 / 8 + 1); Isn't the reserve() here redundant if you're resizing first? @@ +419,5 @@ > + } else if (label == "alg") { > + if (!GetNextLabel(aCtx, value)) return false; > + isExpectedAlg = value == "A128KW"; > + } else if (label == "k") { > + // XXX is this always a string? The spec says "The base64url encoding of the octet sequence containing the symmetric key value", so yes, it's a string. Ditto for kid. @@ +507,5 @@ > + SkipToken(ctx); > + } > + > + // Consume ',' between object members. > + if (PeekSymbol(ctx) != ',') { Doesn't the (PeekSymbol(ctx) != ',') make the EXPECT_SYMBOL(ctx, ',') check redundant? ::: media/gmp-clearkey/0.1/ClearKeyUtils.h @@ +21,5 @@ > +#define CK_LOGD(...) > +#define CK_LOGW(...) > +#endif > + > +class GMPPlatformAPI; s/class/struct/ GMPPlatformAPI is a struct. This can matter on some versions of Windows. ::: media/gmp-clearkey/0.1/gmp-clearkey.cpp @@ +16,5 @@ > +GetPlatform() > +{ > + return sPlatform; > +} > + Nit: extra line break.

Comment on attachment 8488413 [details] [diff] [review] Assorted fixes for multi-track encrypted MP4s Review of attachment 8488413 [details] [diff] [review]: ----------------------------------------------------------------- ::: media/libstagefright/binding/include/mp4_demuxer/DecoderData.h @@ -98,5 @@ > int8_t frequency_index; > int8_t aac_profile; > mozilla::Vector<uint8_t> extra_data; > mozilla::Vector<uint8_t> audio_specific_config; > - CryptoTrack crypto; Removing this doesn't look right; how can we decrypt audio without this?

(In reply to Chris Pearce (:cpearce) from comment #10) > Comment on attachment 8488413 [details] [diff] [review] > Assorted fixes for multi-track encrypted MP4s > > Review of attachment 8488413 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: media/libstagefright/binding/include/mp4_demuxer/DecoderData.h > @@ -98,5 @@ > > int8_t frequency_index; > > int8_t aac_profile; > > mozilla::Vector<uint8_t> extra_data; > > mozilla::Vector<uint8_t> audio_specific_config; > > - CryptoTrack crypto; > > Removing this doesn't look right; how can we decrypt audio without this? It was shadowing CryptoTrack::crypto causing crypto sadness.

Comment on attachment 8488416 [details] [diff] [review] Mochitest for EME Review of attachment 8488416 [details] [diff] [review]: ----------------------------------------------------------------- r-; I want a shorter test file there if possible. ::: content/media/test/gizmo-cenc.xml @@ +3,5 @@ > +<!-- > + This XML file describes the encryption applied to gizmo-cenc.mp4. To generate > + gizmo-cenc, run the following command: > + > + MP4Box -crypt gizmo-cenc.xml -out gizmo-cenc.mp4 gizmo.mp4 Please can you use a shorter duration file, say less than 1 second in length? The longer the files we have, the longer our test suite takes to run. They all add up... Like: http://people.mozilla.org/~cpearce/h264_baseline_lvl3.mp4 I can give you the script that generated that file if you need it. ::: content/media/test/test_encryptedMediaExtensions.html @@ +146,5 @@ > +SimpleTest.waitForExplicitFinish(); > +SpecialPowers.pushPrefEnv( > + {"set": [ > + [ "media.eme.enabled", true ], > + [ "media.fragmented-mp4.exposed", true ], "media.fragmented-mp4.exposed" should be true by default on platforms we will support EME on. @@ +148,5 @@ > + {"set": [ > + [ "media.eme.enabled", true ], > + [ "media.fragmented-mp4.exposed", true ], > + // XXX remove once we have mp4 PlatformDecoderModules on all platforms. > + [ "media.fragmented-mp4.use-blank-decoder", true ], Can you please only set "media.fragmented-mp4.use-blank-decoder" to true if HTMLMediaElement.canPlayType() reports that we can't play H.264/AAC, or we're on Linux (where we don't have a PDM, but we'll still report we can play H.264/AAC due to having a GStreamerReader). Then we can have more confidence that we can decode decrypted samples on some platforms, as opposed to knowing that the APIs were exercised... It would be good to test what happens if the decryption fails, does the decoder fire an error event, etc. It may not, it may just output noisy frames.

Comment on attachment 8488413 [details] [diff] [review] Assorted fixes for multi-track encrypted MP4s Review of attachment 8488413 [details] [diff] [review]: ----------------------------------------------------------------- Thanks.

Comment on attachment 8488413 [details] [diff] [review] Assorted fixes for multi-track encrypted MP4s Review of attachment 8488413 [details] [diff] [review]: ----------------------------------------------------------------- Thanks.

Comment on attachment 8488411 [details] [diff] [review] OpenSSL AES-CTR implementation needed for ClearKey CDM Review of attachment 8488411 [details] [diff] [review]: ----------------------------------------------------------------- r+ pending gerv's f+, and adding the blurb to toolkit/content/license.html.

(In reply to Chris Pearce (:cpearce) from comment #8) > Gerv: Are we OK to use this OpenSSL code in Gecko? No :-| > ::: media/gmp-clearkey/0.1/openssl/LICENSE > @@ +41,5 @@ > > + * permission of the OpenSSL Project. > > + * > > + * 6. Redistributions of any form whatsoever must retain the following > > + * acknowledgment: > > + * "This product includes software developed by the OpenSSL Project > > This means you need to include a blurb in toolkit/content/license.html to > acknowledge the OpenSSL project. It would, yes, but that's not the obnoxious bit: + * 3. All advertising materials mentioning features or use of this + * software must display the following acknowledgment: + * "This product includes software developed by the OpenSSL Project + * for use in the OpenSSL Toolkit. (http://www.openssl.org/)" This is the "BSD advertising clause", which is famously part of OpenSSL's license. I would strongly like to avoid having to deal with the uncertainty of interpreting it correctly for Gecko. Do we have other options at all? Gerv

Comment on attachment 8488411 [details] [diff] [review] OpenSSL AES-CTR implementation needed for ClearKey CDM Review of attachment 8488411 [details] [diff] [review]: ----------------------------------------------------------------- OK. We need a plan B then...

Crypto++ looks like a good fit and is licensed under the Boost Software License. Would that be kosher? We have some Boost-licensed code in m-c currently, but only headers at the moment.

http://www.boost.org/users/license.html would be absolutely fine. Gerv

Comment on attachment 8488415 [details] [diff] [review] Make ClearKey CDM accessible in mochitests Review of attachment 8488415 [details] [diff] [review]: ----------------------------------------------------------------- ::: media/gmp-clearkey/0.1/Makefile.in @@ +12,3 @@ > > libs:: > cp $(srcdir)/clearkey.info . This rule doesn't seem useful (and copying files every time we build slows down the build). ::: testing/mochitest/runtests.py @@ +1177,5 @@ > if options.gmp_path: > return options.gmp_path > > + gmp_parentdirs = [ > + # For local builds, gmp-fake will be under dist/bin. You might want to change the wording here to not refer to just gmp-fake. @@ +1191,5 @@ > + > + gmp_paths = [os.path.join(parent, sub) > + for parent in gmp_parentdirs > + for sub in gmp_subdirs > + if os.path.isdir(os.path.join(parent, sub))] I'm not wild about the double-list-comprehension, but I wouldn't r- it. @@ +1193,5 @@ > + for parent in gmp_parentdirs > + for sub in gmp_subdirs > + if os.path.isdir(os.path.join(parent, sub))] > + > + if len(gmp_paths) == 0: if not gmp_paths:

Addressed review comments; fixed broken tests; changed to OpenAES for decryption.

Once more, with feeling.

Turns out Crypto++ uses a lot of shiny, new features that we can't support on some of our ancient compilers. OpenAES is licensed under the 2-clase BSD license, with bits of public domain stuff as well.

Green on try: https://tbpl.mozilla.org/?tree=Try&rev=274605be3d6f

Comment on attachment 8493294 [details] [diff] [review] ClearKey CDM for testing EME Review of attachment 8493294 [details] [diff] [review]: ----------------------------------------------------------------- ::: media/gmp-clearkey/0.1/ClearKeyDecryptionManager.cpp @@ +25,5 @@ > + > + void QueueDecrypt(GMPBuffer* aBuffer, GMPEncryptedBufferMetadata* aMetadata); > + > + uint32_t AddRef(); > + uint32_t DecRef(); Nit: normally the opposite of AddRef is Release, and the opposite of Dec is Inc... @@ +223,5 @@ > + ClearKeySession* session = mSessions[sessionId]; > + > + assert(session); > + > + vector<KeyId> keyIds = session->GetKeyIds(); This copies the vector, or moves if you're lucky. Did you mean: const vector<KeyId> keyIds& ? ::: media/gmp-clearkey/0.1/ClearKeyUtils.h @@ +21,5 @@ > +#define CK_LOGD(...) > +#define CK_LOGW(...) > +#endif > + > +struct GMPPlatformAPI; class GMPPlatformAPI; There are some differences to symbol names of classes vs structs under some version of Visual Studio, so you need to fix this before landing.

Comment on attachment 8493297 [details] [diff] [review] OpenAES implementation of AES Review of attachment 8493297 [details] [diff] [review]: ----------------------------------------------------------------- ::: media/gmp-clearkey/0.1/openaes/LICENSE @@ +8,5 @@ > +modification, are permitted provided that the following conditions are met: > + > + - Redistributions of source code must retain the above copyright notice, > + this list of conditions and the following disclaimer. > + - Redistributions in binary form must reproduce the above copyright "Redistributions in binary form must reproduce the above copyright notice, this list of conditions and the following disclaimer in the documentation and/or other materials provided with the distribution." That means we must reproduce this license, so add the license to the blurb to toolkit/content/license.html before landing please.

(In reply to Chris Pearce (:cpearce) from comment #28) > Comment on attachment 8493294 [details] [diff] [review] > ClearKey CDM for testing EME > ::: media/gmp-clearkey/0.1/ClearKeyUtils.h > @@ +21,5 @@ > > +#define CK_LOGD(...) > > +#define CK_LOGW(...) > > +#endif > > + > > +struct GMPPlatformAPI; > > class GMPPlatformAPI; > > There are some differences to symbol names of classes vs structs under some > version of Visual Studio, so you need to fix this before landing. "struct GMPPlatformAPI" is correct, sorry.

https://hg.mozilla.org/integration/mozilla-inbound/rev/a57419135bb5 https://hg.mozilla.org/integration/mozilla-inbound/rev/79cb9483483a https://hg.mozilla.org/integration/mozilla-inbound/rev/2abe5eac8b16 https://hg.mozilla.org/integration/mozilla-inbound/rev/3855948c6f30 https://hg.mozilla.org/integration/mozilla-inbound/rev/8f13101a6dbb

8f13101a6dbb was backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/06fd49990368 for mochitest-4 test failures: https://tbpl.mozilla.org/php/getParsedLog.php?id=48727987&tree=Mozilla-Inbound

Comment on attachment 8494196 [details] [diff] [review] Add EME globals to test_interfaces.html These should be marked conditional on the EME pref, right? r=me with that fixed

https://hg.mozilla.org/integration/mozilla-inbound/rev/76395a87471c https://hg.mozilla.org/integration/mozilla-inbound/rev/9f9b3c194a76 https://hg.mozilla.org/integration/mozilla-inbound/rev/156b285c2025

https://hg.mozilla.org/mozilla-central/rev/a57419135bb5 https://hg.mozilla.org/mozilla-central/rev/79cb9483483a https://hg.mozilla.org/mozilla-central/rev/2abe5eac8b16 https://hg.mozilla.org/mozilla-central/rev/3855948c6f30 https://hg.mozilla.org/mozilla-central/rev/8f13101a6dbb https://hg.mozilla.org/mozilla-central/rev/06fd49990368 https://hg.mozilla.org/mozilla-central/rev/870d0aa48c67 https://hg.mozilla.org/mozilla-central/rev/76395a87471c https://hg.mozilla.org/mozilla-central/rev/9f9b3c194a76 https://hg.mozilla.org/mozilla-central/rev/156b285c2025

Forgot to take off the [leave-open] after landing test fixes.

Comment on attachment 8493297 [details] [diff] [review] OpenAES implementation of AES This 2-clause BSD is fine; if this ships with Firefox, file a bug to get it added to about:license. Gerv

Mass update firefox-status to track EME uplift.