GNOME Bugzilla – Bug 747220
aggregator: Does not unlock sink pads on downstream error
Last modified: 2015-04-02 20:01:21 UTC
This example pipelines that don't fail (and stop) as they should. Expect an not-linked error: gst-launch-1.0 audiotestsrc ! audiomixer name=m latency=100000 audiotestsrc is-live=1 ! identity drop-probability=1 ! m.
Created attachment 300786 [details] [review] aggregator: Unify code to set a pad flushing
Created attachment 300787 [details] [review] aggregator: Reset pending_eos on pad flush
Created attachment 300788 [details] [review] aggregator: Flushing is always in pad lock, no need to atomics The usage of atomics was always doubtful as it was used to release a GCond
Created attachment 300789 [details] [review] aggregator: Unify downstream flow return and flushing Also means that having a non-OK downstream flow return wakes up the chain functions.
Review of attachment 300786 [details] [review]: OK
Review of attachment 300788 [details] [review]: Apart from those 2 comments, looks good. ::: gst-libs/gst/base/gstaggregator.c @@ -1802,3 @@ PAD_FLUSH_LOCK (aggpad); - if (g_atomic_int_get (&aggpad->priv->flushing) == TRUE) I do not recall the exact case, but that was necessary in some corner cases. @@ -1905,3 @@ PAD_LOCK (aggpad); - if (g_atomic_int_get (&aggpad->priv->flushing) == TRUE Same here, I am pretty sure that this code path is necessary.
Review of attachment 300787 [details] [review]: Indeed.
Review of attachment 300789 [details] [review]: ::: gst-libs/gst/base/gstaggregator.c @@ +632,2 @@ static void +gst_aggregator_pad_set_flushing (GstAggregatorPad * aggpad, Naming is weird, it is not really setting to flushing as it also sets the FlowReturn now. @@ +691,3 @@ GST_LOG_OBJECT (self, "flow return is %s", gst_flow_get_name (flow_return)); + if (flow_return != GST_FLOW_OK) { I am not sure we should use an iterator here as if a pad gets added meanwhile its flow return will be set before it has a chance to do anything. I have the impression it would be better to take the object lock and iterate normally.
(In reply to Thibault Saunier from comment #6) > ::: gst-libs/gst/base/gstaggregator.c > - if (g_atomic_int_get (&aggpad->priv->flushing) == TRUE) > > I do not recall the exact case, but that was necessary in some corner cases. Now that the lock is held everywhere where the flushing state changes, the while body is never entered if the flushing flag was set before the lock is taken, so this if was just duplicated.
Fix Thibault's comment and removed the iterator, and merged: commit 43d4d3c5ca4fa70547b9aba29e0a1eefcfe4c049 Author: Olivier Crête <olivier.crete@collabora.com> Date: Wed Apr 1 22:10:11 2015 -0400 aggregator: Unify downstream flow return and flushing Also means that having a non-OK downstream flow return wakes up the chain functions. https://bugzilla.gnome.org/show_bug.cgi?id=747220 commit b9a422d8d0c2c68e86db2ebc7389dd8e7fcbbd07 Author: Olivier Crête <olivier.crete@collabora.com> Date: Wed Apr 1 21:45:01 2015 -0400 aggregator: Flushing is always in pad lock, no need to atomics The usage of atomics was always doubtful as it was used to release a GCond https://bugzilla.gnome.org/show_bug.cgi?id=747220 commit d1bae208029657afd82b359fe39060cefb54d672 Author: Olivier Crête <olivier.crete@collabora.com> Date: Wed Apr 1 21:38:11 2015 -0400 aggregator: Reset pending_eos on pad flush https://bugzilla.gnome.org/show_bug.cgi?id=747220 commit 38d8db801aa1c4393cd0a853425e0b1a0b64ae94 Author: Olivier Crête <olivier.crete@collabora.com> Date: Wed Apr 1 21:37:25 2015 -0400 aggregator: Unify code to set a pad flushing https://bugzilla.gnome.org/show_bug.cgi?id=747220