pad: gst_pad_activate_mode() always succeed if same mode

Checking that the pad is in the correct mode before the parent is
checked makes the call always succeed if the mode is ok.

This fixes a race with ghostpad where gst_pad_activate_mode() could
trigger a g_critical() if the ghostpad is unparented while the
proxypad is deactivating, for instance if the ghostpad is released.
More specifically, gst_ghost_pad_internal_activate_push_default()'s
call to gst_pad_activate_mode() would fail if ghostpad doesn't have a
parent. With this patch it will return true of mode is already
correct.

diff --git a/gst/gstpad.c b/gst/gstpad.c
index 5cb3d1b..48dde24 100644 (file)
--- a/gst/gstpad.c
+++ b/gst/gstpad.c
@@ -1303,11 +1303,19 @@ gst_pad_activate_mode (GstPad * pad, GstPadMode mode, gboolean active)
 {
   GstObject *parent;
   gboolean res;
+  GstPadMode old, new;
 
   g_return_val_if_fail (GST_IS_PAD (pad), FALSE);
 
   GST_OBJECT_LOCK (pad);
+
+  old = GST_PAD_MODE (pad);
+  new = active ? mode : GST_PAD_MODE_NONE;
+  if (old == new)
+    goto was_ok;
+
   ACQUIRE_PARENT (pad, parent, no_parent);
+
   GST_OBJECT_UNLOCK (pad);
 
   res = activate_mode_internal (pad, parent, mode, active);
@@ -1316,6 +1324,13 @@ gst_pad_activate_mode (GstPad * pad, GstPadMode mode, gboolean active)
 
   return res;
 
+was_ok:
+  {
+    GST_OBJECT_UNLOCK (pad);
+    GST_CAT_DEBUG_OBJECT (GST_CAT_PADS, pad, "already %s in %s mode",
+        active ? "activated" : "deactivated", gst_pad_mode_get_name (mode));
+    return TRUE;
+  }
 no_parent:
   {
     GST_WARNING_OBJECT (pad, "no parent");

diff --git a/tests/check/gst/gstghostpad.c b/tests/check/gst/gstghostpad.c
index 94ec14f..6c6f518 100644 (file)
--- a/tests/check/gst/gstghostpad.c
+++ b/tests/check/gst/gstghostpad.c
@@ -1322,6 +1322,7 @@ GST_START_TEST (test_activate_sink_switch_mode)
 GST_END_TEST;
 
 static gboolean thread_running;
+
 static gpointer
 send_query_to_pad_func (GstPad * pad)
 {
@@ -1366,6 +1367,38 @@ GST_START_TEST (test_stress_upstream_queries_while_tearing_down)
 
 GST_END_TEST;
 
+GST_START_TEST (test_deactivate_already_deactive_with_no_parent)
+{
+  /* This simulates the behavior where a ghostpad is released while
+   * deactivating (for instance because of a state change).
+   * gst_pad_activate_mode() may be be called from
+   * gst_ghost_pad_internal_activate_push_default() on a pad that is already
+   * deactivate and unparented. The call chain is really like somethink like
+   * this:
+   *   gst_pad_activate_mode(ghostpad)
+   *    -> ...
+   *    -> gst_pad_activate_mode(proxypad)
+   *    -> ...
+   *    -> gst_pad_activate_mode(ghostpad)
+   */
+  GstElement *bin = gst_bin_new ("testbin");
+  GstPad *pad = gst_ghost_pad_new_no_target ("src", GST_PAD_SRC);
+  gst_object_ref (pad);
+
+  /* We need to add/remove pad because that will update the pad's flags */
+  fail_unless (gst_element_add_pad (bin, pad));
+  fail_unless (gst_element_remove_pad (bin, pad));
+
+  /* Setting a pad that's already deactive to deactive should not fail. */
+  fail_if (gst_pad_is_active (pad));
+  fail_unless (gst_pad_activate_mode (pad, GST_PAD_MODE_PUSH, FALSE));
+
+  gst_object_unref (bin);
+  gst_object_unref (pad);
+}
+
+GST_END_TEST;
+
 static Suite *
 gst_ghost_pad_suite (void)
 {
@@ -1396,6 +1429,7 @@ gst_ghost_pad_suite (void)
   tcase_add_test (tc_chain, test_activate_sink_and_src);
   tcase_add_test (tc_chain, test_activate_src_pull_mode);
   tcase_add_test (tc_chain, test_activate_sink_switch_mode);
+  tcase_add_test (tc_chain, test_deactivate_already_deactive_with_no_parent);
   tcase_add_test (tc_chain, test_stress_upstream_queries_while_tearing_down);
 
   return s;