[webkit-reviews] review denied: [Bug 180081] [EME] Add the CENC initData support in ClearKey CDM : [Attachment 327751] Patch

[bugzilla-daemon at webkit.org]

Xabier Rodríguez Calvar <calvaris at igalia.com> has denied Yacine Bandou
<bandou.yacine at gmail.com>'s request for review:
Bug 180081: [EME] Add the CENC initData support in ClearKey CDM
https://bugs.webkit.org/show_bug.cgi?id=180081

Attachment 327751: Patch

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




--- Comment #2 from Xabier Rodríguez Calvar <calvaris at igalia.com> ---
Comment on attachment 327751
  --> https://bugs.webkit.org/attachment.cgi?id=327751
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.