diff --git a/ChangeLog b/ChangeLog index ae6f284021..b4b58d769a 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,31 @@ +2008-10-06 Wim Taymans + + Base on Patch by: Olivier Crete + + * gst/gstbin.c: (gst_bin_init), (gst_bin_add_func), + (gst_bin_remove_func), (update_degree), + (gst_bin_sort_iterator_new), (gst_bin_handle_message_func): + Keep track of pads that are being linked/unlinked and resync the state + changes. + + * gst/gstpad.c: (gst_pad_get_direction), + (gst_pad_set_chain_function), (gst_pad_set_getrange_function), + (gst_pad_set_checkgetrange_function), (gst_pad_unlink), + (gst_pad_link_prepare), (gst_pad_link), + (gst_pad_event_default_dispatch), (gst_pad_chain), (gst_pad_push), + (gst_pad_check_pull_range), (gst_pad_get_range), + (gst_pad_pull_range): + Some code cleanups, use macros to check pad direction. + Don't need to take the lock on the pad direction. + Post structure change when pads are linked/unlinked. + Change some checks into _return_if_fail(). + + * tests/check/gst/gstbin.c: + (test_link_structure_change_state_changed_sync_cb), + (GST_START_TEST), (gst_bin_suite): + Add testcase for pad link/unlinke resync during a state change. + Fixes #510354. + 2008-10-06 Wim Taymans * docs/gst/gstreamer-sections.txt: diff --git a/gst/gstbin.c b/gst/gstbin.c index 1f2af602c5..7b78dd1d87 100644 --- a/gst/gstbin.c +++ b/gst/gstbin.c @@ -201,6 +201,8 @@ struct _GstBinPrivate * have to process the message after finishing the state change even when no * child returned GST_STATE_CHANGE_ASYNC. */ gboolean pending_async_done; + + guint32 structure_cookie; }; typedef struct @@ -485,6 +487,7 @@ gst_bin_init (GstBin * bin) bin->priv = GST_BIN_GET_PRIVATE (bin); bin->priv->asynchandling = DEFAULT_ASYNC_HANDLING; + bin->priv->structure_cookie = 0; } static void @@ -748,7 +751,7 @@ find_message (GstBin * bin, GstObject * src, GstMessageType types) return result; } -/* with LOCK, returns TRUE if message had a valid SRC, takes ref on +/* with LOCK, returns TRUE if message had a valid SRC, takes ownership of * the message. * * A message that is cached and has the same SRC and type is replaced @@ -916,6 +919,7 @@ gst_bin_add_func (GstBin * bin, GstElement * element) bin->children = g_list_prepend (bin->children, element); bin->numchildren++; bin->children_cookie++; + bin->priv->structure_cookie++; /* distribute the bus */ gst_element_set_bus (element, bin->child_bus); @@ -1125,6 +1129,7 @@ gst_bin_remove_func (GstBin * bin, GstElement * element) * so that others can detect a change in the children list. */ bin->numchildren--; bin->children_cookie++; + bin->priv->structure_cookie++; if (is_sink && !othersink) { /* we're not a sink anymore */ @@ -1150,19 +1155,44 @@ gst_bin_remove_func (GstBin * bin, GstElement * element) for (walk = bin->messages; walk; walk = next) { GstMessage *message = (GstMessage *) walk->data; GstElement *src = GST_ELEMENT_CAST (GST_MESSAGE_SRC (message)); + gboolean remove; next = g_list_next (walk); + remove = FALSE; - if (GST_MESSAGE_TYPE (message) == GST_MESSAGE_ASYNC_START) { - if (src == element) - this_async = TRUE; - else - other_async = TRUE; + switch (GST_MESSAGE_TYPE (message)) { + case GST_MESSAGE_ASYNC_START: + if (src == element) + this_async = TRUE; + else + other_async = TRUE; - GST_DEBUG_OBJECT (GST_MESSAGE_SRC (message), - "looking at message %p", message); + GST_DEBUG_OBJECT (GST_MESSAGE_SRC (message), + "looking at message %p", message); + break; + case GST_MESSAGE_STRUCTURE_CHANGE: + { + GstElement *owner; + + GST_DEBUG_OBJECT (GST_MESSAGE_SRC (message), + "looking at structure change message %p", message); + /* it's unlikely that this message is still in the list of messages + * because this would mean that a link/unlink is busy in another thread + * while we remove the element. We still have to remove the message + * because we might not receive the done message anymore when the element + * is removed from the bin. */ + gst_message_parse_structure_change (message, NULL, &owner, NULL); + if (owner == element) + remove = TRUE; + break; + } + default: + break; } - if (src == element) { + if (src == element) + remove = TRUE; + + if (remove) { /* delete all message types */ GST_DEBUG_OBJECT (GST_MESSAGE_SRC (message), "deleting message %p of element \"%s\"", message, elem_name); @@ -1652,18 +1682,28 @@ update_degree (GstElement * element, GstBinSortIterator * bit) gboolean linked = FALSE; GST_OBJECT_LOCK (element); - /* don't touch degree if element has no sourcepads */ + /* don't touch degree if element has no sinkpads */ if (element->numsinkpads != 0) { /* loop over all sinkpads, decrement degree for all connected * elements in this bin */ GList *pads; for (pads = element->sinkpads; pads; pads = g_list_next (pads)) { - GstPad *peer; + GstPad *pad, *peer; - if ((peer = gst_pad_get_peer (GST_PAD_CAST (pads->data)))) { + pad = GST_PAD_CAST (pads->data); + + if ((peer = gst_pad_get_peer (pad))) { GstElement *peer_element; + /* we're iterating over the sinkpads, this is the peer and thus the + * srcpad, check if it's busy in a link/unlink */ + if (G_UNLIKELY (find_message (bit->bin, GST_OBJECT_CAST (peer), + GST_MESSAGE_STRUCTURE_CHANGE))) { + gst_object_unref (peer); + continue; + } + if ((peer_element = gst_pad_get_parent_element (peer))) { GST_OBJECT_LOCK (peer_element); /* check that we don't go outside of this bin */ @@ -1686,7 +1726,10 @@ update_degree (GstElement * element, GstBinSortIterator * bit) GST_ELEMENT_NAME (peer_element), old_deg, new_deg, GST_ELEMENT_NAME (element)); - /* update degree */ + /* update degree, it is possible that an element was in 0 and + * reaches -1 here. This would mean that the element had no sinkpads + * but became linked while the state change was happening. We will + * resync on this with the structure change message. */ if (new_deg == 0) { /* degree hit 0, add to queue */ add_to_queue (bit, peer_element); @@ -1814,7 +1857,7 @@ gst_bin_sort_iterator_new (GstBin * bin) gst_iterator_new (sizeof (GstBinSortIterator), GST_TYPE_ELEMENT, GST_OBJECT_GET_LOCK (bin), - &bin->children_cookie, + &bin->priv->structure_cookie, (GstIteratorNextFunction) gst_bin_sort_iterator_next, (GstIteratorItemFunction) NULL, (GstIteratorResyncFunction) gst_bin_sort_iterator_resync, @@ -2928,6 +2971,34 @@ gst_bin_handle_message_func (GstBin * bin, GstMessage * message) break; } } + case GST_MESSAGE_STRUCTURE_CHANGE: + { + gboolean busy; + + gst_message_parse_structure_change (message, NULL, NULL, &busy); + + GST_OBJECT_LOCK (bin); + if (busy) { + /* while the pad is busy, avoid following it when doing state changes. + * Don't update the cookie yet, we will do that after the structure + * change finished and we are ready to inspect the new updated + * structure. */ + bin_replace_message (bin, message, GST_MESSAGE_STRUCTURE_CHANGE); + message = NULL; + } else { + /* a pad link/unlink ended, signal the state change iterator that we + * need to resync by updating the structure_cookie. */ + bin_remove_messages (bin, GST_MESSAGE_SRC (message), + GST_MESSAGE_STRUCTURE_CHANGE); + bin->priv->structure_cookie++; + } + GST_OBJECT_UNLOCK (bin); + + if (message) + gst_message_unref (message); + + break; + } default: goto forward; } diff --git a/gst/gstpad.c b/gst/gstpad.c index 6a52cef0d9..b9c78886b8 100644 --- a/gst/gstpad.c +++ b/gst/gstpad.c @@ -559,9 +559,7 @@ gst_pad_get_direction (GstPad * pad) * error return value */ g_return_val_if_fail (GST_IS_PAD (pad), GST_PAD_UNKNOWN); - GST_OBJECT_LOCK (pad); result = GST_PAD_DIRECTION (pad); - GST_OBJECT_UNLOCK (pad); return result; } @@ -1185,7 +1183,7 @@ void gst_pad_set_chain_function (GstPad * pad, GstPadChainFunction chain) { g_return_if_fail (GST_IS_PAD (pad)); - g_return_if_fail (GST_PAD_DIRECTION (pad) == GST_PAD_SINK); + g_return_if_fail (GST_PAD_IS_SINK (pad)); GST_PAD_CHAINFUNC (pad) = chain; GST_CAT_DEBUG_OBJECT (GST_CAT_PADS, pad, "chainfunc set to %s", @@ -1205,7 +1203,7 @@ void gst_pad_set_getrange_function (GstPad * pad, GstPadGetRangeFunction get) { g_return_if_fail (GST_IS_PAD (pad)); - g_return_if_fail (GST_PAD_DIRECTION (pad) == GST_PAD_SRC); + g_return_if_fail (GST_PAD_IS_SRC (pad)); GST_PAD_GETRANGEFUNC (pad) = get; @@ -1226,7 +1224,7 @@ gst_pad_set_checkgetrange_function (GstPad * pad, GstPadCheckGetRangeFunction check) { g_return_if_fail (GST_IS_PAD (pad)); - g_return_if_fail (GST_PAD_DIRECTION (pad) == GST_PAD_SRC); + g_return_if_fail (GST_PAD_IS_SRC (pad)); GST_PAD_CHECKGETRANGEFUNC (pad) = check; @@ -1567,23 +1565,36 @@ gst_pad_set_bufferalloc_function (GstPad * pad, gboolean gst_pad_unlink (GstPad * srcpad, GstPad * sinkpad) { + gboolean result = FALSE; + GstElement *parent = NULL; + g_return_val_if_fail (GST_IS_PAD (srcpad), FALSE); + g_return_val_if_fail (GST_PAD_IS_SRC (srcpad), FALSE); g_return_val_if_fail (GST_IS_PAD (sinkpad), FALSE); + g_return_val_if_fail (GST_PAD_IS_SINK (sinkpad), FALSE); GST_CAT_INFO (GST_CAT_ELEMENT_PADS, "unlinking %s:%s(%p) and %s:%s(%p)", GST_DEBUG_PAD_NAME (srcpad), srcpad, GST_DEBUG_PAD_NAME (sinkpad), sinkpad); + /* We need to notify the parent before taking any pad locks as the bin in + * question might be waiting for a lock on the pad while holding its lock + * that our message will try to take. */ + if ((parent = GST_ELEMENT_CAST (gst_pad_get_parent (srcpad)))) { + if (GST_IS_ELEMENT (parent)) { + gst_element_post_message (parent, + gst_message_new_structure_change (GST_OBJECT_CAST (srcpad), + GST_STRUCTURE_CHANGE_TYPE_PAD_UNLINK, parent, TRUE)); + } else { + gst_object_unref (parent); + parent = NULL; + } + } + GST_OBJECT_LOCK (srcpad); - if (G_UNLIKELY (GST_PAD_DIRECTION (srcpad) != GST_PAD_SRC)) - goto not_srcpad; - GST_OBJECT_LOCK (sinkpad); - if (G_UNLIKELY (GST_PAD_DIRECTION (sinkpad) != GST_PAD_SINK)) - goto not_sinkpad; - if (G_UNLIKELY (GST_PAD_PEER (srcpad) != sinkpad)) goto not_linked_together; @@ -1609,28 +1620,25 @@ gst_pad_unlink (GstPad * srcpad, GstPad * sinkpad) GST_CAT_INFO (GST_CAT_ELEMENT_PADS, "unlinked %s:%s and %s:%s", GST_DEBUG_PAD_NAME (srcpad), GST_DEBUG_PAD_NAME (sinkpad)); - return TRUE; + result = TRUE; -not_srcpad: - { - g_critical ("pad %s is not a source pad", GST_PAD_NAME (srcpad)); - GST_OBJECT_UNLOCK (srcpad); - return FALSE; - } -not_sinkpad: - { - g_critical ("pad %s is not a sink pad", GST_PAD_NAME (sinkpad)); - GST_OBJECT_UNLOCK (sinkpad); - GST_OBJECT_UNLOCK (srcpad); - return FALSE; +done: + if (parent != NULL) { + gst_element_post_message (parent, + gst_message_new_structure_change (GST_OBJECT_CAST (srcpad), + GST_STRUCTURE_CHANGE_TYPE_PAD_UNLINK, parent, FALSE)); + gst_object_unref (parent); } + return result; + + /* ERRORS */ not_linked_together: { /* we do not emit a warning in this case because unlinking cannot * be made MT safe.*/ GST_OBJECT_UNLOCK (sinkpad); GST_OBJECT_UNLOCK (srcpad); - return FALSE; + goto done; } } @@ -1793,26 +1801,16 @@ wrong_grandparents: static GstPadLinkReturn gst_pad_link_prepare (GstPad * srcpad, GstPad * sinkpad) { - /* generic checks */ - g_return_val_if_fail (GST_IS_PAD (srcpad), GST_PAD_LINK_REFUSED); - g_return_val_if_fail (GST_IS_PAD (sinkpad), GST_PAD_LINK_REFUSED); - GST_CAT_INFO (GST_CAT_PADS, "trying to link %s:%s and %s:%s", GST_DEBUG_PAD_NAME (srcpad), GST_DEBUG_PAD_NAME (sinkpad)); GST_OBJECT_LOCK (srcpad); - if (G_UNLIKELY (GST_PAD_DIRECTION (srcpad) != GST_PAD_SRC)) - goto not_srcpad; - if (G_UNLIKELY (GST_PAD_PEER (srcpad) != NULL)) goto src_was_linked; GST_OBJECT_LOCK (sinkpad); - if (G_UNLIKELY (GST_PAD_DIRECTION (sinkpad) != GST_PAD_SINK)) - goto not_sinkpad; - if (G_UNLIKELY (GST_PAD_PEER (sinkpad) != NULL)) goto sink_was_linked; @@ -1829,12 +1827,6 @@ gst_pad_link_prepare (GstPad * srcpad, GstPad * sinkpad) return GST_PAD_LINK_OK; -not_srcpad: - { - g_critical ("pad %s is not a source pad", GST_PAD_NAME (srcpad)); - GST_OBJECT_UNLOCK (srcpad); - return GST_PAD_LINK_WRONG_DIRECTION; - } src_was_linked: { GST_CAT_INFO (GST_CAT_PADS, "src %s:%s was already linked to %s:%s", @@ -1845,13 +1837,6 @@ src_was_linked: GST_OBJECT_UNLOCK (srcpad); return GST_PAD_LINK_WAS_LINKED; } -not_sinkpad: - { - g_critical ("pad %s is not a sink pad", GST_PAD_NAME (sinkpad)); - GST_OBJECT_UNLOCK (sinkpad); - GST_OBJECT_UNLOCK (srcpad); - return GST_PAD_LINK_WRONG_DIRECTION; - } sink_was_linked: { GST_CAT_INFO (GST_CAT_PADS, "sink %s:%s was already linked to %s:%s", @@ -1895,12 +1880,31 @@ GstPadLinkReturn gst_pad_link (GstPad * srcpad, GstPad * sinkpad) { GstPadLinkReturn result; + GstElement *parent; + + g_return_val_if_fail (GST_IS_PAD (srcpad), GST_PAD_LINK_REFUSED); + g_return_val_if_fail (GST_PAD_IS_SRC (srcpad), GST_PAD_LINK_WRONG_DIRECTION); + g_return_val_if_fail (GST_IS_PAD (sinkpad), GST_PAD_LINK_REFUSED); + g_return_val_if_fail (GST_PAD_IS_SINK (sinkpad), + GST_PAD_LINK_WRONG_DIRECTION); + + /* Notify the parent early. See gst_pad_unlink for details. */ + if ((parent = GST_ELEMENT_CAST (gst_pad_get_parent (srcpad)))) { + if (GST_IS_ELEMENT (parent)) { + gst_element_post_message (parent, + gst_message_new_structure_change (GST_OBJECT_CAST (srcpad), + GST_STRUCTURE_CHANGE_TYPE_PAD_LINK, parent, TRUE)); + } else { + gst_object_unref (parent); + parent = NULL; + } + } /* prepare will also lock the two pads */ result = gst_pad_link_prepare (srcpad, sinkpad); if (result != GST_PAD_LINK_OK) - goto prepare_failed; + goto done; /* must set peers before calling the link function */ GST_PAD_PEER (srcpad) = sinkpad; @@ -1946,12 +1950,16 @@ gst_pad_link (GstPad * srcpad, GstPad * sinkpad) GST_OBJECT_UNLOCK (sinkpad); GST_OBJECT_UNLOCK (srcpad); } - return result; -prepare_failed: - { - return result; +done: + if (parent) { + gst_element_post_message (parent, + gst_message_new_structure_change (GST_OBJECT_CAST (srcpad), + GST_STRUCTURE_CHANGE_TYPE_PAD_LINK, parent, FALSE)); + gst_object_unref (parent); } + + return result; } static void @@ -3242,7 +3250,7 @@ gst_pad_event_default_dispatch (GstPad * pad, GstEvent * event) break; } - if (GST_PAD_DIRECTION (eventpad) == GST_PAD_SRC) { + if (GST_PAD_IS_SRC (eventpad)) { /* for each pad we send to, we should ref the event; it's up * to downstream to unref again when handled. */ GST_LOG_OBJECT (pad, "Reffing and sending event %p (%s) to %s:%s", @@ -3287,7 +3295,7 @@ no_iter: * return TRUE. This is so that when using the default handler on a sink * element, we don't fail to push it. */ if (!pushed_pads) - result = (GST_PAD_DIRECTION (pad) == GST_PAD_SINK); + result = GST_PAD_IS_SINK (pad); g_list_free (pushed_pads); @@ -3957,8 +3965,7 @@ GstFlowReturn gst_pad_chain (GstPad * pad, GstBuffer * buffer) { g_return_val_if_fail (GST_IS_PAD (pad), GST_FLOW_ERROR); - g_return_val_if_fail (GST_PAD_DIRECTION (pad) == GST_PAD_SINK, - GST_FLOW_ERROR); + g_return_val_if_fail (GST_PAD_IS_SINK (pad), GST_FLOW_ERROR); g_return_val_if_fail (GST_IS_BUFFER (buffer), GST_FLOW_ERROR); return gst_pad_chain_unchecked (pad, buffer); @@ -4000,7 +4007,7 @@ gst_pad_push (GstPad * pad, GstBuffer * buffer) gboolean caps_changed; g_return_val_if_fail (GST_IS_PAD (pad), GST_FLOW_ERROR); - g_return_val_if_fail (GST_PAD_DIRECTION (pad) == GST_PAD_SRC, GST_FLOW_ERROR); + g_return_val_if_fail (GST_PAD_IS_SRC (pad), GST_FLOW_ERROR); g_return_val_if_fail (GST_IS_BUFFER (buffer), GST_FLOW_ERROR); GST_OBJECT_LOCK (pad); @@ -4111,7 +4118,7 @@ gst_pad_check_pull_range (GstPad * pad) g_return_val_if_fail (GST_IS_PAD (pad), FALSE); GST_OBJECT_LOCK (pad); - if (GST_PAD_DIRECTION (pad) != GST_PAD_SINK) + if (!GST_PAD_IS_SINK (pad)) goto wrong_direction; if (G_UNLIKELY ((peer = GST_PAD_PEER (pad)) == NULL)) @@ -4189,7 +4196,7 @@ gst_pad_get_range (GstPad * pad, guint64 offset, guint size, gboolean emit_signal; g_return_val_if_fail (GST_IS_PAD (pad), GST_FLOW_ERROR); - g_return_val_if_fail (GST_PAD_DIRECTION (pad) == GST_PAD_SRC, GST_FLOW_ERROR); + g_return_val_if_fail (GST_PAD_IS_SRC (pad), GST_FLOW_ERROR); g_return_val_if_fail (buffer != NULL, GST_FLOW_ERROR); GST_PAD_STREAM_LOCK (pad); @@ -4323,8 +4330,7 @@ gst_pad_pull_range (GstPad * pad, guint64 offset, guint size, gboolean emit_signal; g_return_val_if_fail (GST_IS_PAD (pad), GST_FLOW_ERROR); - g_return_val_if_fail (GST_PAD_DIRECTION (pad) == GST_PAD_SINK, - GST_FLOW_ERROR); + g_return_val_if_fail (GST_PAD_IS_SINK (pad), GST_FLOW_ERROR); g_return_val_if_fail (buffer != NULL, GST_FLOW_ERROR); GST_OBJECT_LOCK (pad); diff --git a/tests/check/gst/gstbin.c b/tests/check/gst/gstbin.c index f0d4e6f9c5..26558fba16 100644 --- a/tests/check/gst/gstbin.c +++ b/tests/check/gst/gstbin.c @@ -893,6 +893,82 @@ GST_START_TEST (test_iterate_sorted) GST_END_TEST; +static void +test_link_structure_change_state_changed_sync_cb (GstBus * bus, + GstMessage * message, gpointer data) +{ + GstPipeline *pipeline = GST_PIPELINE (data); + GstElement *src, *identity, *sink; + GstState old, snew, pending; + + sink = gst_bin_get_by_name (GST_BIN (pipeline), "sink"); + fail_unless (sink != NULL, "Could not get sink"); + + gst_message_parse_state_changed (message, &old, &snew, &pending); + if (message->src != GST_OBJECT (sink) || snew != GST_STATE_READY) { + gst_object_unref (sink); + return; + } + + src = gst_bin_get_by_name (GST_BIN (pipeline), "src"); + fail_unless (src != NULL, "Could not get src"); + + identity = gst_bin_get_by_name (GST_BIN (pipeline), "identity"); + fail_unless (identity != NULL, "Could not get identity"); + + /* link src to identity, the pipeline should detect the new link and + * resync the state change */ + fail_unless (gst_element_link (src, identity) == TRUE); + + gst_object_unref (src); + gst_object_unref (identity); + gst_object_unref (sink); +} + +GST_START_TEST (test_link_structure_change) +{ + GstElement *src, *identity, *sink, *pipeline; + GstBus *bus; + GstState state; + + pipeline = gst_pipeline_new (NULL); + fail_unless (pipeline != NULL, "Could not create pipeline"); + + bus = gst_pipeline_get_bus (GST_PIPELINE (pipeline)); + fail_unless (bus != NULL, "Could not get bus"); + + /* use the sync signal handler to link elements while the pipeline is still + * doing the state change */ + gst_bus_set_sync_handler (bus, gst_bus_sync_signal_handler, pipeline); + g_object_connect (bus, "signal::sync-message::state-changed", + G_CALLBACK (test_link_structure_change_state_changed_sync_cb), pipeline, + NULL); + + src = gst_element_factory_make ("fakesrc", "src"); + fail_if (src == NULL, "Could not create fakesrc"); + + identity = gst_element_factory_make ("identity", "identity"); + fail_if (identity == NULL, "Could not create identity"); + + sink = gst_element_factory_make ("fakesink", "sink"); + fail_if (sink == NULL, "Could not create fakesink1"); + + gst_bin_add_many (GST_BIN (pipeline), src, identity, sink, NULL); + + gst_element_set_state (pipeline, GST_STATE_READY); + gst_element_get_state (pipeline, NULL, NULL, GST_CLOCK_TIME_NONE); + + /* the state change will be done on src only if the pipeline correctly resyncs + * after that filesrc has been linked to identity */ + gst_element_get_state (src, &state, NULL, 0); + fail_unless_equals_int (state, GST_STATE_READY); + + gst_object_unref (bus); + gst_object_unref (pipeline); +} + +GST_END_TEST; + static Suite * gst_bin_suite (void) { @@ -913,6 +989,7 @@ gst_bin_suite (void) tcase_add_test (tc_chain, test_add_linked); tcase_add_test (tc_chain, test_add_self); tcase_add_test (tc_chain, test_iterate_sorted); + tcase_add_test (tc_chain, test_link_structure_change); return s; }