Summary: | [GStreamer] Implement MediaPlayerPrivateGStreamerMSE::attempToDecryptWithInstance() | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Zan Dobersek <zan> | ||||||||||
Component: | Media | Assignee: | Zan Dobersek <zan> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | buildbot, calvaris, cturner, webkit-bug-importer | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | WebKit Nightly Build | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
Zan Dobersek
2017-09-06 02:49:16 PDT
Created attachment 319997 [details]
Patch
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.
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? 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. Created attachment 319998 [details]
Patch
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.
Created attachment 320000 [details]
Patch
Now uses a functor to group the common code for GStreamer value list appends.
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. Comment on attachment 320000 [details]
Patch
Please, have a look at the comments before landing.
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. Created attachment 320015 [details]
Patch for landing
Comment on attachment 320015 [details] Patch for landing Clearing flags on attachment: 320015 Committed r221718: <http://trac.webkit.org/changeset/221718> All reviewed patches have been landed. Closing bug. |