GNOME Bugzilla – Bug 642433
[playbin2] improve autoplugging by considering direct connectivity to the sink element
Last modified: 2011-03-17 01:12:54 UTC
Created attachment 180962 [details] [review] [playbin2] patch for improved autoplugging This patch adds a new factory sort criteria to the auto-plugging in playbin2, which favors the elements that directly connects to the sink elements regardless of their ranks. The patch is mainly against playbin2::autoplug_factories_cb(), as the internal uridecodebin does'nt expose "autoplug-sort". It firstly merges the sinkcaps of all the (possibly custom) sinks, and then checks that the src caps of each factory is a subset of, or can intersect with, the merged caps. If it's true, the factory is listed earlier than the other non intersecting factories. We need this feature, for example, in the following use case. I wanted to get AAC hw pass through output in totem (playbin2), so wrote a aac-to-iec958 element(E0) and a alsa-digital-sink element(S0). Their caps templates would be like this. E0 (aac-to-iec958) S0 (alsa-digital-sink) sink caps audio/mpeg,version={2,4} audio/x-iec958 src caps audio/x-iec958 --- and currently in my systems, faad(E1) and alsasink(S1) are installed. E1 (faad) S1 (alsasink) sink caps audio/mpeg,version={2,4} audio/x-raw-*,{*}; audio/x-iec958 src caps audio/x-raw-*,{*} ---- Then, I'd like to explicitly indicate playbin2 to output to S/PDIF, by setting the "audio-sink" property of playbin2 as alsa-digital-sink. playbin2 uri=file:///foo.aac audio-sink=alsa-digital-sink and in addition, I'd like to select analog output in some cases, so I'd like to indicate this by not specifying digital-audiosink, or by specifying alsasink as an audio-sink, and playbin2 automatically build the proper pipiline based on the audio-sink property. But currently, the above way does not work, regardless of how you set the ranks of elements or audio-sink. The result was like this. E0 > E1 E0 < E1 ---------+-------------+-------------- sink:S0 | OK (digtal) | error (*) sink:S1 | NG (digital)| OK (analog) (*): Internal Gstreamer Error: pad problem, failed to configure the audio sink Thus, if you set E0 > E1, you cannot get analog output, and if you set E0 < E1, you cannot get digital output. This is because, autoplug considers just the sink caps of the plugee element and its ranks, not the connectivity to the sink element at all. For example, in E0 < E1 and sink:S0 case, auto-plug firstly select the highest ranked element E1, but E1 has a non-fixed sink caps so the connection is regarded as success and no lower-ranked elements(E0) are tried. After caps_notify_cb() is called on E1, it's found out to be un-connectable to S0 due to the incompatible caps, but they are not unlinked, the list of the lower-ranked candidates is now already gone away and lost. Adding back-tracking feature to the auto-plugging may work but it's too complicated to implement easily. So just incorpating the direct connectivity to the sink element into the selection criteria of auto-plugging will be simple to implement and at the same time rescue the errors in simple cases like the above use case. Naturally, you can build your own pipline to output just to digital I/F or to the analog I/F without modifying auto-plugging process, but applications like totem usually do not allow users to configure the components of the pipiline except the sink elements, so it would be convenient if playbin2 can automatically handle the situation.
It would also make sense to expose autoplug-sort (and maybe other autoplug-*) signals in uridecodebin. Also you can assume that the sinkpad of the elements is called "sink", otherwise it won't work in decodebin2 anyway. And then I think this behaviour should be used always if we think it makes sense, no need to add a property for it :) (Just some first comments, I didn't look too close yet and I didn't think about your approach yet and if it could cause problems)
Created attachment 180991 [details] [review] [playbin2, uridecodebin] add autoplug-sort signal and improved sort of the factory list Thank you for your comment, I re-newed the patch based on your comment. 1) add "autoplug-sort" signal to uridecodebin 2) moved the factory list sort into the proper place, autoplug_sort_cb().
Review of attachment 180991 [details] [review]: Only reviewing the uridecodebin part for now, will look at the playbin2 part later today. Looks good in general but see the comments below. Also please put the uridecodebin part into a separate commit (and use "git format-patch" to create patches) ::: gst/playback/gsturidecodebin.c @@ +336,3 @@ + GValueArray *result; + + result = g_value_array_copy (factories); Wouldn't it be possible to just return factories here instead of creating a copy? The default handler of decodebin2 already creates a copy and this way playbin2 will *always* create two copies of the factories array for no good reason @@ +533,3 @@ + * + * Returns: A new sorted array of #GstElementFactory objects. + */ Please add a "Since: 0.10.33" marker here
ensonic suggested that i take a look at this bug and indeed this is pretty much the same thing that i am currently struggeling with so i'll reference it here: https://bugzilla.gnome.org/show_bug.cgi?id=642274 https://bugzilla.gnome.org/show_bug.cgi?id=632788
Created attachment 181099 [details] [review] [PATCH 1/2] [uridecodebin] expose "autoplug-sort" signal split the previous patch, part. 1 [uridecodebin] part.
Created attachment 181100 [details] [review] [PATCH 2/2] [playbin2] improved the factory selection in auto-plugging split the previous patch, part 2/2. [playbin2] part. I added some modification to the previous patch, because some sink elements like gconf*sinks report "ANY" sink caps in NULL state, which messes up auto-plugging again. Thus, when sink caps are found to be ANY in NULL state, the element's state will be temporarily raised to READY and re-tried, in order to increase the possibility to get the actual caps. now finally I have got AAC digital output in totem, by configuring a gconf key.
> https://bugzilla.gnome.org/show_bug.cgi?id=642274 > https://bugzilla.gnome.org/show_bug.cgi?id=632788 I guess those are rather more related to #642174, although I coudn't fully understand the problem yet.
Ok, the uridecodebin patch is committed and lots of changes on top of it. commit 09750a01326cf42a4c0752d8cee3134cd1216f64 Author: Sebastian Dröge <sebastian.droege@collabora.co.uk> Date: Fri Feb 18 12:06:30 2011 +0100 uridecodebin: Add default handler for autoplug-select uridecodebin proxies this signal and only the first signal handler will ever be called from decodebin2, which is uridecodebin's proxy signal handler. commit 91122e4efc2d2a6306f09b13b36583efe93c2c40 Author: Sebastian Dröge <sebastian.droege@collabora.co.uk> Date: Fri Feb 18 12:02:07 2011 +0100 uridecodebin: Return NULL from the default autoplug-sort handler ...instead of copying the array. Returning NULL will result in the original factories array to be used and prevents a useless array copy in most use cases. commit 2a6602d994268b77e7135d28ff9d5b04c23aafb0 Author: Sebastian Dröge <sebastian.droege@collabora.co.uk> Date: Fri Feb 18 12:01:05 2011 +0100 decodebin2: Return NULL from the default autoplug-sort handler ...instead of copying the array. Returning NULL will result in the original factories array to be used and prevents a useless array copy in most use cases. commit da4b5bf9f9cfa06241c8f85a8b5e1f7f2c67aac3 Author: Sebastian Dröge <sebastian.droege@collabora.co.uk> Date: Fri Feb 18 12:00:34 2011 +0100 uridecodebin: Update autoplug-* signal docs from decodebin2 uridecodebin proxies these signals. commit ef5f73206dd3ad515abea7e2c1a8e24a3fcc2814 Author: Sebastian Dröge <sebastian.droege@collabora.co.uk> Date: Fri Feb 18 11:58:44 2011 +0100 decodebin2: Update documentation of the autoplug-* signals Add notes about the behaviour if multiple signal handlers are connected. For most autoplug-* signals only the first signal handler will ever be invoked. Also add to the autoplug-sort docs that the signal handler can return NULL to specify that the order should change and other handlers get the chance to sort the array. commit 785f35a48e46ff99a7db1715e5edd182301e3555 Author: Sebastian Dröge <sebastian.droege@collabora.co.uk> Date: Fri Feb 18 11:57:12 2011 +0100 decodebin2: Keep the original factory list if the sort signal handlers returned NULL commit 0641eabeb2da1fa161eeeb70647ea1c49519f4ac Author: tskd2@yahoo.co.jp <tskd2@yahoo.co.jp> Date: Wed Feb 16 20:14:25 2011 +0900 uridecodebin: expose "autoplug-sort" signal It is a proxy of the decodebin2's one, and was missing in the previous code. See bug #642433.
While the sorting patch makes sense I don't think this is the way to go for iec958 passthrough. For this to work you have to give your aac-to-iec958 payloader a rank >= marginal and have to give it a classification as Decoder, Parser or Depayloader. This is going to break if your sink does not support iec958 and you don't have a AAC decoder with a higher rank than this iec958 payloader. I think the iec958 payloaders need to get a different classification to prevent them from being autoplugged by decodebin2 in normal scenarios and playbin2 should insert them at the front of the factories list in autoplug-factories if the sink supports iec958. Btw, could you make your aac-to-iec958 payloader available somewhere? :)
(In reply to comment #9) > This is going to break if your sink does not support iec958 and > you don't have a AAC decoder with a higher rank than this iec958 payloader. I missed the case where faad or ffdec_aac is not installed in the system. so, I will think of the way you proposed. > .... Btw, could you make your aac-to-iec958 payloader > available somewhere? :) Yes, of course. though the code quality might be pretty poor, my code is based on ffmpeg's spdifenc (hoping to be easily subclassed for other audio codecs), so it is GPL'ed. one thing to note is that I found alsaspdif don't work somehow, and I made a subclass of alsasink just to restrict the pad templates to "x-iec958", and which works fine. I think that alsaspdifsink should be patched and used, but this is not done yet.
(In reply to comment #10) > (In reply to comment #9) > > This is going to break if your sink does not support iec958 and > > you don't have a AAC decoder with a higher rank than this iec958 payloader. > > I missed the case where faad or ffdec_aac is not installed in the system. > so, I will think of the way you proposed. Better wait until we get some more input, my proposed solution has some problems too. For now you could create your own GstBin subclass that internally plugs the iec958 payloaders if necessary and an alsasink. > > .... Btw, could you make your aac-to-iec958 payloader > > available somewhere? :) > > Yes, of course. though the code quality might be pretty poor, my code is > based on ffmpeg's spdifenc (hoping to be easily subclassed for other audio > codecs), > so it is GPL'ed. > one thing to note is that I found alsaspdif don't work somehow, and > I made a subclass of alsasink just to restrict the pad templates to "x-iec958", > and which works fine. > I think that alsaspdifsink should be patched and used, but this is not done > yet. alsaspdifsink is dead (and was removed from GIT). Everything needed is available inside alsasink
Created attachment 181217 [details] IEC958(IEC61937) payloader element for AAC attached the source code of my plugin (IEC958 payloader for AAC) that I used in this report. JFYI
Please open a new bug for the IEC958 payloader and another one for the IEC958 passthrough support. I think we can keep this one here open only for the issue mentioned in the subject: while autoplugging prefer elements that allow direct connectivity to the sinks
Ok, I have to say that this idea is definitely not a good one :) If we first try elements that could directly link to the sink we would use mad directly instead of using mp3parse first for example. This is going to break a lot of things. I'm going to close this bug as WONTFIX unless there's really something to fix here... IMHO this is just an optimization.
I understand. And at any rate, my final goal of getting hardware AAC output working in playbin2 seems to be achieved soon by the recent commits to playbin2.