GNOME Bugzilla – Bug 766596
vaapipostproc: some video info is dropped on src caps.
Last modified: 2016-10-31 14:24:02 UTC
While I'm looking into enabling passthrough (#751876) in vaapipostproc, I found some src caps attrs are missing. In my case, sink caps : video/x-raw(memory:VASurface), format=(string)NV12, width=(int)854, height=(int)480, interlace-mode=(string)progressive, pixel-aspect-ratio=(fraction)1/1, chroma-site=(string)jpeg, colorimetry=(string)bt601, framerate=(fraction)25/1 src caps : video/x-raw(memory:VASurface), format=(string)NV12, width=(int)854, height=(int)480, framerate=(fraction)25/1, interlace-mode=(string)progressive, pixel-aspect-ratio=(fraction)1/1 You can see chroma-site and colorimetry is missing in src caps. This is caused by this patch https://cgit.freedesktop.org/gstreamer/gstreamer-vaapi/commit/?id=4d1b11ed03759ef702c675cbacdb5ea543daa305 IMHO, this should be fixed to enable passthrough in vaapipostproc.
Created attachment 328124 [details] [review] postproc: set remaining video info to src caps
Comment on attachment 328124 [details] [review] postproc: set remaining video info to src caps The colorimetry and the chroma siting should be defined by the src caps, not the sink caps, since there could be color transformations. That's why I preferred to eliminate them. Though it could be nice to set them back, given that we have fixated the src caps color format.
Created attachment 328168 [details] [review] vaapipostproc: Add colorimetry attributes to src caps Victor, understood. I re-write a patch to do this, based on fixated information.
Review of attachment 328168 [details] [review]: ::: gst/vaapi/gstvaapipostprocutil.c @@ +562,3 @@ + return FALSE; + + gst_video_info_set_format (&vinfo, format, width, height); It is a shame that we have to use gst_video_info_set_format for this, since it is expensive, but I don't see another option either. @@ +646,3 @@ if (!_fixate_frame_rate (postproc, vinfo, structure)) goto fixate_failed; + if (!_set_colorimetry (postproc, format, structure)) We also have to check if the processed feature is GST_CAPS_FEATURE_MEMORY_SYSTEM_MEMORY. Other features, like GLTextureUpload or VASurface don't need that information in the caps. @@ +647,3 @@ goto fixate_failed; + if (!_set_colorimetry (postproc, format, structure)) + goto fixate_failed; The only way that colorimetry could fail is if the frame size is not set, which should failed before. But it is OK to play safe.
(In reply to Hyunjun Ko from comment #0) > While I'm looking into enabling passthrough (#751876) in vaapipostproc, I > found some src caps attrs are missing. > > In my case, > sink caps : video/x-raw(memory:VASurface), format=(string)NV12, > width=(int)854, height=(int)480, interlace-mode=(string)progressive, > pixel-aspect-ratio=(fraction)1/1, chroma-site=(string)jpeg, > colorimetry=(string)bt601, framerate=(fraction)25/1 > > src caps : video/x-raw(memory:VASurface), format=(string)NV12, > width=(int)854, height=(int)480, framerate=(fraction)25/1, > interlace-mode=(string)progressive, pixel-aspect-ratio=(fraction)1/1 > > You can see chroma-site and colorimetry is missing in src caps. > > This is caused by this patch > https://cgit.freedesktop.org/gstreamer/gstreamer-vaapi/commit/ > ?id=4d1b11ed03759ef702c675cbacdb5ea543daa305 > > IMHO, this should be fixed to enable passthrough in vaapipostproc. mmmh... IMHO, we should remove chroma-site and colorimetry to capsfeature VASurface in src caps of the decoder.
Victor, (In reply to Víctor Manuel Jáquez Leal from comment #5) > > mmmh... IMHO, we should remove chroma-site and colorimetry to capsfeature > VASurface in src caps of the decoder. I also think it's better than this patch :) This patch also produces chroma-site and colorimetry even in case that sink caps doesn't contain them :(, which is not good.
Created attachment 329234 [details] [review] vaapidecode: remove chroma-site and colorimetry from src caps Doing this in case of using VASurface
Created attachment 329235 [details] [review] vaapidecode: remove chroma-site and colorimetry from src caps Doing this in case of using VASurface
(In reply to Víctor Manuel Jáquez Leal from comment #4) > > @@ +646,3 @@ > if (!_fixate_frame_rate (postproc, vinfo, structure)) > goto fixate_failed; > + if (!_set_colorimetry (postproc, format, structure)) > > We also have to check if the processed feature is > GST_CAPS_FEATURE_MEMORY_SYSTEM_MEMORY. Other features, like GLTextureUpload > or VASurface don't need that information in the caps. What about this check?
(In reply to Víctor Manuel Jáquez Leal from comment #9) > (In reply to Víctor Manuel Jáquez Leal from comment #4) > > > > @@ +646,3 @@ > > if (!_fixate_frame_rate (postproc, vinfo, structure)) > > goto fixate_failed; > > + if (!_set_colorimetry (postproc, format, structure)) > > > > We also have to check if the processed feature is > > GST_CAPS_FEATURE_MEMORY_SYSTEM_MEMORY. Other features, like GLTextureUpload > > or VASurface don't need that information in the caps. > > What about this check? Oh, I thought that this patch would be ignored once vaapidecoder drops these attributes. This is necessary in case that caps feature is GST_CAPS_FEATURE_MEMORY_SYSTEM_MEMORY?
(In reply to Hyunjun Ko from comment #10) > (In reply to Víctor Manuel Jáquez Leal from comment #9) > > (In reply to Víctor Manuel Jáquez Leal from comment #4) > > > > > > @@ +646,3 @@ > > > if (!_fixate_frame_rate (postproc, vinfo, structure)) > > > goto fixate_failed; > > > + if (!_set_colorimetry (postproc, format, structure)) > > > > > > We also have to check if the processed feature is > > > GST_CAPS_FEATURE_MEMORY_SYSTEM_MEMORY. Other features, like GLTextureUpload > > > or VASurface don't need that information in the caps. > > > > What about this check? > > Oh, I thought that this patch would be ignored once vaapidecoder drops these > attributes. > This is necessary in case that caps feature is > GST_CAPS_FEATURE_MEMORY_SYSTEM_MEMORY? Noup, because you patch extracts the colorimetry value from gst_video_info_set_format (&vinfo, format, width, height); Where format is the output format. And that is true only when the caps feature is GST_CAPS_FEATURE_MEMORY_SYSTEM_MEMORY
Review of attachment 329235 [details] [review]: ::: gst/vaapi/gstvaapidecode.c @@ +311,3 @@ + + /* fall-through */ + case GST_VAAPI_CAPS_FEATURE_GL_TEXTURE_UPLOAD_META: AFAIK, also when GLTextureUpload is preferred we should drop them.
Created attachment 329354 [details] [review] vaapipostproc: Add colorimetry attributes to src caps
Created attachment 329355 [details] [review] vaapidecode: remove chroma-site and colorimetry from src caps
Comment on attachment 329354 [details] [review] vaapipostproc: Add colorimetry attributes to src caps LGTM
Comment on attachment 329355 [details] [review] vaapidecode: remove chroma-site and colorimetry from src caps LGTM
Attachment 329354 [details] pushed as 08ee0f9 - vaapipostproc: Add colorimetry attributes to src caps Attachment 329355 [details] pushed as 1e52269 - vaapidecode: remove chroma-site and colorimetry from src caps