(In reply to Florent Thiéry from comment #0)
> Created attachment 317488 [details]
> screenshot
> 
> Running the following test pipeline results in a wrong display
> gst-launch-1.0 videotestsrc ! video/x-raw\,\ format\=\(string\)YUY2 !
> vaapisink
> 
> Adding vaapipostproc is a workaround, but why does vaapisink advertise YUY2
> caps in the first place ?
> 
> gst-launch-1.0 videotestsrc ! video/x-raw\,\ format\=\(string\)YUY2 !
> vaapipostproc ! vaapisink

vaapisink, by default (gst-inspect-1.0) shows that it can handle all the available formats, but the element will only know which chromas can upload into a VAAPI surface when the driver (backend) is opened and queried. In this case, for the intel backend, the YUY2 format is not well supported.

Perhaps this issue is related with bug 752958, but for upload rather than download.

We should narrow the supported capabilities and expose them by query-caps by check them at initialization.

Or maybe vaapisink should be a bin like glimagesink which contains glupload that does color conversion whenever necessary (or passthrough when not) ?

(In reply to Florent Thiéry from comment #2)
> Or maybe vaapisink should be a bin like glimagesink which contains glupload
> that does color conversion whenever necessary (or passthrough when not) ?

That's another option, but we already have a vaapidecodebin that bundles vaapidecode and vaapipostproc.

I don't know.

Moving to Product:GStreamer, Component:gstreamer-vaapi

Right now I'm working on bug 765435 and I ran across this issue again.

I'm using a haswell chipset and either YUY2 and UYVY are not well handled by vaapisink, like if vaapisink are displaying a different color space. Meanwhile, NV12, YV12, I420, RGBx and BGRx are handled correctly.

Perhaps we are forgetting to set some parameter in the case of packet formats but non-RGB.

YUY2 and UYVY are unimplemented for rendering in the intel-driver. I submitted a patch to the driver to fail vaPutSurface instead of rendering junk:

 https://lists.freedesktop.org/archives/libva/2016-July/004201.html

With just that patch we're not yet in a great place though. vaapisink will still advertise the 422 formats in the caps but then fail to actually play. If we blacklist those formats from the vaapisink caps then pipelines could properly autoplug format conversion.

@Scott, is there a way in libva to query which formats are supported by vaPutSurface?

Otherwise we'll only fix it hackishly: "if this is a sink and the backend is intel, then only expose these color spaces" and that's horrible.

Created attachment 339995 [details] [review]
[PATCH 1/2] pluginbase: Add vmethod verify_put_surface

To advertise only supported format in the sink caps of vaapisink,
it need to verify what's supported when making allowed raw caps.

This patch creates vmethod and default method in vaapipluginbase first.

Created attachment 339996 [details] [review]
[PATCH 2/2] vaapisink: Implement verify_put_surface vmethod

Implements verify_put_surface vmethod.
This patch avoids advertising unsupported format in the sink caps.

I'm not sure if this is proper approach.
Untested on non-intel driver.
I guess, crash might occur depending on driver.

(In reply to Víctor Manuel Jáquez Leal from comment #7)
> @Scott, is there a way in libva to query which formats are supported by
> vaPutSurface?
> 
> Otherwise we'll only fix it hackishly: "if this is a sink and the backend is
> intel, then only expose these color spaces" and that's horrible.

No, there's no API to query this. The render/export constraints will vary by backend too, so it would need to be a per-backend query API. I think the ideal is for the va api would just export from VASurface to backend_surface and then rendering would follow the normal path for the backend, so I don't think we're eager to add more porcelain around vaPutSurface.

Review of attachment 339996 [details] [review]:

If a foreign_window is set then will these test surfaces be displayed to it?

Review of attachment 339996 [details] [review]:

If a foreign_window is set then will these test surfaces be displayed to it?

(In reply to Scott D Phillips from comment #13)
> Review of attachment 339996 [details] [review] [review]:
> 
> If a foreign_window is set then will these test surfaces be displayed to it?

No. app can set its own window by only gst_video_overlay_set_window_handle on bus_sync_handler afaik, which means that happens after this operation is done.

If I'm missing something, please let me know.
Does it need to check foreing_window flag?

Comment on attachment 339996 [details] [review]
[PATCH 2/2] vaapisink: Implement verify_put_surface vmethod

Overall, I don't feel right these patches. They are too "hackyish". And, as Scott said, we were taking the risk to screw the backend or to show those frames in the screen.

A proper fix would be to add API in libva to query what formats are supported by vaPutSurface.

Another fix would be to add a VPP in the X11 backend (similar to Wayland backend) and do the required color transformation there.

(In reply to Víctor Manuel Jáquez Leal from comment #15)
> 
> A proper fix would be to add API in libva to query what formats are
> supported by vaPutSurface.
> 
> Another fix would be to add a VPP in the X11 backend (similar to Wayland
> backend) and do the required color transformation there.

I'm interested in the second way and I have an idea for it.
I'll propose a new idea soon.

Just a note. 
https://github.com/01org/intel-vaapi-driver/pull/55
This PR is related to this.

Created attachment 347017 [details] [review]
RFC: libs: window: x11: Add VPP to X11 backend

This is not new idea, but following victor's the second idea and imitating current wayland window's behaviour.

If this idea is acceptable, we can move duplicated logic to parent.
Comments?

Review of attachment 347017 [details] [review]:

I like this idea. We need to refactor it with the wayland backend

::: gst-libs/gst/vaapi/gstvaapiwindow_x11.c
@@ +272,3 @@
+
+  priv->surface_format = GST_VIDEO_FORMAT_ENCODED;
+  priv->use_vpp = GST_VAAPI_DISPLAY_HAS_VPP (GST_VAAPI_OBJECT_DISPLAY (window));

I would call this variable has_vpp

@@ +434,3 @@
+      goto error_unsupported_format;
+  }
+  if (!gst_vaapi_filter_set_cropping_rectangle (priv->filter, src_rect))

is this cropping really needed?

@@ +436,3 @@
+  if (!gst_vaapi_filter_set_cropping_rectangle (priv->filter, src_rect))
+    return NULL;
+  if (!gst_vaapi_filter_set_target_rectangle (priv->filter, dst_rect))

also is this rectangle?

@@ +525,3 @@
     return FALSE;
 
+  if (need_vpp && priv->use_vpp) {

what would happen if need_vpp but doesn't have use_vpp? the method will return TRUE without signal any error condition

Created attachment 348406 [details]
script to test all the vaapisink formats

As you can see with this script

NV12, YV12 and I420: works OK
YUY2, UYVY: shows artifacts in the borders (looks like bug #777820)
422H: the pipeline fails
RGBx, BGRx: the output is corrupt
P010_10LE: the filter (intel) can't import this color space

Thanks for the script.

(In reply to Víctor Manuel Jáquez Leal from comment #19)
> Created attachment 348406 [details]
> script to test all the vaapisink formats
> 
> As you can see with this script
> 
> NV12, YV12 and I420: works OK
> YUY2, UYVY: shows artifacts in the borders (looks like bug #777820)
Weird, I can't see any artifact with vaapisink.

> 422H: the pipeline fails
videotestsrc doesn't support 422H, maybe gstreamer neither?

> RGBx, BGRx: the output is corrupt
I suspect driver, which looks it doesn't support correctly on X11.

> P010_10LE: the filter (intel) can't import this color space
Indeed. Fortunately, vaapisink doesn't accept this format during negotiation.


I guess this is why you really want a query for this in the level of libva.
Even though we can workaround with vpp with some formats like YUY2/UYVY,
there are still problems with others :( 

But what can we do... :P

(In reply to Hyunjun Ko from comment #20)
> Thanks for the script.
> 
> (In reply to Víctor Manuel Jáquez Leal from comment #19)
> > Created attachment 348406 [details]
> > script to test all the vaapisink formats
> > 
> > As you can see with this script
> > 
> > NV12, YV12 and I420: works OK
> > YUY2, UYVY: shows artifacts in the borders (looks like bug #777820)
> Weird, I can't see any artifact with vaapisink.

Perhaps is something that happens only in HSW.

> 
> > 422H: the pipeline fails
> videotestsrc doesn't support 422H, maybe gstreamer neither?

True. D'ah! Sorry.

> 
> > RGBx, BGRx: the output is corrupt
> I suspect driver, which looks it doesn't support correctly on X11.

Yeah. It seems that we need to patch the driver.

> 
> > P010_10LE: the filter (intel) can't import this color space
> Indeed. Fortunately, vaapisink doesn't accept this format during negotiation.
> 
>
> I guess this is why you really want a query for this in the level of libva.
> Even though we can workaround with vpp with some formats like YUY2/UYVY,
> there are still problems with others :( 
> 
> But what can we do... :P

I like the idea of having postprocessor inside the vaapisink, but I would rather imitate glimagesink, of being a bin. Just that we are exposing more bugs in the driver.

Created attachment 348776 [details] [review]
libs: window: create an api   gst_vaapi_window_vpp_convert_internal

If a backend doesn't support specific format, we can use vpp for conversion
and make it playing.

This api is originated from GstVaapiWindowWayland and moved to GstVaapiWindow,
so that GstVaapiWindowX11 could use it.

Created attachment 348777 [details] [review]
libs: window: x11/wayland: use   gst_vaapi_window_vpp_convert_internal for conversion

Since gst_vaapi_window_vpp_convert_internal is created,
GstVaapiWindowX11/Wayland can use it for conversion.

Review of attachment 348776 [details] [review]:

::: gst-libs/gst/vaapi/gstvaapiwindow.c
@@ +120,3 @@
+  GstVaapiSurface *vpp_surface = NULL;
+  GstVaapiFilterStatus status;
+

should we check first for 

if (!window->has_vapp)
  return NULL;

???

@@ +123,3 @@
+  /* Ensure VA surface pool is created */
+  /* XXX: optimize the surface format to use. e.g. YUY2 */
+  if (!window->surface_pool) {

I would create a boolean function ensure_filter_surface_pool()

@@ +132,3 @@
+
+  /* Ensure VPP pipeline is built */
+  if (!window->filter) {

Again, I'd create a boolean function ensure_filter().

It might call ensure_filter_surface_pool() or roll it in.

@@ +141,3 @@
+
+  if (src_rect
+      && !gst_vaapi_filter_set_cropping_rectangle (window->filter, src_rect))

IMO, this is hard to read. I'd change it to

if (src_rect) {
   blah
}

@@ +144,3 @@
+    return NULL;
+  if (dst_rect
+      && !gst_vaapi_filter_set_target_rectangle (window->filter, dst_rect))

ditto

Review of attachment 348777 [details] [review]:

::: gst-libs/gst/vaapi/gstvaapiwindow_wayland.c
@@ +331,3 @@
 
+  gst_vaapi_filter_replace (&window->filter, NULL);
+  gst_vaapi_video_pool_replace (&window->surface_pool, NULL);

Somehow I feel we should move this into gstvaapiwindow, since these variables are private to GstVaapiWindow

@@ +347,3 @@
   GST_DEBUG ("resize window, new size %ux%u", width, height);
 
+  gst_vaapi_video_pool_replace (&window->surface_pool, NULL);

ditto

::: gst-libs/gst/vaapi/gstvaapiwindow_x11.c
@@ +300,3 @@
+
+  gst_vaapi_video_pool_replace (&window->surface_pool, NULL);
+  gst_vaapi_filter_replace (&window->filter, NULL);

move to GstVaapiWindow class

@@ +463,3 @@
+          gst_vaapi_window_x11_put_surface (window, surface_id, src_rect,
+          dst_rect, flags);
+      gst_vaapi_video_pool_put_object (window->surface_pool, vpp_surface);

This mean that right after vaPutSurface() the vpp_surface can be removed at anytime. Are we sure that vaPutSurface works synchronously?

@@ +465,3 @@
+      gst_vaapi_video_pool_put_object (window->surface_pool, vpp_surface);
+
+      if (!(ret = vaapi_check_status (status, "vaPutSurface()"))) {

this if () is really needed. AFAIK we only need to get the ret = vaapi_check_status() because the next sentence is return, either true o false.

Review of attachment 348777 [details] [review]:

::: gst-libs/gst/vaapi/gstvaapiwindow_x11.c
@@ +450,3 @@
+      status == VA_STATUS_ERROR_UNIMPLEMENTED ||
+      status == VA_STATUS_ERROR_INVALID_IMAGE_FORMAT) {
+    need_vpp = TRUE;

I would keep this variable for all the session so the next frame, instead of calling vaPutSurface again, and fail again, will jump to convert_internal() automatically.

(In reply to Víctor Manuel Jáquez Leal from comment #25)
> Review of attachment 348777 [details] [review] [review]:
> 
> ::: gst-libs/gst/vaapi/gstvaapiwindow_wayland.c
> @@ +331,3 @@
>  
> +  gst_vaapi_filter_replace (&window->filter, NULL);
> +  gst_vaapi_video_pool_replace (&window->surface_pool, NULL);
> 
> Somehow I feel we should move this into gstvaapiwindow, since these
> variables are private to GstVaapiWindow
>

Good point.
I was trying to destroy them in GstVaapiWindow, but currently, X11/Wayland VaapiWindow are not decendant of it. 
So we should choose:
1. Make them children of GstVaapiWindow.
2. Just implements new internal function to destroy stuff in GstVaapiWindow.

Probably, we should do option 1, opinion?


> @@ +463,3 @@
> +          gst_vaapi_window_x11_put_surface (window, surface_id, src_rect,
> +          dst_rect, flags);
> +      gst_vaapi_video_pool_put_object (window->surface_pool, vpp_surface);
> 
> This mean that right after vaPutSurface() the vpp_surface can be removed at
> anytime. Are we sure that vaPutSurface works synchronously?

Another good point:)
Actually I had doubt of this, but not found something unexpected.
But I think I can call surface sync api after put surface.

Created attachment 349165 [details] [review]
libs: window: x11/wayland: chaining up to GstVaapiWindow

Currently, GstVaapiWindowX11/Wayland are not descendants of GstVaapiWindow.
This patch chains them up to GstVaapiWindow to handle common members in GstVaapiWindow.

Created attachment 349166 [details] [review]
libs: window: create an api   gst_vaapi_window_vpp_convert_internal

If a backend doesn't support specific format, we can use vpp for conversion
and make it playing.

This api is originated from GstVaapiWindowWayland and moved to GstVaapiWindow,
so that GstVaapiWindowX11 could use it. 

Note that once it choose to use vpp, it's going to use vpp until the session is finished.

In addition, there is another new api gst_vaapi_window_set_vpp_surface_pool
to set an instance of surface pool for vpp if needed.

Created attachment 349167 [details] [review]
libs: window: x11/wayland: use   gst_vaapi_window_vpp_convert_internal for conversion

Since gst_vaapi_window_vpp_convert_internal is created,
GstVaapiWindowX11/Wayland can use it for conversion.

Created attachment 349207 [details] [review]
libs: window: create an api   gst_vaapi_window_vpp_convert_internal

If a backend doesn't support specific format, we can use vpp for conversion
and make it playing.

This api is originated from GstVaapiWindowWayland and moved to GstVaapiWindow,
so that GstVaapiWindowX11 could use it. 

Note that once it choose to use vpp, it's going to use vpp until the session is finished.

Created attachment 349208 [details] [review]
libs: window: x11/wayland: use   gst_vaapi_window_vpp_convert_internal for conversion

Since gst_vaapi_window_vpp_convert_internal is created,
GstVaapiWindowX11/Wayland can use it for conversion.

Review of attachment 349165 [details] [review]:

lgtm.

I wonder if we can change these classes to GObject easily.

Review of attachment 349207 [details] [review]:

a couple things

::: gst-libs/gst/vaapi/gstvaapiwindow.c
@@ +64,3 @@
+
+  /* Ensure VPP pipeline is built */
+  if (!window->filter) {

gstreamer-vaapi code generally uses the "early return" idiom ;)

if (window->filter)
  return TRUE;

And save indentation in the next lines :D

@@ +96,3 @@
+  /* Ensure VA surface pool is created */
+  /* XXX: optimize the surface format to use. e.g. YUY2 */
+  if (!window->surface_pool) {

ditto

::: gst-libs/gst/vaapi/gstvaapiwindow_priv.h
@@ +84,3 @@
+  GstVaapiFilter *filter;
+  gboolean has_vpp;
+  gboolean need_vpp;

looking at the following patches, I don't see why this variable is needed in the base class. It looks to me that it can be held as a local variable inside the functions or the child classes.

Review of attachment 349208 [details] [review]:

besides the need_vpp variable, this patch lgtm.

Review of attachment 349208 [details] [review]:

another nit: try to keep the subject length to 50 chars max ;)

Created attachment 349579 [details] [review]
libs: window: create an api   gst_vaapi_window_vpp_convert_internal

If a backend doesn't support specific format, we can use vpp for conversion
and make it playing.

This api is originated from GstVaapiWindowWayland and moved to GstVaapiWindow,
so that GstVaapiWindowX11 could use it.

Created attachment 349580 [details] [review]
libs: window: x11/wayland: use new api for conversion

Since gst_vaapi_window_vpp_convert_internal is created,
GstVaapiWindowX11/Wayland can use it for conversion.

Note that once it chooses to use vpp, it's going to use vpp 
until the session is finished.

(In reply to Víctor Manuel Jáquez Leal from comment #33)
> Review of attachment 349165 [details] [review] [review]:
> 
> lgtm.
> 
> I wonder if we can change these classes to GObject easily.

Yes, I guess we can just like GstVaapiDisplay :)

Created attachment 349587 [details] [review]
libs: window: x11/wayland: use new api for conversion

Since gst_vaapi_window_vpp_convert_internal is created,
GstVaapiWindowX11/Wayland can use it for conversion.

Note that once it chooses to use vpp, it's going to use vpp
until the session is finished.


------
Reupload this patch after removing use of static need_vpp, which is thread-unsafe.

Review of attachment 349587 [details] [review]:

::: gst-libs/gst/vaapi/gstvaapiwindow_wayland.c
@@ +435,3 @@
       return FALSE;
+  } else {
+    GST_ERROR ("directly goto conversion!");

I guess this message slipped from the development cycle.

Created attachment 349691 [details] [review]
libs: window: remove surface_format member

Since we always convert to NV12, there is no need to keep a
variable for that. Let us hard code it.

Created attachment 349692 [details] [review]
libs: window: don't add an unused function

The macro GST_VAAPI_OBJECT_DEFINE_CLASS_WITH_CODE only defines
a function that is never used, thus when compiling we might see
this warning (clang):

gstvaapiwindow.c:147:1: warning: unused function 'gst_vaapi_window_class' [-Wunused-function]
GST_VAAPI_OBJECT_DEFINE_CLASS_WITH_CODE (GstVaapiWindow,
^

Attachment 349165 [details] pushed as 4752f68 - libs: window: x11/wayland: chaining up to GstVaapiWindow
Attachment 349587 [details] pushed as c5b3577 - libs: window: x11/wayland: use new api for conversion
Attachment 349691 [details] pushed as 8e8280e - libs: window: remove surface_format member
Attachment 349692 [details] pushed as bd2e304 - libs: window: don't add an unused function