Revert "pad: Handle changing sticky events in pad probes"

This reverts commit 11e0f451eb498e92d05d8208f7217625dc62848b.

When pushing a sticky event out of a pad with a pad probe or pad offset,
those should not be applied to the event that is actually stored in the
event but only in the event sent downstream. The pad probe and pad
offsets are conceptually *after* the pad, added by external code and
should not affect any internal state of pads/elements.

Also storing the modified event has the side-effect that a re-sent event
would arrive with any previous modifications done by the same pad probe
again inside that pad probe, and it would have to check if its
modifications are already applied or not.

For sink pads and generally for events arriving in a pad, some further
changes are still needed and those are tracked in
  https://bugzilla.gnome.org/show_bug.cgi?id=765049

In addition, the commit also had a refcounting problem with events,
causing already destroyed events to be stored inside pads.

diff --git a/gst/gstpad.c b/gst/gstpad.c
index 5007c4c..3911dcf 100644 (file)
--- a/gst/gstpad.c
+++ b/gst/gstpad.c
@@ -190,7 +190,7 @@ static GstFlowReturn gst_pad_chain_list_default (GstPad * pad,
 static GstFlowReturn gst_pad_send_event_unchecked (GstPad * pad,
     GstEvent * event, GstPadProbeType type);
 static GstFlowReturn gst_pad_push_event_unchecked (GstPad * pad,
-    GstEvent ** event, GstPadProbeType type);
+    GstEvent * event, GstPadProbeType type);
 
 static gboolean activate_mode_internal (GstPad * pad, GstObject * parent,
     GstPadMode mode, gboolean active);
@@ -644,8 +644,7 @@ restart:
 
 /* should be called with LOCK */
 static GstEvent *
-_apply_pad_offset (GstPad * pad, GstEvent * event, gint64 applied_offset,
-    gboolean upstream)
+_apply_pad_offset (GstPad * pad, GstEvent * event, gboolean upstream)
 {
   gint64 offset;
 
@@ -661,16 +660,16 @@ _apply_pad_offset (GstPad * pad, GstEvent * event, gint64 applied_offset,
     gst_event_copy_segment (event, &segment);
     gst_event_unref (event);
 
-    gst_segment_offset_running_time (&segment, segment.format, applied_offset);
+    gst_segment_offset_running_time (&segment, segment.format, pad->offset);
     event = gst_event_new_segment (&segment);
   }
 
   event = gst_event_make_writable (event);
   offset = gst_event_get_running_time_offset (event);
   if (upstream)
-    offset -= applied_offset;
+    offset -= pad->offset;
   else
-    offset += applied_offset;
+    offset += pad->offset;
   gst_event_set_running_time_offset (event, offset);
 
   return event;
@@ -680,7 +679,7 @@ static inline GstEvent *
 apply_pad_offset (GstPad * pad, GstEvent * event, gboolean upstream)
 {
   if (G_UNLIKELY (pad->offset != 0))
-    return _apply_pad_offset (pad, event, pad->offset, upstream);
+    return _apply_pad_offset (pad, event, upstream);
   return event;
 }
 
@@ -3841,16 +3840,9 @@ gst_pad_get_offset (GstPad * pad)
   return result;
 }
 
-/* This function will make sure that previously set offset is
- * reverted as otherwise we would end up applying the new offset
- * on top of the previously set one, which is not what we want.
- * The event is also marked as not received. */
 static gboolean
-reschedule_event (GstPad * pad, PadEvent * ev, gint64 * prev_offset)
+mark_event_not_received (GstPad * pad, PadEvent * ev, gpointer user_data)
 {
-  if (*prev_offset != 0)
-    ev->event = _apply_pad_offset (pad, ev->event, -*prev_offset, FALSE);
-
   ev->received = FALSE;
   return TRUE;
 }
@@ -3865,7 +3857,6 @@ reschedule_event (GstPad * pad, PadEvent * ev, gint64 * prev_offset)
 void
 gst_pad_set_offset (GstPad * pad, gint64 offset)
 {
-  gint64 prev_offset;
   g_return_if_fail (GST_IS_PAD (pad));
 
   GST_OBJECT_LOCK (pad);
@@ -3873,16 +3864,14 @@ gst_pad_set_offset (GstPad * pad, gint64 offset)
   if (pad->offset == offset)
     goto done;
 
-  prev_offset = pad->offset;
   pad->offset = offset;
   GST_DEBUG_OBJECT (pad, "changed offset to %" GST_STIME_FORMAT,
       GST_STIME_ARGS (offset));
 
   /* resend all sticky events with updated offset on next buffer push */
-  events_foreach (pad, (PadEventFunction) reschedule_event, &prev_offset);
+  events_foreach (pad, mark_event_not_received, NULL);
   GST_OBJECT_FLAG_SET (pad, GST_PAD_FLAG_PENDING_EVENTS);
 
-
 done:
   GST_OBJECT_UNLOCK (pad);
 }
@@ -3924,10 +3913,8 @@ push_sticky (GstPad * pad, PadEvent * ev, gpointer user_data)
       GST_EVENT_TYPE (data->event) < GST_EVENT_TYPE (event)) {
     data->ret = GST_FLOW_CUSTOM_SUCCESS_1;
   } else {
-    gst_event_ref (event);
-    data->ret = gst_pad_push_event_unchecked (pad, &event,
+    data->ret = gst_pad_push_event_unchecked (pad, gst_event_ref (event),
         GST_PAD_PROBE_TYPE_EVENT_DOWNSTREAM);
-    gst_event_replace (&ev->event, event);
     if (data->ret == GST_FLOW_CUSTOM_SUCCESS_1)
       data->ret = GST_FLOW_OK;
   }
@@ -3998,8 +3985,7 @@ check_sticky (GstPad * pad, GstEvent * event)
       PadEvent *ev = find_event_by_type (pad, GST_EVENT_EOS, 0);
 
       if (ev && !ev->received) {
-        gst_event_ref (ev->event);
-        data.ret = gst_pad_push_event_unchecked (pad, &ev->event,
+        data.ret = gst_pad_push_event_unchecked (pad, gst_event_ref (ev->event),
             GST_PAD_PROBE_TYPE_EVENT_DOWNSTREAM);
         /* the event could have been dropped. Because this can only
          * happen if the user asked for it, it's not an error */
@@ -5301,13 +5287,12 @@ sticky_changed (GstPad * pad, PadEvent * ev, gpointer user_data)
 
 /* should be called with pad LOCK */
 static GstFlowReturn
-gst_pad_push_event_unchecked (GstPad * pad, GstEvent ** in_event,
+gst_pad_push_event_unchecked (GstPad * pad, GstEvent * event,
     GstPadProbeType type)
 {
   GstFlowReturn ret;
   GstPad *peerpad;
   GstEventType event_type;
-  GstEvent *event = *in_event;
 
   /* pass the adjusted event on. We need to do this even if
    * there is no peer pad because of the probes. */
@@ -5419,9 +5404,6 @@ gst_pad_push_event_unchecked (GstPad * pad, GstEvent ** in_event,
     PROBE_NO_DATA (pad, GST_PAD_PROBE_TYPE_PUSH | GST_PAD_PROBE_TYPE_IDLE,
         idle_probe_stopped, ret);
   }
-
-  *in_event = event;
-
   return ret;
 
   /* ERROR handling */
@@ -5542,7 +5524,7 @@ gst_pad_push_event (GstPad * pad, GstEvent * event)
     GstFlowReturn ret;
 
     /* other events are pushed right away */
-    ret = gst_pad_push_event_unchecked (pad, &event, type);
+    ret = gst_pad_push_event_unchecked (pad, event, type);
     /* dropped events by a probe are not an error */
     res = (ret == GST_FLOW_OK || ret == GST_FLOW_CUSTOM_SUCCESS
         || ret == GST_FLOW_CUSTOM_SUCCESS_1);

diff --git a/tests/check/gst/gstpad.c b/tests/check/gst/gstpad.c
index 1a3ee8b..9968db3 100644 (file)
--- a/tests/check/gst/gstpad.c
+++ b/tests/check/gst/gstpad.c
@@ -23,7 +23,6 @@
 #endif
 
 #include <gst/check/gstcheck.h>
-#include <gst/check/gstharness.h>
 
 static GstSegment dummy_segment;
 
@@ -3283,61 +3282,6 @@ GST_START_TEST (test_pad_offset_src)
 
 GST_END_TEST;
 
-static GstPadProbeReturn
-update_stream_start_event_cb (GstPad * pad, GstPadProbeInfo * info,
-    const gchar * wanted_stream_id)
-{
-  GstEvent *event = info->data;
-
-  if (GST_EVENT_TYPE (event) == GST_EVENT_STREAM_START) {
-    const gchar *stream_id;
-
-    gst_event_parse_stream_start (event, &stream_id);
-    g_assert_cmpstr (stream_id, !=, wanted_stream_id);
-
-    gst_event_unref (event);
-
-    info->data = gst_event_new_stream_start (wanted_stream_id);
-  }
-
-  return GST_PAD_PROBE_OK;
-}
-
-GST_START_TEST (test_sticky_events_changed_in_probe)
-{
-  GstPad *srcpad;
-  GstHarness *harness;
-  GstEvent *stream_start;
-  const gchar *stream_id;
-  const gchar *wanted_stream_id = "The right stream ID";
-  harness = gst_harness_new ("fakesrc");
-
-  gst_harness_add_probe (harness, "fakesrc", "src",
-      GST_PAD_PROBE_TYPE_EVENT_DOWNSTREAM,
-      (GstPadProbeCallback) update_stream_start_event_cb,
-      (gpointer) wanted_stream_id, NULL);
-  gst_harness_play (harness);
-  gst_harness_set_blocking_push_mode (harness);
-  gst_buffer_unref (gst_harness_pull (harness));
-
-  stream_start =
-      gst_pad_get_sticky_event (harness->sinkpad, GST_EVENT_STREAM_START, 0);
-  gst_event_parse_stream_start (stream_start, &stream_id);
-  fail_unless_equals_string (stream_id, wanted_stream_id);
-  gst_event_unref (stream_start);
-
-  srcpad = gst_element_get_static_pad (harness->element, "src");
-  stream_start = gst_pad_get_sticky_event (srcpad, GST_EVENT_STREAM_START, 0);
-  gst_event_unref (stream_start);
-  gst_object_unref (srcpad);
-  gst_event_parse_stream_start (stream_start, &stream_id);
-  fail_unless_equals_string (stream_id, wanted_stream_id);
-
-  gst_harness_teardown (harness);
-}
-
-GST_END_TEST;
-
 static Suite *
 gst_pad_suite (void)
 {
@@ -3391,7 +3335,6 @@ gst_pad_suite (void)
   tcase_add_test (tc_chain, test_block_async_full_destroy_dispose);
   tcase_add_test (tc_chain, test_block_async_replace_callback_no_flush);
   tcase_add_test (tc_chain, test_sticky_events);
-  tcase_add_test (tc_chain, test_sticky_events_changed_in_probe);
   tcase_add_test (tc_chain, test_last_flow_return_push);
   tcase_add_test (tc_chain, test_last_flow_return_pull);
   tcase_add_test (tc_chain, test_flush_stop_inactive);