GNOME Bugzilla – Bug 677335
Incompatible caps can be accepted and dataflow happens
Last modified: 2012-06-05 06:33:20 UTC
Hi, currently it is possible in 0.10 and 0.11 that a pad accepts caps and buffers with these caps although the acceptcaps function and the template caps of the pad in question don't accept it. A simple test case for 0.10 would be gst-launch-0.10 audiotestsrc ! vorbisenc ! vorbisdec ! autoaudiosink with only alsasink installed (alsasink does not accept float audio, vorbisdec only outputs float audio). The pipeline is linked in NULL state, in READY state the alsasink is instantiated and the ghostpad target is set and up to this point no concrete caps are known (well, autoaudiosink could check the upstream caps before setting the ghostpad target, does it need to? Or should the ghostpad functions check for this?). Now the complete pipeline is linked. When the first buffer from vorbisdec arrives at autoaudiosink, gst_pad_configure_sink() is called on the sinkpad of autoaudiosink, this checks the template caps (ANY caps) and then calls gst_pad_set_caps() on the sinkpad. The ghostpad setcaps function now gets the target pad (alsasink's sinkpad) and calls gst_pad_set_caps() on it without checking anything. alsasink's setcaps function (well, baseaudiosink) assumes that the caps are compatible to the pad caps at this point and just continues to set everything without failing. And soon later buffers are pushed and buffers that are incompatible with alsasink are accepted by alsasink. 1) A possible fix would be to call gst_pad_accept_caps() in gst_pad_configure_sink() instead of just intersecting with the template caps, but this would possibly be a performance problem as acceptcaps usually gathers all caps further downstream through multiple elements. (Using acceptcaps here will work because the ghostpad acceptcaps function proxies to the target and alsasink's acceptcaps function will simply not accept the invalid caps.) 2) Another solution would be to let gst_proxy_pad_setcaps_default() not set the caps on the target but only on the internal pad. And let caps propagation with buffers happen as usual internally in gst_pad_chain_data_unchecked() with gst_pad_configure_sink(). This still has the problem that incompatible caps could be allowed though as gst_pad_configure_sink() only does a subset check on the *template* caps (alsasink likely has much more restricted caps in states >= READY than the template caps). I tested both fixes and they both work in the above mentioned testcase. But 1) has the mentioned performance problem (while it is the correct and bulletproof solution) and 2) does not fix the bug completely (but has no performance problems). 1) would also fix pads accepting wrong caps in other situations. A similar problem also exists in 0.11. gstpad.c:pre_eventfunc_check() only does a subset check with the template caps and the template caps could be more liberal than the caps that are actually accepted by the pad at this point. The subset-template check is done properly for ghostpads though, so in 0.11 the state of this bug is basically the same as if it was "fixed" by 2) in 0.10.
For 0.11 the "real" fix (i.e. 1)) can be improved by adding a "non-recursive" flag to the acceptcaps/getcaps queries, that will make them only return what the pad itself can support without considering downstream/upstream caps.
Both fixes implemented now, the acceptcaps change in 0.10 might need to be reverted later if the performance problems are too severe. commit 7558fd21eeb0bf7fcaaf78dcc9159c05780e0b56 Author: Sebastian Dröge <sebastian.droege@collabora.co.uk> Date: Mon Jun 4 09:31:07 2012 +0200 pad: Check via gst_pad_accept_caps() if a sinkpad accepts caps instead of just checking if the pad template caps would allow the caps. The actually supported caps can be far more restrictive than the template caps and only checking for the template caps can cause incompatible caps to be set on a pad. Fixes bug #677335. commit 1c0764f764a9ce574cbea4ef0fdbd161f606ba12 Author: Sebastian Dröge <sebastian.droege@collabora.co.uk> Date: Mon Jun 4 09:29:22 2012 +0200 ghostpad: Don't propagate setcaps() calls to the target pads The core will do this during dataflow already and additionally does compatibility checks then. Let it be handled by the core as for every other pad too. Fixes bug #677335. commit 991ac561e19b71ec273559ead1f78bd216202359 Author: Sebastian Dröge <sebastian.droege@collabora.co.uk> Date: Mon Jun 4 09:27:35 2012 +0200 pad: Only forward caps events to a pad if it accepts the caps Fixes bug #677335.
I have problem with this. It seems to be breaking parsers; See #677401 example: gst_ac3_parse_get_sink_caps returns caps that have channels and rate set (as range); The caps in caps event doesn't have any of these so it get rejected.
Reverting this patch fixes the problem with parsers for me.
Note that I'm not saying this patch is bad, perhaps the parsers need to be fixed instead.
To reproduce try to send caps "audio/x-ac3" (without any additional fields) to ac3parser
(In reply to comment #3) > I have problem with this. It seems to be breaking parsers; See #677401 > > example: > > > gst_ac3_parse_get_sink_caps returns caps that have channels and rate set (as > range); The caps in caps event doesn't have any of these so it get rejected. Yes, that's a bug in the parser's getcaps function. But as we don't want to break everything in 0.10, 7558fd21eeb0bf7fcaaf78dcc9159c05780e0b56 should be reverted there.