Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Merge r247215 - REGRESSION(r243197): [GStreamer] Web process hangs wh…
…en scrolling twitter timeline which contains HLS videos https://bugs.webkit.org/show_bug.cgi?id=197558 Reviewed by Xabier Rodriguez-Calvar. Source/WebCore: 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. Tools: 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.
- Loading branch information
1 parent
3a8d0d2
commit ecb1272
Showing
4 changed files
with
158 additions
and
4 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
89 changes: 89 additions & 0 deletions
89
Tools/gstreamer/patches/gst-plugins-bad-do-not-retry-downloads-during-shutdown.patch
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 | ||
|
|