Bug 174779 - [EME][GStreamer] Multi-key support in the GStreamer ClearKey decryptor
Summary: [EME][GStreamer] Multi-key support in the GStreamer ClearKey decryptor
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Zan Dobersek
URL:
Keywords:
Depends on:
Blocks: 174858
  Show dependency treegraph
 
Reported: 2017-07-24 02:23 PDT by Zan Dobersek
Modified: 2017-07-26 07:33 PDT (History)
1 user (show)

See Also:


Attachments
WIP (9.76 KB, patch)
2017-07-24 02:27 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff
Patch (10.78 KB, patch)
2017-07-26 03:35 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff
Patch (10.74 KB, patch)
2017-07-26 07:02 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Zan Dobersek 2017-07-24 02:23:00 PDT
[EME][GStreamer] Multi-key support in the GStreamer ClearKey decryptor
Comment 1 Zan Dobersek 2017-07-24 02:27:07 PDT
Created attachment 316277 [details]
WIP
Comment 2 Zan Dobersek 2017-07-26 03:35:49 PDT
Created attachment 316441 [details]
Patch
Comment 3 Xabier Rodríguez Calvar 2017-07-26 06:37:33 PDT
Comment on attachment 316441 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/eme/WebKitClearKeyDecryptorGStreamer.cpp:47
> +static gboolean webKitMediaClearKeyDecryptorSetupCipher(WebKitMediaCommonEncryptionDecrypt*, GstBuffer* kidBuffer);

Remove parameter name.

> Source/WebCore/platform/graphics/gstreamer/eme/WebKitClearKeyDecryptorGStreamer.cpp:152
> +        GRefPtr<GstBuffer> keyIDBuffer(adoptGRef(gst_buffer_copy(static_cast<GstBuffer*>(g_value_get_boxed(gst_value_list_get_value(keyIDsList, i))))));
> +        GRefPtr<GstBuffer> keyValueBuffer(adoptGRef(gst_buffer_copy(static_cast<GstBuffer*>(g_value_get_boxed(gst_value_list_get_value(keyValuesList, i))))));

I think, and it's funny that I said this, that it is better to use gst_value_get_buffer here to avoid the cast. I also think that you could just use GRefPtr constructor, avoid the copy and let the GRefPtr constructor ref the buffer for you instead of forcing the copy. Why would you want to copy it?

Just saying, though this code works and does not leak.

> Source/WebCore/platform/graphics/gstreamer/eme/WebKitClearKeyDecryptorGStreamer.cpp:159
> +static gboolean webKitMediaClearKeyDecryptorSetupCipher(WebKitMediaCommonEncryptionDecrypt* self, GstBuffer* kidBuffer)

I'd name it keyIDBuffer of keyIdBuffer.

> Source/WebCore/platform/graphics/gstreamer/eme/WebKitClearKeyDecryptorGStreamer.cpp:167
> +        if (!gst_buffer_map(kidBuffer, &kidBufferMap, GST_MAP_READ)) {

You need to unmap after use or we are leaking crap.

> Source/WebCore/platform/graphics/gstreamer/eme/WebKitCommonEncryptionDecryptorGStreamer.cpp:263
> +    GstBuffer* kidBuffer = gst_value_get_buffer(value);

rename variable

> Source/WebCore/platform/graphics/gstreamer/eme/WebKitCommonEncryptionDecryptorGStreamer.h:57
> +    gboolean (*setupCipher)(WebKitMediaCommonEncryptionDecrypt*, GstBuffer* kidBuffer);

Remove parameter name.
Comment 4 Zan Dobersek 2017-07-26 07:02:38 PDT
Created attachment 316446 [details]
Patch
Comment 5 Zan Dobersek 2017-07-26 07:33:13 PDT
Comment on attachment 316446 [details]
Patch

Clearing flags on attachment: 316446

Committed r219946: <http://trac.webkit.org/changeset/219946>
Comment 6 Zan Dobersek 2017-07-26 07:33:17 PDT
All reviewed patches have been landed.  Closing bug.