<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body><span class="vcard"><a class="email" href="mailto:calvaris@igalia.com" title="Xabier Rodríguez Calvar <calvaris@igalia.com>"> <span class="fn">Xabier Rodríguez Calvar</span></a>
</span> changed
<a class="bz_bug_link
bz_status_NEW "
title="NEW - [GTK][GStreamer] ClearKey EME v1 decryption support"
href="https://bugs.webkit.org/show_bug.cgi?id=154235">bug 154235</a>
<br>
<table border="1" cellspacing="0" cellpadding="8">
<tr>
<th>What</th>
<th>Removed</th>
<th>Added</th>
</tr>
<tr>
<td style="text-align:right;">Attachment #272321 Flags</td>
<td>review+
</td>
<td>review-
</td>
</tr></table>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - [GTK][GStreamer] ClearKey EME v1 decryption support"
href="https://bugs.webkit.org/show_bug.cgi?id=154235#c43">Comment # 43</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - [GTK][GStreamer] ClearKey EME v1 decryption support"
href="https://bugs.webkit.org/show_bug.cgi?id=154235">bug 154235</a>
from <span class="vcard"><a class="email" href="mailto:calvaris@igalia.com" title="Xabier Rodríguez Calvar <calvaris@igalia.com>"> <span class="fn">Xabier Rodríguez Calvar</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=272321&action=diff" name="attach_272321" title="patch">attachment 272321</a> <a href="attachment.cgi?id=272321&action=edit" title="patch">[details]</a></span>
patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=272321&action=review">https://bugs.webkit.org/attachment.cgi?id=272321&action=review</a>
To complement a bit more what Carlos said, some more comments. Most of them are nits. The important thing here is the test. I do think it is needed.
<span class="quote">> Source/WebCore/ChangeLog:24
> + There are no layout tests unskipped because this feature is
> + disabled by default.</span >
If there is none, I think we should add some test to ensure at least the basic functionality is not broken, even when this is not active by default. This would be important specially if you honor Michael's comment later.
<span class="quote">> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:878
> +MediaPlayer::MediaKeyException MediaPlayerPrivateGStreamerBase::addKey(const String& keySystem, const unsigned char* keyData, unsigned keyLength, const unsigned char* /* initData */, unsigned /* initDataLength */ , const String& sessionId)</span >
Remove the comment marks, leave the parameters. They are meaningful though unused.
<span class="quote">> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:901
> +MediaPlayer::MediaKeyException MediaPlayerPrivateGStreamerBase::cancelKeyRequest(const String& /* keySystem */ , const String& /* sessionId */)</span >
Ditto.
<span class="quote">> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.h:133
> + MediaPlayer::MediaKeyException addKey(const String&, const unsigned char*, unsigned, const unsigned char*, unsigned, const String&) override;
> + MediaPlayer::MediaKeyException generateKeyRequest(const String&, const unsigned char*, unsigned) override;
> + MediaPlayer::MediaKeyException cancelKeyRequest(const String&, const String&) override;
> + void needKey(const String&, const String&, const unsigned char*, unsigned);</span >
Parameters seem meaningful here. I wouldn't omit them.
<span class="quote">> Source/WebCore/platform/graphics/gstreamer/WebKitClearKeyDecryptorGStreamer.cpp:41
> +#define CLEAR_KEY_PROTECTION_SYSTEM_ID "58147ec8-0423-4659-92e6-f52c5ce8c3cc"</span >
Where does this come from? Maybe a comment?
<span class="quote">> Source/WebCore/platform/graphics/gstreamer/WebKitClearKeyDecryptorGStreamer.cpp:70
> + "key-system-id", G_TYPE_STRING, "org.w3.clearkey", nullptr)));</span >
nullptr -> NULL
<span class="quote">> Source/WebCore/platform/graphics/gstreamer/WebKitClearKeyDecryptorGStreamer.cpp:155
> + gst_buffer_unmap(buffer, &map);
> + return FALSE;</span >
We could do this GStreamer style, with a goto.
<span class="quote">> Source/WebCore/platform/graphics/gstreamer/WebKitClearKeyDecryptorGStreamer.cpp:170
> + gst_byte_reader_free(reader);
> + gst_buffer_unmap(buffer, &map);
> + gst_buffer_unmap(subSamplesBuffer, &subSamplesMap);</span >
Ditto.
<span class="quote">> Source/WebCore/platform/graphics/gstreamer/WebKitClearKeyDecryptorGStreamer.cpp:189
> + gst_byte_reader_free(reader);
> + gst_buffer_unmap(buffer, &map);
> + gst_buffer_unmap(subSamplesBuffer, &subSamplesMap);</span >
Ditto.
<span class="quote">> Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.cpp:96
> + unsigned size = gst_caps_get_size(caps);</span >
guint?
<span class="quote">> Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.cpp:99
> + GstStructure* in = gst_caps_get_structure(caps, i);
> + GUniquePtr<GstStructure> out;</span >
I prefer more meaningful names, inputStructure/outputStructure
<span class="quote">> Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.cpp:121
> + for (int index = gst_structure_n_fields(tmp.get()) - 1; index >= 0; --index) {</span >
gint?
<span class="quote">> Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.cpp:147
> + unsigned size = gst_caps_get_size(transformedCaps);</span >
guint?
<span class="quote">> Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.cpp:154
> + for (unsigned index = 0; index < size; ++index) {
> + GstStructure* item = gst_caps_get_structure(transformedCaps, index);
> + if (gst_structure_is_equal(item, out.get())) {
> + isDuplicated = true;
> + break;
> + }
> + }</span >
Not strong preference, but:
for (unsigned index = 0; index < size && !isDuplicated; ++index) {
GstStructure* item = gst_caps_get_structure(transformedCaps, index);
isDuplicated = gst_structure_is_equal(item, out.get());
}
<span class="quote">> Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.cpp:194
> + gst_buffer_remove_meta(buffer, meta);
> + return GST_FLOW_NOT_SUPPORTED;</span >
We can go GStreamer style with gotos.
<span class="quote">> Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.cpp:201
> + gst_buffer_remove_meta(buffer, meta);
> + return GST_FLOW_NOT_SUPPORTED;</span >
Ditto.
<span class="quote">> Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.cpp:206
> + gst_buffer_remove_meta(buffer, meta);
> + return GST_FLOW_OK;</span >
Ditto.
<span class="quote">> Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.cpp:214
> + GST_ERROR_OBJECT(self, "Failed to get subsample_count");
> + gst_buffer_remove_meta(buffer, meta);
> + return GST_FLOW_NOT_SUPPORTED;</span >
Ditto.
<span class="quote">> Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.cpp:224
> + GST_ERROR_OBJECT(self, "Failed to get subsamples");
> + gst_buffer_remove_meta(buffer, meta);
> + return GST_FLOW_NOT_SUPPORTED;</span >
Ditto.
<span class="quote">> Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.cpp:232
> + GST_ERROR_OBJECT(self, "Failed to configure cipher");
> + gst_buffer_remove_meta(buffer, meta);
> + return GST_FLOW_NOT_SUPPORTED;</span >
Ditto.
<span class="quote">> Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.cpp:240
> + webkitCommonEncryptionDecryptorReleaseCipher(self);
> + gst_buffer_remove_meta(buffer, meta);
> + return GST_FLOW_NOT_SUPPORTED;</span >
Ditto.
<span class="quote">> Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.cpp:248
> + webkitCommonEncryptionDecryptorReleaseCipher(self);
> + gst_buffer_remove_meta(buffer, meta);
> + return GST_FLOW_NOT_SUPPORTED;</span >
Ditto.
<span class="quote">>> Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.cpp:276
>> + RunLoop::main().dispatch([protectedThis, protectedEvent, initDataBuffer] {
>
> I would add a comment here, explaining that we capture the event because it's the owner of the buffer, otherwise it looks weird to capture something that is not used in the lambda.</span >
I don't know what the spec says about this, but I even wonder if this could even get compiled out at some degree of automatic compiler optimization.
<span class="quote">> Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.h:57
> + gboolean (*handleKeyResponse) (WebKitCommonEncryptionDecryptor*, GstEvent* event);</span >
Remove the event parameter name here. Not meaningful
<span class="quote">> Source/cmake/FindLibGcrypt.cmake:18
> +# Copyright 2014 Nicolás Alvarez <<a href="mailto:nicolas.alvarez@gmail.com">nicolas.alvarez@gmail.com</a>></span >
Fix enconding
<span class="quote">> Source/cmake/FindLibGpgError.cmake:19
> +# Copyright 2014 Nicolás Alvarez <<a href="mailto:nicolas.alvarez@gmail.com">nicolas.alvarez@gmail.com</a>></span >
Ditto.</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>