Bug 180081 – [EME] Add the CENC initData support in ClearKey CDM

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Yacine Bandou]

Add the "cenc" initDataType support in ClearKey CDM.
Parse the CENC initData and extract the KIDs by following the W3C spec https://www.w3.org/TR/eme-initdata-cenc/#common-system

[Yacine Bandou]

Created attachment 327751 [details]
Patch

[Xabier Rodríguez Calvar]

Comment on attachment 327751 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=327751&action=review

> Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.cpp:50
> +const unsigned ClearKeyCencSystemIdSize = sizeof(ClearKeyCencSystemId) / sizeof(uint8_t);

Is there any architecture where sizeof(uint8_t) is not 1? If you know of any, leave it, if not, please remove.

> Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.cpp:168
> +    Ref<SharedBuffer> keyids = SharedBuffer::create();

keyIds

> Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.cpp:207
> +    unsigned nbKeyId =  data[index + 3];

The variable name should full words.

> Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.cpp:208
> +    index += 4; // KID_count size.

Why KID_count with an underscore?

> Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.cpp:210
> +    // check the overflow.

Check

> Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.cpp:215
> +    auto kidsArray = InspectorArray::create();

keyIdsArray

> Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.cpp:217
> +    // Read the KeyID

Missing period.

> Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.cpp:372
> +    if (!(isKeyids || isCenc))

if (!isKeyids && !isCenc)

> Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.cpp:380
> +    if (isCenc && (initData.isEmpty()))

I wonder if we should actually parse the initData instead of just ensuring it is not empty. I think we should and for that we need to split the extraction function into two because just for consistency you don't need to create the JSON, just just need the information of how many keys are there and where they are located. So I'd write the following functions:

* std::pair<unsigned, unsigned> extractKeyidsLocationFromCencInitData(...) that returns the number of keys and the position in the SharedBuffer data.
* isCencInitData that invokes the first and checks keys are not zero.
* extractKeyidsFromCencInitData that uses the first and continues the algorithm by extracting the keys and creating the JSON.

> Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.cpp:383
>      return true;

I think we should change the logic here, we consider it's a failure by default unless we manage to ensure initDatas check out.

[Xabier Rodríguez Calvar]

And again, I forgot to ask the same question. Are there any tests for this?

[Michael Catanzaro]

(In reply to Xabier Rodríguez Calvar from comment #2)
> Is there any architecture where sizeof(uint8_t) is not 1? If you know of
> any, leave it, if not, please remove.

Surely not any that can run glib or WebKit.

[Yacine Bandou]

(In reply to Xabier Rodríguez Calvar from comment #3)
> And again, I forgot to ask the same question. Are there any tests for this?

I am working to add a ClearKey tests.
I rework the patch as soon as possible.

[Yacine Bandou]

Created attachment 329713 [details]
Patch

[Xabier Rodríguez Calvar]

Comment on attachment 329713 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=329713&action=review

> Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.cpp:50
> +const uint8_t ClearKeyCencSystemId[] = { 0x10, 0x77, 0xef, 0xec, 0xc0, 0xb2, 0x4d, 0x02, 0xac, 0xe3, 0x3c, 0x1e, 0x52, 0xe2, 0xfb, 0x4b };
> +const unsigned ClearKeyCencSystemIdSize = sizeof(ClearKeyCencSystemId);
> +const unsigned KIDSize = 16;

These things are variables, their names should begin with non-capital. I guess the naming rules would require that KIDSize -> keyIdSize;

> Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.cpp:172
> +    if (initData.isEmpty() ||  initData.size() > std::numeric_limits<unsigned>::max())

There are two spaces before initData.size().

> Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.cpp:185
> +        if (index + 12 +  ClearKeyCencSystemIdSize >= initDataSize)

There are two spaces before ClearKeyCenc...

> Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.cpp:208
> +    keyIdsMap.first = data[index + 3]; // Read the KeyIdsCount

Missing period at the end of the comment.

> Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.cpp:220
> +// This function checks if the initData sharedBuffer is a validate CENC initData.

...is a valid...

> Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.cpp:251
> +        String keyID = WTF::base64URLEncode(&data[index], KIDSize);

keyId

[Yacine Bandou]

Created attachment 330125 [details]
Patch

[Xabier Rodríguez Calvar]

Comment on attachment 330125 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=330125&action=review

> Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.cpp:172
> +    if (initData.isEmpty() ||  initData.size() > std::numeric_limits<unsigned>::max())

(In reply to Xabier Rodríguez Calvar from comment #7)
> > Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.cpp:172
> > +    if (initData.isEmpty() ||  initData.size() > std::numeric_limits<unsigned>::max())
> 
> There are two spaces before initData.size().

Not addressed.

[Yacine Bandou]

Created attachment 330715 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 330715 [details]
Patch

Clearing flags on attachment: 330715

Committed r226621: <https://trac.webkit.org/changeset/226621>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/36372226>