Bug 181855 – [EME][GStreamer] Move the decryptor from AppendPipeline to PlaybackPipeline.

• Component: WPE WebKit
• Status: RESOLVED FIXED

[Yacine Bandou]

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.

[Yacine Bandou]

Created attachment 332158 [details]
Patch

[Yacine Bandou]

(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.

[Yacine Bandou]

Created attachment 333486 [details]
Patch

[Yacine Bandou]

Created attachment 333487 [details]
Patch

[Yacine Bandou]

Created attachment 333490 [details]
Patch

[Xabier Rodríguez Calvar]

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?

[Yacine Bandou]

(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.

[Yacine Bandou]

Created attachment 338892 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 338892 [details]
Patch

Clearing flags on attachment: 338892

Committed r231089: <https://trac.webkit.org/changeset/231089>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Xabier Rodríguez Calvar]

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.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 185071

[Yacine Bandou]

Created attachment 339486 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Xabier Rodríguez Calvar]

(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?

[Xabier Rodríguez Calvar]

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.

[Yacine Bandou]

(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

[Yacine Bandou]

Created attachment 339721 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[Xabier Rodríguez Calvar]

Comment on attachment 339721 [details]
Patch

This looks good, just ensure that the changelog reflects the changes it should.

[Xabier Rodríguez Calvar]

Comment on attachment 339721 [details]
Patch

Clearing flags on attachment: 339721

Committed r231633: <https://trac.webkit.org/changeset/231633>

[Xabier Rodríguez Calvar]

All reviewed patches have been landed.  Closing bug.