GNOME Bugzilla – Bug 435880
[GstBin] Property to allow bins to handle child async changes
Last modified: 2007-05-22 11:10:53 UTC
When modifying the state of childs of a bin in a different way than standard, that bin becomes responsible for handling the state changes of element that change state asynchronously. With the current async-start/async-done additions, this doesn't happen anymore if the bin is not a toplevel one (GstPipeline). There should be a property on GstBin to specify that the given bin needs to respond to async-start/async-done itself, not post them upstream, and start state-continuation threads in order to move the async-changing elements to the proper state.
Created attachment 87545 [details] [review] New property patch This patch adds a 'async-handling' boolean property. If set to TRUE, the bin will behave as a toplevel bin in regards to async-start/async-done message handling. Tested with GNonLin, seems to work fine.
Two small nitpicks and a question: - there should be a gtk-doc blurb in the class_init function for the new property with a 'Since: 0.10.13' line, so it shows up in the docs that it's a new addition - the padding adjustment doesn't look right to me, I don't think you can count on sizeof(gboolean)/sizeof(int) == sizeof(gpointer). Maybe it would be better to add a private struct for this, like GstBus has with struct _GstBusPrivate? That would keep the padding free for future public additions that may be required. Dunno, just a thought. - should we also add a setter and getter function, ie. gst_bin_set_async_handling () gst_bin_get_async_handling () ? (also, somehow the phrase 'async handling' doesn't seem very boolean-y to me, but I can't really think of anything better either besides 'do-async-handling' or 'handle-async-states')
(In reply to comment #2) > Two small nitpicks and a question: > > - there should be a gtk-doc blurb in the class_init function > for the new property with a 'Since: 0.10.13' line, so it > shows up in the docs that it's a new addition Indeed, this was a first throw to get general feeling about the patch and feedback. Don't worry, docs will be there :) > > > - the padding adjustment doesn't look right to me, I don't think > you can count on sizeof(gboolean)/sizeof(int) == sizeof(gpointer). Then something must be broken in the ABI tests (run make check). It didn't fail on my machine... which is a 64bit machine (?!?!?). > > Maybe it would be better to add a private struct for this, like > GstBus has with struct _GstBusPrivate? That would keep the padding > free for future public additions that may be required. Dunno, just > a thought. Good idea, will do that. > > > - should we also add a setter and getter function, ie. > gst_bin_set_async_handling () > gst_bin_get_async_handling () > ? Dunno really... > > > (also, somehow the phrase 'async handling' doesn't seem very boolean-y to me, > but I can't really think of anything better either besides 'do-async-handling' > or 'handle-async-states') > indeed. Thanks for the input.
> > - the padding adjustment doesn't look right to me, I don't think > > you can count on sizeof(gboolean)/sizeof(int) == sizeof(gpointer). > > Then something must be broken in the ABI tests (run make check). It didn't > fail on my machine... which is a 64bit machine (?!?!?). There are platforms with 32-bit ints and 64-bit pointers IIRC (IA-64 maybe?)
> There are platforms with 32-bit ints and 64-bit pointers IIRC (IA-64 maybe?) Yeah, all of them. (ok, not really, but *almost*) Most importantly, x86_64-linux has 32-bit ints and 64-bit pointers.
In this case, you're being saved by internal structure padding leaving an extra 32-bits space between your new gboolean and our padding array. The patch can't rely on that on all platforms. Tim's suggestion of a private structure sounds good. Otherwise, I want to be sure this API is correct (which it looks like to me, but what do I know?) and that it won't need changing - we are right before a release, and adding API that won't have much time for testing makes me nervous.
Created attachment 88584 [details] [review] Updated patch Cleaned up the patch: * Now uses a private structure for holding the value. * property is documented I am not adding the convenience functions since: * it is a feature that should only be used by a very small range of classes * we are too close to the release to start adding public API for this.
OK, this looks alright to me.
2007-05-22 Edward Hervey <edward@fluendo.com> * docs/gst/gstreamer-sections.txt: * gst/gstbin.c: (gst_bin_class_init), (gst_bin_init), (gst_bin_dispose), (gst_bin_set_property), (gst_bin_get_property), (gst_bin_remove_func), (gst_bin_handle_message_func): * gst/gstbin.h: Add a property for bins that handle the state change of their childs. Fixes #435880