GNOME Bugzilla – Bug 117342
[PATCH] Change types for callbacks from G_TYPE_POINTER to more specific type (eg, GST_TYPE_ELEMENT, etc).
Last modified: 2005-08-15 01:50:29 UTC
There are many callbacks in the gst core, but a surprising number of them use G_TYPE_POINTER as the callback data type. Now, this works fine for C code where, in the callback functions, you can cast on a case-by-case basis, but when wrapping the API for another language, the lack of a specific type makes it exceedingly difficult to write generic code for marshalling callback types into their wrapped equivalents. Now, the only controversial case involves boxed types. There are a few objects (I believe GST_CAPS is one of them) which are derived from G_BOXED_TYPE, but also have their own type macros (eg, GST_TYPE_CAPS). The controversy comes in when determing which type to use for the callback data (G_BOXED_TYPE or the specific type). Now, my contention is that, if we have the type macros, why not use them? Put a little differently, if we're *not* allowed to use GST_TYPE_CAPS (for example) to specify the data type, why have it available at all?
Created attachment 18265 [details] [review] Patch which changes generic callbacks data types to the specific ones. This patch uses specific types, even if the object is a boxed type.
We should look up how gtk+ / glib do that and how it's supposed to be done and then do it exactly that way. Or ask someone of mthose developers hwho know about it. gtk+ / glib are open source after all. ;)
Note that you can't actually use G_TYPE_BOXED in a signal. You can use GST_TYPE_CAPS, though. I'll apply this when I get a chance and/or remember. Have you checked signals in the plugins?
No, the attached patch only applies to the core. I can certainly scour gst-plugins as well, although that should probably have a separate bug reported on it. BTW, the current patch appears to miss one case in gstmultidisksrc.c, although that one is tricky, as a GSList is passed in which, AFAIK, doesn't have a GType...
Just to push forward on this conversation, I've done an analysis of the gst-plugins, and it appears there's only a couple places where gpointers are being specified as callback parameters. These are in: mixertrack gstvideosink dxr3spusink And so should be relatively easy to fix. It looks like most of the offenders are in gstreamer proper. Incidentally, I'm going to create a new patch soon that'll take care of all the cases I've come across in gstreamer (there are a bunch that were missed in the original patch), followed by an additional patch to the gst-plugins stuff (still not sure if I'll file a separate bug on that).
Dave just did most of this in the core, by the way... Dave, any reason why you're not using boxed in the closures (for GstBuffer and so on)? Glib provides boxed-type-g_closures for this reason (see grep -i boxed /usr/include/glib-2.0/gobject/* | grep closure). I think you're supposed to use these... g_signal_new ("handoff", G_TYPE_FROM_CLASS(klass), G_SIGNAL_RUN_LAST, G_STRUCT_OFFSET (GstIdentityClass, handoff), NULL, NULL, gst_marshal_VOID__POINTER, G_TYPE_NONE, 1, GST_TYPE_BUFFER); then becomes: g_signal_new ("handoff", G_TYPE_FROM_CLASS(klass), G_SIGNAL_RUN_LAST, G_STRUCT_OFFSET (GstIdentityClass, handoff), NULL, NULL, gst_marshal_VOID__BOXED, G_TYPE_NONE, 1, GST_TYPE_BUFFER); I think glib/gtk use this too.
Cool. Presumably this work is still ongoing, since, as of my 9:30 checkout, there's still a couple fixable cases kicking around (in gstautoplugcache and gstpad). I'm still not sure what to do with the more problematic cases, though. There are a number of places where parameters aren't true Glib objects. eg, gstindex, gstmultidisksrc, the xml-related callbacks... BTW, shall I look at the plugins, dave, or are you planning to take care of those yourself?
On those problematic cases, most of them don't appear easily changeable to use proper GTypes (although, it seems to me that the XML node stuff should be proper gobjects, or at least gboxed objects, but that's just MHO). However, there is support in the new Glib-Perl bindings to perform custom signal marshalling, so I should be able to write workarounds for those few cases.
Adding the PATCH keyword.
I've reverted many of the changes from the core, because it causes bugs. Signal type specifications are not for the wrapper's convenience, or for introspection, they are for properly marshalling a signal. In particular, we almost always want to use POINTER for caps, since then the signal marshalling code doesn't copy the caps multiple times. This means that the signal handler needs to treat the parameter as a 'const GstCaps *'. I realize this makes wrappers difficult, but there's no way to make it easy in this case.