GNOME Bugzilla – Bug 758212
playbin adds the template caps on autoplug-query
Last modified: 2016-02-18 07:44:40 UTC
Right, so attempting to select some caps feature vs not with a capsfilter doesn't seem to work correctly as playbin adds the template caps to the result of the caps query when going through the autoplug-query mechanism. e.g. a pipeline on android like playbin uri=uri video-sink='video/x-raw ! glimagesink' Will fail to negotiate as amcvideodec will see that GLMemory is supported (from amcvideodec's template caps) and attempt to output GL which sends a caps event with GLMemory features which the caps filter outright refuses (understandably). git blame points to the following commit however there isn't any justification as to why. commit 45dfceacdb6506a001d435c94ba160389264f3dd Author: Matthieu Bouron <matthieu.bouron@collabora.com> Date: Thu Feb 20 20:01:30 2014 +0000 playbin: improve autoplug_query_caps return Makes autoplug_query_caps return downstream_caps + intersect_first(filter_caps, element_caps) https://bugzilla.gnome.org/show_bug.cgi?id=724828
I'm not sure either what the rationale behind that change was, it looks like just an optimization (filtering not more caps than necessary). But how is this causing problems exactly? The template caps (of amcvideodec) would be filtered with the same filter previously the whole caps used. All other caps that are part of the whole caps should already be filtered before, are they not?
amc uses the template caps as the filter (which includes GLMemory) so it isn't filtered out. The only thing filtering out GLMemory is the capsfilter. I think that the problem is this line which adds the GLMemory (from amc's template caps. result = gst_caps_merge (result, target_caps);
That was done before though, just that this part is now filtered before merging while before it was filtered after merging everything together. Not sure how that can cause different behaviour
Oh, its not the commit that changed behaviour, its the fact that it exists at all.
I see, that makes more sense :) It exists at all because autoplug_query only asks the sink... so say your decoder can only output I420 and the sink can only handle RGB, then this would fail. But later things would work nonetheless because of converters and stuff.
Created attachment 316490 [details] [review] playbin: only add the template caps when the result is empty
commit 023af2d3b192f8ebf1bd4fe75a22a4adaedc1e05 Author: Matthew Waters <matthew@centricular.com> Date: Mon Nov 30 10:28:55 2015 +1100 playbin: only add the template caps when the result is empty Unconditionally adding the template caps when proxying the caps query will play havoc with decoders that attempt to choose an output format based on some caps features. Creating a sink that does not include those caps features and a decoder/parser/etc that preferentially chooses some specific caps feature when available, will always return the decoder/parser/etc template caps and choose a feature that downstream will be unable to support. Fix by limiting the addition of the template caps to when the result is actually empty. https://bugzilla.gnome.org/show_bug.cgi?id=758212
This breaks autoplugging in playbin if a video decoders outputs some raw video format that the sink does not support. What happens is 1) sink accept-caps 2) __gst_video_element_proxy_getcaps() 3) __gst_video_element_proxy_caps() creates a filter caps that only contains the width/height/par/framerate restrictions, not a format restriction 4) Downstreams gets a CAPS query with that filter, the sink answers with a format that our decoder does not support 5) Intersecting with the source pad template caps yields EMPTY
I think the behaviour in playbin doesn't make any sense and is wrong. What we basically do here is to return completely different caps based on what the filter is. We can even return more strict caps with a less strict filter now, e.g.: filter=video/x-raw,format=RGBA and downstream only supports BGRA => we return video/x-raw filter=NULL and downstream only supports BGRA => we return video/x-raw,format=BGRA. IMHO this violates some assumptions about caps behaviour we have.
Maybe instead of all this, we should add audio/video specific behaviour here? And only add the pad template with video/x-raw(sysmem) or audio/x-raw(sysmem)? Because others we clearly can't convert if the sink does not support them already (videoconvert, audioconvert, etc can't). This is only going to be too strict if playsink has an audio/video-filter that can actually do conversion.
Created attachment 318515 [details] [review] Revert "playbin: only add the template caps when the result is empty" This reverts commit 023af2d3b192f8ebf1bd4fe75a22a4adaedc1e05.
Created attachment 318516 [details] [review] playbin: Only append non-raw and sysmem pad template caps to the autoplug-query result Otherwise a decoder supporting GL memory will think that all downstream can support GL memory because of seeing its own template caps.
Review of attachment 318516 [details] [review]: It's probably better than mine however it's still not as good as sending the query through playsink directly
In what way is it not as good, what example are you thinking of that would still break? It's not a nice solution and feels more like a hack, but the situation without your patch or before was definitely worse.
(In reply to Sebastian Dröge (slomo) from comment #14) > In what way is it not as good, what example are you thinking of that would > still break? It's not a nice solution and feels more like a hack, but the > situation without your patch or before was definitely worse. e.g. video-filter/audio-filter on playsink but you've already mentioned that. I agree the situation is bad whichever way you take this as you don't really have enough context to make proper decisions. Pushing the query through playsink's pads would be better (assuming playsink creates its pipeline early enough).
The problem is that playsink can only create the pads once it knows the caps of the stream, and e.g. if there is an audio or video stream at all. What my patch does now is that we assume that we can convert every raw format on sysmem, and for everything else we need direct sink support. Which should be the case in all normal situations but we might also want to check the playsink flags here (e.g. native-video means we actually can't convert, no deinterlace flag means we can't deinterlace, etc).
(In reply to Sebastian Dröge (slomo) from comment #16) > The problem is that playsink can only create the pads once it knows the caps > of the stream, and e.g. if there is an audio or video stream at all. And why can't the pad request create the chain for the specified playsink flags? > What my patch does now is that we assume that we can convert every raw > format on sysmem, and for everything else we need direct sink support. Which > should be the case in all normal situations but we might also want to check > the playsink flags here (e.g. native-video means we actually can't convert, > no deinterlace flag means we can't deinterlace, etc). Again, would be much simpler if one could push the caps query through playsink. One could also emulate pushing the caps query through the audio-filter/video-filter elements as well if absolutely necessary although that's starting to become ridiculous.
If it makes things better then sure, we can try it.
(In reply to Matthew Waters (ystreet00) from comment #17) > (In reply to Sebastian Dröge (slomo) from comment #16) > > The problem is that playsink can only create the pads once it knows the caps > > of the stream, and e.g. if there is an audio or video stream at all. > > And why can't the pad request create the chain for the specified playsink > flags? Because we don't know at this point which pads we're going to request. At this point we might e.g. only have a demuxer doing a caps query downstream but we don't know yet if the stream it created is going to be usable as a audio or video stream (parsers, decoders might be missing or not working). > > What my patch does now is that we assume that we can convert every raw > > format on sysmem, and for everything else we need direct sink support. Which > > should be the case in all normal situations but we might also want to check > > the playsink flags here (e.g. native-video means we actually can't convert, > > no deinterlace flag means we can't deinterlace, etc). > > Again, would be much simpler if one could push the caps query through > playsink. I agree, that would be the ideal solution :) Do you see a way of making that happen? We would have to request the playsink pads from autoplug-query / autoplug-continue / autoplug-select. > One could also emulate pushing the caps query through the > audio-filter/video-filter elements as well if absolutely necessary although > that's starting to become ridiculous. Yes, I wouldn't go that route :)
Note that autoplug-query already starts to become useful for *parsers* (non-raw sink, can do bytestream h264 but not avc. parser needs to know that it has to convert), so limiting it to decoders only doesn't seem like a good solution either. OTOH when having a parser we usually know if it's audio or video and could request pads... but it's not guaranteed that we can find a decoder for that so maybe we don't have audio/video after all. Also this might cause problems with subtitles: playbin could have a second uridecodebin for subtitles, so we will likely configure playsink first without subtitle pads and only on no-more-pads (of both uridecodebin) configure subtitles. In theory that should work just fine but it might uncover new bugs (which should of course be fixed in any case).
Also playsink only creates the internal elements when receiving the CAPS events IIRC (the configure() thing inside playsink).
Proxying to playsink also won't work... everything that is not a decoder would most likely return EMPTY caps.
Let's go with this for now to at least have things work better. It's not ideal but at least not breaking anything that worked before :) commit fccf83e69f108fd1bdc10b785761c54d3f413b36 Author: Sebastian Dröge <sebastian@centricular.com> Date: Fri Jan 8 18:53:52 2016 +0200 playbin: Only append non-raw and sysmem pad template caps to the autoplug-query result Otherwise a decoder supporting GL memory will think that all downstream can support GL memory because of seeing its own template caps. https://bugzilla.gnome.org/show_bug.cgi?id=758212 commit 9713ab06cd081c520ab14641df1e974b5bbbe50c Author: Sebastian Dröge <sebastian@centricular.com> Date: Fri Jan 8 18:37:16 2016 +0200 Revert "playbin: only add the template caps when the result is empty" This reverts commit 023af2d3b192f8ebf1bd4fe75a22a4adaedc1e05. https://bugzilla.gnome.org/show_bug.cgi?id=758212
What is left to be done here at this point now? Matthew?
I have no idea, you worked on it last.
I only fixed something with your patch, I don't know if anything else needs work here... other than all this being not very beautiful :) Is there any (non-theoretical) scenario that still does not work?
I don't know of anything that doesn't work.
Let's close this then until something appears :)