GNOME Bugzilla – Bug 748635
videorate: caps negotiation regression
Last modified: 2015-08-16 13:38:41 UTC
Created attachment 302564 [details] file that show the issue please try this pipeline with the attached file: gst-launch-1.0 -v filesrc location=/tmp/test.mkv ! matroskademux ! videorate ! image/jpeg,framerate=3/1 ! jpegdec ! videoscale ! videoconvert ! video/x-raw,width=320,pixel-aspect-ratio=1/1 ! xvimagesink it works with 1.4 but not with git master a workaround is to use something like this: gst-launch-1.0 -v filesrc location=/tmp/test.mkv ! matroskademux ! jpegdec ! videorate ! videoscale ! videoconvert ! video/x-raw,width=320,pixel-aspect-ratio=1/1,framerate=3/1 ! xvimagesink but this way you'll decode all input frames while with the previous method only 3 fps are decoded
I suspect this has to do with the changes in videorate, so moving it there for the time being.
Bisected it to: commit f24075887f9d876af59f27a389f2e8c7d6c957ca Author: Thiago Santos <thiagoss@osg.samsung.com> Date: Mon Dec 8 16:33:33 2014 -0300 videodecoder: implement caps query Refactor the encoder's caps query proxying function to a common place and use it in the videodecoder to proxy downstream restrictions. The new function is private to the gstvideo lib. https://bugzilla.gnome.org/show_bug.cgi?id=741263
matroskademux sets "image/jpeg, framerate=(fraction)15/1, width=(int)480, height=(int)640" as the caps and videorate asks downstream for what it can accept. This reply contains the pixel-aspect-ratio field and videorate fails the accept-caps query as gst_caps_is_subset will fail when the subset doesn't have all the fields from the superset.
Any idea on how to proceed here? <videorate0> allowed caps image/jpeg, framerate=(fraction)15/1, width=(int)480, height=(int)640, pixel-aspect-ratio=(fraction)[ 1/2147483647, 2147483647/1 ] <videorate0> transform could not transform image/jpeg, framerate=(fraction)15/1, width=(int)480, height=(int)640 in anything we support. Should we special case pixel-aspect-ratio or is there any generic solution?
I think we should, yes. pixel-aspect-ratio=1/1 is the same as not having it in the caps. So videodecoder should probably remove it from the downstream caps if it's 1/1, and add it to its output caps with 1/1 if the output caps don't have it. Or something like this.
Created attachment 309085 [details] [review] basetransform: respect accept-caps intersect flag
Created attachment 309086 [details] [review] basetransform: try fixating harder by removing extra added fields Downstream can 'suggest' fields that when intersected with the caps from the caps event will be added to the intersection. It seems wrong to pass those values downstream if they were not added in the transform caps operation of the element.
Created attachment 309087 [details] [review] videorate: set intersect as accept-caps caps operation
This is a possible solution. Sorry for the short commit messages and please review with care as those were made during flight layover time.
(In reply to Thiago Sousa Santos from comment #9) > This is a possible solution. Sorry for the short commit messages and please > review with care as those were made during flight layover time. *those* includes the code and the messages, just to be extra clear.
As discussed, what should happen here is that caps without pixel-aspect-ratio should get 1/1 set. But only fixed caps from the CAPS event, during negotiation it would need to be left unset.
commit 7ec54c2217ad1c1c00b616e9644d33902df802fb Author: Thiago Santos <thiagoss@osg.samsung.com> Date: Tue Aug 11 08:07:53 2015 -0300 basetransform: respect accept-caps intersect flag GstPad has a flag for suggesting if the accept-caps query should use intersect instead of the default subset caps operation to verify if the caps would be acceptable. basetransform currently always uses the subset check and this patch makes it honor the flag for using intersect if it is set. https://bugzilla.gnome.org/show_bug.cgi?id=748635 First patch pushed (unrelated to the direction we're going when fixing this, but useful anyway)
Created attachment 309225 [details] [review] basetransform: rework accept-caps According to the design docs: The ACCEPT_CAPS query is not required to work recursively, it can simply return TRUE if a subsequent CAPS event with those caps would return success. So make it a shallow check instead of recursivelly check downstream.
The question about this patch is if we can use the pad accept-intersect flag here to use subset/intersect or just go with intersect as it is always with the template that is usually simpler. Another option is we want to honor the flag is to have basetransform use the accept-intersect flag on by default and people can use subset if they disable it.
Created attachment 309226 [details] [review] videorate: fixate the pixel-aspect-ratio If the pixel-aspect-ratio is not fixed, try to get it as close to 1/1 as possible
Review of attachment 309225 [details] [review]: Looks good ::: libs/gst/base/gstbasetransform.c @@ +1312,1 @@ + ocaps = gst_base_transform_transform_caps (trans, direction, caps, NULL); Could we put otempl there as the filter caps, and then just check for non-empty ocaps?
Comment on attachment 309086 [details] [review] basetransform: try fixating harder by removing extra added fields This seems a bit dangerous
Comment on attachment 309226 [details] [review] videorate: fixate the pixel-aspect-ratio I think this will cause 1/1 PAR to be added to caps during negotiation if downstream has no PAR in the caps. No PAR in the caps during negotiation means that all PARs are supported, not only 1/1.
As discussed, the videorate patch is ok. It will only act if there is a PAR already. commit 909f494a5a7f901fcd3f203173370ce45bed0b13 Author: Thiago Santos <thiagoss@osg.samsung.com> Date: Thu Aug 13 13:52:17 2015 -0300 videorate: fixate the pixel-aspect-ratio If the pixel-aspect-ratio is not fixed, try to get it as close to 1/1 as possible https://bugzilla.gnome.org/show_bug.cgi?id=748635
Comment on attachment 309226 [details] [review] videorate: fixate the pixel-aspect-ratio A simplified version was merged. (3 lines shorter)
Comment on attachment 309225 [details] [review] basetransform: rework accept-caps commit e0cc0e0888a1e900b6f5be5119324c5e1db13c95 Author: Thiago Santos <thiagoss@osg.samsung.com> Date: Thu Aug 13 13:08:03 2015 -0300 basetransform: rework accept-caps According to the design docs: The ACCEPT_CAPS query is not required to work recursively, it can simply return TRUE if a subsequent CAPS event with those caps would return success. So make it a shallow check instead of recursivelly check downstream. https://bugzilla.gnome.org/show_bug.cgi?id=748635