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 759533 - vaapisink: doesn't upload packed formats correctly
vaapisink: doesn't upload packed formats correctly
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gstreamer-vaapi
unspecified
Other Linux
: Normal normal
: git master
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks: 774241
 
 
Reported: 2015-12-16 09:50 UTC by Florent Thiéry
Modified: 2017-04-11 17:27 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
screenshot (2.69 KB, image/png)
2015-12-16 09:50 UTC, Florent Thiéry
  Details
[PATCH 1/2] pluginbase: Add vmethod verify_put_surface (2.57 KB, patch)
2016-11-16 08:14 UTC, Hyunjun Ko
none Details | Review
[PATCH 2/2] vaapisink: Implement verify_put_surface vmethod (2.52 KB, patch)
2016-11-16 08:15 UTC, Hyunjun Ko
none Details | Review
RFC: libs: window: x11: Add VPP to X11 backend (6.92 KB, patch)
2017-03-02 03:45 UTC, Hyunjun Ko
none Details | Review
script to test all the vaapisink formats (516 bytes, text/plain)
2017-03-21 12:47 UTC, Víctor Manuel Jáquez Leal
  Details
libs: window: create an api gst_vaapi_window_vpp_convert_internal (4.82 KB, patch)
2017-03-27 08:48 UTC, Hyunjun Ko
none Details | Review
libs: window: x11/wayland: use gst_vaapi_window_vpp_convert_internal for conversion (8.71 KB, patch)
2017-03-27 08:49 UTC, Hyunjun Ko
none Details | Review
libs: window: x11/wayland: chaining up to GstVaapiWindow (5.22 KB, patch)
2017-04-03 09:45 UTC, Hyunjun Ko
committed Details | Review
libs: window: create an api gst_vaapi_window_vpp_convert_internal (6.36 KB, patch)
2017-04-03 09:46 UTC, Hyunjun Ko
none Details | Review
libs: window: x11/wayland: use gst_vaapi_window_vpp_convert_internal for conversion (10.17 KB, patch)
2017-04-03 09:47 UTC, Hyunjun Ko
none Details | Review
libs: window: create an api gst_vaapi_window_vpp_convert_internal (6.14 KB, patch)
2017-04-04 02:09 UTC, Hyunjun Ko
none Details | Review
libs: window: x11/wayland: use gst_vaapi_window_vpp_convert_internal for conversion (10.11 KB, patch)
2017-04-04 02:09 UTC, Hyunjun Ko
none Details | Review
libs: window: create an api gst_vaapi_window_vpp_convert_internal (6.06 KB, patch)
2017-04-10 03:14 UTC, Hyunjun Ko
committed Details | Review
libs: window: x11/wayland: use new api for conversion (9.15 KB, patch)
2017-04-10 03:15 UTC, Hyunjun Ko
none Details | Review
libs: window: x11/wayland: use new api for conversion (10.67 KB, patch)
2017-04-10 08:26 UTC, Hyunjun Ko
committed Details | Review
libs: window: remove surface_format member (2.75 KB, patch)
2017-04-11 17:22 UTC, Víctor Manuel Jáquez Leal
committed Details | Review
libs: window: don't add an unused function (1.28 KB, patch)
2017-04-11 17:22 UTC, Víctor Manuel Jáquez Leal
committed Details | Review

Description Florent Thiéry 2015-12-16 09:50:39 UTC
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
Comment 1 Víctor Manuel Jáquez Leal 2016-01-14 15:43:09 UTC
(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.
Comment 2 Florent Thiéry 2016-01-15 18:02:37 UTC
Or maybe vaapisink should be a bin like glimagesink which contains glupload that does color conversion whenever necessary (or passthrough when not) ?
Comment 3 Víctor Manuel Jáquez Leal 2016-01-15 18:11:58 UTC
(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.
Comment 4 sreerenj 2016-03-24 16:54:13 UTC
Moving to Product:GStreamer, Component:gstreamer-vaapi
Comment 5 Víctor Manuel Jáquez Leal 2016-05-15 09:12:52 UTC
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.
Comment 6 Scott D Phillips 2016-07-19 17:31:56 UTC
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.
Comment 7 Víctor Manuel Jáquez Leal 2016-11-11 09:19:05 UTC
@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.
Comment 8 Hyunjun Ko 2016-11-16 08:14:34 UTC
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.
Comment 9 Hyunjun Ko 2016-11-16 08:15:07 UTC
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.
Comment 10 Hyunjun Ko 2016-11-16 08:17:34 UTC
I'm not sure if this is proper approach.
Untested on non-intel driver.
I guess, crash might occur depending on driver.
Comment 11 Scott D Phillips 2016-11-16 18:03:50 UTC
(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.
Comment 12 Scott D Phillips 2016-11-16 18:13:07 UTC
Review of attachment 339996 [details] [review]:

If a foreign_window is set then will these test surfaces be displayed to it?
Comment 13 Scott D Phillips 2016-11-16 18:13:15 UTC
Review of attachment 339996 [details] [review]:

If a foreign_window is set then will these test surfaces be displayed to it?
Comment 14 Hyunjun Ko 2016-11-17 01:46:13 UTC
(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 15 Víctor Manuel Jáquez Leal 2016-11-17 10:17:07 UTC
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.
Comment 16 Hyunjun Ko 2017-02-18 07:34:03 UTC
(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.
Comment 17 Hyunjun Ko 2017-03-02 03:45:32 UTC
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?
Comment 18 Víctor Manuel Jáquez Leal 2017-03-21 12:42:18 UTC
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
Comment 19 Víctor Manuel Jáquez Leal 2017-03-21 12:47:54 UTC
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
Comment 20 Hyunjun Ko 2017-03-22 08:11:45 UTC
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
Comment 21 Víctor Manuel Jáquez Leal 2017-03-22 17:55:26 UTC
(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.
Comment 22 Hyunjun Ko 2017-03-27 08:48:22 UTC
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.
Comment 23 Hyunjun Ko 2017-03-27 08:49:09 UTC
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.
Comment 24 Víctor Manuel Jáquez Leal 2017-03-28 14:43:50 UTC
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
Comment 25 Víctor Manuel Jáquez Leal 2017-03-28 15:20:18 UTC
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.
Comment 26 Víctor Manuel Jáquez Leal 2017-03-28 15:26:49 UTC
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.
Comment 27 Hyunjun Ko 2017-03-29 01:55:14 UTC
(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.
Comment 28 Hyunjun Ko 2017-04-03 09:45:56 UTC
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.
Comment 29 Hyunjun Ko 2017-04-03 09:46:38 UTC
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.
Comment 30 Hyunjun Ko 2017-04-03 09:47:15 UTC
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.
Comment 31 Hyunjun Ko 2017-04-04 02:09:16 UTC
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.
Comment 32 Hyunjun Ko 2017-04-04 02:09:52 UTC
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.
Comment 33 Víctor Manuel Jáquez Leal 2017-04-07 12:34:51 UTC
Review of attachment 349165 [details] [review]:

lgtm.

I wonder if we can change these classes to GObject easily.
Comment 34 Víctor Manuel Jáquez Leal 2017-04-07 12:42:32 UTC
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.
Comment 35 Víctor Manuel Jáquez Leal 2017-04-07 12:44:34 UTC
Review of attachment 349208 [details] [review]:

besides the need_vpp variable, this patch lgtm.
Comment 36 Víctor Manuel Jáquez Leal 2017-04-07 12:47:58 UTC
Review of attachment 349208 [details] [review]:

another nit: try to keep the subject length to 50 chars max ;)
Comment 37 Hyunjun Ko 2017-04-10 03:14:41 UTC
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.
Comment 38 Hyunjun Ko 2017-04-10 03:15:20 UTC
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.
Comment 39 Hyunjun Ko 2017-04-10 03:17:52 UTC
(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 :)
Comment 40 Hyunjun Ko 2017-04-10 08:26:26 UTC
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.
Comment 41 Víctor Manuel Jáquez Leal 2017-04-11 15:16:06 UTC
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.
Comment 42 Víctor Manuel Jáquez Leal 2017-04-11 17:22:44 UTC
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.
Comment 43 Víctor Manuel Jáquez Leal 2017-04-11 17:22:50 UTC
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,
^
Comment 44 Víctor Manuel Jáquez Leal 2017-04-11 17:27:20 UTC
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