After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 766596 - vaapipostproc: some video info is dropped on src caps.
vaapipostproc: some video info is dropped on src caps.
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gstreamer-vaapi
git master
Other All
: Normal enhancement
: 1.9.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2016-05-18 09:05 UTC by Hyunjun Ko
Modified: 2016-10-31 14:24 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
postproc: set remaining video info to src caps (2.03 KB, patch)
2016-05-18 09:08 UTC, Hyunjun Ko
none Details | Review
vaapipostproc: Add colorimetry attributes to src caps (2.28 KB, patch)
2016-05-19 04:07 UTC, Hyunjun Ko
none Details | Review
vaapidecode: remove chroma-site and colorimetry from src caps (6.92 KB, patch)
2016-06-07 03:48 UTC, Hyunjun Ko
none Details | Review
vaapidecode: remove chroma-site and colorimetry from src caps (1.38 KB, patch)
2016-06-07 03:49 UTC, Hyunjun Ko
none Details | Review
vaapipostproc: Add colorimetry attributes to src caps (2.32 KB, patch)
2016-06-08 01:15 UTC, Hyunjun Ko
committed Details | Review
vaapidecode: remove chroma-site and colorimetry from src caps (1.20 KB, patch)
2016-06-08 01:18 UTC, Hyunjun Ko
committed Details | Review

Description Hyunjun Ko 2016-05-18 09:05:08 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.
Comment 1 Hyunjun Ko 2016-05-18 09:08:02 UTC
Created attachment 328124 [details] [review]
postproc: set remaining video info to src caps
Comment 2 Víctor Manuel Jáquez Leal 2016-05-18 09:30:36 UTC
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.
Comment 3 Hyunjun Ko 2016-05-19 04:07:49 UTC
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.
Comment 4 Víctor Manuel Jáquez Leal 2016-05-19 08:18:43 UTC
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.
Comment 5 Víctor Manuel Jáquez Leal 2016-05-19 08:21:41 UTC
(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.
Comment 6 Hyunjun Ko 2016-05-20 01:35:14 UTC
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.
Comment 7 Hyunjun Ko 2016-06-07 03:48:42 UTC
Created attachment 329234 [details] [review]
vaapidecode: remove chroma-site and colorimetry from src caps

Doing this in case of using VASurface
Comment 8 Hyunjun Ko 2016-06-07 03:49:52 UTC
Created attachment 329235 [details] [review]
vaapidecode: remove chroma-site and colorimetry from src caps

Doing this in case of using VASurface
Comment 9 Víctor Manuel Jáquez Leal 2016-06-07 10:12:05 UTC
(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?
Comment 10 Hyunjun Ko 2016-06-07 10:19:02 UTC
(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?
Comment 11 Víctor Manuel Jáquez Leal 2016-06-07 10:41:27 UTC
(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
Comment 12 Víctor Manuel Jáquez Leal 2016-06-07 10:42:41 UTC
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.
Comment 13 Hyunjun Ko 2016-06-08 01:15:49 UTC
Created attachment 329354 [details] [review]
vaapipostproc: Add colorimetry attributes to src caps
Comment 14 Hyunjun Ko 2016-06-08 01:18:47 UTC
Created attachment 329355 [details] [review]
vaapidecode: remove chroma-site and colorimetry from src caps
Comment 15 Víctor Manuel Jáquez Leal 2016-06-08 08:18:02 UTC
Comment on attachment 329354 [details] [review]
vaapipostproc: Add colorimetry attributes to src caps

LGTM
Comment 16 Víctor Manuel Jáquez Leal 2016-06-08 08:18:20 UTC
Comment on attachment 329355 [details] [review]
vaapidecode: remove chroma-site and colorimetry from src caps

LGTM
Comment 17 Víctor Manuel Jáquez Leal 2016-06-08 08:39:23 UTC
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