GNOME Bugzilla – Bug 785679
element: add gst_element_foreach_*pad()
Last modified: 2017-11-02 16:13:33 UTC
Created attachment 356714 [details] [review] element: add gst_element_foreach_*pad() Add convenience API that iterates over all pads, sink pads or source pads and makes sure that the foreach function is called exactly once for each pad. This is a KISS implementation. It doesn't use GstIterator and doesn't try to do clever things like resync if pads are added or removed while the function is executing. We can still do that in future if we think it's needed, but in practice it will likely make absolutely no difference whatsoever, since these things will have to be handled properly elsewhere by the element anyway if they're important. This is also a replacement for gst_aggregator_iterate_sink_pads().
I actually wrote a thread safe version using GstIterator of this in the flight. I'll attach my patch and the aggregator patches to use it.
Created attachment 356874 [details] [review] iterator: Add filter that returns each value only once. A common pattern is to want to iterate pads and do the action only once on each even if pads are added.
Created attachment 356875 [details] [review] audioaggregator: Remove use of unsafe gst_aggregator_iterate_sinkpads
Created attachment 356876 [details] [review] gl*mixer: Remove use of unsafe gst_aggregator_iterate_sinkpads
Created attachment 356877 [details] [review] videoaggregator: Remove ignored return value in prepare_frame
Created attachment 356878 [details] [review] videoaggregator: Remove use of unsafe gst_aggregator_iterate_sinkpads
Created attachment 356879 [details] [review] aggregator: Remove use of unsafe gst_aggregator_iterate_sinkpads
Created attachment 356880 [details] [review] aggregator: Remove pad iterator function This can be accomplished using the new safe gst_iterator_once() from the core.
My patch is thread-safe just fine :) I would argue that properly supporting iterator resync doesn't really make anything more thread-safe from a global element perspective, since if the function was called just a bit earlier the element would still have to handle the new pad correctly, and for removed pads it doesn't really matter anyway. This is for the normal case where pads are added/removed from another thread. The only case I can think of where it makes a difference is if pads get added/removed from the callback function itself. I quite dislike GstIterator, it causes allocs/frees and way too much lock/unlock, and your patch using GArray causes even more allocs/frees :/
Review of attachment 356714 [details] [review]: Oh I wasn't all there when I first read your patch, clearly it's thread safe. Probably indeed much more simple than my approach for similar result. ::: gst/gstelement.c @@ +1263,3 @@ + GST_OBJECT_LOCK (element); + n_pads = *p_npads; + pads = g_newa (GstPad *, n_pads + 1); why + 1 ?
> + GST_OBJECT_LOCK (element); > + n_pads = *p_npads; > + pads = g_newa (GstPad *, n_pads + 1); > > why + 1 ? Laziness, so we don't have to special-case/catch the n_pads=0 case inside the lock: I don't know if we can expect g_newa() to always behave well for n=0 - do you know if it's fine?
*** Bug 742152 has been marked as a duplicate of this bug. ***
Review of attachment 356714 [details] [review]: Looks generally good to me ::: gst/gstelement.c @@ +1296,3 @@ + * iterating pads and return early. If new sink pads are added or sink pads + * are removed while the sink pads are being iterated, this will not be taken + * into account until next time this function is used. This last sentence should be put in a warning block or similar :)
I don't think it needs to be a warning of any kind. The GstIterator API doesn't make anything any more thread-safe in this respect.
Fine with me. I've btw added something similar to the Rust bindings already, which just gives you a Vec<Pad> (and a Vec<Element> for Bin::get_children()). The worrying about resyncing with GstIterator makes the usage a bit awkward.
Comment on attachment 356714 [details] [review] element: add gst_element_foreach_*pad() commit d106390adce5cee837d2d0aab377d758a45a4b2d (HEAD -> master, origin/master, origin/HEAD) Author: Tim-Philipp Müller <tim@centricular.com> Date: Tue Aug 1 11:06:32 2017 +0100 element: add gst_element_foreach_*pad() Add convenience API that iterates over all pads, sink pads or source pads and makes sure that the foreach function is called exactly once for each pad. This is a KISS implementation. It doesn't use GstIterator and doesn't try to do clever things like resync if pads are added or removed while the function is executing. We can still do that in future if we think it's needed, but in practice it will likely make absolutely no difference whatsoever, since these things will have to be handled properly elsewhere by the element anyway if they're important. After all, it's always possible that a pad is added or removed just after the iterator finishes iterating, but before the function returns. This is also a replacement for gst_aggregator_iterate_sink_pads(). https://bugzilla.gnome.org/show_bug.cgi?id=785679
Comment on attachment 356874 [details] [review] iterator: Add filter that returns each value only once. Can still add something like this in future if the need arises.
Comment on attachment 356880 [details] [review] aggregator: Remove pad iterator function commit 98ac205a4d1a788df698682fbdff3505d9a254ea Author: Olivier Crête <olivier.crete@collabora.com> Date: Wed Aug 2 12:08:26 2017 -0400 aggregator: Remove pad iterator function Use new gst_element_foreach_sink_pad() from core instead. https://bugzilla.gnome.org/show_bug.cgi?id=785679
Comment on attachment 356879 [details] [review] aggregator: Remove use of unsafe gst_aggregator_iterate_sinkpads commit 1116bb00ca1dc0fc04c0029f9e3a9962606fa915 Author: Tim-Philipp Müller <tim@centricular.com> Date: Thu Nov 2 12:46:26 2017 +0000 aggregator: use new gst_element_foreach_sink_pad() Instead of gst_aggregator_iterate_sinkpads() which will soon be removed. https://bugzilla.gnome.org/show_bug.cgi?id=785679
Comment on attachment 356878 [details] [review] videoaggregator: Remove use of unsafe gst_aggregator_iterate_sinkpads commit 13c730776b765164408eddb7ab573cc15ac1557e Author: Tim-Philipp Müller <tim@centricular.com> Date: Thu Nov 2 12:46:26 2017 +0000 videoaggregator: use new gst_element_foreach_sink_pad() Instead of gst_aggregator_iterate_sinkpads() which will soon be removed. https://bugzilla.gnome.org/show_bug.cgi?id=785679
Comment on attachment 356875 [details] [review] audioaggregator: Remove use of unsafe gst_aggregator_iterate_sinkpads commit c7a8e57852b1c9706eec2c5010c14c6e3b464588 Author: Tim-Philipp Müller <tim@centricular.com> Date: Thu Nov 2 12:46:26 2017 +0000 audioaggregator: use new gst_element_foreach_sink_pad() Instead of gst_aggregator_iterate_sinkpads() which will soon be removed. https://bugzilla.gnome.org/show_bug.cgi?id=785679
Comment on attachment 356876 [details] [review] gl*mixer: Remove use of unsafe gst_aggregator_iterate_sinkpads commit 4576e0737da62bc06e430f7ef3c8c4647223c99a Author: Tim-Philipp Müller <tim@centricular.com> Date: Thu Nov 2 12:17:38 2017 +0000 gl: use new gst_element_foreach_sink_pad() Instead of gst_aggregator_iterate_sinkpads() which will soon be removed. https://bugzilla.gnome.org/show_bug.cgi?id=785679
Comment on attachment 356877 [details] [review] videoaggregator: Remove ignored return value in prepare_frame Didn't touch that, since it's still used now. Don't know if it still makes sense to remove it or not.