GNOME Bugzilla – Bug 758548
vaapipostproc: Allow video scaling using capsfilter
Last modified: 2016-05-10 07:49:12 UTC
The current implementation of vaapipostproc will do the scaling(resizing) only if it set explicitly through properties. If you try to use capsfilter for resizing , pipeline will just fail. vaapipost should work like videoscale element for resizing. eg: The following pipeline will fail with the current implementation. gst-launch-1.0 videotestsrc ! video/x-raw, width=320, height=240 ! vaapipostproc ! video/x-raw, width=1280, height=544 ! vaapisink
Created attachment 323677 [details] [review] vaapipostproc: rework scaling in transform_caps Here's a patch that is working for me on some simple pipelines. I'm working on more thorough testing so I can be more sure I'm not adding regressions here.
Hi Scott!!! Sorry for delaying this review :( I just started today and the patch is huge! I wonder if it is possible to break it down into smaller commits to ease the review O:) As a initial comment I think that the class attribute filter_formats_list it is not necessary since it is only used at caps negotiation, and it is not that cpu intensive: converting an integer array into a string array. So, instead of a class attribute a function to call when caps negotiation. Also, I thing that gst_vaapipostproc_supported_caps_features() can be simplified using gst_vaapi_caps_feature_contains(), but not sure if it is less expensive. And that is it for now. :)
Hi, This a head up of what I've been doing lately on this issue. I started studying what Scott did on transform_caps() vmethod because I didn't understand it. Then I realized how this vmethod is implemented in other elements and how Scott bring these implementations into vaapipostproc. Great job, Scott! But I also realized that vaapipostproc is a "super-element" which bundles these video filters * videoscale * videoconvert * deinterlace * videobalance among other tasks, such as skin color enhancement. This means that, if we want to provide the same user experience, as using a combination of those elements, we need to implement these complex functions transform_caps() and fixate_caps() as all those elements do. It is not a trivial task. Though what Scott did is a good start. I have worked a lot, re-writing Scott patch because I saw it is required to handle the color formats differently (querying the available surface-upload and surface-download color formats) and only transforming the caps when the sink caps were not fixated yet. It works but still I'm facing tons of regressions :/
(In reply to Víctor Manuel Jáquez Leal from comment #2) > I just started today and the patch is huge! I wonder if it is possible to > break it down into smaller commits to ease the review O:) It's been a while since I wrote this patch so it's like reading someone else's code now :-) I'm not seeing a good way to make this change incrementally. Like you said, the caps negotiation logic here is quite similar to several software processing elements. Maybe we could extract some utility functions from those components and then use them in vpp as well. Anyway sounds like you're on the track of making the caps negotiation even better than I was, so let me know if there's anything I can do to help.
Created attachment 327371 [details] [review] vaapipostproc: log the caps transformation
Created attachment 327372 [details] [review] vaapipostproc: set early properties restrictions When running transform_caps() vmethod, returning the srcpad caps, the caps are early restricted to the element properties set: width, height, format and force keep aspect. A new file was added gstvaapipostprocutil.{c,h} where the utilities functions are stored.
Created attachment 327373 [details] [review] vaapipostproc: add fixate_caps() vmethod Instead of fixating the srcpad caps in transform_caps() vmethod, this patch implements the fixate_caps() vmethod and moves code around. Original-patch-by: Scott D Phillips <scott.d.phillips@intel.com>
Created attachment 327374 [details] [review] vaapipostproc: use othercaps for preferred caps Instead of the allowed_srcpad_caps variable, this patch uses the othercaps from fixate_caps() vmethod to find the preferred caps feature and color format.
Created attachment 327375 [details] [review] vaapipostproc: simplify code Change a convoluted snippet to find the preferred color format in the peer caps.
Created attachment 327376 [details] [review] vaapipostproc: move gst_vaapipostproc_fixate_srccaps() Move gst_vaapipostproc_fixate_srccaps() to gstvaapiposptprocutil. No functional changes.
Created attachment 327377 [details] [review] vaapipostproc: don't use GstVideoInfo for src caps Instead of using gst_video_info_to_caps () to generated the fixed src caps, this patch enables the first step for caps negotiation with a possible following caps filter. _get_preferred_caps() will traverse the possible src caps looking for the one wit the preferred feature and the preferred color format. Then the color format, the frame size and the frame rate are fixated.
Created attachment 327378 [details] [review] vaapipostproc: negotiate frame size fixation Refactor _fixate_frame_size(). Now, instead of fixating the frame size only using the sink caps, also it use the next capsfilter. This code is a shameless copy of gst_video_scale_fixate_caps() from https://cgit.freedesktop.org/gstreamer/gst-plugins-base/tree/gst/videoscale/gstvideoscale.c?id=1.8.1#n634
Comment on attachment 323677 [details] [review] vaapipostproc: rework scaling in transform_caps I mark this patch as obsolete because it is superseded by the following patch set, which is inspired in this one.
I still have doubts about the framerate negotiation and how the debug category is declared, but overall i thing this is a good improvement in vaapipostroc. I'm unsure if reusing a big chunk of code from videoscale would be an issue.
(In reply to Víctor Manuel Jáquez Leal from comment #14) > I still have doubts about the framerate negotiation and how the debug > category is declared, but overall i thing this is a good improvement in > vaapipostroc. > > I'm unsure if reusing a big chunk of code from videoscale would be an issue. I think there is no issue with it. Also If you think there is significant code moved from gstvaapipostproc.{h,c} to gstvaapipostprocutils.{h,c} , then add Gwenole's name in Author's list too. Noticed a copy & paste typo in gstvaapipostprocutil.h "Copyright (C) 2012-2014 Intel Corporation"
(In reply to sreerenj from comment #15) > Also If you think there is significant code moved from > gstvaapipostproc.{h,c} to gstvaapipostprocutils.{h,c} , then add Gwenole's > name in Author's list too. > > Noticed a copy & paste typo in gstvaapipostprocutil.h > "Copyright (C) 2012-2014 Intel Corporation" True. Let me attach the modifications.
Created attachment 327386 [details] [review] vaapipostproc: log the caps transformation
Created attachment 327387 [details] [review] vaapipostproc: set early properties restrictions When running transform_caps() vmethod, returning the srcpad caps, the caps are early restricted to the element properties set: width, height, format and force keep aspect. A new file was added gstvaapipostprocutil.{c,h} where the utilities functions are stored.
Created attachment 327388 [details] [review] vaapipostproc: add fixate_caps() vmethod Instead of fixating the srcpad caps in transform_caps() vmethod, this patch implements the fixate_caps() vmethod and moves code around. Original-patch-by: Scott D Phillips <scott.d.phillips@intel.com>
Created attachment 327389 [details] [review] vaapipostproc: use othercaps for preferred caps Instead of the allowed_srcpad_caps variable, this patch uses the othercaps from fixate_caps() vmethod to find the preferred caps feature and color format.
Created attachment 327390 [details] [review] vaapipostproc: simplify code Change a convoluted snippet to find the preferred color format in the peer caps.
Created attachment 327391 [details] [review] vaapipostproc: move gst_vaapipostproc_fixate_srccaps() Move gst_vaapipostproc_fixate_srccaps() to gstvaapiposptprocutil. No functional changes.
Created attachment 327392 [details] [review] vaapipostproc: don't use GstVideoInfo for src caps Instead of using gst_video_info_to_caps () to generated the fixed src caps, this patch enables the first step for caps negotiation with a possible following caps filter. _get_preferred_caps() will traverse the possible src caps looking for the one wit the preferred feature and the preferred color format. Then the color format, the frame size and the frame rate are fixated.
Created attachment 327393 [details] [review] vaapipostproc: negotiate frame size fixation Refactor _fixate_frame_size(). Now, instead of fixating the frame size only using the sink caps, also it use the next capsfilter. This code is a shameless copy of gst_video_scale_fixate_caps() from https://cgit.freedesktop.org/gstreamer/gst-plugins-base/tree/gst/videoscale/gstvideoscale.c?id=1.8.1#n634
Attachment 327386 [details] pushed as 54a2d9f - vaapipostproc: log the caps transformation Attachment 327387 [details] pushed as 606d166 - vaapipostproc: set early properties restrictions Attachment 327388 [details] pushed as 8de7faa - vaapipostproc: add fixate_caps() vmethod Attachment 327389 [details] pushed as bde3b07 - vaapipostproc: use othercaps for preferred caps Attachment 327390 [details] pushed as d9b09b6 - vaapipostproc: simplify code Attachment 327391 [details] pushed as 1901e22 - vaapipostproc: move gst_vaapipostproc_fixate_srccaps() Attachment 327392 [details] pushed as 4d1b11e - vaapipostproc: don't use GstVideoInfo for src caps Attachment 327393 [details] pushed as 7baacda - vaapipostproc: negotiate frame size fixation