The goal of this move is to handle the limitation of SVP (Secure Video Path) memory size. When the decryptor is in the AppendPipeline and we use SVP, we buffer in MediaSource queue the decrypted GstBuffers that are in SVP memory (or which use SVP memory). This behavior cause an out-of-memory error, because we are limited in SVP memory size. By moving the decryptor in PlaybackPipeline, we avoid to buffer the decrypted GstBuffers wich use the SVP memory and we buffer the encrypted GstBuffers that are in system memory. This new architecture also allows to start the buffering before obtaining the DRM license and it makes easier to manage dynamic change of the license or Key. SVP: Secure Video Path also named trusted or protected video path, it is a memory which is protected by a hardware access control engine, it is not accessible to other unauthorised software or hardware components.
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>