Bug 191157 – [EME, GStreamer] Ensure key id buffers are present and simplify lifetime management of ClearKey class

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Charlie Turner]

[EME, GStreamer] Ensure key id buffers are present and simplify lifetime management of ClearKey class

[Charlie Turner]

Created attachment 353617 [details]
Patch

[Xabier Rodríguez Calvar]

Comment on attachment 353617 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        [EME, GStreamer] Ensure key id buffers are present and simplify lifetime management of ClearKey class

We usually write it like [EME][GStreamer]

> Source/WebCore/ChangeLog:17
> +        bindings

Period at the end.

> Source/WebCore/platform/graphics/gstreamer/eme/WebKitClearKeyDecryptorGStreamer.cpp:103
> +        ASSERT(!error);
> +        GST_ERROR_OBJECT(self, "Failed to create AES 128 CTR cipher handle: %s", gpg_strerror(error));

Please, swap these lines, we would like to see the GST_ERROR in the logs before we ASSERT and die if in debug.

> Source/WebCore/platform/graphics/gstreamer/eme/WebKitClearKeyDecryptorGStreamer.cpp:153
> +static gboolean webKitMediaClearKeyDecryptorFindAndSetKey(WebKitMediaClearKeyDecryptPrivate* priv, WebCore::GstMappedBuffer& keyIDBuffer)

I wonder if we should make this a  const GstMapperBuffer &. I guess it would imply making the data accessor const as well.

And gosh, we messed up names here... These classes need a deep clean up for style and naming.

> Source/WebCore/platform/graphics/gstreamer/eme/WebKitClearKeyDecryptorGStreamer.cpp:157
> +        if (!gst_buffer_memcmp(key.keyID.get(), 0, keyIDBuffer.data(), keyIDBuffer.size())) {

You know what?  It would be nice to define a operator== for this on the mapped buffer, one could be taking a GstBuffer* and a second one taking another mapped buffer, even the second could be implemented on terms of the first. Of course, you wouldn't need the second now, so we could leave it out of the question for now.

> Source/WebCore/platform/graphics/gstreamer/eme/WebKitClearKeyDecryptorGStreamer.cpp:174
> +    ASSERT(mappedKeyValueBufferr.size() == CLEARKEY_SIZE);

oops! I would advise either making a debug build or writing RELEASE_ASSERTs until just before submitting the patch.

> Source/WebCore/platform/graphics/gstreamer/eme/WebKitCommonEncryptionDecryptorGStreamer.cpp:273
>      GstBuffer* keyIDBuffer = nullptr;
> -    if (value)
> -        keyIDBuffer = gst_value_get_buffer(value);
> -
> -    WebKitMediaCommonEncryptionDecryptClass* klass = WEBKIT_MEDIA_CENC_DECRYPT_GET_CLASS(self);
> -    if (!klass->setupCipher(self, keyIDBuffer)) {
> -        GST_ERROR_OBJECT(self, "Failed to configure cipher");
> +    if (!value) {
> +        GST_ERROR_OBJECT(self, "Failed to get key id for buffer");
>          gst_buffer_remove_meta(buffer, reinterpret_cast<GstMeta*>(protectionMeta));
>          return GST_FLOW_NOT_SUPPORTED;
>      }
> +    keyIDBuffer = gst_value_get_buffer(value);

if (!value)  {
...
}
GstBuffer* keyIDBuffer = gst_value_get_buffer(value);

> Source/WebCore/platform/graphics/gstreamer/eme/WebKitCommonEncryptionDecryptorGStreamer.cpp:275
> +    WebKitMediaCommonEncryptionDecryptClass* klass = WEBKIT_MEDIA_CENC_DECRYPT_GET_CLASS(self);

This can be moved below, right?

> Source/WebCore/platform/graphics/gstreamer/eme/WebKitCommonEncryptionDecryptorGStreamer.h:57
> +    gboolean (*decrypt)(WebKitMediaCommonEncryptionDecrypt*, GstBuffer* ivBuffer, GstBuffer* keyIDBuffer, GstBuffer* buffer, unsigned subSamplesCount, GstBuffer* subSamplesBuffer);

Just a comment for the future here. These are methods that live only in the WebKit world, not GStreamer's. I think we should rework the style, of course not now and nowadays it is well as it is.

[Charlie Turner]

Created attachment 353959 [details]
Patch

[Charlie Turner]

(In reply to Xabier Rodríguez Calvar from comment #2)
> Comment on attachment 353617 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=353617&action=review
> > Source/WebCore/platform/graphics/gstreamer/eme/WebKitClearKeyDecryptorGStreamer.cpp:153
> > +static gboolean webKitMediaClearKeyDecryptorFindAndSetKey(WebKitMediaClearKeyDecryptPrivate* priv, WebCore::GstMappedBuffer& keyIDBuffer)
> 
> I wonder if we should make this a  const GstMapperBuffer &. I guess it would
> imply making the data accessor const as well.
> 
> And gosh, we messed up names here... These classes need a deep clean up for
> style and naming.

Did you mean webKitMediaClearKeyDecryptor<ACTUAL_METHOD_NAME_HERE> convention in this file, or the GstMappedBuffer class? The former can be removed when a single decryptor is introduced.

> 
> > Source/WebCore/platform/graphics/gstreamer/eme/WebKitClearKeyDecryptorGStreamer.cpp:157
> > +        if (!gst_buffer_memcmp(key.keyID.get(), 0, keyIDBuffer.data(), keyIDBuffer.size())) {
> 
> You know what?  It would be nice to define a operator== for this on the
> mapped buffer, one could be taking a GstBuffer* and a second one taking
> another mapped buffer, even the second could be implemented on terms of the
> first. Of course, you wouldn't need the second now, so we could leave it out
> of the question for now.

It would be, I added one in this patch which triggered me to set r? again.

> > Source/WebCore/platform/graphics/gstreamer/eme/WebKitCommonEncryptionDecryptorGStreamer.h:57
> > +    gboolean (*decrypt)(WebKitMediaCommonEncryptionDecrypt*, GstBuffer* ivBuffer, GstBuffer* keyIDBuffer, GstBuffer* buffer, unsigned subSamplesCount, GstBuffer* subSamplesBuffer);
> 
> Just a comment for the future here. These are methods that live only in the
> WebKit world, not GStreamer's. I think we should rework the style, of course
> not now and nowadays it is well as it is.

I would have made them bool rather than gboolean for example, but just following coding convention. This file will be removed soon.

[Xabier Rodríguez Calvar]

(In reply to Charlie Turner from comment #4)
> > > Source/WebCore/platform/graphics/gstreamer/eme/WebKitClearKeyDecryptorGStreamer.cpp:153
> > > +static gboolean webKitMediaClearKeyDecryptorFindAndSetKey(WebKitMediaClearKeyDecryptPrivate* priv, WebCore::GstMappedBuffer& keyIDBuffer)
> > 
> > I wonder if we should make this a  const GstMapperBuffer &. I guess it would
> > imply making the data accessor const as well.
> > 
> > And gosh, we messed up names here... These classes need a deep clean up for
> > style and naming.
> 
> Did you mean webKitMediaClearKeyDecryptor<ACTUAL_METHOD_NAME_HERE>
> convention in this file, or the GstMappedBuffer class? The former can be
> removed when a single decryptor is introduced.

Yes, I mean the former. But I see you didn't make the keyIDBuffer const. Why?

> 
> > > Source/WebCore/platform/graphics/gstreamer/eme/WebKitCommonEncryptionDecryptorGStreamer.h:57
> > > +    gboolean (*decrypt)(WebKitMediaCommonEncryptionDecrypt*, GstBuffer* ivBuffer, GstBuffer* keyIDBuffer, GstBuffer* buffer, unsigned subSamplesCount, GstBuffer* subSamplesBuffer);
> > 
> > Just a comment for the future here. These are methods that live only in the
> > WebKit world, not GStreamer's. I think we should rework the style, of course
> > not now and nowadays it is well as it is.
> 
> I would have made them bool rather than gboolean for example, but just
> following coding convention. This file will be removed soon.

Yes, I mean that and some other "irregularities".

[Charlie Turner]

Created attachment 354075 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 354075 [details]
Patch

Clearing flags on attachment: 354075

Committed r237921: <https://trac.webkit.org/changeset/237921>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.