Attachment 373593 Details for Bug 199505 – Patch for landing

[patch] Patch for landing

bug-199505-20190707095917.patch (text/plain), 15.75 KB, created by Charlie Turner on 2019-07-07 01:59:18 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Charlie Turner

Created: 2019-07-07 01:59:18 PDT

Size: 15.75 KB

patch

obsolete

>Subversion Revision: 247135
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 07cd6ac5cbeb6bd3bc7f3289a272d6dfed27db49..dfbd1eafce0daba3635521d320cc70814e75a6d0 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,34 @@
>+2019-07-04  Charlie Turner  <cturner@igalia.com>
>+
>+        REGRESSION(r243197): [GStreamer] Web process hangs when scrolling twitter timeline which contains HLS videos
>+        https://bugs.webkit.org/show_bug.cgi?id=197558
>+
>+        Reviewed by Xabier Rodriguez-Calvar.
>+
>+        Not covered, I have a test locally that would probably trigger the
>+        deadlock if the network requests took a realistic amount of time,
>+        but from a local webserver the window of time to hit this deadlock
>+        is too narrow.
>+
>+        * platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:
>+        (webkit_web_src_init): Make the websrc start asynchronously, this
>+        allows the main thread to be free to complete resource loader
>+        setup.
>+        (webKitWebSrcCreate): Calling start() from the create() vfunc is a
>+        recipe for deadlock, since BaseSrc holds the streaming lock during
>+        seeks, and then calls create(). In these cases, we do not want to
>+        notify async-completion, since we've already completed from the
>+        necessarily preceeding start() vfunc, and calling it again would
>+        require the stream-lock and deadlock us.
>+        (webKitWebSrcStart): Refactor to use webKitWebSrcMakeRequest, but
>+        ensuring that we do perform an async-complete notification.
>+        (webKitWebSrcMakeRequest): What Start() used to be, but now can be
>+        toggled when to notify of async-completion. Start() no longer
>+        blocks, since the return value of initiating a resource loader is
>+        of no interest to the callers.
>+        (webKitWebSrcCloseSession): Similarly to Start(), we do not need
>+        to wait for the completion of cancelled net requests.
>+
> 2019-07-03  Eric Carlson  <eric.carlson@apple.com>
> 
>         [MSE] Add more debug and error logging
>diff --git a/Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp b/Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp
>index 121884a10ad51151f97ac07c550c236e95e6ded9..12b21ae3bfd3c67691a5c5ba567f9b06c1a2f372 100644
>--- a/Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp
>+++ b/Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp
>@@ -156,6 +156,7 @@ static void webKitWebSrcSetProperty(GObject*, guint propertyID, const GValue*, G
> static void webKitWebSrcGetProperty(GObject*, guint propertyID, GValue*, GParamSpec*);
> static GstStateChangeReturn webKitWebSrcChangeState(GstElement*, GstStateChange);
> static GstFlowReturn webKitWebSrcCreate(GstPushSrc*, GstBuffer**);
>+static gboolean webKitWebSrcMakeRequest(GstBaseSrc*, bool);
> static gboolean webKitWebSrcStart(GstBaseSrc*);
> static gboolean webKitWebSrcStop(GstBaseSrc*);
> static gboolean webKitWebSrcGetSize(GstBaseSrc*, guint64* size);
>@@ -260,6 +261,7 @@ static void webkit_web_src_init(WebKitWebSrc* src)
> 
>     webkitWebSrcReset(src);
>     gst_base_src_set_automatic_eos(GST_BASE_SRC_CAST(src), FALSE);
>+    gst_base_src_set_async(GST_BASE_SRC_CAST(src), TRUE);
> }
> 
> static void webKitWebSrcDispose(GObject* object)
>@@ -361,7 +363,12 @@ static GstFlowReturn webKitWebSrcCreate(GstPushSrc* pushSrc, GstBuffer** buffer)
>         uint64_t requestedPosition = priv->requestedPosition;
>         webKitWebSrcStop(baseSrc);
>         priv->requestedPosition = requestedPosition;
>-        webKitWebSrcStart(baseSrc);
>+        // Do not notify async-completion, in seeking flows, we will
>+        // be called from GstBaseSrc's perform_seek vfunc, which holds
>+        // a streaming lock in our frame. Hence, we would deadlock
>+        // trying to notify async completion, since that also requires
>+        // the streaming lock.
>+        webKitWebSrcMakeRequest(baseSrc, false);
>     }
> 
>     {
>@@ -496,6 +503,14 @@ static gboolean webKitWebSrcProcessExtraHeaders(GQuark fieldId, const GValue* va
> }
> 
> static gboolean webKitWebSrcStart(GstBaseSrc* baseSrc)
>+{
>+    // This method should only be called by BaseSrc, do not call it
>+    // from ourselves unless you ensure the streaming lock is not
>+    // held. If it is, you will deadlock the WebProcess.
>+    return webKitWebSrcMakeRequest(baseSrc, true);
>+}
>+
>+static gboolean webKitWebSrcMakeRequest(GstBaseSrc* baseSrc, bool notifyAsyncCompletion)
> {
>     WebKitWebSrc* src = WEBKIT_WEB_SRC(baseSrc);
>     WebKitWebSrcPrivate* priv = src->priv;
>@@ -582,7 +597,7 @@ static gboolean webKitWebSrcStart(GstBaseSrc* baseSrc)
>     request.setHTTPHeaderField(HTTPHeaderName::IcyMetadata, "1");
> 
>     GRefPtr<WebKitWebSrc> protector = WTF::ensureGRef(src);
>-    priv->notifier->notifyAndWait(MainThreadSourceNotification::Start, [protector, request = WTFMove(request)] {
>+    priv->notifier->notify(MainThreadSourceNotification::Start, [protector, request = WTFMove(request), src, notifyAsyncCompletion] {
>         WebKitWebSrcPrivate* priv = protector->priv;
>         if (!priv->loader)
>             priv->loader = priv->player->createResourceLoader();
>@@ -594,13 +609,16 @@ static gboolean webKitWebSrcStart(GstBaseSrc* baseSrc)
>         if (priv->resource) {
>             priv->resource->setClient(std::make_unique<CachedResourceStreamingClient>(protector.get(), ResourceRequest(request)));
>             GST_DEBUG_OBJECT(protector.get(), "Started request");
>+            if (notifyAsyncCompletion)
>+                gst_base_src_start_complete(GST_BASE_SRC(src), GST_FLOW_OK);
>         } else {
>             GST_ERROR_OBJECT(protector.get(), "Failed to setup streaming client");
>+            if (notifyAsyncCompletion)
>+                gst_base_src_start_complete(GST_BASE_SRC(src), GST_FLOW_ERROR);
>             priv->loader = nullptr;
>         }
>     });
> 
>-    GST_DEBUG_OBJECT(src, "Resource loader started");
>     return TRUE;
> }
> 
>@@ -609,7 +627,7 @@ static void webKitWebSrcCloseSession(WebKitWebSrc* src)
>     WebKitWebSrcPrivate* priv = src->priv;
>     GRefPtr<WebKitWebSrc> protector = WTF::ensureGRef(src);
> 
>-    priv->notifier->notifyAndWait(MainThreadSourceNotification::Stop, [protector, keepAlive = priv->keepAlive] {
>+    priv->notifier->notify(MainThreadSourceNotification::Stop, [protector, keepAlive = priv->keepAlive] {
>         WebKitWebSrcPrivate* priv = protector->priv;
> 
>         GST_DEBUG_OBJECT(protector.get(), "Stopping resource loader");
>diff --git a/Tools/ChangeLog b/Tools/ChangeLog
>index 0e006ba8d170a250289e62a9bb4499052ce6c873..686234f48c9b1ea2d0704da960d36fab59dd0857 100644
>--- a/Tools/ChangeLog
>+++ b/Tools/ChangeLog
>@@ -1,3 +1,19 @@
>+2019-07-04  Charlie Turner  <cturner@igalia.com>
>+
>+        REGRESSION(r243197): [GStreamer] Web process hangs when scrolling twitter timeline which contains HLS videos
>+        https://bugs.webkit.org/show_bug.cgi?id=197558
>+
>+        Reviewed by Xabier Rodriguez-Calvar.
>+
>+        On shutdown we can easily deadlock the web process if we don't
>+        ensure all network operations are completed before comitting state
>+        changes. In HLS, make sure the network operations are cancelled,
>+        and also prevent hlsdemux's retry logic from scuppering our
>+        efforts.
>+
>+        * gstreamer/jhbuild.modules: Include the patch.
>+        * gstreamer/patches/gst-plugins-bad-do-not-retry-downloads-during-shutdown.patch: Added.
>+
> 2019-07-01  Philippe Normand  <pnormand@igalia.com>
> 
>         Unreviewed, GTK a11y tests fix after r247071
>diff --git a/Tools/gstreamer/jhbuild.modules b/Tools/gstreamer/jhbuild.modules
>index c39c58dc4e5fccdda3bcd0c82af5d5053dd42e3d..4a1b43cd628559d05a8627437375abb902868a99 100644
>--- a/Tools/gstreamer/jhbuild.modules
>+++ b/Tools/gstreamer/jhbuild.modules
>@@ -92,6 +92,7 @@
>       <dep package="libsrtp"/>
>     </dependencies>
>     <branch hash="sha256:22139de35626ada6090bdfa3423b27b7fc15a0198331d25c95e6b12cb1072b05" module="gst-plugins-bad/gst-plugins-bad-${version}.tar.xz" repo="gstreamer" version="1.16.0">
>+      <patch file="gst-plugins-bad-do-not-retry-downloads-during-shutdown.patch" strip="1"/> 
>     </branch>
>   </meson>
> 
>diff --git a/Tools/gstreamer/patches/gst-plugins-bad-do-not-retry-downloads-during-shutdown.patch b/Tools/gstreamer/patches/gst-plugins-bad-do-not-retry-downloads-during-shutdown.patch
>new file mode 100644
>index 0000000000000000000000000000000000000000..b617f314a50754b18f26ffd3a3db19e65294d3fe
>--- /dev/null
>+++ b/Tools/gstreamer/patches/gst-plugins-bad-do-not-retry-downloads-during-shutdown.patch
>@@ -0,0 +1,89 @@
>+From 4c21593e5fcd1337b433119b8c7800dc5565f514 Mon Sep 17 00:00:00 2001
>+From: Charlie Turner <cturner@igalia.com>
>+Date: Tue, 2 Jul 2019 12:27:40 +0100
>+Subject: [PATCH] WIP: adaptivedemux: do not retry downloads during shutdown.
>+
>+---
>+ ext/hls/gsthlsdemux.c | 15 +++++++++++++--
>+ ext/hls/gsthlsdemux.h |  4 ++++
>+ 2 files changed, 17 insertions(+), 2 deletions(-)
>+
>+diff --git a/ext/hls/gsthlsdemux.c b/ext/hls/gsthlsdemux.c
>+index 4317d65c3..f9583ad1a 100644
>+--- a/ext/hls/gsthlsdemux.c
>++++ b/ext/hls/gsthlsdemux.c
>+@@ -73,6 +73,7 @@ static gboolean gst_hls_demux_update_playlist (GstHLSDemux * demux,
>+     gboolean update, GError ** err);
>+ static gchar *gst_hls_src_buf_to_utf8_playlist (GstBuffer * buf);
>+ 
>++/* FIXME: the return value is never used? */
>+ static gboolean gst_hls_demux_change_playlist (GstHLSDemux * demux,
>+     guint max_bitrate, gboolean * changed);
>+ static GstBuffer *gst_hls_demux_decrypt_fragment (GstHLSDemux * demux,
>+@@ -193,6 +194,8 @@ gst_hls_demux_init (GstHLSDemux * demux)
>+ 
>+   demux->keys = g_hash_table_new_full (g_str_hash, g_str_equal, g_free, g_free);
>+   g_mutex_init (&demux->keys_lock);
>++
>++  demux->cancelling_downloads = FALSE;
>+ }
>+ 
>+ static GstStateChangeReturn
>+@@ -205,6 +208,11 @@ gst_hls_demux_change_state (GstElement * element, GstStateChange transition)
>+     case GST_STATE_CHANGE_READY_TO_PAUSED:
>+       gst_hls_demux_reset (GST_ADAPTIVE_DEMUX_CAST (demux));
>+       break;
>++    case GST_STATE_CHANGE_PAUSED_TO_READY:
>++      GST_DEBUG_OBJECT (demux, "PAUSED->READY cancelling downloads");
>++      demux->cancelling_downloads = TRUE;
>++      gst_uri_downloader_cancel (GST_ADAPTIVE_DEMUX (demux)->downloader);
>++      break;
>+     default:
>+       break;
>+   }
>+@@ -1158,6 +1166,8 @@ gst_hls_demux_reset (GstAdaptiveDemux * ademux)
>+ {
>+   GstHLSDemux *demux = GST_HLS_DEMUX_CAST (ademux);
>+ 
>++  GST_DEBUG_OBJECT (demux, "resetting");
>++
>+   GST_M3U8_CLIENT_LOCK (hlsdemux->client);
>+   if (demux->master) {
>+     gst_hls_master_playlist_unref (demux->master);
>+@@ -1379,7 +1389,8 @@ retry:
>+   if (download == NULL) {
>+     gchar *base_uri;
>+ 
>+-    if (!update || main_checked || demux->master->is_simple) {
>++    if (!update || main_checked || demux->master->is_simple
>++        || demux->cancelling_downloads) {
>+       g_free (uri);
>+       return FALSE;
>+     }
>+@@ -1612,7 +1623,7 @@ retry_failover_protection:
>+     if (changed)
>+       *changed = TRUE;
>+     stream->discont = TRUE;
>+-  } else {
>++  } else if (!demux->cancelling_downloads) {
>+     GstHLSVariantStream *failover_variant = NULL;
>+     GList *failover;
>+ 
>+diff --git a/ext/hls/gsthlsdemux.h b/ext/hls/gsthlsdemux.h
>+index 0cab19627..9c0decabf 100644
>+--- a/ext/hls/gsthlsdemux.h
>++++ b/ext/hls/gsthlsdemux.h
>+@@ -147,6 +147,10 @@ struct _GstHLSDemux
>+   GstHLSMasterPlaylist *master;
>+ 
>+   GstHLSVariantStream  *current_variant;
>++
>++  /* Set when the parent is state-changing down from PAUSED to avoid
>++     making further network requests. */
>++  gboolean cancelling_downloads;
>+ };
>+ 
>+ struct _GstHLSDemuxClass
>+-- 
>+2.17.1
>+
>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index a88e8cc5a7d8a1c1e8a1acf98a634f7cf0298321..004d48b067ad4cb093f05d08700458fb222ee39c 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,19 @@
>+2019-07-05  Charlie Turner  <cturner@igalia.com>
>+
>+        [GStreamer] media/video-volume.html broken after switching from cubic to linear scaling
>+        https://bugs.webkit.org/show_bug.cgi?id=199505
>+
>+        Reviewed by Xabier Rodriguez-Calvar.
>+
>+        PulseAudio has a conversion process from volume's in
>+        double-precision to uint32_t volumes. Depending on the environment
>+        can introduce rounding errors. Be more lenient in our comparison
>+        code.
>+
>+        * media/video-volume-expected.txt: Update baseline
>+        * media/video-volume.html: Compare volume values within a
>+        reasonable tolerance.
>+
> 2019-07-03  Justin Fan  <justin_fan@apple.com>
> 
>         [WHLSL] Support float4x4 in test harness
>diff --git a/LayoutTests/media/video-seek-to-current-time.html b/LayoutTests/media/video-seek-to-current-time.html
>index 1be0747ca0ff5344d4900ecb250b6d6edb664cea..9b824e9e75ef8b142cab267bc24b6dc7f1ab5ce6 100644
>--- a/LayoutTests/media/video-seek-to-current-time.html
>+++ b/LayoutTests/media/video-seek-to-current-time.html
>@@ -46,6 +46,7 @@
> 
>         function runTest()
>         {
>+	    consoleWrite("runTest()");
>             if (++seekedCount == 1)
>                 return;
> 
>diff --git a/LayoutTests/media/video-volume-expected.txt b/LayoutTests/media/video-volume-expected.txt
>index b6c4cb1a993c564483ae283a7baa82910dd63107..6840e0da0b963493f6649e395afb59af2c3656d3 100644
>--- a/LayoutTests/media/video-volume-expected.txt
>+++ b/LayoutTests/media/video-volume-expected.txt
>@@ -1,18 +1,18 @@
> 
> Test 'volume' attribute
> 
>-EXPECTED (video.volume == '1') OK
>+EXPECTED (compareWithTolerance(video.volume, 1.0) == 'true') OK
> RUN(video.volume = 0.5)
>-EXPECTED (video.volume == '0.5') OK
>+EXPECTED (compareWithTolerance(video.volume, 0.5) == 'true') OK
> RUN(video.volume = 0)
>-EXPECTED (video.volume == '0') OK
>+EXPECTED (compareWithTolerance(video.volume, 0) == 'true') OK
> TEST(video.volume = 1.5) THROWS(DOMException.INDEX_SIZE_ERR) OK
> TEST(video.volume = -0.5) THROWS(DOMException.INDEX_SIZE_ERR) OK
> RUN(video.load())
> EVENT(canplaythrough)
>-EXPECTED (video.volume == '0') OK
>+EXPECTED (compareWithTolerance(video.volume, 0) == 'true') OK
> RUN(video.volume = 0.5)
>-EXPECTED (video.volume == '0.5') OK
>+EXPECTED (compareWithTolerance(video.volume, 0.5) == 'true') OK
> TEST(video.volume = 1.5) THROWS(DOMException.INDEX_SIZE_ERR) OK
> TEST(video.volume = -0.5) THROWS(DOMException.INDEX_SIZE_ERR) OK
> END OF TEST
>diff --git a/LayoutTests/media/video-volume.html b/LayoutTests/media/video-volume.html
>index 59b0b4cf1276b595bfca780fff350f24ca43848a..783ed94406296670c8848d20deb8c7fd2b1a7d2e 100644
>--- a/LayoutTests/media/video-volume.html
>+++ b/LayoutTests/media/video-volume.html
>@@ -3,19 +3,23 @@
> <script src=media-file.js></script>
> <script src=video-test.js></script>
> <script>
>-    testExpected("video.volume", 1.0);
>+    function compareWithTolerance(a, b) {
>+        tolerance = 0.00001;
>+        return (Math.abs(a - b) < tolerance);
>+    }
>+    testExpected("compareWithTolerance(video.volume, 1.0)", true);
>     run("video.volume = 0.5");
>-    testExpected("video.volume", 0.5);
>+    testExpected("compareWithTolerance(video.volume, 0.5)", true);
>     run("video.volume = 0");
>-    testExpected("video.volume", 0);
>+    testExpected("compareWithTolerance(video.volume, 0)", true);
>     testDOMException("video.volume = 1.5", "DOMException.INDEX_SIZE_ERR");
>     testDOMException("video.volume = -0.5", "DOMException.INDEX_SIZE_ERR");
>     video.src = findMediaFile("video", "content/test");
>     run("video.load()");
>     waitForEvent("canplaythrough", function () {
>-        testExpected("video.volume", 0);
>+        testExpected("compareWithTolerance(video.volume, 0)", true);
>         run("video.volume = 0.5");
>-        testExpected("video.volume", 0.5);
>+        testExpected("compareWithTolerance(video.volume, 0.5)", true);
>         testDOMException("video.volume = 1.5", "DOMException.INDEX_SIZE_ERR");
>         testDOMException("video.volume = -0.5", "DOMException.INDEX_SIZE_ERR");
>         endTest();

Actions: View | Formatted Diff | Diff

Attachments on bug 199505: 373482 | 373593 | 373620