[Webkit-unassigned] [Bug 154235] [GTK][GStreamer] ClearKey EME v1 decryption support
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Feb 26 02:25:20 PST 2016
https://bugs.webkit.org/show_bug.cgi?id=154235
--- Comment #33 from Carlos Garcia Campos <cgarcia 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
Some more general comments, mostly very minor nits
> Source/WebCore/PlatformGTK.cmake:834
> + ${LIBGCRYPT_LIBRARIES} -lgpg-error
See Michael question about this.
> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:75
> +#if ENABLE(ENCRYPTED_MEDIA)
> +#include "WebKitClearKeyDecryptorGStreamer.h"
> +#endif
The headers ia already guarded, no need for more ifdefs here.
> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:246
> + GstBuffer* data;
GRefPtr
> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:911
> +void MediaPlayerPrivateGStreamerBase::needKey(const String& keySystem, const String& sessionId, const unsigned char* initData, unsigned initDataLength)
previously you were using sessionID, so use that here as well for consistency
> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.h:134
> + void needKey(const String&, const String&, const unsigned char*, unsigned);
> + void dispatchDecryptionKey(GstBuffer*);
Do these need to be public? they are only used by MediaPlayerPrivateGStreamerBase
> Source/WebCore/platform/graphics/gstreamer/WebKitClearKeyDecryptorGStreamer.cpp:61
> + WebKitMediaClearKeyDecrypt* self = WEBKIT_MEDIA_CK_DECRYPT(object);
> + WebKitMediaClearKeyDecryptPrivate* priv = self->priv;
We are using self just to get the priv, this could be just:
WebKitMediaClearKeyDecryptPrivate* priv = WEBKIT_MEDIA_CK_DECRYPT(object)->priv;
> Source/WebCore/platform/graphics/gstreamer/WebKitClearKeyDecryptorGStreamer.cpp:63
> + priv->~WebKitMediaClearKeyDecryptPrivate();
Or even use WEBKIT_MEDIA_CK_DECRYPT(object)->priv directly here
> 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().
> Source/WebCore/platform/graphics/gstreamer/WebKitClearKeyDecryptorGStreamer.h:38
> +#include "WebKitCommonEncryptionDecryptorGStreamer.h"
> +
> +G_BEGIN_DECLS
> +
> +#define WEBKIT_TYPE_MEDIA_CK_DECRYPT (webkit_media_clear_key_decrypt_get_type())
> +#define WEBKIT_MEDIA_CK_DECRYPT(obj) (G_TYPE_CHECK_INSTANCE_CAST((obj), WEBKIT_TYPE_MEDIA_CK_DECRYPT, WebKitMediaClearKeyDecrypt))
> +#define WEBKIT_MEDIA_CK_DECRYPT_CLASS(klass) (G_TYPE_CHECK_CLASS_CAST((klass), WEBKIT_TYPE_MEDIA_CK_DECRYPT, WebKitMediaClearKeyDecryptClass))
> +#define WEBKIT_IS_MEDIA_CK_DECRYPT(obj) (G_TYPE_CHECK_INSTANCE_TYPE((obj), WEBKIT_TYPE_MEDIA_CK_DECRYPT))
> +#define WEBKIT_IS_MEDIA_CK_DECRYPT_CLASS(obj) (G_TYPE_CHECK_CLASS_TYPE((klass), WEBKIT_TYPE_MEDIA_CK_DECRYPT))
> +
> +typedef struct _WebKitMediaClearKeyDecrypt WebKitMediaClearKeyDecrypt;
> +typedef struct _WebKitMediaClearKeyDecryptClass WebKitMediaClearKeyDecryptClass;
> +typedef struct _WebKitMediaClearKeyDecryptPrivate WebKitMediaClearKeyDecryptPrivate;
I'm very confused with the names here, I think it's important to follow the conventions to make this easier to follow. Here we have a file called WebKitClearKeyDecryptorGStreamer.h that implements the GObject WebKitMediaClearKeyDecrypt, with GObject macros prefixed with WEBKIT_MEDIA_CK_DECRYPT, and the internal methods prefixed with webKitMediaClearKeyDecryptor. We need to make all this consistent, if this is a Decryptor, we should have a file called WebKitMediaClearKeyDecryptor.cpp thata implements WebKitMediaClearKeyDecryptor using macros prefixed with WEBKIT_MEDIA_CLEAR_KEY_DECRYPTOR and internal methods with webKitMediaClearKeyDecryptor. Or any other name, because I have no idea what this is for, but consistent.
> 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?
> Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.cpp:42
> +G_DEFINE_TYPE(WebKitMediaCommonEncryptionDecrypt, webkit_media_common_encryption_decrypt, GST_TYPE_BASE_TRANSFORM)
Should this be an abstract class?
> Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.cpp:49
> + WebKitMediaCommonEncryptionDecrypt* self = WEBKIT_MEDIA_CENC_DECRYPT(object);
> + WebKitMediaCommonEncryptionDecryptPrivate* priv = self->priv;
> +
> + priv->~WebKitMediaCommonEncryptionDecryptPrivate();
Same comments here.
> Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.cpp:56
> + WebKitMediaCommonEncryptionDecryptClass* cencClass = WEBKIT_MEDIA_CENC_DECRYPT_GET_CLASS(self);
> + 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.
> Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.cpp:77
> +static const char* webkitMediaCommonEncryptionDecryptProtectionSystemID(WebKitMediaCommonEncryptionDecrypt* self)
I think we should be consistent everywhere with Id and ID.
> Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.cpp:122
> + GUniquePtr<GstStructure> tmp;
> + tmp.reset(gst_structure_copy(in));
GUniquePtr<GstStructure> tmp(gst_structure_copy(in));
> Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.cpp:145
> + gst_structure_set(out.get(), "protection-system", G_TYPE_STRING, klass->protectionSystemId,
Use webkitMediaCommonEncryptionDecryptProtectionSystemID()
> Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.cpp:153
> + for (unsigned index = 0; !isDuplicated && index < size; ++index) {
You don't need to check !isDuplicated here since you are breaking the loop already when setting it to true.
> Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.cpp:163
> + gst_caps_append_structure(transformedCaps, out.get());
> + out.release();
gst_caps_append_structure(transformedCaps, out.release());
> Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.cpp:285
> + GRefPtr<GstEvent> protectedEvent(event);
I think we are leaking the event this way. We are supposed to unref the event, but we are taking yet another ref, so when the lambda is destroyed there will still be one ref left. I think it's safer to adopt the ref at the beginning of the function, capture the GrefPtr in the lambda, and remove all explicit unrefs
> Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.cpp:288
> + if (protectedThis)
This will never be null, this is not a weak pointer. If what you want is to not do the request if the object is destroyed before the task is dispatched, you need to use weak pointers, otherwise, since you are taking a reference of the object to protected it, here you will always have a valid pointer and a valid object, so the request will always happen.
> Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.cpp:290
> + webkitMediaCommonEncryptionDecryptRequestDecryptionKey(WEBKIT_MEDIA_CENC_DECRYPT(protectedThis.get()), initDataBuffer);
> + });
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?
> Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.cpp:297
> + priv->condition.notifyOne();
Don't you need to lock the mutex before calling notifyOne?
> 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
> Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.cpp:317
> + self->priv->condition.notifyOne();
Don't you need to lock the mutex before calling notifyOne?
> Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.cpp:328
> + return true;
TRUE
> Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.h:44
> +#define WEBKIT_TYPE_MEDIA_CENC_DECRYPT (webkit_media_common_encryption_decrypt_get_type())
> +#define WEBKIT_MEDIA_CENC_DECRYPT(obj) (G_TYPE_CHECK_INSTANCE_CAST((obj), WEBKIT_TYPE_MEDIA_CENC_DECRYPT, WebKitMediaCommonEncryptionDecrypt))
> +#define WEBKIT_MEDIA_CENC_DECRYPT_CLASS(klass) (G_TYPE_CHECK_CLASS_CAST((klass), WEBKIT_TYPE_MEDIA_CENC_DECRYPT, WebKitMediaCommonEncryptionDecryptClass))
> +#define WEBKIT_MEDIA_CENC_DECRYPT_GET_CLASS(obj) (G_TYPE_INSTANCE_GET_CLASS((obj), WEBKIT_TYPE_MEDIA_CENC_DECRYPT, WebKitMediaCommonEncryptionDecryptClass))
> +
> +#define WEBKIT_IS_MEDIA_CENC_DECRYPT(obj) (G_TYPE_CHECK_INSTANCE_TYPE((obj), WEBKIT_TYPE_MEDIA_CENC_DECRYPT))
> +#define WEBKIT_IS_MEDIA_CENC_DECRYPT_CLASS(obj) (G_TYPE_CHECK_CLASS_TYPE((klass), WEBKIT_TYPE_MEDIA_CENC_DECRYPT))
> +
> +typedef struct _WebKitMediaCommonEncryptionDecrypt WebKitMediaCommonEncryptionDecrypt;
> +typedef struct _WebKitMediaCommonEncryptionDecryptClass WebKitMediaCommonEncryptionDecryptClass;
> +typedef struct _WebKitMediaCommonEncryptionDecryptPrivate WebKitMediaCommonEncryptionDecryptPrivate;
> +
> +GType webkit_media_common_encryption_decrypt_get_type(void);
I'm confused with the names here again.
--
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/99dfb25d/attachment.html>
More information about the webkit-unassigned
mailing list