Skip to content

Commit

Permalink
Forward port GStreamer sticky events race fix (#77)
Browse files Browse the repository at this point in the history
Applies
https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/7632
on top of stable GStreamer. That merge request fixes a long standing
race condition prone to occur in MSE.
  • Loading branch information
ntrrgc authored Nov 18, 2024
1 parent 3bb7d24 commit 384a3e7
Show file tree
Hide file tree
Showing 2 changed files with 82 additions and 0 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
From 59b714edc36c6d5887d54741cd628f38efe9e30c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Alicia=20Boya=20Garc=C3=ADa?= <[email protected]>
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: <https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/7632>
---
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) {
--
2.47.0

1 change: 1 addition & 0 deletions images/wkdev_sdk/jhbuild/webkit-sdk-deps.modules
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,7 @@
<patch file="0001-webrtcbin-create-and-associate-transceivers-earlier-.patch" strip="1"/>
<patch file="0002-webrtcbin-reverse-direction-from-remote-media.patch" strip="1"/>
<patch file="0003-webrtcbin-connect-output-stream-on-recv-transceivers.patch" strip="1"/>
<patch file="0004-pad-Never-push-sticky-events-in-response-to-a-FLUSH_.patch" strip="1"/>
</branch>
<dependencies>
<dep package="openh264"/>
Expand Down

0 comments on commit 384a3e7

Please sign in to comment.