GNOME Bugzilla – Bug 407282
[decodebin2] autoplug-sort signal has GList ** parameter
Last modified: 2007-10-24 11:08:06 UTC
This slipped into decodebin2: /* signal fired to reorder the proposed list of factories */ gboolean (*autoplug_sort) (GstElement * element, GstCaps * caps, GList ** list); As if a GList * parameter wasn't bad enough as a signal parameter, but a GList ** ?! Can we please quickly change this into a GValueArray before anyone starts using decodebin2 for anything? (I'd argue that it was only semi-functional in the last release, so chances that anyone was using it are pretty slim IMHO, and those that were using decodebin2 are very unlikely to have been using this signal, I'd think; better anounce this on the mailing list and fix it than be stuck with it until the end of the 0.10 cycle ...)
agreed.
graaah, hadn't time to finish my answer. It's also not wrappable by bindings (think gst-python).
Created attachment 83809 [details] [review] Patch changing GList ** parameter to a GValueArray *
Created attachment 83813 [details] [review] Patch that actually works, even if it is only slightly less ugly than the original GList ** approach Unfortunately the above patch doesn't work though, because a _copy_ of the GValueArray will be passed to the callback. Only way to prevent this is to use G_TYPE_POINTER again, which defeats half of the purpose of the proposed change *sigh*.
(In reply to comment #4) ... > Unfortunately the above patch doesn't work though, because a _copy_ of the > GValueArray will be passed to the callback. Only way to prevent this is to use > G_TYPE_POINTER again, which defeats half of the purpose of the proposed change > *sigh*. I'm getting the impression that it's better to avoid the current design of the autoplug-sort signal altogether. I don't see the point of letting an app sort the list of compatible factories: decodebin2 takes the list (which was possibly mutated by a signal handler) and tries to create elements from it until it finds one that actually has a sink pad, is able to switch to the READY state and can be linked to the discovered pad. Now I'm wondering: Doesn't this succeed for the very first element factory most of the time? How about doing this instead: Rename the autoplug-sort signal to autoplug-select (or something). Keep the caps and factory list/array as paramters, but make the signal return a factory instead of a boolean. Where autoplug-sort is emitted, emit autoplug-select instead, with the caps and the factory list as before (except for the list being const of course). Try to plug using the returned factory (dismiss the pad on a NULL return, like -sort does with a FALSE return). On failure, remove the failed factory from the list and emit the signal again (until a proper element is found or the list is exhausted).
Created attachment 90066 [details] [review] Replace autoplug-sort signal with autoplug-select This patch implements my proposal. Bindings seem to like it (tested with gst-python). There is a drawback though: It's no longer possible to reject caps, i.e. the FALSE return from autoplug-sort is not mapped to the new autoplug-select semantics. Giving a NULL return from autoplug-select this meaning breaks the ability to connect multiple user handlers to the signal. It seems that caps rejecting should be implemented using a dedicated signal, if deemed useful. I noticed that the code, comments and docs surrounding this functionality are slightly out of sync. Is there a drawback of returning FALSE from autoplug-continue for unwanted caps?
With the suggestion in #6 it's even easier to return a gint, the index in the array of the factory to try next. This would avoid abuse when a factory is returned that is actually not in the list.
Inspired by patch of: René Stadler <mail at renestadler dot de> * gst/playback/gstdecodebin2.c: (gst_decode_bin_class_init), (gst_decode_bin_autoplug_continue), (gst_decode_bin_autoplug_factories), (gst_decode_bin_autoplug_select), (analyze_new_pad), (connect_pad), (find_compatibles): * gst/playback/gstplay-marshal.list: Remove the autoplug-sort signal and replace it with a binding friendly autoplug-select signal. Add an autoplug-factories signal that can be used to generate a list of factories to try to autoplug. Add the GstPad to the autoplugging signal args as it might be needed to make a good factory selection. Fix up the marshallers for this. Fixes #407282.