Created attachment 39288 [details]
implementation of the interface

Created attachment 39289 [details]
header for the interface

Created attachment 39290 [details] [review]
include in build process

Created attachment 39291 [details] [review]
include it in the docs

the code has been commited to BRANCH-GSTREAMER-0_8

Created attachment 39835 [details] [review]
implementation of the interface

My important comments from our IRC discussion:
<Company> ensonic: the "property name '%s' has no '::' separator" should be
g_warning's
<Company> ensonic: and you need to check that the child in gst_parent_get does exist
<Company> you have to collect the values yourself with
G_VALUE_COLLECT/G_VALUE_LCOPY and use g_object_get/set_property
<Company> allow gst_parent_get to be used without "::"
<Company> i still wonder if we should implement it recursively so that
gst_parent_get (object, "instrument1::voice7::note" &note, NULL) would work
<Company> ensonic: the interface is missing a way to get names for objects

nagging/layout:
* comments don't use // .., but /* .. */
* use code blocks, don't clump all lines together in a single function

functionality:
* it is not recursive, although that isn't all too hard. can you add that?
* adding this to core would at the very least imply that something in core
requires it (e.g. you want GstBin to implement this), or that it is incredibly
important and basic. I don't see either.
* I'd suggest to use GstObject, not GObject, as base type of contained items,
since you require names of childs. qdata is a gross hack.
* GstParent is not a very good name. It clashes with functions such as
gst_object_get_parent(). Maybe not literally, but functionally/understandingly.
Even if you implement this in GstBin, a child may still be contained in a bin
(which _get_parent() returns), but also be the child of a non-bin element
implementing this interface on his own.

on the idea itself:
* all this interface does, is provide ways to set properties of children through
a parent. That is interesting, but what is the practical need? What is your use
case? What exact problem do you want to solve through this? Can you give
examples? I don't see this beloning in core without at least gstbin implementing
this, and even then do I consider it questionable at the very least.

@Benjamin: I don't get the G_VALUE_COLLECT/G_VALUE_LCOPY part?

@Ronald:
* layout: when committing: gst-indent will change the layout anyway
* naming: what about GstMultiChild?
* the idea:
please have a look at the URL mentioned at the top. There you'll find use cases.
To repeat it, gst-lauch/gst-inspect will use the interface. That's why I would
like to see it in core.

I want it documented at the place where it is committed. Pointers to URLs that
may not exist at a later date are not good enough. They may disappear. I want it
all in one place.

gst-launch is not a good reason to put it in core. gst-launch is a *test toy*,
nothing more. Anything serious will need test apps anyway. Same for gst-inspect.
gst-inspect doesn't link to gstinterfaces either, same reason. Is there a
*technical* reason why *core*, so libgstreamer-0.8.so, would need to link this
in? You can put a copy of gst-launch or gst-inspect in the package where we
commit this if you really need it, other interfaces have local test apps for
that as well. If gst-launch is the only reason, then I automatically conclude
that this does not necessarily need to go in core.
Again, just to be clear:
* gst-launch is not an all-featured demo tool for gstreamer.
* gst-launch is not for end users.
* gst-launch can not be a reason to put stuff in core.
* gst-launch merely has some basic features to make testing certain features for
certain developers easier that is all.
* we should never think that gst-launch is important in any way; it is not.
* s/gst-launch/gst{-inspect,etc}/: same.

As for layout: // etc. are not done by gst-indent, and it also does not take
care of newlines. I didn't commit on other layout stuff *because* gst-indent
does take care of the other concerns I had there. :).

-

I'm not against this patch, I'm trying to make it look better and make it fit in
with the basic requirements that I have for all patches. Other people (Wim,
Benjamin) will do the same. I do that to keep code quality high in gstreamer. My
requirements are not bad or outrageous at all, and have a really obvious goal.

Purpose of the Interface:
1.) it tells the application that this element handles multiple children
2.) it allows to control the properties of the children

Potential use:
Personally I need it fro audio generator plugins (softwynths). They have
multiple voices. This needs another two methods to add and remove voices (which
are specific to this audio_case). Therefore I have them in another interface
(GstPolyVoice). This second iface can be put into gst-plugins.
Another kind of plugins that can use it are mixers. Mixers have multiple
incomming streamers and would like to expose controls per pad (volume for audio,
alpha for video).
GstBin could use it too. Especially when the interface handles recursion, it
offers a convinient way to access child-properties.

Finally gst-inspect would *need* to show the child-properties. Otherwise the
output would not reflect the capabilities of the plugin.
And this questions the purpose of gst-lauch.
And having gst-launch to support this would be good for developers too.

Open questions:
1.) How to call it?
    GstParent: clashes with other method names
    GstContainer: it is no a container
    GstMultiChild
    GstMultiChildControl

2.) Where to put it?
    Question clearly is core or plugins.
    That boils down to GstBin, gst-inspect, gst-launch.

Created attachment 45146 [details] [review]
2nd version of the implementation of the interface

Created attachment 45154 [details] [review]
cleanup of the patch

includes:
- pipeline parsing is parent aware
- GstBin implements GstParent
- recursiveness

Here's a short summary of what Benjamin and I were discussing yesterday:

* it would be nice if this interface was implemented, be it as virtual
function-based interface or ginterface-based interface, in GstElement/GstBin.
That would show the obviousness of the core implementation, would make it
recursive automatically, and allows derived elements to implement their own
virtual function for non-standard behaviour (e.g. per-channel options etc.).
This requires more than a get/set for retrospectivity. It basically is a short
wrapper for gobject properties.
* once you understand the above (you may not get a **bling** directly), then
think: is this useful? Will every app in the world use it instead of
g_object_set()? If yes: then it should be in core, because it fits together with
other stuff that we're trying to centralize, such as events/queries (which, in
0.9, you can send to a bin instead of to sinks), etc. The answer to this
question is the same answer as the one that applies to the "should this be in
core" question. I'm thinking about this one right now.

I haven't been following this discussion too closely, but I agree with the
assessment -- don't fear putting features into the core.

Created attachment 45189 [details] [review]
2nd cleanup of the patch

- fixed build issues with docs
- little cleanup in gst_parent_lookup and the bin impl.

About the naming:
A descriptive name will likly to be quite long, e.g. GstChildPropertyDelegate
or GstChildPropertyProxy or even GstMultiChildPropertyProxy ... (you'll get the
idea).

OK, with the GstBin implementation, we're getting somewhere.

- The child-added/removed signals are unused and unimplemented, please either
implement or remove. Will this interface be used in a dynamic way?
- Is it just me, or is the get_child_count()/get_child_by_index() stuff a bit on
crack? You can do it in one GList* vfunc (get_child_list()) and implement the
rest as utility code. This makes implementations smaller.

As for the rest, I'm OK with this. One thing I'd appreciate, but not require, is
a testcase in which you use the GstBin implementation, i.e. have a bin called
bin0, add a child called fakesrc0 (and a fakesink0) and set the property
fakesrc0::num-buffers to, say, 10. Iterate pipeline and make sure that works.
this is both good for the testsuite and shows a practical use for this interface.

About naming: not too long please. :). Coders hate long names. I'm ok with
'GstPropertyProxy' or something like that ('GstPropertyDelegate', etc.). No need
to explicitely mention 'child', that is already obvious enough if you ask me; a
bin isn't called GstChildBin, is it? :).

I personally don't need the signals, but are happy to implement them.

get_child_count()/get_child_by_index():
True, the list is a bit more flexible. Even if the element needs to create one.

my naming favourite: 'GstPropertyProxy'

OK, so:
* can you remove the signals
* can you move from count/by_index to list
* rename
* add a test like I described above

then you're free to commit

Lists suck as vfuncs because they:
a) return internal memory or
b) require a copy
c) scale O(N)

As for the signals: Haven't they been allowed on GInterface's forever? IIRC it
was just properties that were added later, so it should be no problem to use them.
And yes, I'd like test cases too, I was just too lazy to write them. :)

Don't name it GstPropertyProxy, that name sucks. There's no reason to not use
short names. (And if GstParent confuses anyone wrt gst_object_get_parent, then
how about GstDad? ;)

its now in CVS, please close this issue if its fine.

This was applied, so closing bug.