GNOME Bugzilla – Bug 742924
decodebin: Initial decoder negotiation will always fail
Last modified: 2018-05-04 12:36:39 UTC
Introduced mostly by these commits: commit 994680b04eb390a116a9b5bda2abd647df233be3 Author: Sebastian Dröge <sebastian@centricular.com> Date: Thu Jul 10 12:50:17 2014 +0200 decodebin: Only link elements further after setting them to PAUSED They might fail to go to PAUSED, and when connecting them further we might already expose their srcpads on decodebin if we're unlucky. This prevents us to handle failures going to PAUSED gracefully. commit 57999c28fd7720f5f3e9b3adf55528ddad21cc9c Author: Sebastian Dröge <sebastian@centricular.com> Date: Thu Jul 10 12:51:22 2014 +0200 decodebin: Send sticky events to the new element after setting it to PAUSED ... and if this fails for whatever reason we skip the element and instead try with the next element. This allows us to handle elements that fail when setting caps on them by just skipping to the next alternative element. The problem is essentially that we can no longer do (proper/optimal/any) initial negotiation in decoders. Current status: When decoders are created, they are added to the bin, but not linked, and we check that it can accept pending sticky events (i.e. CAPS). Only after that will it be properly linked downstream. It results in these issues. 1) When decoders receive CAPS_EVENT, they might need to query caps downstream (which should eventually reach the actual sinks via the autoplug-query decodebin2/playbin mechanism) to know 1.1) *if* they can produce something compatible with downstream. This will not be possible since the CAPS_QUERY will just return the source pad template (the pad is not linked). ==> The commits were intended to avoid using elements that won't work later on. This is impossible since they are not linked and the autoplug-query system can't be used. 1.2) and to figure out what downstream prefers (potentially including special features (special memory, buffer type, ...)) ==> This won't be possible (not linked), so they will always fallback to the default behavior. ==> The decoder might even pick CAPS that are *not* compatible with downstream. ==> QUERY_CAPS will never work before the element receives data (i.e. it's been linked and the sinkpad stream lock has been released). ==> The CAPS selected might not be compatible with downstream, let alone the optimal choice/features. 2) For decoders that need to configure themselves appropriately before they consume any buffers (i.e. when receiving a CAPS event), they are no longer guaranteed to be linked downstream after setting CAPS downstream (since they are not linked). This was the case before these patches. Sending a NEGOTIATION query downstream will always fail (not linked) ==> QUERY_NEGOTIATION will never work before the elements While the intent of those commits is perfectly valid, we end up in a situation where the only decoders that will be able to do optimal negotiations with sinks are the ones that can reconfigure themselves after having consumed data. I don't even think we have any of those ... Maybe we should link the elements normally (like for parsers/converters/...) BUT with the sinkpad stream lock taken, send the sticky events, and unroll the chain if it fails ? At least elements will have a way of properly negotiating like this.
Related to bug #743687, which fails without these changes apparently. So we need to find a way to make both work :)
Hi, we are also facing similar issue on gst 1.2 first we ensure that we have passed three below events downstream from decoder as they are received from upstream. 1> stream start event 2> segment event 3> caps event I had this assumption that once the caps event call returns back from downstream then pipline would have been created till sink i.e. some sink would have been identified / inserted in pipeline and it will be synchronized with caps event call so that we can make other queries to sink just after caps event call. Based on this assumption after caps event our decoder is sending a custom query to sink, which is suppose to be answered by our own sink in pipeline. but it is always failing with not linked error and we end up configuring our decoder in a wrong way. just for the experiment sake we started sending drain query, after caps event, to downstream and this way we ensured that pipeline creation is synchronized and we are sure about the success of custom query. as such drain query is not what is expected to do this job of pipeline creation but somehow it should be synchronized with caps event call. BR/ Rajesh
This bug is completely unrelated to anything in 1.2, it's a regression that happened between 1.4 and git master. Please report a different bug for the problem you see, after making sure it still happens with 1.4.5 or git master.
For the record, the idea of linking before raising the state was introduced in 0.10 when the ancestor of context sharing was introduced. In the internal negotiation phase of context sharing, we want a decoder to be able to retrieve the context from the display sink if already active. This is done through a query. commit cf9da5c280603edde373f71ff1319aea4222130a bug #662330
Easy way to show the issue : GST_DEBUG=*videodecoder*:5 gst-launch-1.0 filesrc location=/home/meh/Videos/screw.mp4 ! qtdemux ! h264parse ! vaapidecode ! glimagesink 2>&1 | grep "ALLOCATION" The negotiation is immediately done to system memory GST_DEBUG=*videodecoder*:5 gst-launch-1.0 uridecodebin uri=file:///home/meh/Videos/screw.mp4 ! glimagesink 2>&1 | grep "ALLOCATION" The first negotiated caps have the VASurface feature, even though glimagesink can't handle that.
Scratch that, I read my debug wrong, this doesn't exhibit the issue as vaapidecodebin adds a vaapipostproc element that I hadn't included in my manual command line, and which can "convert" features :/
Is the AMC decoder the only decoder that has issues regarding that ? Maybe there is an AMC specific solution to that ?
What exactly is the problem? Does someone know by now? ;) Is it that the decoder srcpad is not blocked and as such the CAPS event and following ALLOCATION query goes into nothing and fails then? And that this now happens newly because we set the decoders to PAUSED inside decodebin manually to check if they themselves handle the CAPS event properly (and skip decoders that fail to handle specific caps)? If so, maybe we could change decodebin to already expose pads when they receive the final CAPS event instead of waiting for all pads to be ready before exposing them all at once?
I think that's the case yes, more precisely the queries go into the autoplug-query mechanism and then indeed go to nothing. I've delayed my investigation of this because I investigate a bug on Nexus 7, where decodebin works precisely because of that bug (when outside of decodebin the code fails because it can't actually do direct rendering on that device, at least with android 4.2, that's a completely different bug) IMHO exposing pads once they've got the final CAPS event is just a half measure, it will work for the cases where allocation was the problem, but it will not make optimal caps negotiation more possible AFAIU ?
Only blocker for the android zerocopy stuff, which is not going to land this cycle anyway -> 1.7.x.
Ok, so the problem is that the decoder src pad is not linked to anything when decodebin attempts setting the decoder to PAUSED. All events/queries therefore disappear into the void. Theoretically, the GstDecodePad should be targeted at the decoder src pad when setting the decoder to PAUSED so that caps/allocation queries can at least be propagated by the autoplug-query infrastructure inside decodebin.
Yes the idea was that this goes via autoplug-query, however doing that will cause the ALLOCATION queries to go downstream before the CAPS events... which according to Mateo (he tested that) causes crashes and other fun in various places. Do you have any other ideas than proxying the query? :)
No, the only way is to proxy some kind of data between the decoder and sink and they're currently not connected at all to each other. It doesn't have to be an allocation query. GstContext probably fits better here (already exists for GL) and doesn't have any existing ordering issues.
This doesn't solve the general case problem that the decoder<->sink have no way of communicating *at all*.
Would it be easier to fix tha various elements to support allocation-query-before-caps ?
(In reply to Olivier Crête from comment #15) > Would it be easier to fix tha various elements to support > allocation-query-before-caps ? If you can find a solution for BaseTransform, of course. BaseTransform need the caps to decide to pass-through or not, which greatly affect the result of an allocation query.
It could decide based on the caps in the allocation query maybe... but probably not :)
(In reply to Sebastian Dröge (slomo) from comment #17) > It could decide based on the caps in the allocation query maybe... but > probably not :) I'm not against that but I have a hard time to see how to keep that compatible. Right now the sub class tells the base class by calling gst_base_transform_set_passthrough(). There exist some pseudo automatic case, we'd probably need a virtual method to ask if it's passthrough for specific caps, and we could try and reuse the automatic case (if that does not involved anything then caps queryies) for the default implementation. We need to be careful too, since we'll break third parties that would not be expecting this (short story, it remains an ABI break).
Created attachment 313409 [details] [review] decodebin: set the decode pad target before setting the element to PAUSED This makes the autoplug-query stuff work properly again although I'm not sure if the "if ((is_simple_demuxer || is_parser_converter) && to_connect)" removal is too far reaching.
Comment on attachment 313409 [details] [review] decodebin: set the decode pad target before setting the element to PAUSED Makes sense I thnk, let's give it some testing!
Comment on attachment 313409 [details] [review] decodebin: set the decode pad target before setting the element to PAUSED commit d50b713f44eb17ebf69aa771c26f8eb19e226319 Author: Matthew Waters <matthew@centricular.com> Date: Fri Oct 16 03:40:43 2015 +1100 decodebin: set the decode pad target before setting elements to PAUSED Otherwise caps and context queries will disappear into nothing and therefore fail. With autoplug-query now actually working, users (such as playbin) can proxy these queries to the selected video sink and be able to select an more appropriate configuration.
Created attachment 313683 [details] [review] decodebin: return the possibly new chain in analzye_new_pad
Created attachment 313684 [details] [review] decodebin: track all the exposable pads in connect_pad
Note that these make gst-validate pass for me again ;)
Comment on attachment 313684 [details] [review] decodebin: track all the exposable pads in connect_pad commit 44871680f02f5eae970c3d4f87b4e168ec412a25 Author: Matthew Waters <matthew@centricular.com> Date: Tue Oct 20 03:58:26 2015 +1100 decodebin: track the exposable pads through connect_pad The logic introduced by [d50b713: decodebin: set the decode pad target before setting elements to PAUSED] to expose pads would only ever be able to possibly expose one (the last) pad per element. Make it so that any exposable pads are able to be exposed rather than just the last pad returned by connect_element. https://bugzilla.gnome.org/show_bug.cgi?id=742924 commit 94d81fc713d31830e6ab61da1d6856232f0095c2 Author: Matthew Waters <matthew@centricular.com> Date: Tue Oct 20 03:52:24 2015 +1100 decodebin: return the possibly new chain in analyze_new_pad In the case of analyzing a demuxer chain, analyze_new_pad may create a new GstDecodeChain. This was not propagated to the calling function which as of [d50b713f decodebin: set the decode pad target before setting elements to PAUSED] is now required to be able to expose the correct pad. https://bugzilla.gnome.org/show_bug.cgi?id=742924
Anything left ?
playbin only makes the caps query/context query propagate which in some cases is enough. ALLOCATION queries still fail which could be a source of errors. The caps event is not propagated between decoder<->sink and doing an allocation query before caps is not the most well tested path and is not really defined for maybe-passthrough basetransform elements ;)
Ah ok, so we don't attempt to set caps downstream. Btw. for basetransform, the allocation query before caps will just return FALSE, that's all ;-)
These patches caused a small regression, fix in bug #760949
*** Bug 761738 has been marked as a duplicate of this bug. ***
Adding myself to receive notification about this bug.
Dropping this. trivially fixed by playbin3/decodebin3.