Bug 176445 – [GStreamer] Implement MediaPlayerPrivateGStreamerMSE::attempToDecryptWithInstance()

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Zan Dobersek]

[GStreamer] Implement MediaPlayerPrivateGStreamerMSE::attempToDecryptWithInstance()

[Zan Dobersek]

Created attachment 319997 [details]
Patch

[Build Bot]

Attachment 319997 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:952:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Charlie Turner]

Comment on attachment 319997 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:944
> +        if (keys.isEmpty())

Same check as above?

> Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:952
> +            {

Why are the extra scopes used in the loop, is there some problem with have G variables as autos outside the loop?

[Zan Dobersek]

Comment on attachment 319997 [details]
Patch

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

>> Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:952
>> +            {
> 
> Why are the extra scopes used in the loop, is there some problem with have G variables as autos outside the loop?

It's used for clarity, and to not accidentally access resources across the two GValue constructions.

[Zan Dobersek]

Created attachment 319998 [details]
Patch

[Build Bot]

Attachment 319998 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:948:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Zan Dobersek]

Created attachment 320000 [details]
Patch

Now uses a functor to group the common code for GStreamer value list appends.

[Xabier Rodríguez Calvar]

Comment on attachment 320000 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:950
> +                GRefPtr<GstBuffer> gstBuffer(gst_buffer_new_wrapped(

You can use a single line

> Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:954
> +                g_value_set_boxed(bufferValue, gstBuffer.get());

Use gst_value_take_buffer or adopt the reference and use set.

> Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:968
> +        GUniquePtr<GstStructure> structure(gst_structure_new_empty("drm-cipher-clearkey"));
> +        gst_structure_set_value(structure.get(), "key-ids", &keyIDList);
> +        gst_structure_set_value(structure.get(), "key-values", &keyValueList);
> +
> +        for (auto it : m_appendPipelinesMap)
> +            it.value->dispatchDecryptionStructure(GUniquePtr<GstStructure>(gst_structure_copy(structure.get())));

I have comments about this code:
* Shouldn't we WTFMove the GUniquePtr as it the method takes a && ?
* Considering that the copy is not shallow and can mean some memory copies, I think it would be interesting to unroll the loop once and copy the the structure in all cases but one, where we'd move.

> Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.h:89
> +    void attemptToDecryptWithInstance(const CDMInstance&) override;

You can go with final here.

[Xabier Rodríguez Calvar]

Comment on attachment 320000 [details]
Patch

Please, have a look at the comments before landing.

[Zan Dobersek]

Comment on attachment 320000 [details]
Patch

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

>> Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:954
>> +                g_value_set_boxed(bufferValue, gstBuffer.get());
> 
> Use gst_value_take_buffer or adopt the reference and use set.

I'll just inline the gst_buffer_new_wrapped() in the gst_value_take_buffer then.

>> Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:968
>> +            it.value->dispatchDecryptionStructure(GUniquePtr<GstStructure>(gst_structure_copy(structure.get())));
> 
> I have comments about this code:
> * Shouldn't we WTFMove the GUniquePtr as it the method takes a && ?
> * Considering that the copy is not shallow and can mean some memory copies, I think it would be interesting to unroll the loop once and copy the the structure in all cases but one, where we'd move.

* No WTFMove is necessary, GUniquePtr<> as constructed here is an rvalue that binds fine to the GUniquePtr<>&& type used by the callee.
* This isn't worth the complexity. The copied data isn't large in size, and this altogether is not a frequent operation.

[Zan Dobersek]

Created attachment 320015 [details]
Patch for landing

[Zan Dobersek]

Comment on attachment 320015 [details]
Patch for landing

Clearing flags on attachment: 320015

Committed r221718: <http://trac.webkit.org/changeset/221718>

[Zan Dobersek]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/34693553>