Bug 185948 – [GStreamer] Handle changes in the "drm-preferred-decryption-system-id" NEED_CONTEXT message.

• Component: WPE WebKit
• Status: RESOLVED FIXED

[Yacine Bandou]

After updating to GStreamer  1.14 the function "gst_protection_filter_systems_by_available_decryptors" returns NULL, when there is no decryptor plugin that supports any of the systemid, instead of an empty array in Gstreamer-1.12.

Thus, the "gst_qtdemux_request_protection_context" function of qtdemux in the "gst-plugins-good-0002-qtdemux-add-context-for-a-preferred-protection" patch doesn't wait for the function "gst_protection_filter_systems_by_available_decryptors" to return a NULL, and then, it crashes when send the "drm-preferred-decryption-system-id" GstMessage with a NULL value in the field "stream-encryption-systems".

[Yacine Bandou]

Created attachment 341199 [details]
Patch

[Michael Catanzaro]

Comment on attachment 341199 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=341199&action=review

> Tools/gstreamer/patches/gst-plugins-good-0002-qtdemux-add-context-for-a-preferred-protection.patch:195
> ++  if (!filtered_sys_ids)
> ++    filtered_sys_ids = g_new0 (char *, 1);

Is there an upstream bug report for this? The patch that you are modifying has already gone upstream, and will be dropped when we upgrade to 1.14.1, so your change will be lost then.

Look at https://cgit.freedesktop.org/gstreamer/gst-plugins-good/log/, there's hardly been a better time to upstream some qtdemux changes!

[Xabier Rodríguez Calvar]

Comment on attachment 341199 [details]
Patch

Michael is very right.

[Thibault Saunier]

Created attachment 341264 [details]
[GStreamer] Add qtdemux patches to fix use of NULL pointers

[Yacine Bandou]

Comment on attachment 341264 [details]
[GStreamer] Add qtdemux patches to fix use of NULL pointers

View in context: https://bugs.webkit.org/attachment.cgi?id=341264&action=review

> Tools/gstreamer/patches/gst-plugins-good-0001-qtdemux-Do-not-run-the-preferred-decryptor-context-q.patch:27
> ++

You should know that in ClearKey WPT tests the used MP4 media content contains only the PlayReady and Widevine systemids in their PSSH BOX.
If we return with an error when no available decryptor support any of systemids, so all the clearKey tests of WPT fail. 
Did you test this patch with encrypted-media WPT tests ?

[Thibault Saunier]

Created attachment 341271 [details]
Added a new patch to GStreamer (that I will merge once this one is accepted) that moves back to previous behaviour

and clarifies the name of the fields in the REQUEST_CONTEXT message (this hasn't been released yet so it is the
right time to change the API).

Now that new patch still uses a NULL pointer to represent avalaible system IDs, so handle that in WebKit itself.

[Xabier Rodríguez Calvar]

Comment on attachment 341271 [details]
Added a new patch to GStreamer (that I will merge once this one is accepted) that moves back to previous behaviour

View in context: https://bugs.webkit.org/attachment.cgi?id=341271&action=review

> Source/WebCore/ChangeLog:3
> +        [GStreamer] Handle changes in the "drm-preferred-decryption-system-id" NEED_CONTEXT message.

This line should reflect the bug title. If you want to keep this informative text I'd suggest to move it down to the long description section.

> Source/WebCore/ChangeLog:12
> +        Some tests already exist and where failling (crashing) without that patch.

I would list the tests here, something like:
Tests: imported/w3c/web-platform-tests/encrypted-media/clearkey-mp4-*

Side comment: "where failling"  -> "were failing"

> Tools/ChangeLog:3
> +        [GStreamer] Handle changes in the "drm-preferred-decryption-system-id" NEED_CONTEXT message.

Ditto.

> Tools/ChangeLog:10
> +        properly taken into account in the qtdemux patch that uses it and got upstreamed. Those 3 new patches fixes that and changes the

... Those 3 new patches fix that and change ...

> Tools/gstreamer/jhbuild.modules:73
> +      <patch file="gst-plugins-good-0002-qtdemux-Do-not-unref-a-NULL-stream_tags.patch" strip="1" /> <!-- Merged as 43a540b1cd9f162d3dae5d50e36703dfaf558a3e -->

I don't understand why this patch is needed here. Besides, it looks like we are missing Tools/gstreamer/patches/gst-plugins-good-0003-qtdemux-Clarify-field-name-about-stream-encryption-s.patch

[Yacine Bandou]

IMHO, we don't need to use "gst_protection_filter_systems_by_available_decryptors" when we dispatch the GstContext, we simply send all the systemIDs present in the media to the application and let it select any systemID, the application know if this system is supported or not.

[Thibault Saunier]

Created attachment 341282 [details]
Patch.

> Comment on attachment 341271 [details]
> Added a new patch to GStreamer (that I will merge once this one is accepted)
> that moves back to previous behaviour
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=341271&action=review
> 
> > Source/WebCore/ChangeLog:3
> > +        [GStreamer] Handle changes in the "drm-preferred-decryption-system-id" NEED_CONTEXT message.
> 
> This line should reflect the bug title. If you want to keep this informative
> text I'd suggest to move it down to the long description section.

Updated bug title.

> > Source/WebCore/ChangeLog:12
> > +        Some tests already exist and where failling (crashing) without that patch.
> 
> I would list the tests here, something like:
> Tests: imported/w3c/web-platform-tests/encrypted-media/clearkey-mp4-*

Done.
 
> Side comment: "where failling"  -> "were failing"
> 
> > Tools/ChangeLog:3
> > +        [GStreamer] Handle changes in the "drm-preferred-decryption-system-id" NEED_CONTEXT message.
> 
> Ditto.

Done.

> > Tools/ChangeLog:10
> > +        properly taken into account in the qtdemux patch that uses it and got upstreamed. Those 3 new patches fixes that and changes the
> 
> ... Those 3 new patches fix that and change ...
> 
> > Tools/gstreamer/jhbuild.modules:73
> > +      <patch file="gst-plugins-good-0002-qtdemux-Do-not-unref-a-NULL-stream_tags.patch" strip="1" /> <!-- Merged as 43a540b1cd9f162d3dae5d50e36703dfaf558a3e -->
> 
> I don't understand why this patch is needed here. Besides, it looks like we
> are missing
> Tools/gstreamer/patches/gst-plugins-good-0003-qtdemux-Clarify-field-name-
> about-stream-encryption-s.patch

I added the missing one (forgot to `git add` this file, sorry), The second one is needed so that the patches are the same as upstream (this patch has been merged already).
Basically I think that we should try to have exactly the same patches as what is merged upstream to avoid this kind of issues when rebasing ;)

(In reply to Yacine Bandou from comment #8)
> IMHO, we don't need to use
> "gst_protection_filter_systems_by_available_decryptors" when we dispatch the
> GstContext, we simply send all the systemIDs present in the media to the
> application and let it select any systemID, the application know if this
> system is supported or not.

The thing is that maybe in WebKit we know but it might not be the case in other apps, I think it makes the API sensibly simpler to use.

[Xabier Rodríguez Calvar]

Comment on attachment 341282 [details]
Patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=341282&action=review

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:319
> +    if (streamEncryptionAllowedSystems) {
> +        for (i = 0; streamEncryptionAllowedSystems[i]; ++i)
> +            streamEncryptionAllowedSystemsVector.append(streamEncryptionAllowedSystems[i]);
> +    }

We don't need the brackets here, I think.

> Tools/gstreamer/jhbuild.modules:74
> +      <patch file="gst-plugins-good-0003-qtdemux-Clarify-field-name-about-stream-encryption-s.patch" strip="1" />

You can add a comment <!-- Merged in master, scheduled for 1.16.0 --> because we don't have the commit id yet but we will and we want to ensure that it is clear where it is now and will be release. Actually, the information about the patches scheduled for 1.16 should be in the others too.

[Carlos Garcia Campos]

(In reply to Xabier Rodríguez Calvar from comment #10)
> Comment on attachment 341282 [details]
> Patch.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=341282&action=review
> 
> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:319
> > +    if (streamEncryptionAllowedSystems) {
> > +        for (i = 0; streamEncryptionAllowedSystems[i]; ++i)
> > +            streamEncryptionAllowedSystemsVector.append(streamEncryptionAllowedSystems[i]);
> > +    }
> 
> We don't need the brackets here, I think.

https://webkit.org/code-style-guidelines/#braces-one-line

> > Tools/gstreamer/jhbuild.modules:74
> > +      <patch file="gst-plugins-good-0003-qtdemux-Clarify-field-name-about-stream-encryption-s.patch" strip="1" />
> 
> You can add a comment <!-- Merged in master, scheduled for 1.16.0 -->
> because we don't have the commit id yet but we will and we want to ensure
> that it is clear where it is now and will be release. Actually, the
> information about the patches scheduled for 1.16 should be in the others too.

[Thibault Saunier]

Created attachment 341290 [details]
Update patches to add a comment about our intention of merging the GStreamer patch

(not that I particularly agree it should be neede at all).

[Yacine Bandou]

> (In reply to Yacine Bandou from comment #8)
> > IMHO, we don't need to use
> > "gst_protection_filter_systems_by_available_decryptors" when we dispatch the
> > GstContext, we simply send all the systemIDs present in the media to the
> > application and let it select any systemID, the application know if this
> > system is supported or not.
> 
> The thing is that maybe in WebKit we know but it might not be the case in
> other apps, I think it makes the API sensibly simpler to use.

This API simply used in order to set the "protection-system" field in padsrc caps of qtdemux.

There are 3 cases : 
1.The application knows the supported systems. 
2.The application doesn't know.
3.The application doesn't respond to the GstContext.

For 1 and 3 there is no problem to dispatch all systemIds, no change. 
For 2 the risk that we can set caps with a default systemID which isn't supported, but in this case the pipeline send an error that no plugin supports these caps and I don't see the problem with this behavior.

[WebKit Commit Bot]

Comment on attachment 341290 [details]
Update patches to add a comment about our intention of merging the GStreamer patch

Clearing flags on attachment: 341290

Committed r232240: <https://trac.webkit.org/changeset/232240>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.