Closed Bug 1044742 Opened 6 years ago Closed 5 years ago

[EME] Create a ClearKey GMP, use it in mochitests

Categories

(Core :: Audio/Video, defect)

35 Branch
x86_64
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
firefox37 --- fixed
firefox38 --- fixed
firefox39 --- fixed

People

(Reporter: cpearce, Assigned: eflores)

References

(Blocks 1 open bug)

Details

Attachments

(7 files, 5 obsolete files)

5.98 KB, patch
cpearce
: review+
Details | Diff | Splinter Review
5.57 KB, patch
ted
: review+
Details | Diff | Splinter Review
35.77 KB, patch
cpearce
: review+
Details | Diff | Splinter Review
20.70 KB, patch
Details | Diff | Splinter Review
36.04 KB, patch
cpearce
: review+
Details | Diff | Splinter Review
61.54 KB, patch
cpearce
: review+
gerv
: feedback+
Details | Diff | Splinter Review
1.64 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
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.
Assignee: nobody → edwin
Blocks: 1032660
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.
Attachment #8488413 - Flags: review?(cpearce)
Attached patch Mochitest for EME (obsolete) — Splinter Review
Attachment #8488416 - Flags: review?(cpearce)
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.
Attachment #8488415 - Flags: review?(rjesup)
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.
Attachment #8488411 - Flags: feedback?(gerv)
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.
Attachment #8488412 - Flags: review?(cpearce) → review-
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.
Attachment #8488416 - Flags: review?(cpearce) → review-
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.
Attachment #8488413 - Flags: review?(cpearce) → review+
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.
Attachment #8488411 - Flags: review?(cpearce) → review+
(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...
Attachment #8488411 - Flags: review+
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.
Flags: needinfo?(gerv)
http://www.boost.org/users/license.html would be absolutely fine.

Gerv
Flags: needinfo?(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:
Attachment #8488415 - Flags: review?(ted) → review+
Attached patch ClearKey CDM for testing EME (obsolete) — Splinter Review
Addressed review comments; fixed broken tests; changed to OpenAES for decryption.
Attachment #8488412 - Attachment is obsolete: true
Attachment #8493290 - Flags: review?(cpearce)
Once more, with feeling.
Attachment #8493290 - Attachment is obsolete: true
Attachment #8493290 - Flags: review?(cpearce)
Attachment #8493294 - Flags: review?(cpearce)
Attachment #8488416 - Attachment is obsolete: true
Attachment #8493296 - Flags: review?(cpearce)
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.
Attachment #8488411 - Attachment is obsolete: true
Attachment #8488411 - Flags: feedback?(gerv)
Attachment #8493297 - Flags: review?(cpearce)
Attachment #8493297 - Flags: feedback?(gerv)
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.
Attachment #8493294 - Flags: review?(cpearce) → review+
Attachment #8493296 - Flags: review?(cpearce) → review+
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.
Attachment #8493297 - Flags: review?(cpearce) → review+
(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.
Whiteboard: [leave open]
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
Attachment #8494196 - Flags: review?(bzbarsky) → review+
Forgot to take off the [leave-open] after landing test fixes.
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [leave open]
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
Attachment #8493297 - Flags: feedback?(gerv) → feedback+
Depends on: 1092023
Version: 29 Branch → 35 Branch
Depends on: 1133291
Mass update firefox-status to track EME uplift.
See Also: → 1572846
You need to log in before you can comment on or make changes to this bug.