Bug 191157 - [EME, GStreamer] Ensure key id buffers are present and simplify lifetime management of ClearKey class
Summary: [EME, GStreamer] Ensure key id buffers are present and simplify lifetime mana...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Charlie Turner
URL:
Keywords:
Depends on:
Blocks: 191316
  Show dependency treegraph
 
Reported: 2018-11-01 10:38 PDT by Charlie Turner
Modified: 2018-11-07 03:50 PST (History)
3 users (show)

See Also:


Attachments
Patch (14.92 KB, patch)
2018-11-01 10:44 PDT, Charlie Turner
no flags Details | Formatted Diff | Diff
Patch (16.76 KB, patch)
2018-11-06 03:19 PST, Charlie Turner
no flags Details | Formatted Diff | Diff
Patch (16.77 KB, patch)
2018-11-07 02:00 PST, Charlie Turner
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Charlie Turner 2018-11-01 10:38:02 PDT
[EME, GStreamer] Ensure key id buffers are present and simplify lifetime management of ClearKey class
Comment 1 Charlie Turner 2018-11-01 10:44:21 PDT
Created attachment 353617 [details]
Patch
Comment 2 Xabier Rodríguez Calvar 2018-11-02 07:56:18 PDT
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.
Comment 3 Charlie Turner 2018-11-06 03:19:34 PST
Created attachment 353959 [details]
Patch
Comment 4 Charlie Turner 2018-11-06 03:23:30 PST
(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.
Comment 5 Xabier Rodríguez Calvar 2018-11-06 22:13:35 PST
(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".
Comment 6 Charlie Turner 2018-11-07 02:00:29 PST
Created attachment 354075 [details]
Patch
Comment 7 WebKit Commit Bot 2018-11-07 03:50:38 PST
Comment on attachment 354075 [details]
Patch

Clearing flags on attachment: 354075

Committed r237921: <https://trac.webkit.org/changeset/237921>
Comment 8 WebKit Commit Bot 2018-11-07 03:50:39 PST
All reviewed patches have been landed.  Closing bug.