[webkit-reviews] review granted: [Bug 191157] [EME, GStreamer] Ensure key id buffers are present and simplify lifetime management of ClearKey class : [Attachment 353617] Patch

[bugzilla-daemon at webkit.org]

Xabier Rodríguez Calvar <calvaris at igalia.com> has granted Charlie Turner
<cturner at igalia.com>'s request for review:
Bug 191157: [EME, GStreamer] Ensure key id buffers are present and simplify
lifetime management of ClearKey class
https://bugs.webkit.org/show_bug.cgi?id=191157

Attachment 353617: Patch

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




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

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

> Source/WebCore/ChangeLog:3
> +	   [EME, GStreamer] Ensure key id buffers are present and simplify
lifetime management of ClearKey class

We usually write it like [EME][GStreamer]

> Source/WebCore/ChangeLog:17
> +	   bindings

Period at the end.

>
Source/WebCore/platform/graphics/gstreamer/eme/WebKitClearKeyDecryptorGStreamer
.cpp:103
> +	   ASSERT(!error);
> +	   GST_ERROR_OBJECT(self, "Failed to create AES 128 CTR cipher handle:
%s", gpg_strerror(error));

Please, swap these lines, we would like to see the GST_ERROR in the logs before
we ASSERT and die if in debug.

>
Source/WebCore/platform/graphics/gstreamer/eme/WebKitClearKeyDecryptorGStreamer
.cpp:153
> +static gboolean
webKitMediaClearKeyDecryptorFindAndSetKey(WebKitMediaClearKeyDecryptPrivate*
priv, WebCore::GstMappedBuffer& keyIDBuffer)

I wonder if we should make this a  const GstMapperBuffer &. I guess it would
imply making the data accessor const as well.

And gosh, we messed up names here... These classes need a deep clean up for
style and naming.

>
Source/WebCore/platform/graphics/gstreamer/eme/WebKitClearKeyDecryptorGStreamer
.cpp:157
> +	   if (!gst_buffer_memcmp(key.keyID.get(), 0, keyIDBuffer.data(),
keyIDBuffer.size())) {

You know what?	It would be nice to define a operator== for this on the mapped
buffer, one could be taking a GstBuffer* and a second one taking another mapped
buffer, even the second could be implemented on terms of the first. Of course,
you wouldn't need the second now, so we could leave it out of the question for
now.

>
Source/WebCore/platform/graphics/gstreamer/eme/WebKitClearKeyDecryptorGStreamer
.cpp:174
> +    ASSERT(mappedKeyValueBufferr.size() == CLEARKEY_SIZE);

oops! I would advise either making a debug build or writing RELEASE_ASSERTs
until just before submitting the patch.

>
Source/WebCore/platform/graphics/gstreamer/eme/WebKitCommonEncryptionDecryptorG
Streamer.cpp:273
>      GstBuffer* keyIDBuffer = nullptr;
> -    if (value)
> -	   keyIDBuffer = gst_value_get_buffer(value);
> -
> -    WebKitMediaCommonEncryptionDecryptClass* klass =
WEBKIT_MEDIA_CENC_DECRYPT_GET_CLASS(self);
> -    if (!klass->setupCipher(self, keyIDBuffer)) {
> -	   GST_ERROR_OBJECT(self, "Failed to configure cipher");
> +    if (!value) {
> +	   GST_ERROR_OBJECT(self, "Failed to get key id for buffer");
>	   gst_buffer_remove_meta(buffer,
reinterpret_cast<GstMeta*>(protectionMeta));
>	   return GST_FLOW_NOT_SUPPORTED;
>      }
> +    keyIDBuffer = gst_value_get_buffer(value);

if (!value)  {
...
}
GstBuffer* keyIDBuffer = gst_value_get_buffer(value);

>
Source/WebCore/platform/graphics/gstreamer/eme/WebKitCommonEncryptionDecryptorG
Streamer.cpp:275
> +    WebKitMediaCommonEncryptionDecryptClass* klass =
WEBKIT_MEDIA_CENC_DECRYPT_GET_CLASS(self);

This can be moved below, right?

>
Source/WebCore/platform/graphics/gstreamer/eme/WebKitCommonEncryptionDecryptorG
Streamer.h:57
> +    gboolean (*decrypt)(WebKitMediaCommonEncryptionDecrypt*, GstBuffer*
ivBuffer, GstBuffer* keyIDBuffer, GstBuffer* buffer, unsigned subSamplesCount,
GstBuffer* subSamplesBuffer);

Just a comment for the future here. These are methods that live only in the
WebKit world, not GStreamer's. I think we should rework the style, of course
not now and nowadays it is well as it is.