bin: undo upward state changes on children when a child fails

When a bin changes states upwards, and a child fails to change,
any child that was already switched will not be reset to its
original state, leaving its state inconsistent with the bin,
which does not change state due to the failure.

If the state change was from NULL to READY, it means that deleting
this bin will cause those children to be deleted while not in
NULL state, which is a Bad Thing. For other upward changes, it
is less of a problem, as a subsequent switch back to NULL will
cause an actual downwards change on those inconsistent elements,
albeit from the "wrong" state.

We now reset state to the original one when a child fails.

Includes unit test.

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

diff --git a/gst/gstbin.c b/gst/gstbin.c
index 28b0436..cdd585a 100644 (file)
--- a/gst/gstbin.c
+++ b/gst/gstbin.c
@@ -2540,6 +2540,17 @@ gst_bin_post_message (GstElement * element, GstMessage * msg)
   return ret;
 }
 
+static void
+reset_state (const GValue * data, gpointer user_data)
+{
+  GstElement *e = g_value_get_object (data);
+  GstState state = GPOINTER_TO_INT (user_data);
+
+  if (gst_element_set_state (e, state) == GST_STATE_CHANGE_FAILURE)
+    GST_WARNING_OBJECT (e, "Failed to switch back down to %s",
+        gst_element_state_get_name (state));
+}
+
 static GstStateChangeReturn
 gst_bin_change_state_func (GstElement * element, GstStateChange transition)
 {
@@ -2696,7 +2707,7 @@ restart:
             if (parent == GST_OBJECT_CAST (element)) {
               /* element is still in bin, really error now */
               gst_object_unref (parent);
-              goto done;
+              goto undo;
             }
             /* child removed from bin, let the resync code redo the state
              * change */
@@ -2809,6 +2820,24 @@ activate_failure:
         "failure (de)activating src pads");
     return GST_STATE_CHANGE_FAILURE;
   }
+
+undo:
+  {
+    if (current < next) {
+      GstIterator *it = gst_bin_iterate_sorted (GST_BIN (element));
+      GstIteratorResult ret;
+
+      GST_DEBUG_OBJECT (element,
+          "Bin failed to change state, switching children back to %s",
+          gst_element_state_get_name (current));
+      do {
+        ret =
+            gst_iterator_foreach (it, &reset_state, GINT_TO_POINTER (current));
+      } while (ret == GST_ITERATOR_RESYNC);
+      gst_iterator_free (it);
+    }
+    goto done;
+  }
 }
 
 /*

diff --git a/tests/check/generic/states.c b/tests/check/generic/states.c
index 251f433..fb37523 100644 (file)
--- a/tests/check/generic/states.c
+++ b/tests/check/generic/states.c
@@ -205,6 +205,66 @@ GST_START_TEST (test_state_changes_down_seq)
 
 GST_END_TEST;
 
+static gboolean
+element_state_is (GstElement * e, GstState s)
+{
+  GstStateChangeReturn ret;
+  GstState state;
+
+  ret = gst_element_get_state (e, &state, NULL, GST_CLOCK_TIME_NONE);
+  return (ret == GST_STATE_CHANGE_SUCCESS && state == s);
+}
+
+GST_START_TEST (test_state_changes_up_failure)
+{
+  GstElement *bin;
+  GstElement *mid[3];
+  int n;
+
+  /* we want at least one before and one after */
+  g_assert (G_N_ELEMENTS (mid) >= 3);
+
+  /* make a bin */
+  bin = gst_element_factory_make ("bin", NULL);
+
+  /* add children */
+  for (n = 0; n < G_N_ELEMENTS (mid); ++n) {
+    const char *element = n != 1 ? "identity" : "fakesink";
+    mid[n] = gst_element_factory_make (element, NULL);
+    gst_bin_add (GST_BIN (bin), mid[n]);
+    if (n == 1)
+      g_object_set (mid[n], "async", FALSE, NULL);
+  }
+
+  /* This one should work */
+  for (n = 0; n < G_N_ELEMENTS (mid); ++n)
+    fail_unless (element_state_is (mid[n], GST_STATE_NULL));
+  gst_element_set_state (bin, GST_STATE_READY);
+  for (n = 0; n < G_N_ELEMENTS (mid); ++n)
+    fail_unless (element_state_is (mid[n], GST_STATE_READY));
+  gst_element_set_state (bin, GST_STATE_NULL);
+  for (n = 0; n < G_N_ELEMENTS (mid); ++n)
+    fail_unless (element_state_is (mid[n], GST_STATE_NULL));
+
+  /* make the middle element fail to switch up */
+  g_object_set (mid[1], "state-error", 1 /* null-to-ready */ , NULL);
+
+  /* This one should not */
+  for (n = 0; n < G_N_ELEMENTS (mid); ++n)
+    fail_unless (element_state_is (mid[n], GST_STATE_NULL));
+  gst_element_set_state (bin, GST_STATE_READY);
+  for (n = 0; n < G_N_ELEMENTS (mid); ++n)
+    fail_unless (element_state_is (mid[n], GST_STATE_NULL));
+  gst_element_set_state (bin, GST_STATE_NULL);
+  for (n = 0; n < G_N_ELEMENTS (mid); ++n)
+    fail_unless (element_state_is (mid[n], GST_STATE_NULL));
+
+  /* cleanup */
+  gst_object_unref (bin);
+}
+
+GST_END_TEST;
+
 
 static Suite *
 states_suite (void)
@@ -217,6 +277,7 @@ states_suite (void)
   tcase_add_test (tc_chain, test_state_changes_up_and_down_seq);
   tcase_add_test (tc_chain, test_state_changes_up_seq);
   tcase_add_test (tc_chain, test_state_changes_down_seq);
+  tcase_add_test (tc_chain, test_state_changes_up_failure);
 
   return s;
 }