A small clarification. I use playbin2 to decode a video/audio stream, and the real problem is that the element-added signal is not generated when the decoders are added. This makes it impossible to set properties on these if needed. The fact that there is no element added signal for the sink is a bit less serious since the sink can be set as a property.

Have you tried "deep-notify::element-added" as well?

Not sure how the deep-notify signal would help. If I understand that signal correct it only fires when a property has changed, but the decoder element is not a property of playbin so I would never get a signal.

Maybe you should look at FsElementAddedNotifier from farsight2, it is basically a "deep-element-added"

Patrick: you are perfectly right of course. Not sure what I was thinking (it was late) :)

What's your use case exactly? What kind of properties do you need to set?

I think the play was that playbin2 was only going to add the playsink bin when going from NULL->READY and remove it again when doing READY->NULL.

I just tested again and now I actually get a signal when decodebin and what's in it gets added! I haven't tested this since I upgraded to gst-plugins-base 10-22. Either I did something wrong before or something has changed, either way all that's in decodebin is now accessible, which is good since that was the most serious problem. 

The only thing still not signaled is the sink part. This is less serious since you can set your own video/audio sink as a property on playbin, but for consistency I think it would still be nice if that worked to. Also, perhaps someone needs to be able to set a property on some of the other elements of vbin or abin? It simply gives an application greater flexibility if all elements of the pipeline is available.

I had a look in playbin2.c and found that the reason that elemtent-added doesn't work for the sink is that the playsink element is added to playbin inside the playbin init function. If this is instead done in the state change function when going from NULL to READY it works a bit better. Then you get a signal for playsink, vbin and abin. However for some unknown reason you still don't get a signal for the elements being added to vbin and abin.


Tim, I'm using decoder and sink for and embedded system using TI hardware. Both elements have a number of properties that needs to be set, device file names, buffer sizes, output resolutions etc etc.
Since playbin can't know anything about this it's up to the application to set these properties 'manually'. This is why I must have access to these elements.

You can get playsink from playbin2 by using the GstBin API to get the children of playbin2. You can then get the abin and vbin from playsink by looking at the element-added signal and the elements inside this bins by using the GstBin API again (they are added to abin/vbin before abin/vbin is added to playsink).

Not sure what we can do about this to make it easier

(In reply to comment #8)
> You can get playsink from playbin2 by using the GstBin API to get the children
> of playbin2. You can then get the abin and vbin from playsink by looking at the
> element-added signal and the elements inside this bins by using the GstBin API
> again (they are added to abin/vbin before abin/vbin is added to playsink).
> 
> Not sure what we can do about this to make it easier

What about wims suggestion in comment #6? That sounds good to me.

How would that really help? There are still other bins that contain elements before going to READY. Or would you suggest to disallow this and require all bins to add elements when going to READY and removing them again when going back to NULL?

Btw, something like the deep-element-added idea would be nice to have IMHO

(In reply to comment #10)
> How would that really help? There are still other bins that contain elements
> before going to READY. Or would you suggest to disallow this and require all
> bins to add elements when going to READY and removing them again when going
> back to NULL?

Yes, that would be the idea. Otherwise people won't catch elements that are added at bin_init time.

> 
> Btw, something like the deep-element-added idea would be nice to have IMHO

(In reply to comment #11)
> (In reply to comment #10)
> > How would that really help? There are still other bins that contain elements
> > before going to READY. Or would you suggest to disallow this and require all
> > bins to add elements when going to READY and removing them again when going
> > back to NULL?
> 
> Yes, that would be the idea. Otherwise people won't catch elements that are
> added at bin_init time.

Or when a child bin is added to a parent bin, a deep-element-added signal is emitted from the parent bin for the child bin and *all* children of that child bin.

*** Bug 708889 has been marked as a duplicate of this bug. ***

*** Bug 753637 has been marked as a duplicate of this bug. ***

playbin "element-setup" is not enough for what people have been using FsElementAddedNotifier for, we need it also for encodebin, farstream, etc. Basically, anything that does some kind of auto-plugging. The current implementation is not amazing and we could do something much better inside GstBin. Maybe have GstBin call an internal function on the parent GstBin to emit the "deep-element-added" signal? With a helper function that hooks the signal and calls the callback on all current elements, so it can be easily done race-free, which is a big feature of FsElementAddedNotifier

Of course. I think the general feeling is just that something nicer/better can be done from inside GstBin/GStreamer.

And that's also where it belongs IMHO, and is more discoverable for users.

Created attachment 327864 [details] [review]
bin: add "element-added-deep" signal

This means applications and bin sub-classes can easily track when a new child element is added to the pipeline sub-hierarchy.

Created attachment 327865 [details] [review]
playbin: add "element-setup" signal

Allows configuration of plugged elements.

Comment on attachment 327864 [details] [review]
bin: add "element-added-deep" signal

This seems asymmetric :) Please add a element_removed_deep too. Also I don't like the name, what about deep-element-added/removed? It's also deep-notify and not notify-deep.

(In reply to Sebastian Dröge (slomo) from comment #20)
> Comment on attachment 327864 [details] [review] [review]
> bin: add "element-added-deep" signal
> 
> This seems asymmetric :) Please add a element_removed_deep too. Also I don't
> like the name, what about deep-element-added/removed? It's also deep-notify
> and not notify-deep.

Also maybe when a bin is added to another bin, we would maybe want to recursively emit the signal for everything that is inside the bin? The behaviour should be documented in any case (i.e. that it doesn't do that currently)

Review of attachment 327865 [details] [review]:

::: gst/playback/gstplaybin2.c
@@ +3041,3 @@
+{
+  GST_ERROR_OBJECT (playbin, "element %" GST_PTR_FORMAT " was added to "
+      "%" GST_PTR_FORMAT, child, sub_bin);

Debug leftover, this should probably be GST_DEBUG_OBJECT() instead

- Name: yes, I didn't like it either, "deep-element-added" works for me

- asymetric: sure, I was going to add an element-removed equivalent too, just didn't make the patch yet.

- I'll try to make it work for the existing hierarchy of a bin being added

Created attachment 327935 [details] [review]
bin: add "deep-element-added" and "deep-element-removed" signals

Created attachment 327936 [details] [review]
bin: emit deep-element-{added,removed} for children of newly-added/removed bin

Created attachment 327937 [details] [review]
playbin: add "element-setup" signal

It's symmetric now (add + remove), but we use two more vfunc slots of course, so only two left now.

Comment on attachment 327937 [details] [review]
playbin: add "element-setup" signal

You don't emit the signal anywhere :)

Review of attachment 327936 [details] [review]:

::: gst/gstbin.c
@@ +1139,3 @@
+      ires = gst_iterator_foreach (it, bin_deep_iterator_foreach, &elements);
+      if (ires != GST_ITERATOR_DONE) {
+        g_queue_foreach (&elements, (GFunc) g_object_unref, NULL);

You can also get RESYNC which is not really a problem and you just iterate again

@@ +1146,3 @@
+      GstElement *e;
+
+      while ((e = g_queue_pop_head (&elements))) {

Why don't you just emit in the foreach function? No lock is taken (which is why you can get RESYNC).

> You can also get RESYNC which is not really a problem and
> you just iterate again

Yes, this is handled two lines below those lines you quoted, the while() repeats if we get a RESYNC, but we need to clear the list if we get a RESYNC (or ERROR) otherwise we get duplicate entries in the list.

 
> @@ +1146,3 @@
> +      GstElement *e;
> +
> +      while ((e = g_queue_pop_head (&elements))) {
> 
> Why don't you just emit in the foreach function? No lock is taken (which is
> why you can get RESYNC).

Because then in case of RESYNC we start over and would possibly emit the signal multiple times for the same element, which is not nice. Or obsolete elements. Unless we keep track of the already-emitted elements in a list, which we could also do I suppose (has the advantage that we don't need to put a ref in the list and can just check pointer values).

Doing it the way it is now has the advantage that you are guaranteed to emit the signals for a specific snapshot of the elements at one point in time. Any other way and it might be a mix of old and new I think.

Created attachment 327961 [details] [review]
playbin: add "element-setup" signal

This time with actual signal emission :)