[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

--- Comment #34 from Philippe Normand <pnormand at igalia.com> ---
Comment on attachment 272316
  --> https://bugs.webkit.org/attachment.cgi?id=272316
patch

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

>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.h:134
>> +    void dispatchDecryptionKey(GstBuffer*);
> 
> Do these need to be public? they are only used by MediaPlayerPrivateGStreamerBase

They are private.

>> Source/WebCore/platform/graphics/gstreamer/WebKitClearKeyDecryptorGStreamer.cpp:84
>> +    WEBKIT_MEDIA_CK_DECRYPT(self)->priv->key = adoptGRef(gst_buffer_copy(gst_value_get_buffer(value)));
> 
> However I think this is easier to read by using a local variable to cast WEBKIT_MEDIA_CK_DECRYPT().

Let's try to be consistent then. In the Finalize review you suggest to not use a local variable. So?

>> Source/WebCore/platform/graphics/gstreamer/WebKitClearKeyDecryptorGStreamer.h:43
>> +    WebKitMediaCommonEncryptionDecrypt parent;
> 
> So the base parent class is for encryption and decryption? Do I understand correctly that this is the ClearKey implementation for the base class for decryption?

CommonEncryption (cenc) is what is specified in MPEG ISO BMF. CommonEncryptionDecrypt is the base class for decryption.

>> Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.cpp:56
>> +    return cencClass->setupCipher(self);
> 
> Is it expected that derived classes implement this method? Then add an assert to ensure cencClass is mot null, otherwise add a null check.

We provide a default implementation for this vfunc, so I don't think this code needs to be changed.

>> Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.cpp:290
>> +        });
> 
> So we capture the protected event because the event is the owner of the initDataBuffer, right? Would it be possible to take a reference of the buffer instead? or do we really need to keep the event alive until the request is done?

Both could work I suppose.

>> Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.cpp:307
>> +    return FALSE;
> 
> If we are not supposed to unref the event here we can leak the adopted ref, or do the adopt in every switch case instead

Well, this return can't possibly hit because the default branch of the switch already does a return. So maybe I can add an ASSERT_NOT_REACHED() here instead?

-- 
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/20160226/17b57fee/attachment-0001.html>