pad: gst_pad_activate_mode() always succeed if same mode
authorStian Selnes <stian@pexip.com>
Fri, 18 Aug 2017 12:30:32 +0000 (14:30 +0200)
committerTim-Philipp Müller <tim@centricular.com>
Fri, 24 Nov 2017 12:40:31 +0000 (13:40 +0100)
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.

gst/gstpad.c
tests/check/gst/gstghostpad.c

index 5cb3d1b..48dde24 100644 (file)
@@ -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");
index 94ec14f..6c6f518 100644 (file)
@@ -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;