I'd like to have some optional (disabled by default) EME support in WebKitGTK as a proof of concept.
Created attachment 271330 [details] patch
Attachment 271330 [details] did not pass style-queue: ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitClearKeyDecryptorGStreamer.h:41: webkit_media_clear_key_decrypt_get_type is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.h:28: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.h:45: webkit_media_common_encryption_decrypt_get_type is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.h:57: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.h:58: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.h:59: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.h:60: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.h:61: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitClearKeyDecryptorGStreamer.cpp:66: webkit_media_clear_key_decrypt_class_init is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitClearKeyDecryptorGStreamer.cpp:96: webkit_media_clear_key_decrypt_init is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.cpp:56: webkit_media_common_encryption_decrypt_class_init is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.cpp:80: webkit_media_common_encryption_decrypt_init is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/PlatformGTK.cmake:835: Alphabetical sorting problem. "platform/graphics/gstreamer/WebKitClearKeyDecryptorGStreamer.cpp" should be before "platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.cpp". [list/order] [5] ERROR: Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.h:132: The parameter name "parameters" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 14 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 271331 [details] patch
Attachment 271331 [details] did not pass style-queue: ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitClearKeyDecryptorGStreamer.h:41: webkit_media_clear_key_decrypt_get_type is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.h:28: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.h:45: webkit_media_common_encryption_decrypt_get_type is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.h:57: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.h:58: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.h:59: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.h:60: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.h:61: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitClearKeyDecryptorGStreamer.cpp:66: webkit_media_clear_key_decrypt_class_init is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitClearKeyDecryptorGStreamer.cpp:96: webkit_media_clear_key_decrypt_init is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.cpp:56: webkit_media_common_encryption_decrypt_class_init is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.cpp:80: webkit_media_common_encryption_decrypt_init is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Total errors found: 12 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 271332 [details] patch Fixes a GstByteReader leak.
Attachment 271332 [details] did not pass style-queue: ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitClearKeyDecryptorGStreamer.h:41: webkit_media_clear_key_decrypt_get_type is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.h:28: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.h:45: webkit_media_common_encryption_decrypt_get_type is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.h:57: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.h:58: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.h:59: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.h:60: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.h:61: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitClearKeyDecryptorGStreamer.cpp:66: webkit_media_clear_key_decrypt_class_init is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitClearKeyDecryptorGStreamer.cpp:96: webkit_media_clear_key_decrypt_init is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.cpp:56: webkit_media_common_encryption_decrypt_class_init is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.cpp:80: webkit_media_common_encryption_decrypt_init is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Total errors found: 12 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 271332 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=271332&action=review We should add exceptions to the style checker for the GObject implementations. I've only reviewed general issues, I can't review the media keys implementation itself. > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:108 > + GRefPtr<GstElementFactory> clearKeyDecryptorFactory = gst_element_factory_find("webkitclearkey"); adoptGRef > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:110 > + gst_element_register(0, "webkitclearkey", GST_RANK_PRIMARY + 100, WEBKIT_TYPE_MEDIA_CK_DECRYPT); 0 -> nullptr > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:261 > + GstBuffer* data; > + const char* keySystemId; > + gboolean valid = gst_structure_get(structure, "data", GST_TYPE_BUFFER, &data, "key-system-id", G_TYPE_STRING, &keySystemId, nullptr); According to the API docs, gst_structure_get returns a new allocated string for strings, and I guess we are also getting a new ref for the GstBuffer. So, this is probably leaking both > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:267 > + needKey(keySystemId, createCanonicalUUIDString(), reinterpret_cast<const unsigned char *>(mapInfo.data), mapInfo.size); const unsigned char * -> const unsigned char* > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:896 > + if (!equalIgnoringASCIICase(keySystem, "org.w3.clearkey")) > + return MediaPlayer::KeySystemNotSupported; Instead of duplication this check here, we could call supportsKeySystem instead, or move this check to a helper function and use it from both places. > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:908 > + if (!equalIgnoringASCIICase(keySystem, "org.w3.clearkey")) > + return MediaPlayer::KeySystemNotSupported; Same check again > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:931 > + if (parameters.keySystem.isNull() || parameters.keySystem.isEmpty()) parameters.keySystem.isEmpty() returns true if it's null > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.h:57 > +void registerWebKitGStreamerElements(); > + Why does this need to be here? Can't this be a static function? > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.h:126 > + MediaPlayer::MediaKeyException addKey(const String&, const unsigned char*, unsigned, const unsigned char*, unsigned, const String&); > + MediaPlayer::MediaKeyException generateKeyRequest(const String&, const unsigned char*, unsigned); > + MediaPlayer::MediaKeyException cancelKeyRequest(const String&, const String&); This should be marked override, and I don't think they need to be public. > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.h:127 > + void needKey(const String&, const String&, const unsigned char*, unsigned); This could be private too, no? > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.h:128 > + virtual void dispatchDecryptionKey(GstBuffer*); Why is this virtual and public? > Source/WebCore/platform/graphics/gstreamer/WebKitClearKeyDecryptorGStreamer.cpp:25 > +#if ENABLE(ENCRYPTED_MEDIA) && USE(GSTREAMER) > +#include "WebKitClearKeyDecryptorGStreamer.h" The header already has guards, so move it right after the config.h to follow the coding style. > Source/WebCore/platform/graphics/gstreamer/WebKitClearKeyDecryptorGStreamer.cpp:32 > +#define CLEARKEY_SIZE 16 Can this be a static const unsigned? > Source/WebCore/platform/graphics/gstreamer/WebKitClearKeyDecryptorGStreamer.cpp:34 > +#define WEBKIT_MEDIA_CK_DECRYPT_GET_PRIVATE(obj) (G_TYPE_INSTANCE_GET_PRIVATE((obj), WEBKIT_TYPE_MEDIA_CK_DECRYPT, WebKitMediaClearKeyDecryptPrivate)) Don't do this, use G_TYPE_INSTANCE_GET_PRIVATE directly in the init method. > Source/WebCore/platform/graphics/gstreamer/WebKitClearKeyDecryptorGStreamer.cpp:45 > +static void webKitMediaClearKeyDecryptorFinalize(GObject*); > +static void webKitMediaClearKeyDecryptorRequestDecryptionKey(WebKitMediaCommonEncryptionDecrypt* self, GstBuffer*); > +static gboolean webKitMediaClearKeyDecryptorHandleKeyResponse(WebKitMediaCommonEncryptionDecrypt* self, GstEvent*); > +static gboolean webKitMediaClearKeyDecryptorSetupCipher(WebKitMediaCommonEncryptionDecrypt*); > +static gboolean webKitMediaClearKeyDecryptorDecrypt(WebKitMediaCommonEncryptionDecrypt*, GstBuffer* iv, GstBuffer* sample, unsigned subSamplesCount, GstBuffer* subSamples); > +static void webKitMediaClearKeyDecryptorReleaseCipher(WebKitMediaCommonEncryptionDecrypt*); Do we really need all this prototypes, or can we reorder the functions to avoid them? > Source/WebCore/platform/graphics/gstreamer/WebKitClearKeyDecryptorGStreamer.cpp:50 > +#define CLEAR_KEY_PROTECTION_SYSTEM_ID "58147ec8-0423-4659-92e6-f52c5ce8c3cc" Can this be static const char*? > Source/WebCore/platform/graphics/gstreamer/WebKitClearKeyDecryptorGStreamer.cpp:64 > +G_DEFINE_TYPE(WebKitMediaClearKeyDecrypt, webkit_media_clear_key_decrypt, WEBKIT_TYPE_MEDIA_CENC_DECRYPT); Trailing ; is not needed. > Source/WebCore/platform/graphics/gstreamer/WebKitClearKeyDecryptorGStreamer.cpp:70 > + WebKitMediaCommonEncryptionDecryptClass* cencClass = WEBKIT_MEDIA_CENC_DECRYPT_CLASS(klass); > + GstElementClass* elementClass = GST_ELEMENT_CLASS(klass); > + GObjectClass* gobjectClass = G_OBJECT_CLASS(klass); Move the declarations to where they are fist used. > Source/WebCore/platform/graphics/gstreamer/WebKitClearKeyDecryptorGStreamer.cpp:133 > + WebKitMediaClearKeyDecryptPrivate* priv = WEBKIT_MEDIA_CK_DECRYPT_GET_PRIVATE(WEBKIT_MEDIA_CK_DECRYPT(self)); WebKitMediaClearKeyDecryptPrivate* priv = self->priv; > Source/WebCore/platform/graphics/gstreamer/WebKitClearKeyDecryptorGStreamer.cpp:141 > + priv->key.clear(); > + priv->key = adoptGRef(gst_buffer_copy(gst_value_get_buffer(value))); We don't need to clear the GRefPtr right before assigning a new value. > Source/WebCore/platform/graphics/gstreamer/WebKitClearKeyDecryptorGStreamer.cpp:147 > + WebKitMediaClearKeyDecryptPrivate* priv = WEBKIT_MEDIA_CK_DECRYPT_GET_PRIVATE(WEBKIT_MEDIA_CK_DECRYPT(self)); WebKitMediaClearKeyDecryptPrivate* priv = self->priv; > Source/WebCore/platform/graphics/gstreamer/WebKitClearKeyDecryptorGStreamer.cpp:153 > + return false; FALSE > Source/WebCore/platform/graphics/gstreamer/WebKitClearKeyDecryptorGStreamer.cpp:156 > + error = gcry_cipher_open(&(priv->handle), GCRY_CIPHER_AES128, GCRY_CIPHER_MODE_CTR, GCRY_CIPHER_SECURE); gcry_error_t error = gcry_cipher_open(&(priv->handle), GCRY_CIPHER_AES128, GCRY_CIPHER_MODE_CTR, GCRY_CIPHER_SECURE); > Source/WebCore/platform/graphics/gstreamer/WebKitClearKeyDecryptorGStreamer.cpp:159 > + return false; FALSE > Source/WebCore/platform/graphics/gstreamer/WebKitClearKeyDecryptorGStreamer.cpp:165 > + return false; FALSE > Source/WebCore/platform/graphics/gstreamer/WebKitClearKeyDecryptorGStreamer.cpp:173 > + return false; FALSE > Source/WebCore/platform/graphics/gstreamer/WebKitClearKeyDecryptorGStreamer.cpp:176 > + return true; TRUE > Source/WebCore/platform/graphics/gstreamer/WebKitClearKeyDecryptorGStreamer.cpp:181 > + WebKitMediaClearKeyDecryptPrivate* priv = WEBKIT_MEDIA_CK_DECRYPT_GET_PRIVATE(WEBKIT_MEDIA_CK_DECRYPT(self)); WebKitMediaClearKeyDecryptPrivate* priv = self->priv; > Source/WebCore/platform/graphics/gstreamer/WebKitClearKeyDecryptorGStreamer.cpp:188 > + GstMapInfo map, ivMap, subSamplesMap; > + unsigned position = 0; > + unsigned sampleIndex = 0; > + uint8_t ctr[CLEARKEY_SIZE]; > + GstByteReader* reader = nullptr; > + gboolean bufferMapped, subsamplesBufferMapped; > + gcry_error_t error; Move all these to where they are first used. > Source/WebCore/platform/graphics/gstreamer/WebKitClearKeyDecryptorGStreamer.cpp:192 > + return false; FALSE > Source/WebCore/platform/graphics/gstreamer/WebKitClearKeyDecryptorGStreamer.cpp:206 > + return false; FALSE > Source/WebCore/platform/graphics/gstreamer/WebKitClearKeyDecryptorGStreamer.cpp:212 > + return false; FALSE > Source/WebCore/platform/graphics/gstreamer/WebKitClearKeyDecryptorGStreamer.cpp:219 > + return false; FALSE > Source/WebCore/platform/graphics/gstreamer/WebKitClearKeyDecryptorGStreamer.cpp:227 > + guint16 nBytesClear = 0; > + guint32 nBytesEncrypted = 0; Nit: We normally use fooCount instead of nFoo > Source/WebCore/platform/graphics/gstreamer/WebKitClearKeyDecryptorGStreamer.cpp:231 > + if (!gst_byte_reader_get_uint16_be(reader, &nBytesClear) > + || !gst_byte_reader_get_uint32_be(reader, &nBytesEncrypted)) { This could be a single line > Source/WebCore/platform/graphics/gstreamer/WebKitClearKeyDecryptorGStreamer.cpp:233 > + gst_byte_reader_free(reader); We could add a GUniquePtr for this. > Source/WebCore/platform/graphics/gstreamer/WebKitClearKeyDecryptorGStreamer.cpp:236 > + return false; FALSE > Source/WebCore/platform/graphics/gstreamer/WebKitClearKeyDecryptorGStreamer.cpp:255 > + return false; FALSE > Source/WebCore/platform/graphics/gstreamer/WebKitClearKeyDecryptorGStreamer.cpp:264 > + return true; TRUE > Source/WebCore/platform/graphics/gstreamer/WebKitClearKeyDecryptorGStreamer.cpp:270 > + WebKitMediaClearKeyDecryptPrivate* priv = WEBKIT_MEDIA_CK_DECRYPT_GET_PRIVATE(WEBKIT_MEDIA_CK_DECRYPT(self)); > + gcry_cipher_close(priv->handle); gcry_cipher_close(self->priv->handle); > Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.cpp:26 > +#if ENABLE(ENCRYPTED_MEDIA) && USE(GSTREAMER) > +#include "WebKitCommonEncryptionDecryptorGStreamer.h" The header already has guards, so move it right after the config.h to follow the coding style. > Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.cpp:32 > +#define WEBKIT_MEDIA_CENC_DECRYPT_GET_PRIVATE(obj) (G_TYPE_INSTANCE_GET_PRIVATE((obj), WEBKIT_TYPE_MEDIA_CENC_DECRYPT, WebKitMediaCommonEncryptionDecryptPrivate)) Don't do this, use G_TYPE_INSTANCE_GET_PRIVATE directly in the init method. > Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.cpp:48 > +static GstStateChangeReturn webKitMediaCommonEncryptionDecryptorChangeState(GstElement*, GstStateChange transition); > +static void webKitMediaCommonEncryptionDecryptorFinalize(GObject*); > +static GstCaps* webkitMediaCommonEncryptionDecryptTransformCaps(GstBaseTransform*, GstPadDirection, GstCaps*, GstCaps*); > +static GstFlowReturn webkitMediaCommonEncryptionDecryptTransformInPlace(GstBaseTransform*, GstBuffer*); > +static gboolean webkitMediaCommonEncryptionDecryptSinkEventHandler(GstBaseTransform*, GstEvent*); > + > +static gboolean webKitMediaCommonEncryptionDecryptDefaultSetupCipher(WebKitMediaCommonEncryptionDecrypt*); > +static void webKitMediaCommonEncryptionDecryptDefaultReleaseCipher(WebKitMediaCommonEncryptionDecrypt*); Do we really need all this prototypes, or can we reorder the functions to avoid them? > Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.cpp:54 > +G_DEFINE_TYPE(WebKitMediaCommonEncryptionDecrypt, webkit_media_common_encryption_decrypt, GST_TYPE_BASE_TRANSFORM); Trailing ; is not needed. > Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.cpp:60 > + GstBaseTransformClass* baseTransformClass = GST_BASE_TRANSFORM_CLASS(klass); > + GstElementClass* elementClass = GST_ELEMENT_CLASS(klass); > + GObjectClass* gobjectClass = G_OBJECT_CLASS(klass); Move the declarations to where they are fist used. > Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.cpp:134 > + GstStructure* tmp = gst_structure_copy(in); GUniquePtr > Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.cpp:164 > + bool duplicate = false; I guess this means whether it's duplicated, not whether to duplicate it or not, no? I would use duplicated or isDuplicated in that case. > Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.cpp:168 > + GstStructure* s = gst_caps_get_structure(transformedCaps, index); Don't use "s", use a full world instead. > Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.cpp:170 > + duplicate = true; I guess we can break the loop here > Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.cpp:176 > + if (!duplicate) > + gst_caps_append_structure(transformedCaps, out); > + else > + gst_structure_free(out); We could uise GUniquePtr for out, and leak the value in the !duplicate case to transfer the ownership. > Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.cpp:183 > + intersection = gst_caps_intersect_full(transformedCaps, filter, GST_CAPS_INTERSECT_FIRST); GstCaps* intersection = gst_caps_intersect_full(transformedCaps, filter, GST_CAPS_INTERSECT_FIRST); > Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.cpp:202 > + WebKitMediaCommonEncryptionDecrypt* self = WEBKIT_MEDIA_CENC_DECRYPT(base); > + WebKitMediaCommonEncryptionDecryptClass* klass = WEBKIT_MEDIA_CENC_DECRYPT_GET_CLASS(self); > + WebKitMediaCommonEncryptionDecryptPrivate* priv = WEBKIT_MEDIA_CENC_DECRYPT_GET_PRIVATE(self); > + guint subSampleCount, ivSize; > + gboolean encrypted; > + const GValue* value; > + GstBuffer* ivBuffer = nullptr; > + GstBuffer* subSamplesBuffer = nullptr; > + GstProtectionMeta* protectionMeta; Move the declarations to where they are fist used. > Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.cpp:208 > + if (!priv->keyReceived) > + priv->condition.wait(priv->mutex); I guess this can't happen in the main thread? Add ASSERT(!isMainThread()) in that case to be sure. > Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.cpp:210 > + protectionMeta = reinterpret_cast<GstProtectionMeta*>(gst_buffer_get_protection_meta(buffer)); Why reinterpret cast? What does gst_buffer_get_protection_meta returns? a GstMeta*? I would make protectionMeta a GstMeta* instead and we don't need all the casts > Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.cpp:250 > + if (!klass->setupCipher(self)) { Use mediaEncClass or any other name that makes it clear what this class is > Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.cpp:298 > + result = TRUE; > + break; return TRUE > Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.cpp:305 > + if (self) { This is not correct way to check if self is still alive. You should either use a weak ref/ptr, or protect the object with GRefPtr, and capture that. > Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.cpp:307 > + WebKitMediaCommonEncryptionDecryptClass* klass = WEBKIT_MEDIA_CENC_DECRYPT_GET_CLASS(self); > + klass->requestDecryptionKey(self, initDataBuffer); I would add a generic interface for all these cases, instead of doing GET_CLASS + klass->foo() add methods that receive a WebKitMediaCommonEncryptionDecrypt and do the get_class + klass->foo > Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.cpp:310 > + if (self->priv->protectionEvent) { > + gst_event_unref(self->priv->protectionEvent); > + self->priv->protectionEvent = nullptr; Use GRefPtr for this. I don't understand why we need to keep the protectionEvent as a member of WebKitMediaCommonEncryptionDecrypt, we could use GrefPtr and capture it in the lambda. > Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.cpp:315 > + result = TRUE; > + break; return TRUE > Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.cpp:323 > + gst_event_unref(event); I think we could adopt the ref with GRefPtr at the beginning of the function, and it will be freed automatically in all cases. > Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.cpp:329 > + result = GST_BASE_TRANSFORM_CLASS(parent_class)->sink_event(trans, event); > + break; return GST_BASE_TRANSFORM_CLASS(parent_class)->sink_event(trans, event); > Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.cpp:332 > + return result; return FALSE > Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.cpp:339 > + GstStateChangeReturn ret = GST_STATE_CHANGE_SUCCESS; The init value is never used, I don't think we need this variable at all. > Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.cpp:344 > + priv->condition.notifyOne(); This could just be self->priv->condition.notifyOne(); since priv is only used one, we can avoid the local variable. > Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.cpp:350 > + ret = GST_ELEMENT_CLASS(parent_class)->change_state(element, transition); return GST_ELEMENT_CLASS(parent_class)->change_state(element, transition); > Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.cpp:352 > + // Add post-transition code here. If this is a fixme, add FIXME: otherwise remove the comment > Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.cpp:360 > + return true; TRUE > Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.cpp:366 > +static void webKitMediaCommonEncryptionDecryptDefaultReleaseCipher(WebKitMediaCommonEncryptionDecrypt*) > +{ > +} Can we just avoid the vfunc initalization, or does the caller expects this to be always non null?
(In reply to comment #7) > > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:896 > > + if (!equalIgnoringASCIICase(keySystem, "org.w3.clearkey")) > > + return MediaPlayer::KeySystemNotSupported; > > Instead of duplication this check here, we could call supportsKeySystem > instead, or move this check to a helper function and use it from both places. > I'm not sure it's worth. > > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:931 > > + if (parameters.keySystem.isNull() || parameters.keySystem.isEmpty()) > > parameters.keySystem.isEmpty() returns true if it's null > So you mean testing only isNull() is enough? > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.h:57 > > +void registerWebKitGStreamerElements(); > > + > > Why does this need to be here? Can't this be a static function? > It will need to be called from the future MediaPlayerPrivateGStreamerMSE. > > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.h:128 > > + virtual void dispatchDecryptionKey(GstBuffer*); > > Why is this virtual and public? > The MediaPlayerPrivateGStreamerMSE player will need to override it. But maybe it can be private though, I'll check. I'll process the remain comments later on :) Thanks!
(In reply to comment #8) > (In reply to comment #7) > > > > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:896 > > > + if (!equalIgnoringASCIICase(keySystem, "org.w3.clearkey")) > > > + return MediaPlayer::KeySystemNotSupported; > > > > Instead of duplication this check here, we could call supportsKeySystem > > instead, or move this check to a helper function and use it from both places. > > > > I'm not sure it's worth. If the condition changes for whatever reason you will have to change it everywhere. > > > > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:931 > > > + if (parameters.keySystem.isNull() || parameters.keySystem.isEmpty()) > > > > parameters.keySystem.isEmpty() returns true if it's null > > > > So you mean testing only isNull() is enough? No, isEmpty is enough. > > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.h:57 > > > +void registerWebKitGStreamerElements(); > > > + > > > > Why does this need to be here? Can't this be a static function? > > > > It will need to be called from the future MediaPlayerPrivateGStreamerMSE. Ah, ok, I think changing that in the future patch makes this easiert o understand. > > > > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.h:128 > > > + virtual void dispatchDecryptionKey(GstBuffer*); > > > > Why is this virtual and public? > > > > The MediaPlayerPrivateGStreamerMSE player will need to override it. But > maybe it can be private though, I'll check. Same here, I guess it could be protected. > I'll process the remain comments later on :) Thanks!
Comment on attachment 271332 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=271332&action=review > Source/WebCore/ChangeLog:24 > + disabled by default. I see the technical advantage to us in upstreaming this code, even if it will never be used by any distros and always disabled by default in OptionsGTK.cmake. But I kinda think it should be enabled by default in FeatureList.pm, for developers, even if it's not going to be used by any distros. Otherwise the build will be broken all the time, and we won't be able to run layout tests on the bot, and without running layout tests we won't notice when the functionality breaks. I understand we'll only be able to test ClearKey, but that seems fine to me, and much better than adding this with no tests. So, I am going to r- this, hoping not to set back your work, just until we're ready to enable it for development builds and can either add or unskip at least a couple of tests, and pending our transition to GStreamer 1.6 on the bots. (I would love to see that sooner rather than later, because our users have had GStreamer 1.6 for a long time now.) > Source/WebCore/PlatformGTK.cmake:830 > + ${LIBGCRYPT_LIBRARIES} -lgpg-error What is -lgpg-error? If it's not specified in a pkg-config module and is part of libgcrypt, it should probably be added to LIBGCRYPT_LIBRARIES by FindLibGcrypt.cmake. > Source/WebCore/PlatformGTK.cmake:837 > + Nit: no blank line here, please. > Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.h:8 > + * modify it under the terms of the GNU Library General Public Nit: grab a newer license header from some other file, should say "Lesser General Public License" > Source/cmake/OptionsGTK.cmake:380 > + Nit: No blank line here, please.
(In reply to comment #9) > (In reply to comment #8) > > (In reply to comment #7) > > > > > > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:896 > > > > + if (!equalIgnoringASCIICase(keySystem, "org.w3.clearkey")) > > > > + return MediaPlayer::KeySystemNotSupported; > > > > > > Instead of duplication this check here, we could call supportsKeySystem > > > instead, or move this check to a helper function and use it from both places. > > > > > > > I'm not sure it's worth. > > If the condition changes for whatever reason you will have to change it > everywhere. > In two places. I know we are lazy developers, but this is not worth the refactoring effort at this point, imho :)
(In reply to comment #10) > Comment on attachment 271332 [details] > patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=271332&action=review > > > Source/WebCore/ChangeLog:24 > > + disabled by default. > > I see the technical advantage to us in upstreaming this code, even if it > will never be used by any distros and always disabled by default in > OptionsGTK.cmake. But I kinda think it should be enabled by default in > FeatureList.pm, for developers, even if it's not going to be used by any > distros. Otherwise the build will be broken all the time, and we won't be > able to run layout tests on the bot, and without running layout tests we > won't notice when the functionality breaks. I understand we'll only be able > to test ClearKey, but that seems fine to me, and much better than adding > this with no tests. > > So, I am going to r- this, hoping not to set back your work, just until > we're ready to enable it for development builds and can either add or unskip > at least a couple of tests, and pending our transition to GStreamer 1.6 on > the bots. (I would love to see that sooner rather than later, because our > users have had GStreamer 1.6 for a long time now.) > Ok so this is the exact same situation as in WebRTC support. We disabled it temporarily in the bots and provided a custom JHBuild moduleset. Would that be OK with you? About the concern of this not being enabled and bitrotting, I think it's not a valid concern because this code is actively used in a downstream fork. So every build break that will happen will be noticed downstream and I'll be happy to fix in ToT whenever needed :)
Comment on attachment 271332 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=271332&action=review >> Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.cpp:210 >> + protectionMeta = reinterpret_cast<GstProtectionMeta*>(gst_buffer_get_protection_meta(buffer)); > > Why reinterpret cast? What does gst_buffer_get_protection_meta returns? a GstMeta*? I would make protectionMeta a GstMeta* instead and we don't need all the casts We need a GstProtectionMeta because we use its info GstStructure below, which the base GstMeta struct doesn't have.
(In reply to comment #11) > (In reply to comment #9) > > (In reply to comment #8) > > > (In reply to comment #7) > > > > > > > > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:896 > > > > > + if (!equalIgnoringASCIICase(keySystem, "org.w3.clearkey")) > > > > > + return MediaPlayer::KeySystemNotSupported; > > > > > > > > Instead of duplication this check here, we could call supportsKeySystem > > > > instead, or move this check to a helper function and use it from both places. > > > > > > > > > > I'm not sure it's worth. > > > > If the condition changes for whatever reason you will have to change it > > everywhere. > > > > In two places. I know we are lazy developers, but this is not worth the > refactoring effort at this point, imho :) You don't need any refactoring, just call the existing function that already checks this.
(In reply to comment #13) > Comment on attachment 271332 [details] > patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=271332&action=review > > >> Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.cpp:210 > >> + protectionMeta = reinterpret_cast<GstProtectionMeta*>(gst_buffer_get_protection_meta(buffer)); > > > > Why reinterpret cast? What does gst_buffer_get_protection_meta returns? a GstMeta*? I would make protectionMeta a GstMeta* instead and we don't need all the casts > > We need a GstProtectionMeta because we use its info GstStructure below, > which the base GstMeta struct doesn't have. I know, my point is that you can get the GstMeta and cast to GstProtectionMeta only once, instead of casting the result to GstProtectionMeta and then casting again to GstMeta everywhere.
(In reply to comment #14) > (In reply to comment #11) > > (In reply to comment #9) > > > (In reply to comment #8) > > > > (In reply to comment #7) > > > > > > > > > > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:896 > > > > > > + if (!equalIgnoringASCIICase(keySystem, "org.w3.clearkey")) > > > > > > + return MediaPlayer::KeySystemNotSupported; > > > > > > > > > > Instead of duplication this check here, we could call supportsKeySystem > > > > > instead, or move this check to a helper function and use it from both places. > > > > > > > > > > > > > I'm not sure it's worth. > > > > > > If the condition changes for whatever reason you will have to change it > > > everywhere. > > > > > > > In two places. I know we are lazy developers, but this is not worth the > > refactoring effort at this point, imho :) > > You don't need any refactoring, just call the existing function that already > checks this. So you suggest to call supportsKeySystem() right? but addKey() and generateKeyRequest() have no mimeType param
(In reply to comment #16) > (In reply to comment #14) > > (In reply to comment #11) > > > (In reply to comment #9) > > > > (In reply to comment #8) > > > > > (In reply to comment #7) > > > > > > > > > > > > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:896 > > > > > > > + if (!equalIgnoringASCIICase(keySystem, "org.w3.clearkey")) > > > > > > > + return MediaPlayer::KeySystemNotSupported; > > > > > > > > > > > > Instead of duplication this check here, we could call supportsKeySystem > > > > > > instead, or move this check to a helper function and use it from both places. > > > > > > > > > > > > > > > > I'm not sure it's worth. > > > > > > > > If the condition changes for whatever reason you will have to change it > > > > everywhere. > > > > > > > > > > In two places. I know we are lazy developers, but this is not worth the > > > refactoring effort at this point, imho :) > > > > You don't need any refactoring, just call the existing function that already > > checks this. > > So you suggest to call supportsKeySystem() right? but addKey() and > generateKeyRequest() have no mimeType param Which is unused, and IIRC you are already passing emptyString() in another caller.
OK then!
Comment on attachment 271332 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=271332&action=review >> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:108 >> + GRefPtr<GstElementFactory> clearKeyDecryptorFactory = gst_element_factory_find("webkitclearkey"); > > adoptGRef #0 0x00007fb7b781d253 in _g_log_abort (breakpoint=1) at /home/phil/WebKit/WebKitBuild/DependenciesGTK/Source/glib-2.46.0/glib/gmessages.c:324 #1 g_logv (log_domain=0x7fb7b7b41170 "GLib-GObject", log_level=G_LOG_LEVEL_CRITICAL, format=<optimized out>, args=args@entry=0x7fff59e44618) at /home/phil/WebKit/WebKitBuild/DependenciesGTK/Source/glib-2.46.0/glib/gmessages.c:1081 #2 0x00007fb7b781d3b2 in g_log (log_domain=log_domain@entry=0x7fb7b7b41170 "GLib-GObject", log_level=log_level@entry=G_LOG_LEVEL_CRITICAL, format=format@entry=0x7fb7b788b786 "%s: assertion '%s' failed") at /home/phil/WebKit/WebKitBuild/DependenciesGTK/Source/glib-2.46.0/glib/gmessages.c:1119 #3 0x00007fb7b781d3d9 in g_return_if_fail_warning (log_domain=log_domain@entry=0x7fb7b7b41170 "GLib-GObject", pretty_function=pretty_function@entry=0x7fb7b7b44af0 <__func__.13475> "g_object_is_floating", expression=expression@entry=0x7fb7b7b4375a "G_IS_OBJECT (object)") at /home/phil/WebKit/WebKitBuild/DependenciesGTK/Source/glib-2.46.0/glib/gmessages.c:1128 #4 0x00007fb7b7b1ed6c in g_object_is_floating (_object=0x0) at /home/phil/WebKit/WebKitBuild/DependenciesGTK/Source/glib-2.46.0/gobject/gobject.c:2815 #5 0x00007fb7c48d6739 in WTF::adoptGRef<_GstElementFactory> (ptr=0x0) at ../../Source/WebCore/platform/graphics/gstreamer/GRefPtrGStreamer.cpp:167 #6 0x00007fb7c48ecf74 in WebCore::registerWebKitGStreamerElements () at ../../Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:108
(In reply to comment #19) > Comment on attachment 271332 [details] > patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=271332&action=review > > >> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:108 > >> + GRefPtr<GstElementFactory> clearKeyDecryptorFactory = gst_element_factory_find("webkitclearkey"); > > > > adoptGRef > > #0 0x00007fb7b781d253 in _g_log_abort (breakpoint=1) at > /home/phil/WebKit/WebKitBuild/DependenciesGTK/Source/glib-2.46.0/glib/ > gmessages.c:324 > #1 g_logv (log_domain=0x7fb7b7b41170 "GLib-GObject", > log_level=G_LOG_LEVEL_CRITICAL, format=<optimized out>, > args=args@entry=0x7fff59e44618) > at > /home/phil/WebKit/WebKitBuild/DependenciesGTK/Source/glib-2.46.0/glib/ > gmessages.c:1081 > #2 0x00007fb7b781d3b2 in g_log (log_domain=log_domain@entry=0x7fb7b7b41170 > "GLib-GObject", log_level=log_level@entry=G_LOG_LEVEL_CRITICAL, > format=format@entry=0x7fb7b788b786 "%s: assertion '%s' failed") > at > /home/phil/WebKit/WebKitBuild/DependenciesGTK/Source/glib-2.46.0/glib/ > gmessages.c:1119 > #3 0x00007fb7b781d3d9 in g_return_if_fail_warning > (log_domain=log_domain@entry=0x7fb7b7b41170 "GLib-GObject", > pretty_function=pretty_function@entry=0x7fb7b7b44af0 <__func__.13475> > "g_object_is_floating", > expression=expression@entry=0x7fb7b7b4375a "G_IS_OBJECT (object)") at > /home/phil/WebKit/WebKitBuild/DependenciesGTK/Source/glib-2.46.0/glib/ > gmessages.c:1128 > #4 0x00007fb7b7b1ed6c in g_object_is_floating (_object=0x0) at > /home/phil/WebKit/WebKitBuild/DependenciesGTK/Source/glib-2.46.0/gobject/ > gobject.c:2815 > #5 0x00007fb7c48d6739 in WTF::adoptGRef<_GstElementFactory> (ptr=0x0) at > ../../Source/WebCore/platform/graphics/gstreamer/GRefPtrGStreamer.cpp:167 > #6 0x00007fb7c48ecf74 in WebCore::registerWebKitGStreamerElements () at > ../../Source/WebCore/platform/graphics/gstreamer/ > MediaPlayerPrivateGStreamerBase.cpp:108 This is a bug of GRefPtr<GstElementFactory> adoptGRef(GstElementFactory* ptr), the assert there is wrong, it should be: ASSERT(!ptr || !g_object_is_floating(G_OBJECT(ptr)));
Comment on attachment 271332 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=271332&action=review >> Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.cpp:176 >> + gst_structure_free(out); > > We could uise GUniquePtr for out, and leak the value in the !duplicate case to transfer the ownership. I've been trying that but without much success. Just to be clear, by leaking the value out mean using GUniquePtr's reset()?
(In reply to comment #21) > Comment on attachment 271332 [details] > patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=271332&action=review > > >> Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.cpp:176 > >> + gst_structure_free(out); > > > > We could uise GUniquePtr for out, and leak the value in the !duplicate case to transfer the ownership. > > I've been trying that but without much success. Just to be clear, by leaking > the value out mean using GUniquePtr's reset()? No, I mean using release()
(In reply to comment #12) > Ok so this is the exact same situation as in WebRTC support. We disabled it > temporarily in the bots and provided a custom JHBuild moduleset. Would that > be OK with you? I'm not sure I understand the decision behind WebRTC either, what was the point in disabling it, just to avoid the upgrade to GStreamer 1.6? I would prefer to get rid of the extra jhbuild moduleset and re-enable WebRTC for developers again. Can't we do that for EME as well? I think the primary difference there is that you guys have concrete plans to reenable WebRTC within a relatively short timespan. Also, WebRTC is a very important feature for the WebKit project, whereas EME is only useful to ease maintenance of downstream forks. And WebRTC support has existed upstream for a long time already. We should apply strict scrutiny when adding new features that would not be enabled by default; in this case, I think it makes strategic sense to have this, but I don't think it makes sense for the bots to not build or test it. (That's reasonable, no? :) > About the concern of this not being enabled and bitrotting, I think it's not > a valid concern because this code is actively used in a downstream fork. So > every build break that will happen will be noticed downstream and I'll be > happy to fix in ToT whenever needed :) I think if you have it enabled by default (for developers) downstream, you can do it upstream too, right? (In reply to comment #21) > I've been trying that but without much success. Just to be clear, by leaking > the value out mean using GUniquePtr's reset()? Reset does an unref. It's basically the same as g_set_object (ref new value, set pointer to new value, unref old value). With no arguments, that's the same as g_clear_object (set pointer to null, unref old value). It's identical to std::unique_ptr, actually; GUniquePtr is just a std::unique_ptr that frees with g_free instead of delete.
(In reply to comment #23) > (In reply to comment #12) > > Ok so this is the exact same situation as in WebRTC support. We disabled it > > temporarily in the bots and provided a custom JHBuild moduleset. Would that > > be OK with you? > > I'm not sure I understand the decision behind WebRTC either, what was the > point in disabling it, just to avoid the upgrade to GStreamer 1.6? I would > prefer to get rid of the extra jhbuild moduleset and re-enable WebRTC for > developers again. This isn't possible yet because OpenWebRTC depends on GStreamer 1.6 at least. And we can't bump the gst version in JHBuild yet either. Hence the decision to disable WebRTC in builds *for* *now* :) > Can't we do that for EME as well? Nope. > > I think the primary difference there is that you guys have concrete plans to > reenable WebRTC within a relatively short timespan. Also, WebRTC is a very > important feature for the WebKit project, whereas EME is only useful to ease > maintenance of downstream forks. Not only that. It's also needed to show that WebKitGTK itself can support EME. > And WebRTC support has existed upstream for > a long time already. Both WebRTC and EME specs have roughly emerged around 2012, IIRC. > We should apply strict scrutiny when adding new > features that would not be enabled by default; in this case, I think it > makes strategic sense to have this, but I don't think it makes sense for the > bots to not build or test it. (That's reasonable, no? :) > Ok, I also would like to have this tested on the bots at some point but as mentioned already, it's not possible yet.
(In reply to comment #24) > This isn't possible yet because OpenWebRTC depends on GStreamer 1.6 at > least. And we can't bump the gst version in JHBuild yet either. Hence the > decision to disable WebRTC in builds *for* *now* :) I really don't understand, why is it not possible to bump the GStreamer version in JHBuild? Most of our users have GStreamer 1.6, if it causes bugs then we should push forward and update test expectations accordingly.
Just for the record, I cross reference this bug with https://bugzilla.gnome.org/show_bug.cgi?id=761700#c5.
Comment on attachment 271332 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=271332&action=review >> Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.cpp:305 >> + if (self) { > > This is not correct way to check if self is still alive. You should either use a weak ref/ptr, or protect the object with GRefPtr, and capture that. I tried to use WeakPtr but the captured value stores nothing in the lambda. >> Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.cpp:310 >> + self->priv->protectionEvent = nullptr; > > Use GRefPtr for this. I don't understand why we need to keep the protectionEvent as a member of WebKitMediaCommonEncryptionDecrypt, we could use GrefPtr and capture it in the lambda. We only need to keep it referenced until the initData is no longer needed. If a GRefPtr captured in the lambda can do that, then yeah, let's use the fancy stuff :)
Comment on attachment 271332 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=271332&action=review >>> Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.cpp:305 >>> + if (self) { >> >> This is not correct way to check if self is still alive. You should either use a weak ref/ptr, or protect the object with GRefPtr, and capture that. > > I tried to use WeakPtr but the captured value stores nothing in the lambda. The thing is that self is not going to be nullptr here if the object is destroyed before the lambda is called, so the check is wrong. You could use weak pointer or GRefPtr depending on what you want. If you want to ensure the object is nos destroyed until the lambda is called, then use GRefPtr, but if you want to ignore the lambda if the object is destroyed, you need a weakptr and check its value in the lambda to return early. Since this is a GObject maybe it's easier to use GObject weak references instead of WeakPtr >>> Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.cpp:310 >>> + self->priv->protectionEvent = nullptr; >> >> Use GRefPtr for this. I don't understand why we need to keep the protectionEvent as a member of WebKitMediaCommonEncryptionDecrypt, we could use GrefPtr and capture it in the lambda. > > We only need to keep it referenced until the initData is no longer needed. If a GRefPtr captured in the lambda can do that, then yeah, let's use the fancy stuff :) You are indeed releasing the protection event inside the lambda.
Created attachment 272313 [details] patch
Attachment 272313 [details] did not pass style-queue: ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitClearKeyDecryptorGStreamer.h:40: webkit_media_clear_key_decrypt_get_type is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.h:27: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.h:44: webkit_media_common_encryption_decrypt_get_type is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.h:56: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.h:57: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.h:58: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.h:59: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.h:60: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitClearKeyDecryptorGStreamer.cpp:23: You should not add a blank line before implementation file's own header. [build/include_order] [4] ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitClearKeyDecryptorGStreamer.cpp:211: webkit_media_clear_key_decrypt_class_init is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitClearKeyDecryptorGStreamer.cpp:240: webkit_media_clear_key_decrypt_init is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.cpp:24: You should not add a blank line before implementation file's own header. [build/include_order] [4] ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.cpp:336: webkit_media_common_encryption_decrypt_class_init is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.cpp:359: webkit_media_common_encryption_decrypt_init is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Total errors found: 14 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 272316 [details] patch
Attachment 272316 [details] did not pass style-queue: ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitClearKeyDecryptorGStreamer.h:40: webkit_media_clear_key_decrypt_get_type is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.h:27: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.h:44: webkit_media_common_encryption_decrypt_get_type is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.h:56: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.h:57: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.h:58: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.h:59: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.h:60: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/cmake/FindLibGcrypt.cmake:44: One space between command "if" and its parentheses, should be "if (" [whitespace/parentheses] [5] ERROR: Source/cmake/FindLibGcrypt.cmake:54: One space between command "endif" and its parentheses, should be "endif (" [whitespace/parentheses] [5] ERROR: Source/cmake/FindLibGcrypt.cmake:55: One space between command "endif" and its parentheses, should be "endif (" [whitespace/parentheses] [5] ERROR: Source/cmake/FindLibGcrypt.cmake:66: One space between command "if" and its parentheses, should be "if (" [whitespace/parentheses] [5] ERROR: Source/cmake/FindLibGcrypt.cmake:69: One space between command "endif" and its parentheses, should be "endif (" [whitespace/parentheses] [5] ERROR: Source/cmake/FindLibGcrypt.cmake:78: One space between command "if" and its parentheses, should be "if (" [whitespace/parentheses] [5] ERROR: Source/cmake/FindLibGcrypt.cmake:81: One space between command "endif" and its parentheses, should be "endif (" [whitespace/parentheses] [5] ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitClearKeyDecryptorGStreamer.cpp:210: webkit_media_clear_key_decrypt_class_init is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitClearKeyDecryptorGStreamer.cpp:239: webkit_media_clear_key_decrypt_init is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.cpp:335: webkit_media_common_encryption_decrypt_class_init is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.cpp:358: webkit_media_common_encryption_decrypt_init is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Total errors found: 19 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 272316 [details] 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.
Comment on attachment 272316 [details] 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?
Comment on attachment 272316 [details] 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. Indeed, I saw the public at the first line and not the private here, I'm sorry. >>> 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? Yes, because here there's two -> making it more difficult to read, but I'm fine either way. >>> 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. What's ClearKey then? >>> 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. Can't there be more implementations? Why are we using two classes then? This is not about what the known implementation does, but what the base class wants to enforce. Think of it as whether this is a pure virtual method or not. >>> 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? Right, I didn't notice that, then yes, ASSERT_NOT_REACHED()
Created attachment 272321 [details] patch
Attachment 272321 [details] did not pass style-queue: ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitClearKeyDecryptorGStreamer.h:40: webkit_clear_key_decryptor_get_type is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.h:27: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.h:44: webkit_common_encryption_decryptor_get_type is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.h:56: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.h:57: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.h:58: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.h:59: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.h:60: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/cmake/FindLibGcrypt.cmake:44: One space between command "if" and its parentheses, should be "if (" [whitespace/parentheses] [5] ERROR: Source/cmake/FindLibGcrypt.cmake:54: One space between command "endif" and its parentheses, should be "endif (" [whitespace/parentheses] [5] ERROR: Source/cmake/FindLibGcrypt.cmake:55: One space between command "endif" and its parentheses, should be "endif (" [whitespace/parentheses] [5] ERROR: Source/cmake/FindLibGcrypt.cmake:66: One space between command "if" and its parentheses, should be "if (" [whitespace/parentheses] [5] ERROR: Source/cmake/FindLibGcrypt.cmake:69: One space between command "endif" and its parentheses, should be "endif (" [whitespace/parentheses] [5] ERROR: Source/cmake/FindLibGcrypt.cmake:78: One space between command "if" and its parentheses, should be "if (" [whitespace/parentheses] [5] ERROR: Source/cmake/FindLibGcrypt.cmake:81: One space between command "endif" and its parentheses, should be "endif (" [whitespace/parentheses] [5] ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitClearKeyDecryptorGStreamer.cpp:207: webkit_clear_key_decryptor_class_init is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitClearKeyDecryptorGStreamer.cpp:236: webkit_clear_key_decryptor_init is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/cmake/FindLibGpgError.cmake:46: One space between command "if" and its parentheses, should be "if (" [whitespace/parentheses] [5] ERROR: Source/cmake/FindLibGpgError.cmake:56: One space between command "endif" and its parentheses, should be "endif (" [whitespace/parentheses] [5] ERROR: Source/cmake/FindLibGpgError.cmake:57: One space between command "endif" and its parentheses, should be "endif (" [whitespace/parentheses] [5] ERROR: Source/cmake/FindLibGpgError.cmake:68: One space between command "if" and its parentheses, should be "if (" [whitespace/parentheses] [5] ERROR: Source/cmake/FindLibGpgError.cmake:71: One space between command "endif" and its parentheses, should be "endif (" [whitespace/parentheses] [5] ERROR: Source/cmake/FindLibGpgError.cmake:80: One space between command "if" and its parentheses, should be "if (" [whitespace/parentheses] [5] ERROR: Source/cmake/FindLibGpgError.cmake:83: One space between command "endif" and its parentheses, should be "endif (" [whitespace/parentheses] [5] ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.cpp:327: webkit_common_encryption_decryptor_class_init is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitCommonEncryptionDecryptorGStreamer.cpp:350: webkit_common_encryption_decryptor_init is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Total errors found: 26 in 14 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 272316 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=272316&action=review >>>> 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. > > What's ClearKey then? It's described in the spec :) https://www.w3.org/TR/encrypted-media/ section 9.1 >>>> 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. > > Can't there be more implementations? Why are we using two classes then? This is not about what the known implementation does, but what the base class wants to enforce. Think of it as whether this is a pure virtual method or not. Yes there can be multiple implementations. At least the PlayReady key system can also rely on common-encryption.
For sure some of the media/encrypted-media tests should be unskipped along with this patch. I'll have a look :)
Comment on attachment 272321 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=272321&action=review > Source/cmake/OptionsGTK.cmake:136 > +WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_ENCRYPTED_MEDIA PUBLIC ON) Please default it to OFF here since it's useless out-of-the-box. But add it to Tools/Scripts/webkitperl/FeatureList.pm and enable it there, that way it will be enabled on the bots and for developers using build-webkit.
Comment on attachment 272321 [details] 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?
Also update the TestExpectations file for the tests passing now.
Comment on attachment 272321 [details] 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@gmail.com> Fix enconding > Source/cmake/FindLibGpgError.cmake:19 > +# Copyright 2014 Nicolás Alvarez <nicolas.alvarez@gmail.com> Ditto.
Might we make the style bot happy too?
Btw, this patch is pretty big. Slicing it would be better, IMHO
Phil did agree to add layout tests, that's why he did the GStreamer 1.6 upgrade on the bots. ;)
V1 is dead.