GNOME Bugzilla – Bug 353970
capsfilter: doesn't filter caps correctly
Last modified: 2013-07-24 10:43:14 UTC
$ gst-launch-0.10 videotestsrc ! video/x-raw-rgb,alpha_mask=255 ! fakesink -v will lead to /pipeline0/fakesink0.sink: caps = video/x-raw-rgb, bpp=(int)32, endianness=(int)4321, depth=(int)24, red_mask=(int)16711680, green_mask=(int)65280, blue_mask=(int)255, width=(int)320, height=(int)240, framerate=(fraction)30/1, alpha_mask=(int)255 (even with an up-to-date videotestsrc from CVS which does have support for RGBA). This is wrong IMHO. capsfilter should _filter_ given caps to my requirements assuming all fields are non-optional. It can't just assume some of the provided fields are optional (In this case I need to add an additional depth=32 to make it pick the caps I want). Similarly, this pipeline $ gst-launch-0.10 videotestsrc ! video/x-raw-rgb,foobar=99 ! fakesink should just error out.
Created attachment 74557 [details] [review] really filter filtercaps (first attempt, can probably be simplified a bit more)
Created attachment 74617 [details] [review] new patch New patch (don't check that field types are equal in both structures, that won't work since there can be ranges, lists, arrays etc.).
The patch is problematic in setups like totem-video-thumbnailer (e.g. as in bug #369502), where conversion will fail in a setup like: ... ! videoscale ! filtercaps ! sink where the filtercaps set are fixed and very specific, including a pixel-aspect-ratio=1/1 field. In the process of negotiation, capsfilter will get the template caps from videoscale's source pad and those don't have a pixel-aspect-ratio field, so things will error out here because caps are allegedly incompatible. Arguably this needs to be added to videoscale's template caps then, but in any case it seems this isn't really a feasable way to go, so I guess it comes down to upstream doing better caps nego.
What's the state of this bug?
(In reply to comment #3) > The patch is problematic in setups like totem-video-thumbnailer (e.g. as in bug > #369502), where conversion will fail in a setup like: > > ... ! videoscale ! filtercaps ! sink > > where the filtercaps set are fixed and very specific, including a > pixel-aspect-ratio=1/1 field. In the process of negotiation, capsfilter will > get the template caps from videoscale's source pad and those don't have a > pixel-aspect-ratio field, so things will error out here because caps are > allegedly incompatible. Arguably this needs to be added to videoscale's > template caps then, but in any case it seems this isn't really a feasable way > to go, so I guess it comes down to upstream doing better caps nego. Adding it to videoscales template caps means, that videoscale only accepts caps with p-a-r fields. Which then means, that we need to include p-a-r in every video caps and the current no-par-is-1:1 convention wouldn't work anymore.
Tim, could you answer Sebastians' question on comment #4?
Yay, another reason to do mandatory and optional flags for caps fields. :) I should note I'm generally against having a special algorithm for capsfilter, as it's very confusing if gst-launch uses a different intersection algorithm from everything else using caps. Long story with lots of theory: The problem with this is (and always has been) the interpretation of optional fields. Optional fields were introduced as a shortcut to defining caps, so you don't need to write down all fields if you didn't care about them. This is mostly used in capsfilter, but is also used in generic templates (which just return "audio/x-raw-int"), though elements have been getting more and more strict in their definition of template caps. In this initial sense omitting a field meant a shortcut to ANY. Unfortunately this has consequences for fixation. A fixed caps is not really fixed, because all undefined fields are still set to ANY: You can intersect the fixed caps "audio/x-mp3" with "audio/x-mp3, parsed={false,true}" and the result is non-fixed. This is an inconsistency in the caps system large enough to attract more bugs than Starship Troopers, oftentimes in the form of weird incompatibilities or broken output files (see the caps above for an example). Luckily g_return_if_fail(gst_caps_is_fixed()) helps against the SEGVs usually. I've been thinking about this problem for a long time, but never came up with a satisfactory answer that would solve this without breaking too much of what people expect. But I do believe that custom ad-hoc solutions like Tim's patch don't help, they just add confusion (though they intuitively feel right).
Comment on attachment 74617 [details] [review] new patch Needs to be rebased against latest git master at least
What should happen here? There were many changes to the filtering algorithm in capsfilter since 2006, is it still relevant? I guess so because this still works: gst-launch-1.0 videotestsrc ! "video/x-raw,foobar=123" ! fakesink Can't we just do a subset check inside capsfilter to make sure fields are all present? However this should be optional because it will break backwards compatibility, I'm pretty sure there's lots of code out there that depends on capsfilter setting non-existing fields to the value specified in the caps.
I think we should just close this, and if it's still a problem in practice, someone run into it again and open a new bug. The original problem we don't have any longer because we now specify those things via the video/audio formats. I suppose the capsfilter is used both to filter and to add new fields to caps, and it can't know which one it's supposed to do, and we can't just repurpose it to "filter only" now.