[Webkit-unassigned] [Bug 180081] [EME] Add the CENC initData support in ClearKey CDM

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Nov 29 04:04:14 PST 2017


Xabier Rodríguez Calvar <calvaris at igalia.com> changed:

           What    |Removed                     |Added
 Attachment #327751|review?, commit-queue?      |review-, commit-queue-
              Flags|                            |

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

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();


> 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.


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


> 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.

You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20171129/afe1a515/attachment.html>

More information about the webkit-unassigned mailing list