[webkit-reviews] review denied: [Bug 180080] [EME][GStreamer] Add the full-sample encryption support in the GStreamer ClearKey decryptor : [Attachment 331346] 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 180080: [EME][GStreamer] Add the full-sample encryption support in the
GStreamer ClearKey decryptor
https://bugs.webkit.org/show_bug.cgi?id=180080

Attachment 331346: Patch

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




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

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

Shouldn't this patch also remove avoid the crash flagged in r226966

>
Source/WebCore/platform/graphics/gstreamer/eme/WebKitClearKeyDecryptorGStreamer
.cpp:242
> +    GstByteReader* reader;
> +    unsigned position = 0;
> +    unsigned sampleIndex = 0;

Can we leave this variables where they're used or is the compiler complaining?

>
Source/WebCore/platform/graphics/gstreamer/eme/WebKitClearKeyDecryptorGStreamer
.cpp:244
> +    bool subsamplesBufferMapped;

Ditto.

>
Source/WebCore/platform/graphics/gstreamer/eme/WebKitClearKeyDecryptorGStreamer
.cpp:252
> +	       error = gcry_cipher_decrypt(priv->handle, map.data, map.size, 0,
0);

We should rename this variable to something more meaningful like cypherError.

>
Source/WebCore/platform/graphics/gstreamer/eme/WebKitClearKeyDecryptorGStreamer
.cpp:289
> +	   GST_TRACE_OBJECT(self, "subsample index %d - %d bytes clear
(todo=%zu)", sampleIndex, nBytesClear, map.size - position);

If I am not mistaken, sampleIndex should be printed with %u and nBytesClear
with %hu.

>
Source/WebCore/platform/graphics/gstreamer/eme/WebKitClearKeyDecryptorGStreamer
.cpp:292
> +	       GST_TRACE_OBJECT(self, "subsample index %d - %d bytes encrypted
(todo=%zu)", sampleIndex, nBytesEncrypted, map.size - position);

Ditto.

>
Source/WebCore/platform/graphics/gstreamer/eme/WebKitClearKeyDecryptorGStreamer
.cpp:295
> +		   GST_ERROR_OBJECT(self, "sub sample index %d decryption
failed: %s", sampleIndex, gpg_strerror(error));

Ditto.

>
Source/WebCore/platform/graphics/gstreamer/eme/WebKitClearKeyDecryptorGStreamer
.cpp:302
> +freeReader:

I think we should rename this as releaseSubsamples

>
Source/WebCore/platform/graphics/gstreamer/eme/WebKitClearKeyDecryptorGStreamer
.cpp:306
> +unmapBuffer:

I think we should rename this as releaseBuffer.