From bdaa8f83aad34e8312fd52ff7b2a8169c89b2b1e Mon Sep 17 00:00:00 2001 From: Jan Schmidt Date: Fri, 11 Nov 2022 22:57:38 +1100 Subject: [PATCH] pad: Fix sticky event ordering for instant-rate-change The event type for instant-rate-change events was poorly chosen, leading to them being re-sent too late and even after EOS. Add a mechanism in GstPad for the sticky event order to be different to the value of the event type to fix that up. Part-of: --- subprojects/gstreamer/gst/gstevent.c | 27 ++++++++++++++++ subprojects/gstreamer/gst/gstevent.h | 5 +++ subprojects/gstreamer/gst/gstpad.c | 31 ++++++++++++++----- .../gstreamer/tests/check/gst/gstpad.c | 21 +++++++++++-- 4 files changed, 73 insertions(+), 11 deletions(-) diff --git a/subprojects/gstreamer/gst/gstevent.c b/subprojects/gstreamer/gst/gstevent.c index 29d15a0abf..3b8332a393 100644 --- a/subprojects/gstreamer/gst/gstevent.c +++ b/subprojects/gstreamer/gst/gstevent.c @@ -213,6 +213,33 @@ gst_event_type_get_flags (GstEventType type) return ret; } +/** + * gst_event_type_to_sticky_ordering + * @type: a #GstEventType + * + * Converts the #GstEventType to an unsigned integer that + * represents the ordering of sticky events when re-sending them. + * A lower value represents a higher-priority event. + * + * Returns: an unsigned integer + * Since: 1.22 + */ +/* FIXME 2.0: Remove the sticky event order overrides once + * the event type numbers are fixed */ +guint +gst_event_type_to_sticky_ordering (GstEventType type) +{ + guint sticky_order = type; + + /* Fix up the sticky event ordering for events where the + * type was chosen poorly */ + if (type == GST_EVENT_INSTANT_RATE_CHANGE) { + sticky_order = GST_EVENT_SEGMENT + 1; + } + + return sticky_order; +} + static void _gst_event_free (GstEvent * event) { diff --git a/subprojects/gstreamer/gst/gstevent.h b/subprojects/gstreamer/gst/gstevent.h index e036a34ad4..a58eb40aa8 100644 --- a/subprojects/gstreamer/gst/gstevent.h +++ b/subprojects/gstreamer/gst/gstevent.h @@ -169,6 +169,7 @@ typedef enum { GST_EVENT_GAP = GST_EVENT_MAKE_TYPE (160, _FLAG(DOWNSTREAM) | _FLAG(SERIALIZED)), /* sticky downstream non-serialized */ + /* FIXME 2.0: change to value 72 and move after the GST_EVENT_SEGMENT event */ GST_EVENT_INSTANT_RATE_CHANGE = GST_EVENT_MAKE_TYPE (180, _FLAG(DOWNSTREAM) | _FLAG(STICKY)), /* upstream events */ @@ -417,6 +418,10 @@ GST_API GstEventTypeFlags gst_event_type_get_flags (GstEventType type); + +GST_API +guint gst_event_type_to_sticky_ordering (GstEventType type) G_GNUC_CONST; + #ifndef GST_DISABLE_MINIOBJECT_INLINE_FUNCTIONS /* refcounting */ static inline GstEvent * diff --git a/subprojects/gstreamer/gst/gstpad.c b/subprojects/gstreamer/gst/gstpad.c index 4802518137..db9893352a 100644 --- a/subprojects/gstreamer/gst/gstpad.c +++ b/subprojects/gstreamer/gst/gstpad.c @@ -128,6 +128,7 @@ enum typedef struct { gboolean received; + guint sticky_order; GstEvent *event; } PadEvent; @@ -459,6 +460,8 @@ remove_events (GstPad * pad) } } +#define _to_sticky_order(t) gst_event_type_to_sticky_ordering(t) + /* should be called with object lock */ static PadEvent * find_event_by_type (GstPad * pad, GstEventType type, guint idx) @@ -466,6 +469,7 @@ find_event_by_type (GstPad * pad, GstEventType type, guint idx) guint i, len; GArray *events; PadEvent *ev; + guint last_sticky_order = _to_sticky_order (type); events = pad->priv->events; len = events->len; @@ -479,7 +483,7 @@ find_event_by_type (GstPad * pad, GstEventType type, guint idx) if (idx == 0) goto found; idx--; - } else if (GST_EVENT_TYPE (ev->event) > type) { + } else if (ev->sticky_order > last_sticky_order) { break; } } @@ -499,11 +503,12 @@ find_event (GstPad * pad, GstEvent * event) events = pad->priv->events; len = events->len; + guint sticky_order = _to_sticky_order (GST_EVENT_TYPE (event)); for (i = 0; i < len; i++) { ev = &g_array_index (events, PadEvent, i); if (event == ev->event) goto found; - else if (GST_EVENT_TYPE (ev->event) > GST_EVENT_TYPE (event)) + else if (ev->sticky_order > sticky_order) break; } ev = NULL; @@ -522,13 +527,15 @@ remove_event_by_type (GstPad * pad, GstEventType type) events = pad->priv->events; len = events->len; + guint last_sticky_order = _to_sticky_order (type); + i = 0; while (i < len) { ev = &g_array_index (events, PadEvent, i); if (ev->event == NULL) goto next; - if (GST_EVENT_TYPE (ev->event) > type) + if (ev->sticky_order > last_sticky_order) break; else if (GST_EVENT_TYPE (ev->event) != type) goto next; @@ -599,6 +606,7 @@ restart: goto next; /* take additional ref, func might release the lock */ + ev_ret.sticky_order = ev->sticky_order; ev_ret.event = gst_event_ref (ev->event); ev_ret.received = ev->received; @@ -4033,12 +4041,17 @@ push_sticky (GstPad * pad, PadEvent * ev, gpointer user_data) return TRUE; } + guint data_sticky_order = 0; + if (data->event) { + data_sticky_order = _to_sticky_order (GST_EVENT_TYPE (data->event)); + } + /* If we're called because of an sticky event, only forward * events that would come before this new event and the * event itself */ if (data->event && GST_EVENT_IS_STICKY (data->event) && - GST_EVENT_TYPE (data->event) <= GST_EVENT_SEGMENT && - GST_EVENT_TYPE (data->event) < GST_EVENT_TYPE (event)) { + data_sticky_order <= _to_sticky_order (GST_EVENT_SEGMENT) && + data_sticky_order < ev->sticky_order) { data->ret = GST_FLOW_CUSTOM_SUCCESS_1; } else { data->ret = gst_pad_push_event_unchecked (pad, gst_event_ref (event), @@ -5295,6 +5308,7 @@ store_sticky_event (GstPad * pad, GstEvent * event) gboolean insert = TRUE; type = GST_EVENT_TYPE (event); + guint sticky_order = _to_sticky_order (type); /* Store all sticky events except SEGMENT/EOS when we're flushing, * otherwise they can be dropped and nothing would ever resend them. @@ -5343,11 +5357,11 @@ store_sticky_event (GstPad * pad, GstEvent * event) break; } - if (type < GST_EVENT_TYPE (ev->event) || (type != GST_EVENT_TYPE (ev->event) + if (sticky_order < ev->sticky_order || (type != GST_EVENT_TYPE (ev->event) && GST_EVENT_TYPE (ev->event) == GST_EVENT_EOS)) { /* STREAM_START, CAPS and SEGMENT must be delivered in this order. By * storing the sticky ordered we can check that this is respected. */ - if (G_UNLIKELY (GST_EVENT_TYPE (ev->event) <= GST_EVENT_SEGMENT + if (G_UNLIKELY (ev->sticky_order <= _to_sticky_order (GST_EVENT_SEGMENT) || GST_EVENT_TYPE (ev->event) == GST_EVENT_EOS)) g_warning (G_STRLOC ":%s:<%s:%s> Sticky event misordering, got '%s' before '%s'", @@ -5359,6 +5373,7 @@ store_sticky_event (GstPad * pad, GstEvent * event) } if (insert) { PadEvent ev; + ev.sticky_order = sticky_order; ev.event = gst_event_ref (event); ev.received = FALSE; g_array_insert_val (events, i, ev); @@ -5438,7 +5453,7 @@ sticky_changed (GstPad * pad, PadEvent * ev, gpointer user_data) /* Forward all sticky events before our current one that are pending */ if (ev->event != data->event - && GST_EVENT_TYPE (ev->event) < GST_EVENT_TYPE (data->event)) + && ev->sticky_order < _to_sticky_order (GST_EVENT_TYPE (data->event))) return push_sticky (pad, ev, data); return TRUE; diff --git a/subprojects/gstreamer/tests/check/gst/gstpad.c b/subprojects/gstreamer/tests/check/gst/gstpad.c index 1b1afe6833..0b545eec91 100644 --- a/subprojects/gstreamer/tests/check/gst/gstpad.c +++ b/subprojects/gstreamer/tests/check/gst/gstpad.c @@ -2591,6 +2591,12 @@ test_sticky_events_handler (GstPad * pad, GstObject * parent, GstEvent * event) case 2: fail_unless (GST_EVENT_TYPE (event) == GST_EVENT_SEGMENT); break; + case 3: + fail_unless (GST_EVENT_TYPE (event) == GST_EVENT_INSTANT_RATE_CHANGE); + break; + case 4: + fail_unless (GST_EVENT_TYPE (event) == GST_EVENT_STREAM_COLLECTION); + break; default: fail_unless (FALSE); break; @@ -2642,6 +2648,15 @@ GST_START_TEST (test_sticky_events) gst_segment_init (&seg, GST_FORMAT_TIME); gst_pad_push_event (srcpad, gst_event_new_segment (&seg)); + /* Push a stream collection */ + GstStreamCollection *collection = gst_stream_collection_new (0); + gst_pad_push_event (srcpad, gst_event_new_stream_collection (collection)); + gst_object_unref (collection); + + /* Push an instant rate change, which should be sent earlier than the preceding stream collection */ + gst_pad_push_event (srcpad, gst_event_new_instant_rate_change (1.0, + GST_SEGMENT_FLAG_NONE)); + /* now make a sinkpad */ sinkpad = gst_pad_new ("sink", GST_PAD_SINK); fail_unless (sinkpad != NULL); @@ -2662,13 +2677,13 @@ GST_START_TEST (test_sticky_events) gst_pad_push_event (srcpad, gst_event_new_caps (caps)); gst_caps_unref (caps); - /* should have triggered 2 events, the segment event is still pending */ + /* should have triggered 2 events, the segment, stream collection and instant-rate events are still pending */ fail_unless_equals_int (sticky_count, 2); fail_unless (gst_pad_push (srcpad, gst_buffer_new ()) == GST_FLOW_OK); - /* should have triggered 3 events */ - fail_unless_equals_int (sticky_count, 3); + /* should have triggered 5 events */ + fail_unless_equals_int (sticky_count, 5); gst_object_unref (srcpad); gst_object_unref (sinkpad);