After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 785679 - element: add gst_element_foreach_*pad()
element: add gst_element_foreach_*pad()
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gstreamer (core)
git master
Other Linux
: Normal enhancement
: 1.13.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
: 742152 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2017-08-01 11:17 UTC by Tim-Philipp Müller
Modified: 2017-11-02 16:13 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
element: add gst_element_foreach_*pad() (11.47 KB, patch)
2017-08-01 11:17 UTC, Tim-Philipp Müller
committed Details | Review
iterator: Add filter that returns each value only once. (7.25 KB, patch)
2017-08-03 16:00 UTC, Olivier Crête
none Details | Review
audioaggregator: Remove use of unsafe gst_aggregator_iterate_sinkpads (2.90 KB, patch)
2017-08-03 16:02 UTC, Olivier Crête
none Details | Review
gl*mixer: Remove use of unsafe gst_aggregator_iterate_sinkpads (4.59 KB, patch)
2017-08-03 16:02 UTC, Olivier Crête
none Details | Review
videoaggregator: Remove ignored return value in prepare_frame (4.91 KB, patch)
2017-08-03 16:02 UTC, Olivier Crête
needs-work Details | Review
videoaggregator: Remove use of unsafe gst_aggregator_iterate_sinkpads (4.16 KB, patch)
2017-08-03 16:02 UTC, Olivier Crête
none Details | Review
aggregator: Remove use of unsafe gst_aggregator_iterate_sinkpads (5.53 KB, patch)
2017-08-03 16:02 UTC, Olivier Crête
none Details | Review
aggregator: Remove pad iterator function (4.70 KB, patch)
2017-08-03 16:02 UTC, Olivier Crête
committed Details | Review

Description Tim-Philipp Müller 2017-08-01 11:17:22 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().
Comment 1 Olivier Crête 2017-08-03 16:00:29 UTC
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.
Comment 2 Olivier Crête 2017-08-03 16:00:41 UTC
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.
Comment 3 Olivier Crête 2017-08-03 16:02:00 UTC
Created attachment 356875 [details] [review]
audioaggregator: Remove use of unsafe gst_aggregator_iterate_sinkpads
Comment 4 Olivier Crête 2017-08-03 16:02:05 UTC
Created attachment 356876 [details] [review]
gl*mixer: Remove use of unsafe gst_aggregator_iterate_sinkpads
Comment 5 Olivier Crête 2017-08-03 16:02:10 UTC
Created attachment 356877 [details] [review]
videoaggregator: Remove ignored return value in prepare_frame
Comment 6 Olivier Crête 2017-08-03 16:02:14 UTC
Created attachment 356878 [details] [review]
videoaggregator: Remove use of unsafe gst_aggregator_iterate_sinkpads
Comment 7 Olivier Crête 2017-08-03 16:02:18 UTC
Created attachment 356879 [details] [review]
aggregator: Remove use of unsafe gst_aggregator_iterate_sinkpads
Comment 8 Olivier Crête 2017-08-03 16:02:22 UTC
Created attachment 356880 [details] [review]
aggregator: Remove pad iterator function

This can be accomplished using the new safe gst_iterator_once() from
the core.
Comment 9 Tim-Philipp Müller 2017-08-03 16:15:17 UTC
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 :/
Comment 10 Olivier Crête 2017-08-03 19:24:33 UTC
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 ?
Comment 11 Tim-Philipp Müller 2017-08-03 19:41:52 UTC
> +  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?
Comment 12 Sebastian Dröge (slomo) 2017-10-17 08:56:05 UTC
*** Bug 742152 has been marked as a duplicate of this bug. ***
Comment 13 Sebastian Dröge (slomo) 2017-10-24 09:27:53 UTC
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 :)
Comment 14 Tim-Philipp Müller 2017-10-24 17:01:07 UTC
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.
Comment 15 Sebastian Dröge (slomo) 2017-10-25 11:35:13 UTC
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 16 Tim-Philipp Müller 2017-11-02 16:02:53 UTC
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 17 Tim-Philipp Müller 2017-11-02 16:03:27 UTC
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 18 Tim-Philipp Müller 2017-11-02 16:07:53 UTC
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 19 Tim-Philipp Müller 2017-11-02 16:08:22 UTC
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 20 Tim-Philipp Müller 2017-11-02 16:09:03 UTC
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 21 Tim-Philipp Müller 2017-11-02 16:10:39 UTC
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 22 Tim-Philipp Müller 2017-11-02 16:11:19 UTC
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 23 Tim-Philipp Müller 2017-11-02 16:12:27 UTC
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.