[Webkit-unassigned] [Bug 154235] [GTK][GStreamer] ClearKey EME v1 decryption support

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=154235

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #272321|review+                     |review-
              Flags|                            |

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

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

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.

> Source/WebCore/ChangeLog:24
> +        There are no layout tests unskipped because this feature is
> +        disabled by default.

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.

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

Remove the comment marks, leave the parameters. They are meaningful though unused.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:901
> +MediaPlayer::MediaKeyException MediaPlayerPrivateGStreamerBase::cancelKeyRequest(const String& /* keySystem */ , const String& /* sessionId */)

Ditto.

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

Parameters seem meaningful here. I wouldn't omit them.

> Source/WebCore/platform/graphics/gstreamer/WebKitClearKeyDecryptorGStreamer.cpp:41
> +#define CLEAR_KEY_PROTECTION_SYSTEM_ID "58147ec8-0423-4659-92e6-f52c5ce8c3cc"

Where does this come from? Maybe a comment?

> Source/WebCore/platform/graphics/gstreamer/WebKitClearKeyDecryptorGStreamer.cpp:70
> +                "key-system-id", G_TYPE_STRING, "org.w3.clearkey", nullptr)));

nullptr -> NULL

> Source/WebCore/platform/graphics/gstreamer/WebKitClearKeyDecryptorGStreamer.cpp:155
> +        gst_buffer_unmap(buffer, &map);
> +        return FALSE;

We could do this GStreamer style, with a goto.

> Source/WebCore/platform/graphics/gstreamer/WebKitClearKeyDecryptorGStreamer.cpp:170
> +                gst_byte_reader_free(reader);
> +                gst_buffer_unmap(buffer, &map);
> +                gst_buffer_unmap(subSamplesBuffer, &subSamplesMap);

Ditto.

> Source/WebCore/platform/graphics/gstreamer/WebKitClearKeyDecryptorGStreamer.cpp:189
> +                gst_byte_reader_free(reader);
> +                gst_buffer_unmap(buffer, &map);
> +                gst_buffer_unmap(subSamplesBuffer, &subSamplesMap);

Ditto.

> Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.cpp:96
> +    unsigned size = gst_caps_get_size(caps);

guint?

> Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.cpp:99
> +        GstStructure* in = gst_caps_get_structure(caps, i);
> +        GUniquePtr<GstStructure> out;

I prefer more meaningful names, inputStructure/outputStructure

> Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.cpp:121
> +            for (int index = gst_structure_n_fields(tmp.get()) - 1; index >= 0; --index) {

gint?

> Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.cpp:147
> +        unsigned size = gst_caps_get_size(transformedCaps);

guint?

> 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;
> +            }
> +        }

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

> Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.cpp:194
> +        gst_buffer_remove_meta(buffer, meta);
> +        return GST_FLOW_NOT_SUPPORTED;

We can go GStreamer style with gotos.

> Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.cpp:201
> +        gst_buffer_remove_meta(buffer, meta);
> +        return GST_FLOW_NOT_SUPPORTED;

Ditto.

> Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.cpp:206
> +        gst_buffer_remove_meta(buffer, meta);
> +        return GST_FLOW_OK;

Ditto.

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

Ditto.

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

Ditto.

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

Ditto.

> Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.cpp:240
> +        webkitCommonEncryptionDecryptorReleaseCipher(self);
> +        gst_buffer_remove_meta(buffer, meta);
> +        return GST_FLOW_NOT_SUPPORTED;

Ditto.

> Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.cpp:248
> +        webkitCommonEncryptionDecryptorReleaseCipher(self);
> +        gst_buffer_remove_meta(buffer, meta);
> +        return GST_FLOW_NOT_SUPPORTED;

Ditto.

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

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.

> Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.h:57
> +    gboolean (*handleKeyResponse) (WebKitCommonEncryptionDecryptor*, GstEvent* event);

Remove the event parameter name here. Not meaningful

> Source/cmake/FindLibGcrypt.cmake:18
> +# Copyright 2014 Nicolás Alvarez <nicolas.alvarez at gmail.com>

Fix enconding

> Source/cmake/FindLibGpgError.cmake:19
> +# Copyright 2014 Nicolás Alvarez <nicolas.alvarez at gmail.com>

Ditto.

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