From 7c7a4b68a1761372e41bbb85ee31178b26a3d5f7 Mon Sep 17 00:00:00 2001 From: Wim Taymans Date: Thu, 28 Jul 2005 11:24:33 +0000 Subject: [PATCH] check/gst/gstghostpad.c: Added some more tests for wrong hierarchy Original commit message from CVS: * check/gst/gstghostpad.c: (GST_START_TEST), (gst_ghost_pad_suite): Added some more tests for wrong hierarchy * docs/design/part-overview.txt: Some updates. * gst/gstbin.c: (gst_bin_remove_func), (gst_bin_dispose): Cleanups. * gst/gstelement.c: (gst_element_remove_pad), (gst_element_seek), (gst_element_dispose): Some more cleanups. * gst/gstpad.c: (gst_pad_link_check_compatible_unlocked), (gst_pad_link_check_hierarchy), (gst_pad_link_prepare), (gst_pad_get_caps_unlocked), (gst_pad_accept_caps), (gst_pad_set_caps), (gst_pad_send_event): Check for correct hierarchy when linking pads. Moving to strict requirement for ghostpads when linking elements in different bins. * gst/gstpad.h: Clean ups. Added WRONG_HIERARCHY return value. --- ChangeLog | 26 ++++++++++++ check/gst/gstghostpad.c | 95 +++++++++++++++++++++++++++++++++++++++++++ docs/design/part-overview.txt | 24 ++++++++--- gst/gstbin.c | 12 +++++- gst/gstelement.c | 18 +++----- gst/gstpad.c | 66 +++++++++++++++++++++++++++++- gst/gstpad.h | 23 ++++++++--- tests/check/gst/gstghostpad.c | 95 +++++++++++++++++++++++++++++++++++++++++++ 8 files changed, 331 insertions(+), 28 deletions(-) diff --git a/ChangeLog b/ChangeLog index f559bf2..fee0cdc 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,29 @@ +2005-07-28 Wim Taymans + + * check/gst/gstghostpad.c: (GST_START_TEST), (gst_ghost_pad_suite): + Added some more tests for wrong hierarchy + + * docs/design/part-overview.txt: + Some updates. + + * gst/gstbin.c: (gst_bin_remove_func), (gst_bin_dispose): + Cleanups. + + * gst/gstelement.c: (gst_element_remove_pad), (gst_element_seek), + (gst_element_dispose): + Some more cleanups. + + * gst/gstpad.c: (gst_pad_link_check_compatible_unlocked), + (gst_pad_link_check_hierarchy), (gst_pad_link_prepare), + (gst_pad_get_caps_unlocked), (gst_pad_accept_caps), + (gst_pad_set_caps), (gst_pad_send_event): + Check for correct hierarchy when linking pads. Moving to + strict requirement for ghostpads when linking elements in + different bins. + + * gst/gstpad.h: + Clean ups. Added WRONG_HIERARCHY return value. + 2005-07-28 Ronald S. Bultje * gst/base/gstbasetransform.c: (gst_base_transform_setcaps): diff --git a/check/gst/gstghostpad.c b/check/gst/gstghostpad.c index 174cf3a..580c630 100644 --- a/check/gst/gstghostpad.c +++ b/check/gst/gstghostpad.c @@ -29,6 +29,99 @@ assert_gstrefcount (gpointer p, gint i) GST_OBJECT_REFCOUNT_VALUE (p)); } +/* test if removing a bin also cleans up the ghostpads + */ +GST_START_TEST (test_remove1) +{ + GstElement *b1, *b2, *src, *sink; + GstPad *srcpad, *sinkpad; + GstPadLinkReturn ret; + + b1 = gst_element_factory_make ("pipeline", NULL); + b2 = gst_element_factory_make ("bin", NULL); + src = gst_element_factory_make ("fakesrc", NULL); + sink = gst_element_factory_make ("fakesink", NULL); + + fail_unless (gst_bin_add (GST_BIN (b2), sink)); + fail_unless (gst_bin_add (GST_BIN (b1), src)); + fail_unless (gst_bin_add (GST_BIN (b1), b2)); + + sinkpad = gst_element_get_pad (sink, "sink"); + gst_element_add_pad (b2, gst_ghost_pad_new ("sink", sinkpad)); + gst_object_unref (sinkpad); + + srcpad = gst_element_get_pad (src, "src"); + /* get the ghostpad */ + sinkpad = gst_element_get_pad (b2, "sink"); + + ret = gst_pad_link (srcpad, sinkpad); + fail_unless (ret == GST_PAD_LINK_OK); + gst_object_unref (srcpad); + gst_object_unref (sinkpad); + + /* now remove the bin with the ghostpad, b2 is disposed + * now. */ + gst_bin_remove (GST_BIN (b1), b2); + + srcpad = gst_element_get_pad (src, "src"); + /* pad cannot be linked now */ + fail_if (gst_pad_is_linked (srcpad)); +} + +GST_END_TEST; + +/* test if linking fails over different bins using a pipeline + * like this: + * + * fakesrc num_buffers=10 ! ( fakesink ) + * + */ +GST_START_TEST (test_link) +{ + GstElement *b1, *b2, *src, *sink; + GstPad *srcpad, *sinkpad, *gpad; + GstPadLinkReturn ret; + + b1 = gst_element_factory_make ("pipeline", NULL); + b2 = gst_element_factory_make ("bin", NULL); + src = gst_element_factory_make ("fakesrc", NULL); + sink = gst_element_factory_make ("fakesink", NULL); + + fail_unless (gst_bin_add (GST_BIN (b2), sink)); + fail_unless (gst_bin_add (GST_BIN (b1), src)); + fail_unless (gst_bin_add (GST_BIN (b1), b2)); + + srcpad = gst_element_get_pad (src, "src"); + fail_unless (srcpad != NULL); + sinkpad = gst_element_get_pad (sink, "sink"); + fail_unless (sinkpad != NULL); + + /* linking in different hierarchies should fail */ + ret = gst_pad_link (srcpad, sinkpad); + fail_unless (ret == GST_PAD_LINK_WRONG_HIERARCHY); + + /* now setup a ghostpad */ + gpad = gst_ghost_pad_new ("sink", sinkpad); + gst_object_unref (sinkpad); + /* need to ref as _add_pad takes ownership */ + gst_object_ref (gpad); + gst_element_add_pad (b2, gpad); + + /* our new sinkpad */ + sinkpad = gpad; + + /* and linking should work now */ + ret = gst_pad_link (srcpad, sinkpad); + fail_unless (ret == GST_PAD_LINK_OK); +} + +GST_END_TEST; + +/* test if ghostpads are created automagically when using + * gst_element_link_pads. + * + * fakesrc num_buffers=10 ! ( identity ) ! fakesink + */ GST_START_TEST (test_ghost_pads) { GstElement *b1, *b2, *src, *i1, *sink; @@ -134,6 +227,8 @@ gst_ghost_pad_suite (void) TCase *tc_chain = tcase_create ("ghost pad tests"); suite_add_tcase (s, tc_chain); + tcase_add_test (tc_chain, test_remove1); + tcase_add_test (tc_chain, test_link); tcase_add_test (tc_chain, test_ghost_pads); return s; diff --git a/docs/design/part-overview.txt b/docs/design/part-overview.txt index 6dd087e..2b6c9d8 100644 --- a/docs/design/part-overview.txt +++ b/docs/design/part-overview.txt @@ -41,6 +41,10 @@ Introduction vorbisdec element decodes the compressed data and sends it to the alsasink element. The alsasink element sends the samples to the audio card for playback. + Downstream and upstream are the term used to describe the direction in the + Pipeline. From source to sink is called "downstream" and "upstream" is + from sink to source. + The task of the application is to construct a pipeline as above using existing elements. This is further explained in the pipeline building topic. @@ -198,7 +202,9 @@ Dataflow and buffers out the buffer. When an element pad receives a buffer, if has to check if it understands the media - type of the buffer before starting processing it. + type of the buffer before starting processing it. The GStreamer core does this + automatically and will call the gst_pad_set_caps() function of the element before + sending the buffer to the element. Both gst_pad_push() and gst_pad_pull_range() have a return value indicating wether the operation succeeded. An error code means that no more data should be send @@ -273,10 +279,14 @@ Pipeline clock One of the important functions of the pipeline is to select a global clock for all the elements in the pipeline. + The purpose of the clock is to provide a stricly increasing value at the rate + of one GST_SECOND per second. Clock values are expressed in nanoseconds. + Elements use the clock time to synchronized the playback of data. + Before the pipeline is set to PAUSED, the pipeline asks each element if they can provide a clock. The clock is selected in the following order: - - If the application selected a clock, us that one. + - If the application selected a clock, use that one. - If a source element provides a clock, use that clock. - Select a clock from any other element that provides a clock, start with the sinks. @@ -284,7 +294,10 @@ Pipeline clock In a typical playback pipeline this will select the clock provided by a sink element such as an audio sink. - + + In capture pipelines, this will typically select the clock of the data producer, which + can in most cases not control the rate at which it delivers data. + Pipeline states --------------- @@ -447,8 +460,9 @@ Pipeline seeking When the seek event reaches an element that will perform the seek operation, that element performs the following steps. - 1) send a FLUSH event to all downstream and upstream peer elements. - 2) make sure the streaming thread is not running. + 1) send a FLUSH_START event to all downstream and upstream peer elements. + 2) make sure the streaming thread is not running. The streaming thread will + always stop because of step 1). 3) perform the seek operation 4) send a FLUSH done event to all downstream and upstream peer elements. 5) send DISCONT event to inform all elements of the new position and to complete diff --git a/gst/gstbin.c b/gst/gstbin.c index 26a00a3..6d151e2 100644 --- a/gst/gstbin.c +++ b/gst/gstbin.c @@ -483,7 +483,7 @@ gst_bin_remove_func (GstBin * bin, GstElement * element) bin->numchildren--; bin->children_cookie++; - /* check if we a sink */ + /* check if we removed a sink */ if (GST_FLAG_IS_SET (element, GST_ELEMENT_IS_SINK)) { GList *other_sink; @@ -691,6 +691,9 @@ bin_element_is_sink (GstElement * child, GstBin * bin) return is_sink ? 0 : 1; } +/* check if object has the given ancestor somewhere up in + * the hierarchy + */ static gboolean has_ancestor (GstObject * object, GstObject * ancestor) { @@ -1008,6 +1011,8 @@ append_child (gpointer child, GQueue * queue) * Each element will have its refcount increased, so unref * after use. * + * Not implemented yet. + * * MT safe. * * Returns: a #GstIterator of #GstElements. gst_iterator_free after use. @@ -1060,7 +1065,8 @@ remove_all_from_queue (GQueue * queue, gpointer elem, gboolean unref) * * MT safe. */ -/* FIXME, make me more elegant */ +/* FIXME, make me more elegant, want to use a topological sort algorithm + * based on indegrees (or outdegrees in our case) */ static GstElementStateReturn gst_bin_change_state (GstElement * element) { @@ -1335,7 +1341,9 @@ gst_bin_dispose (GObject * object) gst_object_ref (object); g_list_free (bin->eosed); + bin->eosed = NULL; gst_object_unref (bin->child_bus); + bin->child_bus = NULL; gst_element_set_bus (GST_ELEMENT (bin), NULL); while (bin->children) { diff --git a/gst/gstelement.c b/gst/gstelement.c index a4e3417..e2b0e27 100644 --- a/gst/gstelement.c +++ b/gst/gstelement.c @@ -590,35 +590,28 @@ gboolean gst_element_remove_pad (GstElement * element, GstPad * pad) { GstPad *peer; - gchar *pad_name; g_return_val_if_fail (GST_IS_ELEMENT (element), FALSE); g_return_val_if_fail (GST_IS_PAD (pad), FALSE); /* locking pad to look at the name and parent */ GST_LOCK (pad); - pad_name = g_strdup (GST_PAD_NAME (pad)); - GST_CAT_INFO_OBJECT (GST_CAT_ELEMENT_PADS, element, "removing pad '%s'", - GST_STR_NULL (pad_name)); + GST_STR_NULL (GST_PAD_NAME (pad))); if (G_UNLIKELY (GST_PAD_PARENT (pad) != element)) goto not_our_pad; GST_UNLOCK (pad); - g_free (pad_name); - - peer = gst_pad_get_peer (pad); - /* unlink */ - if (peer != NULL) { + if ((peer = gst_pad_get_peer (pad))) { /* window for MT unsafeness, someone else could unlink here * and then we call unlink with wrong pads. The unlink * function would catch this and safely return failed. */ if (GST_PAD_IS_SRC (pad)) - gst_pad_unlink (pad, GST_PAD_CAST (peer)); + gst_pad_unlink (pad, peer); else - gst_pad_unlink (GST_PAD_CAST (peer), pad); + gst_pad_unlink (peer, pad); gst_object_unref (peer); } @@ -658,7 +651,6 @@ not_our_pad: GST_ELEMENT_NAME (element)); GST_UNLOCK (element); GST_UNLOCK (pad); - g_free (pad_name); return FALSE; } } @@ -2056,7 +2048,7 @@ gst_element_dispose (GObject * object) /* first we break all our links with the outside */ while (element->pads) { - gst_element_remove_pad (element, GST_PAD (element->pads->data)); + gst_element_remove_pad (element, GST_PAD_CAST (element->pads->data)); } if (G_UNLIKELY (element->pads != 0)) { g_critical ("could not remove pads from element %s", diff --git a/gst/gstpad.c b/gst/gstpad.c index f1794da..ae9906f 100644 --- a/gst/gstpad.c +++ b/gst/gstpad.c @@ -1327,6 +1327,12 @@ gst_pad_is_linked (GstPad * pad) return result; } +/* get the caps from both pads and see if the intersection + * is not empty. + * + * This function should be called with the pad LOCK on both + * pads + */ static gboolean gst_pad_link_check_compatible_unlocked (GstPad * src, GstPad * sink) { @@ -1338,6 +1344,7 @@ gst_pad_link_check_compatible_unlocked (GstPad * src, GstPad * sink) GST_CAT_DEBUG (GST_CAT_CAPS, " src caps %" GST_PTR_FORMAT, srccaps); GST_CAT_DEBUG (GST_CAT_CAPS, "sink caps %" GST_PTR_FORMAT, sinkcaps); + /* if we have caps on both pads we can check the intersection */ if (srccaps && sinkcaps) { GstCaps *icaps; @@ -1349,6 +1356,7 @@ gst_pad_link_check_compatible_unlocked (GstPad * src, GstPad * sink) "intersection caps %p %" GST_PTR_FORMAT, icaps, icaps); if (!icaps || gst_caps_is_empty (icaps)) { + GST_CAT_DEBUG (GST_CAT_CAPS, "intersection is empty"); gst_caps_unref (icaps); return FALSE; } @@ -1358,6 +1366,49 @@ gst_pad_link_check_compatible_unlocked (GstPad * src, GstPad * sink) return TRUE; } +/* check if the grandparents of both pads are the same. + * This check is required so that we don't try to link + * pads from elements in different bins without ghostpads. + * + * The LOCK should be helt on both pads + */ +static gboolean +gst_pad_link_check_hierarchy (GstPad * src, GstPad * sink) +{ + GstObject *psrc, *psink; + gboolean res = TRUE; + + psrc = GST_OBJECT_PARENT (src); + psink = GST_OBJECT_PARENT (sink); + + /* if one of the pads has no parent, we allow the link */ + if (psrc && psink) { + /* if the parents are the same, we have a loop */ + if (psrc == psink) { + GST_CAT_DEBUG (GST_CAT_CAPS, "pads have same parent %" GST_PTR_FORMAT, + psrc); + res = FALSE; + goto done; + } + /* if they both have a parent, we check the grandparents */ + psrc = gst_object_get_parent (psrc); + psink = gst_object_get_parent (psink); + + if (psrc != psink) { + /* if they have grandparents but they are not the same */ + GST_CAT_DEBUG (GST_CAT_CAPS, + "pads have different grandparents %" GST_PTR_FORMAT " and %" + GST_PTR_FORMAT, psrc, psink); + res = FALSE; + } + if (psrc) + gst_object_unref (psrc); + if (psink) + gst_object_unref (psink); + } +done: + return res; +} /* FIXME leftover from an attempt at refactoring... */ /* call with the two pads unlocked */ @@ -1387,10 +1438,14 @@ gst_pad_link_prepare (GstPad * srcpad, GstPad * sinkpad) if (G_UNLIKELY (GST_PAD_PEER (sinkpad) != NULL)) goto sink_was_linked; + /* check hierarchy, pads can only be linked if the grandparents + * are the same. */ + if (!gst_pad_link_check_hierarchy (srcpad, sinkpad)) + goto wrong_hierarchy; + /* check pad caps for non-empty intersection */ - if (!gst_pad_link_check_compatible_unlocked (srcpad, sinkpad)) { + if (!gst_pad_link_check_compatible_unlocked (srcpad, sinkpad)) goto no_format; - } /* FIXME check pad scheduling for non-empty intersection */ @@ -1428,6 +1483,13 @@ sink_was_linked: GST_UNLOCK (srcpad); return GST_PAD_LINK_WAS_LINKED; } +wrong_hierarchy: + { + GST_CAT_INFO (GST_CAT_PADS, "pads have wrong hierarchy"); + GST_UNLOCK (sinkpad); + GST_UNLOCK (srcpad); + return GST_PAD_LINK_WRONG_HIERARCHY; + } no_format: { GST_CAT_INFO (GST_CAT_PADS, "caps are incompatible"); diff --git a/gst/gstpad.h b/gst/gstpad.h index 1188c99..1623a14 100644 --- a/gst/gstpad.h +++ b/gst/gstpad.h @@ -56,13 +56,24 @@ typedef struct _GstPadTemplate GstPadTemplate; typedef struct _GstPadTemplateClass GstPadTemplateClass; typedef struct _GstStaticPadTemplate GstStaticPadTemplate; +/** + * GstPadLinkReturn: + * #GST_PAD_LINK_OK : link ok + * #GST_PAD_LINK_WRONG_HIERARCHY: pads have no common grandparent + * #GST_PAD_LINK_WAS_LINKED : pad was already linked + * #GST_PAD_LINK_WRONG_DIRECTION: pads have wrong direction + * #GST_PAD_LINK_NOFORMAT : pads do not have common format + * #GST_PAD_LINK_NOSCHED : pads cannot cooperate in scheduling + * #GST_PAD_LINK_REFUSED : refused for some reason + */ typedef enum { - GST_PAD_LINK_NOSCHED = -5, /* pads cannot cooperate in scheduling */ - GST_PAD_LINK_NOFORMAT = -4, /* pads do not have common format */ - GST_PAD_LINK_REFUSED = -3, /* refused for some reason */ - GST_PAD_LINK_WRONG_DIRECTION = -2, /* pads have wrong direction */ - GST_PAD_LINK_WAS_LINKED = -1, /* pad was already linked */ - GST_PAD_LINK_OK = 0, /* link ok */ + GST_PAD_LINK_OK = 0, + GST_PAD_LINK_WRONG_HIERARCHY = -1, + GST_PAD_LINK_WAS_LINKED = -2, + GST_PAD_LINK_WRONG_DIRECTION = -3, + GST_PAD_LINK_NOFORMAT = -4, + GST_PAD_LINK_NOSCHED = -5, + GST_PAD_LINK_REFUSED = -6, } GstPadLinkReturn; #define GST_PAD_LINK_FAILED(ret) ((ret) < GST_PAD_LINK_OK) diff --git a/tests/check/gst/gstghostpad.c b/tests/check/gst/gstghostpad.c index 174cf3a..580c630 100644 --- a/tests/check/gst/gstghostpad.c +++ b/tests/check/gst/gstghostpad.c @@ -29,6 +29,99 @@ assert_gstrefcount (gpointer p, gint i) GST_OBJECT_REFCOUNT_VALUE (p)); } +/* test if removing a bin also cleans up the ghostpads + */ +GST_START_TEST (test_remove1) +{ + GstElement *b1, *b2, *src, *sink; + GstPad *srcpad, *sinkpad; + GstPadLinkReturn ret; + + b1 = gst_element_factory_make ("pipeline", NULL); + b2 = gst_element_factory_make ("bin", NULL); + src = gst_element_factory_make ("fakesrc", NULL); + sink = gst_element_factory_make ("fakesink", NULL); + + fail_unless (gst_bin_add (GST_BIN (b2), sink)); + fail_unless (gst_bin_add (GST_BIN (b1), src)); + fail_unless (gst_bin_add (GST_BIN (b1), b2)); + + sinkpad = gst_element_get_pad (sink, "sink"); + gst_element_add_pad (b2, gst_ghost_pad_new ("sink", sinkpad)); + gst_object_unref (sinkpad); + + srcpad = gst_element_get_pad (src, "src"); + /* get the ghostpad */ + sinkpad = gst_element_get_pad (b2, "sink"); + + ret = gst_pad_link (srcpad, sinkpad); + fail_unless (ret == GST_PAD_LINK_OK); + gst_object_unref (srcpad); + gst_object_unref (sinkpad); + + /* now remove the bin with the ghostpad, b2 is disposed + * now. */ + gst_bin_remove (GST_BIN (b1), b2); + + srcpad = gst_element_get_pad (src, "src"); + /* pad cannot be linked now */ + fail_if (gst_pad_is_linked (srcpad)); +} + +GST_END_TEST; + +/* test if linking fails over different bins using a pipeline + * like this: + * + * fakesrc num_buffers=10 ! ( fakesink ) + * + */ +GST_START_TEST (test_link) +{ + GstElement *b1, *b2, *src, *sink; + GstPad *srcpad, *sinkpad, *gpad; + GstPadLinkReturn ret; + + b1 = gst_element_factory_make ("pipeline", NULL); + b2 = gst_element_factory_make ("bin", NULL); + src = gst_element_factory_make ("fakesrc", NULL); + sink = gst_element_factory_make ("fakesink", NULL); + + fail_unless (gst_bin_add (GST_BIN (b2), sink)); + fail_unless (gst_bin_add (GST_BIN (b1), src)); + fail_unless (gst_bin_add (GST_BIN (b1), b2)); + + srcpad = gst_element_get_pad (src, "src"); + fail_unless (srcpad != NULL); + sinkpad = gst_element_get_pad (sink, "sink"); + fail_unless (sinkpad != NULL); + + /* linking in different hierarchies should fail */ + ret = gst_pad_link (srcpad, sinkpad); + fail_unless (ret == GST_PAD_LINK_WRONG_HIERARCHY); + + /* now setup a ghostpad */ + gpad = gst_ghost_pad_new ("sink", sinkpad); + gst_object_unref (sinkpad); + /* need to ref as _add_pad takes ownership */ + gst_object_ref (gpad); + gst_element_add_pad (b2, gpad); + + /* our new sinkpad */ + sinkpad = gpad; + + /* and linking should work now */ + ret = gst_pad_link (srcpad, sinkpad); + fail_unless (ret == GST_PAD_LINK_OK); +} + +GST_END_TEST; + +/* test if ghostpads are created automagically when using + * gst_element_link_pads. + * + * fakesrc num_buffers=10 ! ( identity ) ! fakesink + */ GST_START_TEST (test_ghost_pads) { GstElement *b1, *b2, *src, *i1, *sink; @@ -134,6 +227,8 @@ gst_ghost_pad_suite (void) TCase *tc_chain = tcase_create ("ghost pad tests"); suite_add_tcase (s, tc_chain); + tcase_add_test (tc_chain, test_remove1); + tcase_add_test (tc_chain, test_link); tcase_add_test (tc_chain, test_ghost_pads); return s; -- 2.7.4