Attachment 341271 Details for Bug 185948 – Added a new patch to GStreamer (that I will merge once this one is accepted) that moves back to previous behaviour

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

GStreamer-Add-qtdemux-patches-to-fix-use-of-NULL-p.patch (text/plain), 13.22 KB, created by Thibault Saunier on 2018-05-25 05:06:05 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Thibault Saunier

Created: 2018-05-25 05:06:05 PDT

Size: 13.22 KB

patch

obsolete

>From 3c7b6cc0f6e675151a93f8a2736f220aa2fc342b Mon Sep 17 00:00:00 2001
>From: Thibault Saunier <tsaunier@igalia.com>
>Date: Fri, 25 May 2018 11:05:05 +0200
>Subject: [PATCH xserver] [GStreamer] Add qtdemux patches to fix use of NULL
> pointers
>
>https://bugs.webkit.org/show_bug.cgi?id=185948
>---
> Source/WebCore/ChangeLog                      | 17 ++++++
> .../MediaPlayerPrivateGStreamerBase.cpp       | 19 ++++---
> Tools/ChangeLog                               | 17 ++++++
> Tools/gstreamer/jhbuild.modules               |  2 +
> ...un-the-preferred-decryptor-context-q.patch | 33 +++++++++++
> ...emux-Do-not-unref-a-NULL-stream_tags.patch | 30 ++++++++++
> ...field-name-about-stream-encryption-s.patch | 57 +++++++++++++++++++
> 7 files changed, 166 insertions(+), 9 deletions(-)
> create mode 100644 Tools/gstreamer/patches/gst-plugins-good-0001-qtdemux-Do-not-run-the-preferred-decryptor-context-q.patch
> create mode 100644 Tools/gstreamer/patches/gst-plugins-good-0002-qtdemux-Do-not-unref-a-NULL-stream_tags.patch
> create mode 100644 Tools/gstreamer/patches/gst-plugins-good-0003-qtdemux-Clarify-field-name-about-stream-encryption-s.patch
>
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 8df21124380..82b1db70749 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,20 @@
>+2018-05-25  Thibault Saunier  <tsaunier@igalia.com>
>+
>+        [GStreamer] Handle changes in the "drm-preferred-decryption-system-id" NEED_CONTEXT message.
>+        https://bugs.webkit.org/show_bug.cgi?id=185948
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        - The "stream-encryption-systems" field of the "drm-preferred-decryption-system-id" query was renamed to
>+          "avalaible-stream-encryption-systems"
>+        - It can now be NULL, meaning there is no decryptor avalaible.
>+
>+        Some tests already exist and where failling (crashing) without that patch.
>+
>+        * platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:
>+        (WebCore::extractEventsAndSystemsFromMessage): Handle NULL value for "avalaible-stream-encryption-systems",
>+        moved some code to make the order of the Arrays in the pair clearer.
>+
> 2018-05-24  Chris Dumez  <cdumez@apple.com>
> 
>         Avoid doing unnecessary work in Document::shouldEnforceContentDispositionAttachmentSandbox() when setting is disabled
>diff --git a/Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp b/Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp
>index 0d9b3e5c819..44f5272b589 100644
>--- a/Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp
>+++ b/Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp
>@@ -301,22 +301,23 @@ static std::pair<Vector<GRefPtr<GstEvent>>, Vector<String>> extractEventsAndSyst
> {
>     const GstStructure* structure = gst_message_get_structure(message);
> 
>-    const GValue* streamEncryptionAllowedSystemsValue = gst_structure_get_value(structure, "stream-encryption-systems");
>-    ASSERT(streamEncryptionAllowedSystemsValue && G_VALUE_HOLDS(streamEncryptionAllowedSystemsValue, G_TYPE_STRV));
>-    const char** streamEncryptionAllowedSystems = reinterpret_cast<const char**>(g_value_get_boxed(streamEncryptionAllowedSystemsValue));
>-    ASSERT(streamEncryptionAllowedSystems);
>-    Vector<String> streamEncryptionAllowedSystemsVector;
>-    unsigned i;
>-    for (i = 0; streamEncryptionAllowedSystems[i]; ++i)
>-        streamEncryptionAllowedSystemsVector.append(streamEncryptionAllowedSystems[i]);
>-
>     const GValue* streamEncryptionEventsList = gst_structure_get_value(structure, "stream-encryption-events");
>     ASSERT(streamEncryptionEventsList && GST_VALUE_HOLDS_LIST(streamEncryptionEventsList));
>     unsigned streamEncryptionEventsListSize = gst_value_list_get_size(streamEncryptionEventsList);
>     Vector<GRefPtr<GstEvent>> streamEncryptionEventsVector;
>+
>+    unsigned i;
>     for (i = 0; i < streamEncryptionEventsListSize; ++i)
>         streamEncryptionEventsVector.append(GRefPtr<GstEvent>(static_cast<GstEvent*>(g_value_get_boxed(gst_value_list_get_value(streamEncryptionEventsList, i)))));
> 
>+    Vector<String> streamEncryptionAllowedSystemsVector;
>+    const GValue* streamEncryptionAllowedSystemsValue = gst_structure_get_value(structure, "avalaible-stream-encryption-systems");
>+    if (streamEncryptionAllowedSystemsValue) {
>+        const char** streamEncryptionAllowedSystems = reinterpret_cast<const char**>(g_value_get_boxed(streamEncryptionAllowedSystemsValue));
>+        for (i = 0; streamEncryptionAllowedSystems[i]; ++i)
>+            streamEncryptionAllowedSystemsVector.append(streamEncryptionAllowedSystems[i]);
>+    }
>+
>     return std::make_pair(streamEncryptionEventsVector, streamEncryptionAllowedSystemsVector);
> }
> #endif
>diff --git a/Tools/ChangeLog b/Tools/ChangeLog
>index 645f70cdc55..a85a2766933 100644
>--- a/Tools/ChangeLog
>+++ b/Tools/ChangeLog
>@@ -1,3 +1,20 @@
>+2018-05-25  Thibault Saunier  <tsaunier@igalia.com>
>+
>+        [GStreamer] Handle changes in the "drm-preferred-decryption-system-id" NEED_CONTEXT message.
>+        https://bugs.webkit.org/show_bug.cgi?id=185948
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        When the patch introducing gst_protection_filter_systems_by_available_decryptors was merged, we changed sensibly its behaviour
>+        concerning the way empty results is represented (now returning NULL instead of an array of 1(NULL) element) and this change was not
>+        properly taken into account in the qtdemux patch that uses it and got upstreamed. Those 3 new patches fixes that and changes the
>+        NEED_CONTEXT field names to make clarify the meaning of "stream-encryption-systems".
>+
>+        * gstreamer/jhbuild.modules:
>+        * gstreamer/patches/gst-plugins-good-0001-qtdemux-Do-not-run-the-preferred-decryptor-context-q.patch: Added. Merged upstream as 3e063703b3a51b8aaa7f75f36c4660c583a60e93
>+        * gstreamer/patches/gst-plugins-good-0002-qtdemux-Do-not-unref-a-NULL-stream_tags.patch: Added. Merged upstream as 43a540b1cd9f162d3dae5d50e36703dfaf558a3e
>+        * gstreamer/patches/gst-plugins-good-0003-qtdemux-Clarify-field-name-about-stream-encryption-s.patch: Added.
>+
> 2018-05-24  Chris Dumez  <cdumez@apple.com>
> 
>         Reduce copying of FontCascadeDescription objects by moving them around
>diff --git a/Tools/gstreamer/jhbuild.modules b/Tools/gstreamer/jhbuild.modules
>index 4b91a84bc96..8e300bfc0f2 100644
>--- a/Tools/gstreamer/jhbuild.modules
>+++ b/Tools/gstreamer/jhbuild.modules
>@@ -69,6 +69,8 @@
>     <branch hash="sha:34ec062ddb766a32377532e039781f4a16fbc3e8b449e642605bacab26a99172" module="gst-plugins-good/gst-plugins-good-${version}.tar.xz" repo="gstreamer" version="1.14.1">
>       <patch file="gst-plugins-good-0002-qtdemux-add-context-for-a-preferred-protection.patch" strip="1" /> 
>       <patch file="gst-plugins-good-0003-qtdemux-also-push-buffers-without-encryption-info-in.patch" strip="1" /> 
>+      <patch file="gst-plugins-good-0001-qtdemux-Do-not-run-the-preferred-decryptor-context-q.patch" strip="1" /> 
>+      <patch file="gst-plugins-good-0002-qtdemux-Do-not-unref-a-NULL-stream_tags.patch" strip="1" /> 
>     </branch>
>   </autotools>
> 
>diff --git a/Tools/gstreamer/patches/gst-plugins-good-0001-qtdemux-Do-not-run-the-preferred-decryptor-context-q.patch b/Tools/gstreamer/patches/gst-plugins-good-0001-qtdemux-Do-not-run-the-preferred-decryptor-context-q.patch
>new file mode 100644
>index 00000000000..0b7ab977575
>--- /dev/null
>+++ b/Tools/gstreamer/patches/gst-plugins-good-0001-qtdemux-Do-not-run-the-preferred-decryptor-context-q.patch
>@@ -0,0 +1,33 @@
>+From 3e063703b3a51b8aaa7f75f36c4660c583a60e93 Mon Sep 17 00:00:00 2001
>+From: Thibault Saunier <tsaunier@igalia.com>
>+Date: Fri, 25 May 2018 10:17:29 +0200
>+Subject: [PATCH 1/3] qtdemux: Do not run the preferred decryptor context query
>+ if no decryptor avalaible
>+
>+Ultimately this avoids a segfault as the code expect a non NULL array
>+here.
>+---
>+ gst/isomp4/qtdemux.c | 7 +++++++
>+ 1 file changed, 7 insertions(+)
>+
>+diff --git a/gst/isomp4/qtdemux.c b/gst/isomp4/qtdemux.c
>+index 3e8ce7860..ec4a8adfd 100644
>+--- a/gst/isomp4/qtdemux.c
>++++ b/gst/isomp4/qtdemux.c
>+@@ -8056,6 +8056,13 @@ gst_qtdemux_request_protection_context (GstQTDemux * qtdemux,
>+   g_ptr_array_add (qtdemux->protection_system_ids, NULL);
>+   filtered_sys_ids = gst_protection_filter_systems_by_available_decryptors (
>+       (const gchar **) qtdemux->protection_system_ids->pdata);
>++
>++  if (!filtered_sys_ids) {
>++    GST_INFO_OBJECT (element,
>++        "No avalaible decryptor, not worth asking the user to choose.");
>++    return;
>++  }
>++
>+   g_ptr_array_remove_index (qtdemux->protection_system_ids,
>+       qtdemux->protection_system_ids->len - 1);
>+   GST_TRACE_OBJECT (qtdemux, "detected %u protection systems, we have "
>+-- 
>+2.17.0
>+
>diff --git a/Tools/gstreamer/patches/gst-plugins-good-0002-qtdemux-Do-not-unref-a-NULL-stream_tags.patch b/Tools/gstreamer/patches/gst-plugins-good-0002-qtdemux-Do-not-unref-a-NULL-stream_tags.patch
>new file mode 100644
>index 00000000000..ca4d12a2f23
>--- /dev/null
>+++ b/Tools/gstreamer/patches/gst-plugins-good-0002-qtdemux-Do-not-unref-a-NULL-stream_tags.patch
>@@ -0,0 +1,30 @@
>+From 43a540b1cd9f162d3dae5d50e36703dfaf558a3e Mon Sep 17 00:00:00 2001
>+From: Thibault Saunier <tsaunier@igalia.com>
>+Date: Fri, 25 May 2018 10:49:21 +0200
>+Subject: [PATCH 2/3] qtdemux: Do not unref a NULL stream_tags
>+
>+stream->stream_tags is reset to NULL once we expose the stream and
>+these have been consumed, we need to check that when cleaning up
>+the stream.
>+---
>+ gst/isomp4/qtdemux.c | 4 +++-
>+ 1 file changed, 3 insertions(+), 1 deletion(-)
>+
>+diff --git a/gst/isomp4/qtdemux.c b/gst/isomp4/qtdemux.c
>+index ec4a8adfd..4116f4c69 100644
>+--- a/gst/isomp4/qtdemux.c
>++++ b/gst/isomp4/qtdemux.c
>+@@ -2600,7 +2600,9 @@ gst_qtdemux_stream_clear (QtDemuxStream * stream)
>+     entry->sparse = FALSE;
>+   }
>+ 
>+-  gst_tag_list_unref (stream->stream_tags);
>++  if (stream->stream_tags)
>++    gst_tag_list_unref (stream->stream_tags);
>++
>+   stream->stream_tags = gst_tag_list_new_empty ();
>+   gst_tag_list_set_scope (stream->stream_tags, GST_TAG_SCOPE_STREAM);
>+   g_free (stream->redirect_uri);
>+-- 
>+2.17.0
>+
>diff --git a/Tools/gstreamer/patches/gst-plugins-good-0003-qtdemux-Clarify-field-name-about-stream-encryption-s.patch b/Tools/gstreamer/patches/gst-plugins-good-0003-qtdemux-Clarify-field-name-about-stream-encryption-s.patch
>new file mode 100644
>index 00000000000..d5901e148cb
>--- /dev/null
>+++ b/Tools/gstreamer/patches/gst-plugins-good-0003-qtdemux-Clarify-field-name-about-stream-encryption-s.patch
>@@ -0,0 +1,57 @@
>+From 3de36f28377398e07ad8b90366db9b6b7544c09a Mon Sep 17 00:00:00 2001
>+From: Thibault Saunier <tsaunier@igalia.com>
>+Date: Fri, 25 May 2018 12:28:04 +0200
>+Subject: [PATCH 3/3] qtdemux: Clarify field name about
>+ stream-encryption-system
>+
>+This field is actually only informatory but the user can potentially
>+choose something else. EME tests in WebKit testsuite actually doesn't
>+take it into and force another encryption system to be used, and expects
>+to be given the occasion to be so.
>+
>+This basically also reverts 3e063703b3a51b8aaa7f75f36c4660c583a60e93.
>+---
>+ gst/isomp4/qtdemux.c | 13 +++++--------
>+ 1 file changed, 5 insertions(+), 8 deletions(-)
>+
>+diff --git a/gst/isomp4/qtdemux.c b/gst/isomp4/qtdemux.c
>+index 4116f4c69..a7ef6e634 100644
>+--- a/gst/isomp4/qtdemux.c
>++++ b/gst/isomp4/qtdemux.c
>+@@ -8059,12 +8059,6 @@ gst_qtdemux_request_protection_context (GstQTDemux * qtdemux,
>+   filtered_sys_ids = gst_protection_filter_systems_by_available_decryptors (
>+       (const gchar **) qtdemux->protection_system_ids->pdata);
>+ 
>+-  if (!filtered_sys_ids) {
>+-    GST_INFO_OBJECT (element,
>+-        "No avalaible decryptor, not worth asking the user to choose.");
>+-    return;
>+-  }
>+-
>+   g_ptr_array_remove_index (qtdemux->protection_system_ids,
>+       qtdemux->protection_system_ids->len - 1);
>+   GST_TRACE_OBJECT (qtdemux, "detected %u protection systems, we have "
>+@@ -8096,7 +8090,8 @@ gst_qtdemux_request_protection_context (GstQTDemux * qtdemux,
>+   query = gst_query_new_context ("drm-preferred-decryption-system-id");
>+   st = gst_query_writable_structure (query);
>+   gst_structure_set (st, "track-id", G_TYPE_UINT, stream->track_id,
>+-      "stream-encryption-systems", G_TYPE_STRV, filtered_sys_ids, NULL);
>++      "avalaible-stream-encryption-systems", G_TYPE_STRV, filtered_sys_ids,
>++      NULL);
>+   gst_structure_set_value (st, "stream-encryption-events", &event_list);
>+   if (gst_qtdemux_run_query (element, query, GST_PAD_SRC)) {
>+     gst_query_parse_context (query, &ctxt);
>+@@ -8120,7 +8115,9 @@ gst_qtdemux_request_protection_context (GstQTDemux * qtdemux,
>+         "drm-preferred-decryption-system-id");
>+     st = (GstStructure *) gst_message_get_structure (msg);
>+     gst_structure_set (st, "track-id", G_TYPE_UINT, stream->track_id,
>+-        "stream-encryption-systems", G_TYPE_STRV, filtered_sys_ids, NULL);
>++        "avalaible-stream-encryption-systems", G_TYPE_STRV, filtered_sys_ids,
>++        NULL);
>++
>+     gst_structure_set_value (st, "stream-encryption-events", &event_list);
>+     gst_element_post_message (element, msg);
>+   }
>+-- 
>+2.17.0
>+
>-- 
>2.17.0

Flags:

calvaris: review-
calvaris: commit-queue-

Actions: View | Formatted Diff | Diff

Attachments on bug 185948: 341199 | 341264 | 341271 | 341282 | 341290