GstBaseSink: avoid calling preroll multiple times

Fix a regression introduced by fix for #567725 in commit
1c7ab4ed4f19b63ba046a6f2fe7d09a6c17357c5. We should only call the preroll
function once namely when we did not yet commit the state change.

Add a unit test to check that we call the preroll function when interrupting the
clock_wait (see #567725).

Add a unit test to check that we only call the preroll function once.

diff --git a/libs/gst/base/gstbasesink.c b/libs/gst/base/gstbasesink.c
index 74488db..5fe7569 100644 (file)
--- a/libs/gst/base/gstbasesink.c
+++ b/libs/gst/base/gstbasesink.c
@@ -883,17 +883,21 @@ gst_base_sink_set_last_buffer (GstBaseSink * sink, GstBuffer * buffer)
 {
   GstBuffer *old;
 
-  if (buffer)
-    gst_buffer_ref (buffer);
-
-  GST_DEBUG_OBJECT (sink, "setting last buffer to %p", buffer);
-
   GST_OBJECT_LOCK (sink);
   old = sink->priv->last_buffer;
-  sink->priv->last_buffer = buffer;
+  if (G_LIKELY (old != buffer)) {
+    GST_DEBUG_OBJECT (sink, "setting last buffer to %p", buffer);
+    if (G_LIKELY (buffer))
+      gst_buffer_ref (buffer);
+    sink->priv->last_buffer = buffer;
+  } else {
+    old = NULL;
+  }
   GST_OBJECT_UNLOCK (sink);
 
-  if (old)
+  /* avoid unreffing with the lock because cleanup code might want to take the
+   * lock too */
+  if (G_LIKELY (old))
     gst_buffer_unref (old);
 }
 
@@ -2446,10 +2450,10 @@ gst_base_sink_preroll_object (GstBaseSink * basesink, GstMiniObject * obj)
 {
   GstFlowReturn ret;
 
-  GST_DEBUG_OBJECT (basesink, "do preroll %p", obj);
+  GST_DEBUG_OBJECT (basesink, "prerolling object %p", obj);
 
   /* if it's a buffer, we need to call the preroll method */
-  if (G_LIKELY (GST_IS_BUFFER (obj))) {
+  if (G_LIKELY (GST_IS_BUFFER (obj)) && !basesink->priv->commited) {
     GstBaseSinkClass *bclass;
     GstBuffer *buf;
     GstClockTime timestamp;

diff --git a/tests/check/elements/fakesink.c b/tests/check/elements/fakesink.c
index c1ec8bf..c05cc08 100644 (file)
--- a/tests/check/elements/fakesink.c
+++ b/tests/check/elements/fakesink.c
@@ -218,6 +218,15 @@ GST_START_TEST (test_clipping)
 
 GST_END_TEST;
 
+static gint num_preroll = 0;
+
+static void
+preroll_count (GstElement * sink)
+{
+  num_preroll++;
+  GST_DEBUG ("got preroll handoff %d", num_preroll);
+}
+
 GST_START_TEST (test_preroll_sync)
 {
   GstElement *pipeline, *sink;
@@ -231,6 +240,10 @@ GST_START_TEST (test_preroll_sync)
   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), "signal-handoffs", TRUE, NULL);
+  g_signal_connect (sink, "preroll-handoff", G_CALLBACK (preroll_count), NULL);
+
+  fail_unless (num_preroll == 0);
 
   gst_bin_add (GST_BIN (pipeline), sink);
 
@@ -248,7 +261,7 @@ GST_START_TEST (test_preroll_sync)
 
     GST_DEBUG ("sending segment");
     segment = gst_event_new_new_segment (FALSE,
-        1.0, GST_FORMAT_TIME, 0 * GST_SECOND, 2 * GST_SECOND, 0 * GST_SECOND);
+        1.0, GST_FORMAT_TIME, 0 * GST_SECOND, 102 * GST_SECOND, 0 * GST_SECOND);
 
     eret = gst_pad_send_event (sinkpad, segment);
     fail_if (eret == FALSE);
@@ -277,6 +290,8 @@ GST_START_TEST (test_preroll_sync)
     fail_unless (current == GST_STATE_PAUSED);
     fail_unless (pending == GST_STATE_VOID_PENDING);
 
+    fail_unless (num_preroll == 1);
+
     /* playing should render the buffer */
     ret = gst_element_set_state (pipeline, GST_STATE_PLAYING);
     fail_unless (ret == GST_STATE_CHANGE_SUCCESS);
@@ -284,6 +299,40 @@ GST_START_TEST (test_preroll_sync)
     /* and we should get a success return value */
     fret = chain_async_return (data);
     fail_if (fret != GST_FLOW_OK);
+
+    /* now we are playing no new preroll was done */
+    fail_unless (num_preroll == 1);
+
+    buffer = gst_buffer_new ();
+    /* far in the future to make sure we block */
+    GST_BUFFER_TIMESTAMP (buffer) = 100 * GST_SECOND;
+    GST_BUFFER_DURATION (buffer) = 100 * GST_SECOND;
+    data = chain_async (sinkpad, buffer);
+    fail_if (data == NULL);
+
+    g_usleep (1000000);
+
+    /* pause again. Since the buffer has a humongous timestamp we likely
+     * interrupt the clock_wait and we should preroll on this buffer again */
+    ret = gst_element_set_state (pipeline, GST_STATE_PAUSED);
+    fail_unless (ret == GST_STATE_CHANGE_ASYNC);
+
+    ret =
+        gst_element_get_state (pipeline, &current, &pending,
+        GST_CLOCK_TIME_NONE);
+    fail_unless (ret == GST_STATE_CHANGE_SUCCESS);
+    fail_unless (current == GST_STATE_PAUSED);
+    fail_unless (pending == GST_STATE_VOID_PENDING);
+
+    fail_unless (num_preroll == 2);
+
+    /* shutdown */
+    ret = gst_element_set_state (pipeline, GST_STATE_READY);
+    fail_unless (ret == GST_STATE_CHANGE_SUCCESS);
+
+    /* should be wrong state now */
+    fret = chain_async_return (data);
+    fail_if (fret != GST_FLOW_WRONG_STATE);
   }
   gst_element_set_state (pipeline, GST_STATE_NULL);
   gst_element_get_state (pipeline, NULL, NULL, GST_CLOCK_TIME_NONE);