Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Forward port GStreamer sticky events race fix #77

Merged
merged 1 commit into from
Nov 18, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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