check/gst/gstbin.c: Added checks for hierarchy consistency whan adding linked elements to bins.

Original commit message from CVS:
* check/gst/gstbin.c: (GST_START_TEST), (gst_bin_suite):
Added checks for hierarchy consistency whan adding linked
elements to bins.

* check/gst/gstelement.c: (GST_START_TEST), (gst_element_suite):
Added check to test element scheduling without bin/pipeline.

* check/pipelines/simple_launch_lines.c: (GST_START_TEST):
First add elements to bin, then link.

* gst/gstbin.c: (unlink_pads), (gst_bin_add_func),
(gst_bin_remove_func):
Unlink pads from elements added/removed from bin to maintain
hierarchy consistency.

ChangeLog

@@ -1,3 +1,20 @@
+2005-07-28  Wim Taymans  <wim@fluendo.com>
+
+	* check/gst/gstbin.c: (GST_START_TEST), (gst_bin_suite):
+	Added checks for hierarchy consistency whan adding linked
+	elements to bins.
+
+	* check/gst/gstelement.c: (GST_START_TEST), (gst_element_suite):
+	Added check to test element scheduling without bin/pipeline.
+
+	* check/pipelines/simple_launch_lines.c: (GST_START_TEST):
+	First add elements to bin, then link.
+	
+	* gst/gstbin.c: (unlink_pads), (gst_bin_add_func),
+	(gst_bin_remove_func):
+	Unlink pads from elements added/removed from bin to maintain
+	hierarchy consistency.
+
 2005-07-28  Ronald S. Bultje  <rbultje@ronald.bitfreak.net>
 
 	* gst/base/gstbasetransform.c: (gst_base_transform_setcaps),

check/gst/gstbin.c

@@ -270,6 +270,56 @@ GST_START_TEST (test_message_state_changed_children)
 
 GST_END_TEST;
 
+/* adding an element with linked pads to a bin unlinks the
+ * pads */
+GST_START_TEST (test_add_linked)
+{
+  GstElement *src, *sink;
+  GstPad *srcpad, *sinkpad;
+  GstElement *pipeline;
+
+  pipeline = gst_pipeline_new (NULL);
+  fail_unless (pipeline != NULL, "Could not create pipeline");
+
+  src = gst_element_factory_make ("fakesrc", NULL);
+  fail_if (src == NULL, "Could not create fakesrc");
+  sink = gst_element_factory_make ("fakesink", NULL);
+  fail_if (src == NULL, "Could not create fakesink");
+
+  srcpad = gst_element_get_pad (src, "src");
+  fail_unless (srcpad != NULL);
+  sinkpad = gst_element_get_pad (sink, "sink");
+  fail_unless (sinkpad != NULL);
+
+  fail_unless (gst_pad_link (srcpad, sinkpad) == GST_PAD_LINK_OK);
+
+  /* pads are linked now */
+  fail_unless (gst_pad_is_linked (srcpad));
+  fail_unless (gst_pad_is_linked (sinkpad));
+
+  /* adding element to bin voids hierarchy so pads are unlinked */
+  gst_bin_add (GST_BIN (pipeline), src);
+
+  /* check if pads really are unlinked */
+  fail_unless (!gst_pad_is_linked (srcpad));
+  fail_unless (!gst_pad_is_linked (sinkpad));
+
+  /* cannot link pads in wrong hierarchy */
+  fail_unless (gst_pad_link (srcpad, sinkpad) == GST_PAD_LINK_WRONG_HIERARCHY);
+
+  /* adding other element to bin as well */
+  gst_bin_add (GST_BIN (pipeline), sink);
+
+  /* now we can link again */
+  fail_unless (gst_pad_link (srcpad, sinkpad) == GST_PAD_LINK_OK);
+
+  /* check if pads really are linked */
+  fail_unless (gst_pad_is_linked (srcpad));
+  fail_unless (gst_pad_is_linked (sinkpad));
+}
+
+GST_END_TEST;
+
 Suite *
 gst_bin_suite (void)
 {
@@ -281,6 +331,7 @@ gst_bin_suite (void)
   tcase_add_test (tc_chain, test_message_state_changed);
   tcase_add_test (tc_chain, test_message_state_changed_child);
   tcase_add_test (tc_chain, test_message_state_changed_children);
+  tcase_add_test (tc_chain, test_add_linked);
 
   return s;
 }

check/gst/gstelement.c

@@ -94,6 +94,43 @@ GST_START_TEST (test_error_no_bus)
 
 GST_END_TEST;
 
+/* link and run two elements without putting them in a 
+ * pipeline */
+GST_START_TEST (test_link)
+{
+  GstElement *src, *sink;
+
+  src = gst_element_factory_make ("fakesrc", "source");
+  sink = gst_element_factory_make ("fakesink", "sink");
+
+  fail_unless (gst_element_link_pads (src, "src", sink, "sink"));
+
+  /* do sink to source state change */
+  gst_element_set_state (sink, GST_STATE_PAUSED);
+  gst_element_set_state (src, GST_STATE_PAUSED);
+
+  /* wait for preroll */
+  gst_element_get_state (sink, NULL, NULL, NULL);
+
+  /* play some more */
+  gst_element_set_state (sink, GST_STATE_PLAYING);
+  gst_element_set_state (src, GST_STATE_PLAYING);
+
+  g_usleep (G_USEC_PER_SEC);
+
+  /* and stop */
+  gst_element_set_state (sink, GST_STATE_PAUSED);
+  gst_element_set_state (src, GST_STATE_PAUSED);
+
+  /* wait for preroll */
+  gst_element_get_state (sink, NULL, NULL, NULL);
+
+  gst_element_set_state (sink, GST_STATE_NULL);
+  gst_element_set_state (src, GST_STATE_NULL);
+}
+
+GST_END_TEST;
+
 Suite *
 gst_element_suite (void)
 {
@@ -104,6 +141,7 @@ gst_element_suite (void)
   tcase_add_test (tc_chain, test_add_remove_pad);
   tcase_add_test (tc_chain, test_add_pad_unref_element);
   tcase_add_test (tc_chain, test_error_no_bus);
+  tcase_add_test (tc_chain, test_link);
 
   return s;
 }

check/pipelines/simple_launch_lines.c

@@ -139,8 +139,8 @@ GST_START_TEST (test_stop_from_app)
 
   g_return_if_fail (fakesrc && fakesink && pipeline);
 
-  gst_element_link (fakesrc, fakesink);
   gst_bin_add_many (GST_BIN (pipeline), fakesrc, fakesink, NULL);
+  gst_element_link (fakesrc, fakesink);
 
   g_object_set (fakesink, "signal-handoffs", (gboolean) TRUE, NULL);
   g_signal_connect (fakesink, "handoff", G_CALLBACK (got_handoff), NULL);

gst/gstbin.c

@@ -336,6 +336,21 @@ is_eos (GstBin * bin)
   return result;
 }
 
+static void
+unlink_pads (GstPad * pad)
+{
+  GstPad *peer;
+
+  if ((peer = gst_pad_get_peer (pad))) {
+    if (gst_pad_get_direction (pad) == GST_PAD_SRC)
+      gst_pad_unlink (pad, peer);
+    else
+      gst_pad_unlink (peer, pad);
+    gst_object_unref (peer);
+  }
+  gst_object_unref (pad);
+}
+
 /* add an element to this bin
  *
  * MT safe
@@ -344,13 +359,13 @@ static gboolean
 gst_bin_add_func (GstBin * bin, GstElement * element)
 {
   gchar *elem_name;
+  GstIterator *it;
 
   /* we obviously can't add ourself to ourself */
   if (G_UNLIKELY (GST_ELEMENT_CAST (element) == GST_ELEMENT_CAST (bin)))
     goto adding_itself;
 
-  /* get the element name to make sure it is unique in this bin, FIXME, another
+  /* get the element name to make sure it is unique in this bin. */
-   * thread can change the name after the unlock. */
   GST_LOCK (element);
   elem_name = g_strdup (GST_ELEMENT_NAME (element));
   GST_UNLOCK (element);
@@ -359,9 +374,9 @@ gst_bin_add_func (GstBin * bin, GstElement * element)
 
   /* then check to see if the element's name is already taken in the bin,
    * we can safely take the lock here. This check is probably bogus because
-   * you can safely change the element name after adding it to the bin. */
+   * you can safely change the element name after this check and before setting
-  if (G_UNLIKELY (gst_object_check_uniqueness (bin->children,
+   * the object parent. The window is very small though... */
-              elem_name) == FALSE))
+  if (G_UNLIKELY (!gst_object_check_uniqueness (bin->children, elem_name)))
     goto duplicate_name;
 
   /* set the element's parent and add the element to the bin's list of children */
@@ -369,15 +384,23 @@ gst_bin_add_func (GstBin * bin, GstElement * element)
               GST_OBJECT_CAST (bin))))
     goto had_parent;
 
+  /* if we add a sink we become a sink */
   if (GST_FLAG_IS_SET (element, GST_ELEMENT_IS_SINK))
     GST_FLAG_SET (bin, GST_ELEMENT_IS_SINK);
 
+  /* unlink all linked pads */
+  it = gst_element_iterate_pads (element);
+  gst_iterator_foreach (it, (GFunc) unlink_pads, element);
+  gst_iterator_free (it);
+
   bin->children = g_list_prepend (bin->children, element);
   bin->numchildren++;
   bin->children_cookie++;
 
+  /* distribute the bus */
   gst_element_set_bus (element, bin->child_bus);
 
+  /* propagate the current base time and clock */
   gst_element_set_base_time (element, GST_ELEMENT (bin)->base_time);
   gst_element_set_clock (element, GST_ELEMENT_CLOCK (bin));
 
@@ -416,6 +439,7 @@ had_parent:
   }
 }
 
+
 /**
  * gst_bin_add:
  * @bin: #GstBin to add element to
@@ -424,6 +448,9 @@ had_parent:
  * Adds the given element to the bin.  Sets the element's parent, and thus
  * takes ownership of the element. An element can only be added to one bin.
  *
+ * If the element's pads are linked to other pads, the pads will be unlinked
+ * before the element is added to the bin.
+ *
  * MT safe.
  *
  * Returns: TRUE if the element could be added, FALSE on wrong parameters or
@@ -467,6 +494,7 @@ static gboolean
 gst_bin_remove_func (GstBin * bin, GstElement * element)
 {
   gchar *elem_name;
+  GstIterator *it;
 
   /* grab element name so we can print it */
   GST_LOCK (element);
@@ -478,6 +506,11 @@ gst_bin_remove_func (GstBin * bin, GstElement * element)
   if (G_UNLIKELY (g_list_find (bin->children, element) == NULL))
     goto not_in_bin;
 
+  /* unlink all linked pads */
+  it = gst_element_iterate_pads (element);
+  gst_iterator_foreach (it, (GFunc) unlink_pads, element);
+  gst_iterator_free (it);
+
   /* now remove the element from the list of elements */
   bin->children = g_list_remove (bin->children, element);
   bin->numchildren--;
@@ -546,6 +579,9 @@ not_in_bin:
  * want the element to still exist after removing, you need to call
  * #gst_object_ref before removing it from the bin.
  *
+ * If the element's pads are linked to other pads, the pads will be unlinked
+ * before the element is removed from the bin.
+ *
  * MT safe.
  *
  * Returns: TRUE if the element could be removed, FALSE on wrong parameters or

tests/check/gst/gstbin.c

@@ -270,6 +270,56 @@ GST_START_TEST (test_message_state_changed_children)
 
 GST_END_TEST;
 
+/* adding an element with linked pads to a bin unlinks the
+ * pads */
+GST_START_TEST (test_add_linked)
+{
+  GstElement *src, *sink;
+  GstPad *srcpad, *sinkpad;
+  GstElement *pipeline;
+
+  pipeline = gst_pipeline_new (NULL);
+  fail_unless (pipeline != NULL, "Could not create pipeline");
+
+  src = gst_element_factory_make ("fakesrc", NULL);
+  fail_if (src == NULL, "Could not create fakesrc");
+  sink = gst_element_factory_make ("fakesink", NULL);
+  fail_if (src == NULL, "Could not create fakesink");
+
+  srcpad = gst_element_get_pad (src, "src");
+  fail_unless (srcpad != NULL);
+  sinkpad = gst_element_get_pad (sink, "sink");
+  fail_unless (sinkpad != NULL);
+
+  fail_unless (gst_pad_link (srcpad, sinkpad) == GST_PAD_LINK_OK);
+
+  /* pads are linked now */
+  fail_unless (gst_pad_is_linked (srcpad));
+  fail_unless (gst_pad_is_linked (sinkpad));
+
+  /* adding element to bin voids hierarchy so pads are unlinked */
+  gst_bin_add (GST_BIN (pipeline), src);
+
+  /* check if pads really are unlinked */
+  fail_unless (!gst_pad_is_linked (srcpad));
+  fail_unless (!gst_pad_is_linked (sinkpad));
+
+  /* cannot link pads in wrong hierarchy */
+  fail_unless (gst_pad_link (srcpad, sinkpad) == GST_PAD_LINK_WRONG_HIERARCHY);
+
+  /* adding other element to bin as well */
+  gst_bin_add (GST_BIN (pipeline), sink);
+
+  /* now we can link again */
+  fail_unless (gst_pad_link (srcpad, sinkpad) == GST_PAD_LINK_OK);
+
+  /* check if pads really are linked */
+  fail_unless (gst_pad_is_linked (srcpad));
+  fail_unless (gst_pad_is_linked (sinkpad));
+}
+
+GST_END_TEST;
+
 Suite *
 gst_bin_suite (void)
 {
@@ -281,6 +331,7 @@ gst_bin_suite (void)
   tcase_add_test (tc_chain, test_message_state_changed);
   tcase_add_test (tc_chain, test_message_state_changed_child);
   tcase_add_test (tc_chain, test_message_state_changed_children);
+  tcase_add_test (tc_chain, test_add_linked);
 
   return s;
 }

tests/check/gst/gstelement.c

@@ -94,6 +94,43 @@ GST_START_TEST (test_error_no_bus)
 
 GST_END_TEST;
 
+/* link and run two elements without putting them in a 
+ * pipeline */
+GST_START_TEST (test_link)
+{
+  GstElement *src, *sink;
+
+  src = gst_element_factory_make ("fakesrc", "source");
+  sink = gst_element_factory_make ("fakesink", "sink");
+
+  fail_unless (gst_element_link_pads (src, "src", sink, "sink"));
+
+  /* do sink to source state change */
+  gst_element_set_state (sink, GST_STATE_PAUSED);
+  gst_element_set_state (src, GST_STATE_PAUSED);
+
+  /* wait for preroll */
+  gst_element_get_state (sink, NULL, NULL, NULL);
+
+  /* play some more */
+  gst_element_set_state (sink, GST_STATE_PLAYING);
+  gst_element_set_state (src, GST_STATE_PLAYING);
+
+  g_usleep (G_USEC_PER_SEC);
+
+  /* and stop */
+  gst_element_set_state (sink, GST_STATE_PAUSED);
+  gst_element_set_state (src, GST_STATE_PAUSED);
+
+  /* wait for preroll */
+  gst_element_get_state (sink, NULL, NULL, NULL);
+
+  gst_element_set_state (sink, GST_STATE_NULL);
+  gst_element_set_state (src, GST_STATE_NULL);
+}
+
+GST_END_TEST;
+
 Suite *
 gst_element_suite (void)
 {
@@ -104,6 +141,7 @@ gst_element_suite (void)
   tcase_add_test (tc_chain, test_add_remove_pad);
   tcase_add_test (tc_chain, test_add_pad_unref_element);
   tcase_add_test (tc_chain, test_error_no_bus);
+  tcase_add_test (tc_chain, test_link);
 
   return s;
 }

tests/check/pipelines/simple-launch-lines.c

@@ -139,8 +139,8 @@ GST_START_TEST (test_stop_from_app)
 
   g_return_if_fail (fakesrc && fakesink && pipeline);
 
-  gst_element_link (fakesrc, fakesink);
   gst_bin_add_many (GST_BIN (pipeline), fakesrc, fakesink, NULL);
+  gst_element_link (fakesrc, fakesink);
 
   g_object_set (fakesink, "signal-handoffs", (gboolean) TRUE, NULL);
   g_signal_connect (fakesink, "handoff", G_CALLBACK (got_handoff), NULL);