[webkit-reviews] review denied: [Bug 174779] [EME][GStreamer] Multi-key support in the GStreamer ClearKey decryptor : [Attachment 316441] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Jul 26 06:37:33 PDT 2017
Xabier RodrÃguez Calvar <calvaris at igalia.com> has denied Zan Dobersek
<zan at falconsigh.net>'s request for review:
Bug 174779: [EME][GStreamer] Multi-key support in the GStreamer ClearKey
decryptor
https://bugs.webkit.org/show_bug.cgi?id=174779
Attachment 316441: Patch
https://bugs.webkit.org/attachment.cgi?id=316441&action=review
--- Comment #3 from Xabier RodrÃguez Calvar <calvaris at igalia.com> ---
Comment on attachment 316441
--> https://bugs.webkit.org/attachment.cgi?id=316441
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=316441&action=review
>
Source/WebCore/platform/graphics/gstreamer/eme/WebKitClearKeyDecryptorGStreamer
.cpp:47
> +static gboolean
webKitMediaClearKeyDecryptorSetupCipher(WebKitMediaCommonEncryptionDecrypt*,
GstBuffer* kidBuffer);
Remove parameter name.
>
Source/WebCore/platform/graphics/gstreamer/eme/WebKitClearKeyDecryptorGStreamer
.cpp:152
> + GRefPtr<GstBuffer>
keyIDBuffer(adoptGRef(gst_buffer_copy(static_cast<GstBuffer*>(g_value_get_boxed
(gst_value_list_get_value(keyIDsList, i))))));
> + GRefPtr<GstBuffer>
keyValueBuffer(adoptGRef(gst_buffer_copy(static_cast<GstBuffer*>(g_value_get_bo
xed(gst_value_list_get_value(keyValuesList, i))))));
I think, and it's funny that I said this, that it is better to use
gst_value_get_buffer here to avoid the cast. I also think that you could just
use GRefPtr constructor, avoid the copy and let the GRefPtr constructor ref the
buffer for you instead of forcing the copy. Why would you want to copy it?
Just saying, though this code works and does not leak.
>
Source/WebCore/platform/graphics/gstreamer/eme/WebKitClearKeyDecryptorGStreamer
.cpp:159
> +static gboolean
webKitMediaClearKeyDecryptorSetupCipher(WebKitMediaCommonEncryptionDecrypt*
self, GstBuffer* kidBuffer)
I'd name it keyIDBuffer of keyIdBuffer.
>
Source/WebCore/platform/graphics/gstreamer/eme/WebKitClearKeyDecryptorGStreamer
.cpp:167
> + if (!gst_buffer_map(kidBuffer, &kidBufferMap, GST_MAP_READ)) {
You need to unmap after use or we are leaking crap.
>
Source/WebCore/platform/graphics/gstreamer/eme/WebKitCommonEncryptionDecryptorG
Streamer.cpp:263
> + GstBuffer* kidBuffer = gst_value_get_buffer(value);
rename variable
>
Source/WebCore/platform/graphics/gstreamer/eme/WebKitCommonEncryptionDecryptorG
Streamer.h:57
> + gboolean (*setupCipher)(WebKitMediaCommonEncryptionDecrypt*, GstBuffer*
kidBuffer);
Remove parameter name.
More information about the webkit-reviews
mailing list