libs/gst/base/gstbasesink.c: Improve position reporting in the flushing state.

Original commit message from CVS:
* libs/gst/base/gstbasesink.c: (gst_base_sink_render_object),
(gst_base_sink_event), (gst_base_sink_chain_unlocked),
(gst_base_sink_negotiate_pull), (gst_base_sink_pad_activate_pull),
(gst_base_sink_get_position), (gst_base_sink_change_state):
Improve position reporting in the flushing state.
Also report the position when we are not yet prerolled but we
have a newsegment event. Fixes #543444.
Improve the pull-based negotiation code.
* tests/check/elements/fakesink.c: (GST_START_TEST),
(fakesink_suite):
Add testcase for position reporting while flushing in PAUSED and
PLAYING.
* tests/check/generic/sinks.c: (GST_START_TEST):
Update unit-test, we can now query the position as soon as we receive a
NEWSEGMENT event.

ChangeLog

@@ -1,3 +1,23 @@
+2008-08-19  Wim Taymans  <wim.taymans@collabora.co.uk>
+
+	* libs/gst/base/gstbasesink.c: (gst_base_sink_render_object),
+	(gst_base_sink_event), (gst_base_sink_chain_unlocked),
+	(gst_base_sink_negotiate_pull), (gst_base_sink_pad_activate_pull),
+	(gst_base_sink_get_position), (gst_base_sink_change_state):
+	Improve position reporting in the flushing state.
+	Also report the position when we are not yet prerolled but we
+	have a newsegment event. Fixes #543444.
+	Improve the pull-based negotiation code.
+
+	* tests/check/elements/fakesink.c: (GST_START_TEST),
+	(fakesink_suite):
+	Add testcase for position reporting while flushing in PAUSED and
+	PLAYING.
+
+	* tests/check/generic/sinks.c: (GST_START_TEST):
+	Update unit-test, we can now query the position as soon as we receive a
+	NEWSEGMENT event.
+
 2008-08-19  Wim Taymans  <wim.taymans@collabora.co.uk>
 
 	Based on patch by: Jason Zhao <e3423c at motorola dot com>

libs/gst/base/gstbasesink.c

@@ -2209,8 +2209,7 @@ gst_base_sink_render_object (GstBaseSink * basesink, GstPad * pad,
       event_res = bclass->event (basesink, event);
 
     /* when we get here we could be flushing again when the event handler calls
-     * _wait_eos() or releases the preroll lock in any other way.
+     * _wait_eos(). We have to ignore this object in that case. */
-     * We have to ignore this object in that case. */
     if (G_UNLIKELY (basesink->flushing))
       goto flushing;
 
@@ -2523,8 +2522,11 @@ gst_base_sink_event (GstPad * pad, GstEvent * event)
             GST_MINI_OBJECT_CAST (event), FALSE);
         if (G_UNLIKELY (ret != GST_FLOW_OK))
           result = FALSE;
-        else
+        else {
+          GST_OBJECT_LOCK (basesink);
           basesink->have_newsegment = TRUE;
+          GST_OBJECT_UNLOCK (basesink);
+        }
       }
       GST_PAD_PREROLL_UNLOCK (pad);
       break;
@@ -2568,19 +2570,19 @@ gst_base_sink_event (GstPad * pad, GstEvent * event)
        * event. */
       gst_base_sink_set_flushing (basesink, pad, FALSE);
 
-      /* we need new segment info after the flush. */
-      gst_segment_init (&basesink->segment, GST_FORMAT_UNDEFINED);
-      gst_segment_init (basesink->abidata.ABI.clip_segment,
-          GST_FORMAT_UNDEFINED);
-      basesink->have_newsegment = FALSE;
-
       /* for position reporting */
       GST_OBJECT_LOCK (basesink);
       basesink->priv->current_sstart = -1;
       basesink->priv->current_sstop = -1;
       basesink->priv->eos_rtime = -1;
+      basesink->have_newsegment = FALSE;
       GST_OBJECT_UNLOCK (basesink);
 
+      /* we need new segment info after the flush. */
+      gst_segment_init (&basesink->segment, GST_FORMAT_UNDEFINED);
+      gst_segment_init (basesink->abidata.ABI.clip_segment,
+          GST_FORMAT_UNDEFINED);
+
       gst_event_unref (event);
       break;
     default:
@@ -2687,12 +2689,14 @@ gst_base_sink_chain_unlocked (GstBaseSink * basesink, GstPad * pad,
           ("Received buffer without a new-segment. Assuming timestamps start from 0."));
     }
 
-    basesink->have_newsegment = TRUE;
     /* this means this sink will assume timestamps start from 0 */
+    GST_OBJECT_LOCK (basesink);
     clip_segment->start = 0;
     clip_segment->stop = -1;
     basesink->segment.start = 0;
     basesink->segment.stop = -1;
+    basesink->have_newsegment = TRUE;
+    GST_OBJECT_UNLOCK (basesink);
   }
 
   bclass = GST_BASE_SINK_GET_CLASS (basesink);
@@ -2973,48 +2977,55 @@ static gboolean
 gst_base_sink_negotiate_pull (GstBaseSink * basesink)
 {
   GstCaps *caps;
-  GstPad *pad;
+  gboolean result;
 
-  GST_OBJECT_LOCK (basesink);
+  result = FALSE;
-  pad = basesink->sinkpad;
-  gst_object_ref (pad);
-  GST_OBJECT_UNLOCK (basesink);
 
-  caps = gst_pad_get_allowed_caps (pad);
+  /* this returns the intersection between our caps and the peer caps. If there
-  if (gst_caps_is_empty (caps))
+   * is no peer, it returns NULL and we can't operate in pull mode so we can
+   * fail the negotiation. */
+  caps = gst_pad_get_allowed_caps (GST_BASE_SINK_PAD (basesink));
+  if (caps == NULL || gst_caps_is_empty (caps))
     goto no_caps_possible;
 
+  GST_DEBUG_OBJECT (basesink, "allowed caps: %" GST_PTR_FORMAT, caps);
+
   caps = gst_caps_make_writable (caps);
+  /* get the first (prefered) format */
   gst_caps_truncate (caps);
-  gst_pad_fixate_caps (pad, caps);
+  /* try to fixate */
+  gst_pad_fixate_caps (GST_BASE_SINK_PAD (basesink), caps);
+
+  GST_DEBUG_OBJECT (basesink, "fixated to: %" GST_PTR_FORMAT, caps);
 
   if (gst_caps_is_any (caps)) {
     GST_DEBUG_OBJECT (basesink, "caps were ANY after fixating, "
         "allowing pull()");
     /* neither side has template caps in this case, so they are prepared for
        pull() without setcaps() */
-  } else {
+    result = TRUE;
-    if (!gst_pad_set_caps (pad, caps))
+  } else if (gst_caps_is_fixed (caps)) {
+    if (!gst_pad_set_caps (GST_BASE_SINK_PAD (basesink), caps))
       goto could_not_set_caps;
+    result = TRUE;
   }
 
   gst_caps_unref (caps);
-  gst_object_unref (pad);
 
-  return TRUE;
+  return result;
 
 no_caps_possible:
   {
     GST_INFO_OBJECT (basesink, "Pipeline could not agree on caps");
     GST_DEBUG_OBJECT (basesink, "get_allowed_caps() returned EMPTY");
-    gst_object_unref (pad);
+    if (caps)
+      gst_caps_unref (caps);
     return FALSE;
   }
 could_not_set_caps:
   {
     GST_INFO_OBJECT (basesink, "Could not set caps: %" GST_PTR_FORMAT, caps);
     gst_caps_unref (caps);
-    gst_object_unref (pad);
     return FALSE;
   }
 }
@@ -3049,7 +3060,9 @@ gst_base_sink_pad_activate_pull (GstPad * pad, gboolean active)
           gst_segment_init (&basesink->segment, GST_FORMAT_UNDEFINED);
           gst_segment_init (basesink->abidata.ABI.clip_segment,
               GST_FORMAT_UNDEFINED);
+          GST_OBJECT_LOCK (basesink);
           basesink->have_newsegment = TRUE;
+          GST_OBJECT_UNLOCK (basesink);
 
           /* set the pad mode before starting the task so that it's in the
              correct state for the new thread. also the sink set_caps function
@@ -3232,17 +3245,17 @@ gst_base_sink_get_position (GstBaseSink * basesink, GstFormat format,
       if (G_UNLIKELY (basesink->eos))
         goto in_eos;
 
-      /* in PAUSE we cannot read from the clock so we
+      /* we can only get the segment when we are not NULL or READY */
-       * report time based on the last seen timestamp. */
+      if (!basesink->have_newsegment)
-      if (GST_STATE (basesink) == GST_STATE_PAUSED)
-        goto in_pause;
-
-      /* We get position from clock only in PLAYING, we checked
-       * the PAUSED case above, so this is check is to test 
-       * READY and NULL, where the position is always 0 */
-      if (GST_STATE (basesink) != GST_STATE_PLAYING)
         goto wrong_state;
 
+      /* when not in PLAYING or when we're busy with a state change, we
+       * cannot read from the clock so we report time based on the
+       * last seen timestamp. */
+      if (GST_STATE (basesink) != GST_STATE_PLAYING ||
+          GST_STATE_PENDING (basesink) != GST_STATE_VOID_PENDING)
+        goto in_pause;
+
       /* we need to sync on the clock. */
       if (basesink->sync == FALSE)
         goto no_sync;
@@ -3441,10 +3454,10 @@ gst_base_sink_change_state (GstElement * element, GstStateChange transition)
        * is no data flow in READY so we can safely assume we need to preroll. */
       GST_PAD_PREROLL_LOCK (basesink->sinkpad);
       GST_DEBUG_OBJECT (basesink, "READY to PAUSED");
+      basesink->have_newsegment = FALSE;
       gst_segment_init (&basesink->segment, GST_FORMAT_UNDEFINED);
       gst_segment_init (basesink->abidata.ABI.clip_segment,
           GST_FORMAT_UNDEFINED);
-      basesink->have_newsegment = FALSE;
       basesink->offset = 0;
       basesink->have_preroll = FALSE;
       basesink->need_preroll = TRUE;
@@ -3578,6 +3591,18 @@ gst_base_sink_change_state (GstElement * element, GstStateChange transition)
       break;
     case GST_STATE_CHANGE_PAUSED_TO_READY:
       GST_PAD_PREROLL_LOCK (basesink->sinkpad);
+      /* start by reseting our position state with the object lock so that the
+       * position query gets the right idea. We do this before we post the
+       * messages so that the message handlers pick this up. */
+      GST_OBJECT_LOCK (basesink);
+      basesink->have_newsegment = FALSE;
+      priv->current_sstart = -1;
+      priv->current_sstop = -1;
+      priv->have_latency = FALSE;
+      GST_OBJECT_UNLOCK (basesink);
+
+      gst_base_sink_set_last_buffer (basesink, NULL);
+
       if (!priv->commited) {
         if (priv->async_enabled) {
           GST_DEBUG_OBJECT (basesink, "PAUSED to READY, posting async-done");
@@ -3593,10 +3618,6 @@ gst_base_sink_change_state (GstElement * element, GstStateChange transition)
       } else {
         GST_DEBUG_OBJECT (basesink, "PAUSED to READY, don't need_preroll");
       }
-      priv->current_sstart = -1;
-      priv->current_sstop = -1;
-      priv->have_latency = FALSE;
-      gst_base_sink_set_last_buffer (basesink, NULL);
       GST_PAD_PREROLL_UNLOCK (basesink->sinkpad);
       break;
     case GST_STATE_CHANGE_READY_TO_NULL:

tests/check/elements/fakesink.c

@@ -523,17 +523,306 @@ GST_START_TEST (test_eos2)
 
 GST_END_TEST;
 
+/* test position reporting before, during and after flush 
+ * in PAUSED and PLAYING */
+GST_START_TEST (test_position)
+{
+  GstElement *pipeline, *sink;
+  GstPad *sinkpad;
+  GstStateChangeReturn ret;
+  gboolean qret;
+  GstFormat qformat;
+  gint64 qcur;
+  GstBuffer *buffer;
+  GstFlowReturn fret;
+  ChainData *data;
+  GstEvent *event;
+  gboolean eret;
+  gint i;
+
+  /* create sink */
+  pipeline = gst_pipeline_new ("pipeline");
+  fail_if (pipeline == NULL);
+
+  sink = gst_element_factory_make ("fakesink", "sink");
+  fail_if (sink == NULL);
+  g_object_set (G_OBJECT (sink), "sync", TRUE, NULL);
+  g_object_set (G_OBJECT (sink), "num-buffers", 2, NULL);
+
+  gst_bin_add (GST_BIN (pipeline), sink);
+
+  sinkpad = gst_element_get_static_pad (sink, "sink");
+  fail_if (sinkpad == NULL);
+
+  /* do position query, this should fail, we have nothing received yet */
+  qformat = GST_FORMAT_TIME;
+  qret = gst_element_query_position (sink, &qformat, &qcur);
+  fail_unless (qret == FALSE);
+
+  ret = gst_element_set_state (pipeline, GST_STATE_READY);
+  fail_unless (ret == GST_STATE_CHANGE_SUCCESS);
+
+  /* do position query, this should fail, we have nothing received yet */
+  qformat = GST_FORMAT_TIME;
+  qret = gst_element_query_position (sink, &qformat, &qcur);
+  fail_unless (qret == FALSE);
+
+  /* make pipeline and element ready to accept data */
+  ret = gst_element_set_state (pipeline, GST_STATE_PAUSED);
+  fail_unless (ret == GST_STATE_CHANGE_ASYNC);
+
+  /* do position query, this should fail, we have nothing received yet */
+  qformat = GST_FORMAT_TIME;
+  qret = gst_element_query_position (sink, &qformat, &qcur);
+  fail_unless (qret == FALSE);
+
+  /* send segment, this should work */
+  {
+    GST_DEBUG ("sending segment");
+    event = gst_event_new_new_segment (FALSE,
+        1.0, GST_FORMAT_TIME, 1 * GST_SECOND, 3 * GST_SECOND, 1 * GST_SECOND);
+
+    eret = gst_pad_send_event (sinkpad, event);
+    fail_if (eret == FALSE);
+  }
+
+  /* FIXME, do position query, this should succeed with the time value from the
+   * segment. */
+  qformat = GST_FORMAT_TIME;
+  qret = gst_element_query_position (sink, &qformat, &qcur);
+  fail_unless (qret == TRUE);
+  fail_unless (qcur == 1 * GST_SECOND);
+
+  /* send buffer that we will flush out */
+  buffer = gst_buffer_new ();
+  GST_BUFFER_TIMESTAMP (buffer) = 2 * GST_SECOND;
+  GST_BUFFER_DURATION (buffer) = 1 * GST_SECOND;
+
+  GST_DEBUG ("sending buffer");
+
+  /* this buffer causes the sink to preroll */
+  data = chain_async (sinkpad, buffer);
+  fail_if (data == NULL);
+
+  /* wait for preroll */
+  ret = gst_element_get_state (pipeline, NULL, NULL, -1);
+
+  /* do position query, this should succeed with the time value from the
+   * segment. */
+  qformat = GST_FORMAT_TIME;
+  qret = gst_element_query_position (sink, &qformat, &qcur);
+  fail_unless (qret == TRUE);
+  fail_unless (qcur == 1 * GST_SECOND);
+
+  /* start flushing, no timing is affected yet */
+  {
+    GST_DEBUG ("sending flush_start");
+    event = gst_event_new_flush_start ();
+
+    eret = gst_pad_send_event (sinkpad, event);
+    fail_if (eret == FALSE);
+  }
+
+  /* preroll buffer is flushed out */
+  fret = chain_async_return (data);
+  fail_unless (fret == GST_FLOW_WRONG_STATE);
+
+  /* do position query, this should succeed with the time value from the
+   * segment before the flush. */
+  qformat = GST_FORMAT_TIME;
+  qret = gst_element_query_position (sink, &qformat, &qcur);
+  fail_unless (qret == TRUE);
+  fail_unless (qcur == 1 * GST_SECOND);
+
+  /* stop flushing, timing is affected now */
+  {
+    GST_DEBUG ("sending flush_stop");
+    event = gst_event_new_flush_stop ();
+
+    eret = gst_pad_send_event (sinkpad, event);
+    fail_if (eret == FALSE);
+  }
+
+  /* do position query, this should fail, the segment is flushed */
+  qformat = GST_FORMAT_TIME;
+  qret = gst_element_query_position (sink, &qformat, &qcur);
+  fail_unless (qret == FALSE);
+
+  /* send segment, this should work */
+  {
+    GST_DEBUG ("sending segment");
+    event = gst_event_new_new_segment (FALSE,
+        1.0, GST_FORMAT_TIME, 2 * GST_SECOND, 4 * GST_SECOND, 1 * GST_SECOND);
+
+    eret = gst_pad_send_event (sinkpad, event);
+    fail_if (eret == FALSE);
+  }
+
+  /* send buffer that should return OK */
+  buffer = gst_buffer_new ();
+  GST_BUFFER_TIMESTAMP (buffer) = 3 * GST_SECOND;
+  GST_BUFFER_DURATION (buffer) = 1 * GST_SECOND;
+
+  GST_DEBUG ("sending buffer");
+
+  /* this buffer causes the sink to preroll */
+  data = chain_async (sinkpad, buffer);
+  fail_if (data == NULL);
+
+  /* wait for preroll */
+  ret = gst_element_get_state (pipeline, NULL, NULL, -1);
+
+  /* do position query, this should succeed with the time value from the
+   * segment. */
+  qformat = GST_FORMAT_TIME;
+  qret = gst_element_query_position (sink, &qformat, &qcur);
+  fail_unless (qret == TRUE);
+  fail_unless (qcur == 1 * GST_SECOND);
+
+  ret = gst_element_set_state (pipeline, GST_STATE_PLAYING);
+  fail_unless (ret == GST_STATE_CHANGE_SUCCESS);
+
+  /* position now is increasing but never exceeds the boundaries of the segment */
+  for (i = 0; i < 5; i++) {
+    qformat = GST_FORMAT_TIME;
+    qret = gst_element_query_position (sink, &qformat, &qcur);
+    GST_DEBUG ("position %" GST_TIME_FORMAT, GST_TIME_ARGS (qcur));
+    fail_unless (qret == TRUE);
+    fail_unless (qcur >= 1 * GST_SECOND && qcur <= 3 * GST_SECOND);
+    g_usleep (1000 * 250);
+  }
+
+  /* preroll buffer is rendered, we expect one more buffer after this one */
+  fret = chain_async_return (data);
+  fail_unless (fret == GST_FLOW_OK);
+
+  /* after rendering the position must be bigger then the stream_time of the
+   * buffer */
+  qformat = GST_FORMAT_TIME;
+  qret = gst_element_query_position (sink, &qformat, &qcur);
+  fail_unless (qret == TRUE);
+  fail_unless (qcur >= 2 * GST_SECOND && qcur <= 3 * GST_SECOND);
+
+  /* start flushing in PLAYING */
+  {
+    GST_DEBUG ("sending flush_start");
+    event = gst_event_new_flush_start ();
+
+    eret = gst_pad_send_event (sinkpad, event);
+    fail_if (eret == FALSE);
+  }
+
+  /* this should now just report the stream time of the last buffer */
+  qformat = GST_FORMAT_TIME;
+  qret = gst_element_query_position (sink, &qformat, &qcur);
+  fail_unless (qret == TRUE);
+  fail_unless (qcur == 2 * GST_SECOND);
+
+  {
+    GST_DEBUG ("sending flush_stop");
+    event = gst_event_new_flush_stop ();
+
+    eret = gst_pad_send_event (sinkpad, event);
+    fail_if (eret == FALSE);
+  }
+
+  /* do position query, this should fail, the segment is flushed */
+  qformat = GST_FORMAT_TIME;
+  qret = gst_element_query_position (sink, &qformat, &qcur);
+  fail_unless (qret == FALSE);
+
+  /* send segment, this should work */
+  {
+    GST_DEBUG ("sending segment");
+    event = gst_event_new_new_segment (FALSE,
+        1.0, GST_FORMAT_TIME, 2 * GST_SECOND, 4 * GST_SECOND, 1 * GST_SECOND);
+
+    eret = gst_pad_send_event (sinkpad, event);
+    fail_if (eret == FALSE);
+  }
+
+  /* send buffer that should return UNEXPECTED */
+  buffer = gst_buffer_new ();
+  GST_BUFFER_TIMESTAMP (buffer) = 3 * GST_SECOND;
+  GST_BUFFER_DURATION (buffer) = 1 * GST_SECOND;
+
+  GST_DEBUG ("sending buffer");
+
+  /* this buffer causes the sink to preroll */
+  data = chain_async (sinkpad, buffer);
+  fail_if (data == NULL);
+
+  /* wait for preroll */
+  ret = gst_element_get_state (pipeline, NULL, NULL, -1);
+
+  /* preroll buffer is rendered, we expect no more buffer after this one */
+  fret = chain_async_return (data);
+  fail_unless (fret == GST_FLOW_UNEXPECTED);
+
+  /* do position query, this should succeed with the stream time of the buffer
+   * against the clock. Since the buffer is synced against the clock, the time
+   * should be at least the stream time of the buffer. */
+  qformat = GST_FORMAT_TIME;
+  qret = gst_element_query_position (sink, &qformat, &qcur);
+  fail_unless (qret == TRUE);
+  fail_unless (qcur >= 2 * GST_SECOND && qcur <= 3 * GST_SECOND);
+
+  /* wait 2 more seconds, enough to test if the position was clipped correctly
+   * against the segment */
+  g_usleep (2 * G_USEC_PER_SEC);
+
+  qformat = GST_FORMAT_TIME;
+  qret = gst_element_query_position (sink, &qformat, &qcur);
+  fail_unless (qret == TRUE);
+  fail_unless (qcur == 3 * GST_SECOND);
+
+  GST_DEBUG ("going to PAUSED");
+
+  ret = gst_element_set_state (pipeline, GST_STATE_PAUSED);
+  fail_unless (ret == GST_STATE_CHANGE_ASYNC);
+
+  /* we report the time of the last start of the buffer. This is slightly
+   * incorrect, we should report the exact time when we paused but there is no
+   * record of that anywhere */
+  qformat = GST_FORMAT_TIME;
+  qret = gst_element_query_position (sink, &qformat, &qcur);
+  fail_unless (qret == TRUE);
+  fail_unless (qcur == 2 * GST_SECOND);
+
+  ret = gst_element_set_state (pipeline, GST_STATE_READY);
+  fail_unless (ret == GST_STATE_CHANGE_SUCCESS);
+
+  /* fails again because we are in the wrong state */
+  qformat = GST_FORMAT_TIME;
+  qret = gst_element_query_position (sink, &qformat, &qcur);
+  fail_unless (qret == FALSE);
+
+  gst_element_set_state (pipeline, GST_STATE_NULL);
+
+  qformat = GST_FORMAT_TIME;
+  qret = gst_element_query_position (sink, &qformat, &qcur);
+  fail_unless (qret == FALSE);
+
+  gst_object_unref (sinkpad);
+  gst_object_unref (pipeline);
+}
+
+GST_END_TEST;
+
 static Suite *
 fakesink_suite (void)
 {
   Suite *s = suite_create ("fakesink");
   TCase *tc_chain = tcase_create ("general");
 
+  tcase_set_timeout (tc_chain, 20);
+
   suite_add_tcase (s, tc_chain);
   tcase_add_test (tc_chain, test_clipping);
   tcase_add_test (tc_chain, test_preroll_sync);
   tcase_add_test (tc_chain, test_eos);
   tcase_add_test (tc_chain, test_eos2);
+  tcase_add_test (tc_chain, test_position);
 
   return s;
 }

tests/check/generic/sinks.c

@@ -1079,8 +1079,8 @@ GST_START_TEST (test_async_done)
   format = GST_FORMAT_TIME;
   position = -1;
   qret = gst_element_query_position (sink, &format, &position);
-  fail_unless (qret == FALSE, "position wrong");
+  fail_unless (qret == TRUE, "position wrong");
-  fail_unless (position == -1, "position is wrong");
+  fail_unless (position == 10 * GST_SECOND, "position is wrong");
 
   /* Since we are paused and the preroll queue has a length of 2, this function
    * will return immediatly, the preroll handoff will be called and the stream
@@ -1217,8 +1217,8 @@ GST_START_TEST (test_async_done_eos)
   format = GST_FORMAT_TIME;
   position = -1;
   qret = gst_element_query_position (sink, &format, &position);
-  fail_unless (qret == FALSE, "position wrong");
+  fail_unless (qret == TRUE, "position wrong");
-  fail_unless (position == -1, "position is wrong");
+  fail_unless (position == 10 * GST_SECOND, "position is wrong");
 
   /* Since we are paused and the preroll queue has a length of 1, this function
    * will return immediatly. The EOS will complete the preroll and the