[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

Carlos Garcia Campos <cgarcia at igalia.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #272321|review?                     |review+, commit-queue-
              Flags|                            |

--- Comment #41 from Carlos Garcia Campos <cgarcia 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

This looks good to me in general, I'm r+'ing it, but I have a few more comments, and I would like that someone more familiar with EME would take a look at it too. Please add these files as exceptions to the style checker as we do with other gobject implementation files.

> Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.cpp:53
> +    WebKitCommonEncryptionDecryptorClass* commonEncryptionDecryptorClass = WEBKIT_COMMON_ENCRYPTION_DECRYPTOR_GET_CLASS(self);
> +    return commonEncryptionDecryptorClass->setupCipher(self);

I still think we should either ASEERT, if this considered a pure virtual method, or null check otherwise. And the same for the rest for the vmethods.

> Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.cpp:266
> +        GRefPtr<GstEvent> protectedEvent = adoptGRef(event);

protected is no longer a good name for this now that we are adopting the ref.

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

> Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.cpp:283
> +        GRefPtr<GstEvent> protectedEvent = adoptGRef(event);

Ditto.

> Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.cpp:325
> +static gboolean webKitCommonEncryptionDecryptorDefaultSetupCipher(WebKitCommonEncryptionDecryptor*)
> +{
> +    return TRUE;
> +}
> +
> +static void webKitCommonEncryptionDecryptorDefaultReleaseCipher(WebKitCommonEncryptionDecryptor*)
> +{
> +}

If these don't do anything useful, keep the vmethods as null and add null checks in the wrappers.

> Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.cpp:345
> +    klass->setupCipher = GST_DEBUG_FUNCPTR(webKitCommonEncryptionDecryptorDefaultSetupCipher);
> +    klass->releaseCipher = GST_DEBUG_FUNCPTR(webKitCommonEncryptionDecryptorDefaultReleaseCipher);

What about the other vmethods?

-- 
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/b7fb441c/attachment-0001.html>