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".
Created attachment 341199 [details] Patch
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!
Comment on attachment 341199 [details] Patch Michael is very right.
Created attachment 341264 [details] [GStreamer] Add qtdemux patches to fix use of NULL pointers
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 ?
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.
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
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.
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.
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.
(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.
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).
> (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.
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>
All reviewed patches have been landed. Closing bug.