From: Alicia Boya García <aboya@igalia.com>
Date: Wed, 9 Oct 2024 17:35:33 +0000 (-0400)
Subject: pad: Never push sticky events in response to a FLUSH_STOP
X-Git-Tag: 1.24.10~54
X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=10bb88ab86433ec61591ba902ab26cc0747e0282;p=platform%2Fupstream%2Fgstreamer.git

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: <https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/8003>
---

diff --git a/subprojects/gstreamer/gst/gstpad.c b/subprojects/gstreamer/gst/gstpad.c
index 6411d3a4b6..23418081db 100644
--- a/subprojects/gstreamer/gst/gstpad.c
+++ b/subprojects/gstreamer/gst/gstpad.c
@@ -5569,8 +5569,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);
 
@@ -5729,11 +5733,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) {