[webkit-reviews] review denied: [Bug 206730] [EME][GStreamer] Introduce CDMProxy : [Attachment 388665] Patch

[bugzilla-daemon at webkit.org]

Xabier Rodríguez Calvar <calvaris at igalia.com> has denied Charlie Turner
<cturner at igalia.com>'s request for review:
Bug 206730: [EME][GStreamer] Introduce CDMProxy
https://bugs.webkit.org/show_bug.cgi?id=206730

Attachment 388665: Patch

https://bugs.webkit.org/attachment.cgi?id=388665&action=review




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

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

Since there are some missing files in the patch (and I have to call it a day
and a working week), I'll continue on Tuesday (taking Monday off). Anyway, some
comments:

> Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.cpp:506
> +    keyIDVec.reserveInitialCapacity(in.keyIDNumBytes);

Since you're appending be passing an array with size and you aren't appending
anything else, I'd say this is not needed since the first thing the append is
going to do is reserving the size that you specify.

> Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.cpp:523
> +    // uin8_t* bufferToDecrypt, size_t bufferNumBytes);

?

> Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.cpp:696
> +	       keysChanged = keysChanged || m_keyStore.add(decodedKey);

Short-circuit evaluation is biting you here. I think you should either reverse
both arguments or accumulate some other way.

> Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.h:137
> +	   const uint8_t* keyID;

I think it is better to capitalize as keyId, I see Id instead of ID more used
outside our code. Do you mind checking it patch-wide, please?

> Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.h:138
> +	   size_t keyIDNumBytes;

keyIdSize?

> Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.h:141
> +	   size_t ivNumBytes;

ivSize?

> Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.h:144
> +	   size_t encryptedBufferNumBytes;

encryptedBufferSize?

> Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.h:147
> +	   const uint8_t* subSamplesBuffer;

subsamplesBuffer

> Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.h:148
> +	   size_t subSamplesBufferNumBytes;

subsamplesBufferSize? Mind that the word is subsample, so we should capitalize
it like that IMHO.

> Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.h:149
> +	   size_t numSubSamples;

numberOfSubsamples?

> Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.h:164
> +    // resuse some Crypto APIs in WebCore?

reuse

> Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.h:168
> +    bool CENCSetCounterVector(const CENCDecryptContext&);
> +    bool CENCSetDecryptionKey(const CENCDecryptContext&);
> +    bool CENCDecryptFullSample(CENCDecryptContext&);
> +    bool CENCDecryptSubSampled(CENCDecryptContext&);

I would call these methods cenc*

> Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.h:176
> +

I don't think we need this extra line

>
Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1797
> +	       m_handledProtectionEvents.add(GST_EVENT_SEQNUM(event.get()));

/me dreams of getting rid of this at some point.

>
Source/WebCore/platform/graphics/gstreamer/eme/WebKitClearKeyDecryptorGStreamer
.cpp:117
> +    WebKitMediaClearKeyDecryptPrivate* priv =
WEBKIT_MEDIA_CK_DECRYPT_GET_PRIVATE(WEBKIT_MEDIA_CK_DECRYPT(self));

Mein Gott, we really need to have a look at the style of this file at some
point...

>
Source/WebCore/platform/graphics/gstreamer/eme/WebKitClearKeyDecryptorGStreamer
.cpp:142
> +    RefPtr<GstMappedBuffer> mappedSubSamplesBuffer;

This should be moved to just above the if

>
Source/WebCore/platform/graphics/gstreamer/eme/WebKitClearKeyDecryptorGStreamer
.cpp:143
> +    CDMProxyClearKey::CENCDecryptContext ctx;

context

>
Source/WebCore/platform/graphics/gstreamer/eme/WebKitCommonEncryptionDecryptorG
Streamer.cpp:202
> +    auto removeProtectionMetaOnReturn = makeScopeExit([buffer,
protectionMeta] {
> +	   gst_buffer_remove_meta(buffer,
reinterpret_cast<GstMeta*>(protectionMeta));
> +    });

Beautiful!

>
Source/WebCore/platform/graphics/gstreamer/eme/WebKitCommonEncryptionDecryptorG
Streamer.cpp:277
> +    GRefPtr<GstContext> context =
adoptGRef(gst_element_get_context(GST_ELEMENT(self), "drm-cdm-instance"));

We might want to rename this context name to drm-cdm-proxy

>
Source/WebCore/platform/graphics/gstreamer/eme/WebKitCommonEncryptionDecryptorG
Streamer.cpp:286
> +	   const GValue* value =
gst_structure_get_value(gst_context_get_structure(context.get()),
"cdm-instance");

cdm-proxy

>
Source/WebCore/platform/graphics/gstreamer/eme/WebKitCommonEncryptionDecryptorG
Streamer.cpp:290
> +		   GST_DEBUG_OBJECT(self, "received a new CDM proxy instance
%p, refcount %u", proxy, proxy->refCount());

Do we really need the refcount?

>
Source/WebCore/platform/graphics/gstreamer/eme/WebKitCommonEncryptionDecryptorG
Streamer.cpp:342
>	   // We should be handling protection events in this class in
>	   // addition to out-of-band data. In regular playback, after a
>	   // preferred system ID context is set, any future protection
> -	   // events will not be handled by the demuxer, so the must be
> +	   // events will not be handled by the demuxer, so they must be
>	   // handled in here.

I think this descryption does not belong here cause protection events are not
OOB, are they? I think this should be maybe before the switch.

Another concern of mine is that we are reacting to all OOB events, I guess we
should at least filter by structure name, right?

Which event are we reacting to here?

>
Source/WebCore/platform/graphics/gstreamer/eme/WebKitCommonEncryptionDecryptorG
Streamer.cpp:-350
> -    if (gst_structure_has_name(gst_query_get_structure(query),
"any-decryptor-waiting-for-key")) {

Good!

>
Source/WebCore/platform/graphics/gstreamer/eme/WebKitCommonEncryptionDecryptorG
Streamer.cpp:386
> +	   priv->cdmProxy = value ?
reinterpret_cast<CDMProxy*>(g_value_get_pointer(value)) : nullptr;

Wouldn't it be nice to create a boxed type of a RefPtr<CDMProxy>, something
like:

GType WEBKIT_G_TYPE_REF_PTR_CDM_PROXY = g_boxed_type_register_static
("RefPtrCDMProxy", (GBoxedCopyFunc) WTF::refIfNotNull<CDMProxy>,
(GBoxedFreeFunc) WTF::derefIfNotNull<CDMProxy>);

Then we could use WEBKIT_G_TYPE_REF_PTR_CDM_PROXY (or maybe something more
adecuate to WebKit style) when we declare the content of the GstStructure and
this way we would be able to maintain references inside the structures and not
just loose pointers. I guess we might have as well something like:

template<typename T>
webkitGValueSet(GValue* value, const RefPtr<T>&);
webkitGValueTake(GValue* value, RefPtr<T>&&); // We could even forget this and
overload the Set method for both const & and &&.
RefPtr<T> webkitGValueGet(GValue* value); // In this case I don't think we need
to mimic the get/dup GValue API as we're dealing in smart pointers.


I might be missing anything but I think it can work nicely.

If you think it is too much for now or you are not in the mood of implementing
this so cool stuff, we can give it a try later.