Summary: | [EME][GStreamer] Move the decryptor from AppendPipeline to PlaybackPipeline. | ||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Yacine Bandou <bandou.yacine> | ||||||||||||||||||||||
Component: | WPE WebKit | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||
Severity: | Normal | CC: | bugs-noreply, calvaris, commit-queue, ews-watchlist, olivier.blin, zan | ||||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||
Bug Depends on: | 181858, 181990, 185071 | ||||||||||||||||||||||||
Bug Blocks: | 185277 | ||||||||||||||||||||||||
Attachments: |
|
Description
Yacine Bandou
2018-01-19 08:20:55 PST
Created attachment 332158 [details]
Patch
(In reply to Yacine Bandou from comment #1) > Created attachment 332158 [details] > Patch It doesn't apply to trunck because it depends on the Patch 181990. Created attachment 333486 [details]
Patch
Created attachment 333487 [details]
Patch
Created attachment 333490 [details]
Patch
Comment on attachment 333490 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=333490&action=review > Source/WebCore/platform/graphics/gstreamer/mse/PlaybackPipeline.cpp:186 > + GST_DEBUG_OBJECT(webKitMediaSrc, "It's an encrypted content, the parser plugins will be auto plugged by the decodebin"); Please, just use GST_DEBUG because otherwise you're saying that the source of that message is that object and it is not true. Besides, this message is unclear to me. Do you mean the parser or the decryptor? Plugins plural? My guess is that you just mean decryptor elements. > Source/WebCore/platform/graphics/gstreamer/mse/PlaybackPipeline.cpp:348 > + for (Stream* stream : priv->streams) { > + if (stream->appsrc) > + appsrcs.append(GST_APP_SRC(stream->appsrc)); > + } You don't need the { } here. > Source/WebCore/platform/graphics/gstreamer/mse/PlaybackPipeline.cpp:354 > + WebKitMediaSrcPrivate* priv = m_webKitMediaSrc->priv; > + Vector<GstAppSrc*> appsrcs; > + > + GST_OBJECT_LOCK(m_webKitMediaSrc.get()); > + for (Stream* stream : priv->streams) { > + if (stream->appsrc) > + appsrcs.append(GST_APP_SRC(stream->appsrc)); > + } > + GST_OBJECT_UNLOCK(m_webKitMediaSrc.get()); > + > + for (GstAppSrc* appsrc : appsrcs) { > + GST_TRACE("dispatching key to playback pipeline %p", this); > + gst_element_send_event((GstElement*)appsrc, gst_event_new_custom(GST_EVENT_CUSTOM_DOWNSTREAM_OOB, gst_structure_copy(structure.get()))); > + } What's the problem of just sending the event to the pipeline itself? (In reply to Xabier Rodríguez Calvar from comment #6) > Comment on attachment 333490 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=333490&action=review > > > Source/WebCore/platform/graphics/gstreamer/mse/PlaybackPipeline.cpp:186 > > + GST_DEBUG_OBJECT(webKitMediaSrc, "It's an encrypted content, the parser plugins will be auto plugged by the decodebin"); > > Please, just use GST_DEBUG because otherwise you're saying that the source > of that message is that object and it is not true. Besides, this message is > unclear to me. Do you mean the parser or the decryptor? Plugins plural? My > guess is that you just mean decryptor elements. > I agree, I'll replace it by: + GST_DEBUG("It's encrypted content, parsers are not needed before decrypting the content"); When it is encrypted content, we don't need to add manually the parsers like h264parse or h265parse in the playback pipeline. > > > Source/WebCore/platform/graphics/gstreamer/mse/PlaybackPipeline.cpp:354 > > + WebKitMediaSrcPrivate* priv = m_webKitMediaSrc->priv; > > + Vector<GstAppSrc*> appsrcs; > > + > > + GST_OBJECT_LOCK(m_webKitMediaSrc.get()); > > + for (Stream* stream : priv->streams) { > > + if (stream->appsrc) > > + appsrcs.append(GST_APP_SRC(stream->appsrc)); > > + } > > + GST_OBJECT_UNLOCK(m_webKitMediaSrc.get()); > > + > > + for (GstAppSrc* appsrc : appsrcs) { > > + GST_TRACE("dispatching key to playback pipeline %p", this); > > + gst_element_send_event((GstElement*)appsrc, gst_event_new_custom(GST_EVENT_CUSTOM_DOWNSTREAM_OOB, gst_structure_copy(structure.get()))); > > + } > > What's the problem of just sending the event to the pipeline itself? I was inspired by the function "PlaybackPipeline::markEndOfStream" But I agree, we could just send the event to the pipeline itself. I'll remove the function dispatchDecryptionStructure. Created attachment 338892 [details]
Patch
Comment on attachment 338892 [details] Patch Clearing flags on attachment: 338892 Committed r231089: <https://trac.webkit.org/changeset/231089> All reviewed patches have been landed. Closing bug. Comment on attachment 338892 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=338892&action=review > Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:958 > + gst_element_send_event(m_playbackPipeline->pipeline(), gst_event_new_custom(GST_EVENT_CUSTOM_DOWNSTREAM_OOB, gst_structure_copy(structure.get()))); You don't need to copy the structure since we are given a rvalue reference. Just .release() instead of .get() and avoid the gst_structure_copy. Re-opened since this is blocked by bug 185071 Created attachment 339486 [details]
Patch
Comment on attachment 339486 [details] Patch Attachment 339486 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/7563578 New failing tests: imported/w3c/web-platform-tests/html/syntax/parsing/html5lib_tests21.html Created attachment 339540 [details]
Archive of layout-test-results from ews115 for mac-sierra
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 339486 [details] Patch Attachment 339486 [details] did not pass win-ews (win): Output: http://webkit-queues.webkit.org/results/7563712 New failing tests: http/tests/security/canvas-remote-read-remote-video-blocked-no-crossorigin.html Created attachment 339542 [details]
Archive of layout-test-results from ews200 for win-future
The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews200 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
(In reply to Build Bot from comment #16) > Comment on attachment 339486 [details] > Patch > > Attachment 339486 [details] did not pass win-ews (win): > Output: http://webkit-queues.webkit.org/results/7563712 > > New failing tests: > http/tests/security/canvas-remote-read-remote-video-blocked-no-crossorigin. > html Yacine, can you check if this is related to your patch? Comment on attachment 339486 [details]
Patch
It looks ok, but my only concern is whether tests are going to break again because protection events are not forwarded to the PlaybackPipeline. I wouldn't like to roll out this patch again because of that.
(In reply to Xabier Rodríguez Calvar from comment #18) > (In reply to Build Bot from comment #16) > > Comment on attachment 339486 [details] > > Patch > > > > Attachment 339486 [details] did not pass win-ews (win): > > Output: http://webkit-queues.webkit.org/results/7563712 > > > > New failing tests: > > http/tests/security/canvas-remote-read-remote-video-blocked-no-crossorigin. > > html > > Yacine, can you check if this is related to your patch? No, I don't think. All my patch is in GStreamer platform. I re-submit the patch in order to launch again the EWS tests Created attachment 339721 [details]
Patch
Comment on attachment 339721 [details] Patch Attachment 339721 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/7595712 New failing tests: webrtc/addICECandidate-closed.html Created attachment 339727 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.4
Comment on attachment 339721 [details]
Patch
This looks good, just ensure that the changelog reflects the changes it should.
Comment on attachment 339721 [details] Patch Clearing flags on attachment: 339721 Committed r231633: <https://trac.webkit.org/changeset/231633> All reviewed patches have been landed. Closing bug. |