bin: try harder to avoid state changes in wrong direction

When the bin does an upward state change, try to avoid doing a downward state
change on the child and vice versa.
Add some more unit tests for this fix.

Fixes https://bugzilla.gnome.org/show_bug.cgi?id=621833

diff --git a/gst/gstbin.c b/gst/gstbin.c
index 665259e..d5ce5f6 100644 (file)
--- a/gst/gstbin.c
+++ b/gst/gstbin.c
@@ -2152,7 +2152,7 @@ gst_bin_element_set_state (GstBin * bin, GstElement * element,
     GstState next)
 {
   GstStateChangeReturn ret;
-  GstState pending, child_current, child_pending;
+  GstState child_current, child_pending;
   gboolean locked;
   GList *found;
 
@@ -2182,17 +2182,76 @@ gst_bin_element_set_state (GstBin * bin, GstElement * element,
     goto no_preroll;
   }
 
-  GST_OBJECT_LOCK (bin);
-  pending = GST_STATE_PENDING (bin);
+  GST_CAT_INFO_OBJECT (GST_CAT_STATES, element,
+      "current %s pending %s, desired next %s",
+      gst_element_state_get_name (child_current),
+      gst_element_state_get_name (child_pending),
+      gst_element_state_get_name (next));
 
   /* Try not to change the state of elements that are already in the state we're
    * going to */
-  if (!(next == GST_STATE_PLAYING || child_pending != GST_STATE_VOID_PENDING ||
-          (child_pending == GST_STATE_VOID_PENDING &&
-              ((pending > child_current && next > child_current) ||
-                  (pending < child_current && next < child_current)))))
+  if (child_current == next && child_pending == GST_STATE_VOID_PENDING) {
+    /* child is already at the requested state, return previous return. Note that
+     * if the child has a pending state to next, we will still call the
+     * set_state function */
     goto unneeded;
+  } else if (next > current) {
+    /* upward state change */
+    if (child_pending == GST_STATE_VOID_PENDING) {
+      /* .. and the child is not busy doing anything */
+      if (child_current > next) {
+        /* .. and is already past the requested state, assume it got there
+         * without error */
+        ret = GST_STATE_CHANGE_SUCCESS;
+        goto unneeded;
+      }
+    } else if (child_pending > child_current) {
+      /* .. and the child is busy going upwards */
+      if (child_current >= next) {
+        /* .. and is already past the requested state, assume it got there
+         * without error */
+        ret = GST_STATE_CHANGE_SUCCESS;
+        goto unneeded;
+      }
+    } else {
+      /* .. and the child is busy going downwards */
+      if (child_current > next) {
+        /* .. and is already past the requested state, assume it got there
+         * without error */
+        ret = GST_STATE_CHANGE_SUCCESS;
+        goto unneeded;
+      }
+    }
+  } else if (next < current) {
+    /* downward state change */
+    if (child_pending == GST_STATE_VOID_PENDING) {
+      /* .. and the child is not busy doing anything */
+      if (child_current < next) {
+        /* .. and is already past the requested state, assume it got there
+         * without error */
+        ret = GST_STATE_CHANGE_SUCCESS;
+        goto unneeded;
+      }
+    } else if (child_pending < child_current) {
+      /* .. and the child is busy going downwards */
+      if (child_current <= next) {
+        /* .. and is already past the requested state, assume it got there
+         * without error */
+        ret = GST_STATE_CHANGE_SUCCESS;
+        goto unneeded;
+      }
+    } else {
+      /* .. and the child is busy going upwards */
+      if (child_current < next) {
+        /* .. and is already past the requested state, assume it got there
+         * without error */
+        ret = GST_STATE_CHANGE_SUCCESS;
+        goto unneeded;
+      }
+    }
+  }
 
+  GST_OBJECT_LOCK (bin);
   /* the element was busy with an upwards async state change, we must wait for
    * an ASYNC_DONE message before we attemp to change the state. */
   if ((found =
@@ -2234,25 +2293,22 @@ locked:
     GST_STATE_UNLOCK (element);
     return ret;
   }
-was_busy:
-  {
-    GST_DEBUG_OBJECT (element, "element was busy, delaying state change");
-    GST_OBJECT_UNLOCK (bin);
-    GST_STATE_UNLOCK (element);
-    return GST_STATE_CHANGE_ASYNC;
-  }
 unneeded:
   {
     GST_CAT_INFO_OBJECT (GST_CAT_STATES, element,
-        "skipping transition from %s to  %s, since bin pending"
-        " is %s : last change state return follows",
+        "skipping transition from %s to  %s",
         gst_element_state_get_name (child_current),
-        gst_element_state_get_name (next),
-        gst_element_state_get_name (pending));
-    GST_OBJECT_UNLOCK (bin);
+        gst_element_state_get_name (next));
     GST_STATE_UNLOCK (element);
     return ret;
   }
+was_busy:
+  {
+    GST_DEBUG_OBJECT (element, "element was busy, delaying state change");
+    GST_OBJECT_UNLOCK (bin);
+    GST_STATE_UNLOCK (element);
+    return GST_STATE_CHANGE_ASYNC;
+  }
 }
 
 /* gst_iterator_fold functions for pads_activate

diff --git a/tests/check/gst/gstbin.c b/tests/check/gst/gstbin.c
index b028958..9fb8208 100644 (file)
--- a/tests/check/gst/gstbin.c
+++ b/tests/check/gst/gstbin.c
@@ -1158,6 +1158,125 @@ GST_START_TEST (test_many_bins)
 
 GST_END_TEST;
 
+static void
+fakesrc_pad_blocked_cb (GstPad * pad, gboolean blocked, void *arg)
+{
+  GstPipeline *pipeline = (GstPipeline *) arg;
+  GstElement *src, *sink;
+  GstPad *srcpad;
+
+  if (!blocked) {
+    /* Not interested in unblocking, ignore that... */
+    return;
+  }
+
+  src = gst_bin_get_by_name (GST_BIN (pipeline), "fakesrc");
+  fail_unless (src != NULL, "Could not get fakesrc");
+
+  sink = gst_element_factory_make ("fakesink", "fakesink");
+  fail_unless (sink != NULL, "Could not create fakesink");
+
+  g_object_set (sink, "state-error", 1, NULL);
+  gst_bin_add (GST_BIN (pipeline), sink);
+
+  gst_element_link (src, sink);
+  gst_element_sync_state_with_parent (sink);
+
+  srcpad = gst_element_get_static_pad (src, "src");
+  gst_pad_set_blocked_async (srcpad, FALSE, fakesrc_pad_blocked_cb, pipeline);
+  gst_object_unref (srcpad);
+
+  gst_object_unref (src);
+}
+
+GST_START_TEST (test_state_failure_unref)
+{
+  GstElement *src, *pipeline;
+  GstPad *srcpad;
+  GstBus *bus;
+  GstStateChangeReturn ret;
+  GstMessage *msg;
+
+  pipeline = gst_pipeline_new (NULL);
+  fail_unless (pipeline != NULL, "Could not create pipeline");
+
+  src = gst_element_factory_make ("fakesrc", "fakesrc");
+  fail_unless (src != NULL, "Could not create fakesrc");
+
+  srcpad = gst_element_get_static_pad (src, "src");
+  fail_unless (srcpad != NULL, "Could not get fakesrc srcpad");
+
+  gst_pad_set_blocked_async (srcpad, TRUE, fakesrc_pad_blocked_cb, pipeline);
+  gst_object_unref (srcpad);
+
+  gst_bin_add (GST_BIN (pipeline), src);
+
+  bus = gst_pipeline_get_bus (GST_PIPELINE (pipeline));
+  fail_unless (bus != NULL, "Could not get bus");
+
+  gst_element_set_state (pipeline, GST_STATE_PLAYING);
+
+  /* Wait for an error message from our fakesink (added from the
+     pad block callback). */
+  msg = gst_bus_poll (bus, GST_MESSAGE_ERROR, GST_SECOND);
+  fail_if (msg == NULL, "No error message within 1 second");
+  gst_message_unref (msg);
+
+  /* Check that after this failure, we can still stop, and then unref, the
+     pipeline. This should always be possible. */
+  ret = gst_element_set_state (pipeline, GST_STATE_NULL);
+  fail_unless (ret == GST_STATE_CHANGE_SUCCESS, "downward state change failed");
+
+  gst_object_unref (bus);
+  gst_object_unref (pipeline);
+}
+
+GST_END_TEST;
+
+static void
+on_sync_bus_error (GstBus * bus, GstMessage * msg)
+{
+  fail_if (msg != NULL);
+}
+
+GST_START_TEST (test_state_change_skip)
+{
+  GstElement *sink, *pipeline;
+  GstStateChangeReturn ret;
+  GstBus *bus;
+
+  pipeline = gst_pipeline_new (NULL);
+  fail_unless (pipeline != NULL, "Could not create pipeline");
+
+  bus = gst_pipeline_get_bus (GST_PIPELINE (pipeline));
+  fail_unless (bus != NULL, "Could not get bus");
+
+  /* no errors */
+  gst_bus_enable_sync_message_emission (bus);
+  g_signal_connect (bus, "sync-message::error", (GCallback) on_sync_bus_error,
+      NULL);
+
+  sink = gst_element_factory_make ("fakesink", "fakesink");
+  fail_unless (sink != NULL, "Could not create fakesink");
+  gst_element_set_state (sink, GST_STATE_PAUSED);
+
+  g_object_set (sink, "state-error", 5, NULL);
+
+  gst_bin_add (GST_BIN (pipeline), sink);
+  gst_element_set_state (pipeline, GST_STATE_PLAYING);
+
+  g_object_set (sink, "state-error", 0, NULL);
+
+  /* Check that after this failure, we can still stop, and then unref, the
+     pipeline. This should always be possible. */
+  ret = gst_element_set_state (pipeline, GST_STATE_NULL);
+  fail_unless (ret == GST_STATE_CHANGE_SUCCESS, "downward state change failed");
+
+  gst_object_unref (pipeline);
+}
+
+GST_END_TEST;
+
 static Suite *
 gst_bin_suite (void)
 {
@@ -1181,6 +1300,8 @@ gst_bin_suite (void)
   tcase_add_test (tc_chain, test_iterate_sorted);
   tcase_add_test (tc_chain, test_link_structure_change);
   tcase_add_test (tc_chain, test_state_failure_remove);
+  tcase_add_test (tc_chain, test_state_failure_unref);
+  tcase_add_test (tc_chain, test_state_change_skip);
 
   /* fails on OSX build bot for some reason, and is a bit silly anyway */
   if (0)