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 171735 - add an interface to gst-core that allows multi child elements
add an interface to gst-core that allows multi child elements
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gstreamer (core)
git master
Other Linux
: Normal enhancement
: 0.8.10
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks: 301488
 
 
Reported: 2005-03-26 18:00 UTC by Stefan Sauer (gstreamer, gtkdoc dev)
Modified: 2005-06-09 17:53 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
implementation of the interface (6.70 KB, text/plain)
2005-03-26 18:15 UTC, Stefan Sauer (gstreamer, gtkdoc dev)
  Details
header for the interface (2.62 KB, text/x-chdr)
2005-03-26 18:15 UTC, Stefan Sauer (gstreamer, gtkdoc dev)
  Details
include in build process (580 bytes, patch)
2005-03-26 18:16 UTC, Stefan Sauer (gstreamer, gtkdoc dev)
none Details | Review
include it in the docs (920 bytes, patch)
2005-03-26 18:18 UTC, Stefan Sauer (gstreamer, gtkdoc dev)
none Details | Review
implementation of the interface (11.36 KB, patch)
2005-04-08 12:23 UTC, Stefan Sauer (gstreamer, gtkdoc dev)
none Details | Review
2nd version of the implementation of the interface (12.30 KB, patch)
2005-04-11 17:35 UTC, Stefan Sauer (gstreamer, gtkdoc dev)
none Details | Review
cleanup of the patch (19.23 KB, patch)
2005-04-12 00:10 UTC, Benjamin Otte (Company)
none Details | Review
2nd cleanup of the patch (17.02 KB, patch)
2005-04-12 19:02 UTC, Stefan Sauer (gstreamer, gtkdoc dev)
none Details | Review

Description Stefan Sauer (gstreamer, gtkdoc dev) 2005-03-26 18:00:38 UTC
see
http://www.buzztard.org/index.php/Polyphonic_elemnts
for a discussion. The attached code will implement idea 3
Comment 1 Stefan Sauer (gstreamer, gtkdoc dev) 2005-03-26 18:15:13 UTC
Created attachment 39288 [details]
implementation of the interface
Comment 2 Stefan Sauer (gstreamer, gtkdoc dev) 2005-03-26 18:15:48 UTC
Created attachment 39289 [details]
header for the interface
Comment 3 Stefan Sauer (gstreamer, gtkdoc dev) 2005-03-26 18:16:59 UTC
Created attachment 39290 [details] [review]
include in build process
Comment 4 Stefan Sauer (gstreamer, gtkdoc dev) 2005-03-26 18:18:21 UTC
Created attachment 39291 [details] [review]
include it in the docs
Comment 5 Stefan Sauer (gstreamer, gtkdoc dev) 2005-04-08 11:35:49 UTC
the code has been commited to BRANCH-GSTREAMER-0_8
Comment 6 Stefan Sauer (gstreamer, gtkdoc dev) 2005-04-08 12:23:48 UTC
Created attachment 39835 [details] [review]
implementation of the interface
Comment 7 Benjamin Otte (Company) 2005-04-08 14:21:18 UTC
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
Comment 8 Ronald Bultje 2005-04-08 15:18:33 UTC
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.
Comment 9 Stefan Sauer (gstreamer, gtkdoc dev) 2005-04-11 07:13:53 UTC
@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.
Comment 10 Ronald Bultje 2005-04-11 08:08:16 UTC
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.
Comment 11 Stefan Sauer (gstreamer, gtkdoc dev) 2005-04-11 16:49:20 UTC
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.
Comment 12 Stefan Sauer (gstreamer, gtkdoc dev) 2005-04-11 17:35:58 UTC
Created attachment 45146 [details] [review]
2nd version of the implementation of the interface
Comment 13 Benjamin Otte (Company) 2005-04-12 00:10:06 UTC
Created attachment 45154 [details] [review]
cleanup of the patch

includes:
- pipeline parsing is parent aware
- GstBin implements GstParent
- recursiveness
Comment 14 Ronald Bultje 2005-04-12 07:41:11 UTC
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.
Comment 15 David Schleef 2005-04-12 17:25:33 UTC
I haven't been following this discussion too closely, but I agree with the
assessment -- don't fear putting features into the core.
Comment 16 Stefan Sauer (gstreamer, gtkdoc dev) 2005-04-12 19:02:08 UTC
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).
Comment 17 Ronald Bultje 2005-04-15 09:51:52 UTC
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? :).
Comment 18 Stefan Sauer (gstreamer, gtkdoc dev) 2005-04-18 09:55:17 UTC
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'
Comment 19 Ronald Bultje 2005-04-18 10:01:18 UTC
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
Comment 20 Benjamin Otte (Company) 2005-04-18 10:02:28 UTC
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? ;)
Comment 21 Stefan Sauer (gstreamer, gtkdoc dev) 2005-04-21 17:29:13 UTC
its now in CVS, please close this issue if its fine.
Comment 22 Ronald Bultje 2005-06-09 17:53:29 UTC
This was applied, so closing bug.