From 59b714edc36c6d5887d54741cd628f38efe9e30c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alicia=20Boya=20Garc=C3=ADa?= Date: Wed, 9 Oct 2024 13:35:33 -0400 Subject: [PATCH] pad: Never push sticky events in response to a FLUSH_STOP FLUSH_STOP is meant to clear the flushing state of pads and elements downstream, not to process data. Hence, a FLUSH_STOP should not propagate sticky events. This is also consistent with how flushes are a special case for probes. Currently this is almost always the case, since a FLUSH_STOP is __usually__ preceded by a FLUSH_START, and events (sticky or not) are discarded while a pad has the FLUSHING flag active (set by FLUSH_START). However, it is currently assumed that a FLUSH_STOP not preceded by a FLUSH_START is correct behavior, and this will occur while autoplugging pipelines are constructed. This leaves us with an unhandled edge case! This patch explicitly disables sending sticky events when pushing a FLUSH_STOP, instead of relying on the flushing flag of the pad, which will break in the edge case of a FLUSH_STOP not preceded by a FLUSH_START. If sticky events are propagated in response to a FLUSH_STOP, the flushing thread can end up deadlocked in blocking code of a downstream pad, such as a blocking probe. Instead, those events should be propagated from the streaming thread of the pad when handling a non-flushing synchronized event or buffer. This fixes a deadlock found in WebKit with playbin3 when seeks occur before preroll, where the seeking thread ended up stuck in the blocking probe of playsink: https://github.com/WebPlatformForEmbedded/WPEWebKit/issues/1367 Part-of: --- subprojects/gstreamer/gst/gstpad.c | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/subprojects/gstreamer/gst/gstpad.c b/subprojects/gstreamer/gst/gstpad.c index d8bda692b2..7c159d2816 100644 --- a/subprojects/gstreamer/gst/gstpad.c +++ b/subprojects/gstreamer/gst/gstpad.c @@ -5580,8 +5580,12 @@ gst_pad_push_event_unchecked (GstPad * pad, GstEvent * event, PROBE_PUSH (pad, type | GST_PAD_PROBE_TYPE_PUSH, event, probe_stopped); /* recheck sticky events because the probe might have cause a relink */ + /* Note: FLUSH_STOP is a serialized event, but must not propagate sticky + * events. FLUSH_STOP is only targeted at removing the flushing state from + * pads and elements, and not actually pushing data/events. */ if (GST_PAD_HAS_PENDING_EVENTS (pad) && GST_PAD_IS_SRC (pad) - && (GST_EVENT_IS_SERIALIZED (event))) { + && (GST_EVENT_IS_SERIALIZED (event)) + && GST_EVENT_TYPE (event) != GST_EVENT_FLUSH_STOP) { PushStickyData data = { GST_FLOW_OK, FALSE, event }; GST_OBJECT_FLAG_UNSET (pad, GST_PAD_FLAG_PENDING_EVENTS); @@ -5740,11 +5744,18 @@ gst_pad_push_event (GstPad * pad, GstEvent * event) break; } } - if (GST_PAD_IS_SRC (pad) && serialized) { + if (GST_PAD_IS_SRC (pad) && serialized + && GST_EVENT_TYPE (event) != GST_EVENT_FLUSH_STOP) { /* All serialized events on the srcpad trigger push of sticky events. * * Note that we do not do this for non-serialized sticky events since it - * could potentially block. */ + * could potentially block. + * + * We must NOT propagate sticky events in response to FLUSH_STOP either, as + * FLUSH_STOP is only targeted at removing the flushing state from pads and + * elements, and not actually pushing data/events. This also makes it + * consistent with the way flush events are handled in "blocking" pad + * probes. */ res = (check_sticky (pad, event) == GST_FLOW_OK); } if (!serialized || !sticky) {