Bug 181855 - [EME][GStreamer] Move the decryptor from AppendPipeline to PlaybackPipeline.
Summary: [EME][GStreamer] Move the decryptor from AppendPipeline to PlaybackPipeline.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WPE WebKit (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 181858 181990 185071
Blocks: 185277
  Show dependency treegraph
 
Reported: 2018-01-19 08:20 PST by Yacine Bandou
Modified: 2018-05-10 00:11 PDT (History)
6 users (show)

See Also:


Attachments
Patch (20.60 KB, patch)
2018-01-24 07:20 PST, Yacine Bandou
no flags Details | Formatted Diff | Diff
Patch (20.25 KB, patch)
2018-02-09 07:31 PST, Yacine Bandou
no flags Details | Formatted Diff | Diff
Patch (21.29 KB, patch)
2018-02-09 08:23 PST, Yacine Bandou
no flags Details | Formatted Diff | Diff
Patch (21.29 KB, patch)
2018-02-09 08:31 PST, Yacine Bandou
no flags Details | Formatted Diff | Diff
Patch (19.32 KB, patch)
2018-04-26 10:53 PDT, Yacine Bandou
no flags Details | Formatted Diff | Diff
Patch (19.34 KB, patch)
2018-05-03 16:57 PDT, Yacine Bandou
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews115 for mac-sierra (3.28 MB, application/zip)
2018-05-04 07:00 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews200 for win-future (12.91 MB, application/zip)
2018-05-04 07:04 PDT, Build Bot
no flags Details
Patch (19.34 KB, patch)
2018-05-07 08:16 PDT, Yacine Bandou
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews124 for ios-simulator-wk2 (2.32 MB, application/zip)
2018-05-07 10:04 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Yacine Bandou 2018-01-19 08:20:55 PST
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.
Comment 1 Yacine Bandou 2018-01-24 07:20:28 PST
Created attachment 332158 [details]
Patch
Comment 2 Yacine Bandou 2018-01-24 08:03:47 PST
(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.
Comment 3 Yacine Bandou 2018-02-09 07:31:31 PST
Created attachment 333486 [details]
Patch
Comment 4 Yacine Bandou 2018-02-09 08:23:59 PST
Created attachment 333487 [details]
Patch
Comment 5 Yacine Bandou 2018-02-09 08:31:57 PST
Created attachment 333490 [details]
Patch
Comment 6 Xabier Rodríguez Calvar 2018-04-26 04:57:13 PDT
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?
Comment 7 Yacine Bandou 2018-04-26 10:52:37 PDT
(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.
Comment 8 Yacine Bandou 2018-04-26 10:53:54 PDT
Created attachment 338892 [details]
Patch
Comment 9 WebKit Commit Bot 2018-04-27 00:33:58 PDT
Comment on attachment 338892 [details]
Patch

Clearing flags on attachment: 338892

Committed r231089: <https://trac.webkit.org/changeset/231089>
Comment 10 WebKit Commit Bot 2018-04-27 00:34:00 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 Xabier Rodríguez Calvar 2018-04-27 02:59:29 PDT
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.
Comment 12 WebKit Commit Bot 2018-04-27 03:05:10 PDT
Re-opened since this is blocked by bug 185071
Comment 13 Yacine Bandou 2018-05-03 16:57:08 PDT
Created attachment 339486 [details]
Patch
Comment 14 Build Bot 2018-05-04 07:00:05 PDT
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
Comment 15 Build Bot 2018-05-04 07:00:07 PDT
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 16 Build Bot 2018-05-04 07:04:28 PDT
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
Comment 17 Build Bot 2018-05-04 07:04:39 PDT
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
Comment 18 Xabier Rodríguez Calvar 2018-05-07 00:22:24 PDT
(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 19 Xabier Rodríguez Calvar 2018-05-07 00:29:02 PDT
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.
Comment 20 Yacine Bandou 2018-05-07 08:12:31 PDT
(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
Comment 21 Yacine Bandou 2018-05-07 08:16:54 PDT
Created attachment 339721 [details]
Patch
Comment 22 Build Bot 2018-05-07 10:03:58 PDT
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
Comment 23 Build Bot 2018-05-07 10:04:00 PDT
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 24 Xabier Rodríguez Calvar 2018-05-08 04:54:46 PDT
Comment on attachment 339721 [details]
Patch

This looks good, just ensure that the changelog reflects the changes it should.
Comment 25 Xabier Rodríguez Calvar 2018-05-10 00:11:25 PDT
Comment on attachment 339721 [details]
Patch

Clearing flags on attachment: 339721

Committed r231633: <https://trac.webkit.org/changeset/231633>
Comment 26 Xabier Rodríguez Calvar 2018-05-10 00:11:30 PDT
All reviewed patches have been landed.  Closing bug.