GNOME Bugzilla – Bug 705821
vaapi/wayland: add support for external wl_surface/wl_display
Last modified: 2018-11-03 15:43:40 UTC
in typical video usage, rendering surface is created by media app, not vaapisink. add such feature for wayland platform. when rendering to external surface, vaapsink should care wl_compositor/wl_display interface only.
Created attachment 251328 [details] [review] 0001-add-foreign-wl_display-wl_surface-support
Created attachment 251329 [details] [review] 0002-seems-wl_buffer_destroy-is-not-required
Created attachment 251330 [details] [review] 0003-fixes-when-vappsink-renders-to-simple-wayland-compos
(In reply to comment #0) > in typical video usage, rendering surface is created by media app, not > vaapisink. > add such feature for wayland platform. > > when rendering to external surface, vaapsink should care > wl_compositor/wl_display interface only. How is this performed in vaapisink? The only supported ways to render to user-provided storage or windows are (i) in vaapisink through the GstXOverlay interface, and I believe we could also use it for Wayland purposes, and (ii) after vaapidecode by having the user handle the GstSurfaceMeta interfaces.
Review of attachment 251328 [details] [review]: Please also provide a tests/ patch for test-display for example to exercise this "external" display / window mechanism. ::: gst-libs/gst/vaapi/gstvaapidisplay_wayland.c @@ +434,1 @@ This change is not needed. If this fixes something for you, then this means the problem lies in somewhere else. ::: gst-libs/gst/vaapi/gstvaapiwindow_wayland.c @@ +446,3 @@ + struct wl_display *wl_dpy = gst_vaapi_display_wayland_get_display((GstVaapiDisplayWayland*)display); + g_return_val_if_fail(display, NULL); + g_return_val_if_fail(id > 0, NULL); g_return_val_if_fail() are to be considered like asserts, i.e. they can be disabled. So, this means that if you want id != 0 to be a hard constraint to be always evaluated, then please make if if (!id) return NULL. g_return_val_if_fail(GST_VAAPI_IS_DISPLAY_WAYLAND(display), NULL) can lives in since this is a clear programming error otherwise, in the upper "controlled" layers like plug-in elements. ::: gst-libs/gst/vaapi/gstvaapiwindow_wayland.h @@ +34,3 @@ gst_vaapi_window_wayland_new(GstVaapiDisplay *display, guint width, guint height); +GstVaapiWindow * +gst_vaapi_window_wayland_new_with_id(GstVaapiDisplay *display, guint id); I think that, in Wayland context, the external id could actually represent a pointer (wl_surface object or whatever). So, this needs to be guintptr.
Review of attachment 251329 [details] [review]: Why wouldn't wl_buffer_destroy() be needed? Aren't those internal reference counted objects, with the "unreference" from client perspective be only the wl_buffer_destroy().
Review of attachment 251330 [details] [review]: Can't you just implement the GstVaapiPixmap interfaces?
Created attachment 255005 [details] [review] add foreign wl_display/wl_surface support in vaapisink
Created attachment 255006 [details] [review] 0002-add-frame-rendered-signal-for-wayland-platform
Created attachment 255007 [details] [review] 0003-wayland-retain-additional-3-gst-buffer-after-video-r
Created attachment 255008 [details] [review] 0004-destroy-wl_buffer-by-register-to-wl_buffer_listener
Created attachment 255892 [details] [review] test: add gst_vaapi_display_wayland_new_with_display in test-display
(In reply to comment #4) > How is this performed in vaapisink? The only supported ways to render to > user-provided storage or windows are (i) in vaapisink through the GstXOverlay > interface, and I believe we could also use it for Wayland purposes, and (ii) > after vaapidecode by having the user handle the GstSurfaceMeta interfaces. yes, GstXOverlay interface is used. since wl_buffer data is in client side, there is no difference for on-screen or off-screen rendering in gst-vaapi.
(In reply to comment #5) > Review of attachment 251328 [details] [review]: > > Please also provide a tests/ patch for test-display for example to exercise > this "external" display / window mechanism. added in test-display > ::: gst-libs/gst/vaapi/gstvaapidisplay_wayland.c > @@ +434,1 @@ > > This change is not needed. If this fixes something for you, then this means the > problem lies in somewhere else. removed > ::: gst-libs/gst/vaapi/gstvaapiwindow_wayland.c > @@ +446,3 @@ > + struct wl_display *wl_dpy = > gst_vaapi_display_wayland_get_display((GstVaapiDisplayWayland*)display); > + g_return_val_if_fail(display, NULL); > + g_return_val_if_fail(id > 0, NULL); > > g_return_val_if_fail() are to be considered like asserts, i.e. they can be > disabled. > > So, this means that if you want id != 0 to be a hard constraint to be always > evaluated, then please make if if (!id) return NULL. > g_return_val_if_fail(GST_VAAPI_IS_DISPLAY_WAYLAND(display), NULL) can lives in > since this is a clear programming error otherwise, in the upper "controlled" > layers like plug-in elements. updated. > ::: gst-libs/gst/vaapi/gstvaapiwindow_wayland.h > @@ +34,3 @@ > gst_vaapi_window_wayland_new(GstVaapiDisplay *display, guint width, guint > height); > +GstVaapiWindow * > +gst_vaapi_window_wayland_new_with_id(GstVaapiDisplay *display, guint id); > > I think that, in Wayland context, the external id could actually represent a > pointer (wl_surface object or whatever). So, this needs to be guintptr. updated with gpointer. (guintptr leads to some warning).
(In reply to comment #6) > Review of attachment 251329 [details] [review]: > > Why wouldn't wl_buffer_destroy() be needed? Aren't those internal reference > counted objects, with the "unreference" from client perspective be only the > wl_buffer_destroy(). it is not correct to wl_buffer_destroy upon frame redraw callback. I add new patch to destory wl_buffer by wl_buffer_listener.
(In reply to comment #7) > Review of attachment 251330 [details] [review]: > > Can't you just implement the GstVaapiPixmap interfaces? since wl_buffer data is in client side, there is few/no difference of off-screen and on-screen rendering in gst-vaapi. so seems GstVaapiPixmap is not necessary. this patch still addresses the difference between external and self-screated wl_surface.
Some comments: -- ASFAIK, the way to set native display pointer to gstreamer pipeline(with h/w acceleration) is via context message from the application(eg: clutter-gst). And there is no need for a new display property. What do you think? -- wl_display_dispatch_queue_pending() is available from wayland version 1.0.2 on wards. So we need some version checking of wayland to maintain the backward compatibility. -- If we really need the "frame-rendered" signal as provided in patch2, then i prefer to add a signal instead of sending custom “frame-rendered” message, and which should happen irrespective of the native display server.
Created attachment 259635 [details] [review] 0001-add-foreign-wl_display-wl_surface-support
Created attachment 259636 [details] [review] 0002-add-frame-rendered-signal
Created attachment 259637 [details] [review] 0003-wayland-retain-additional-3-gst-buffer-after-video-r
Created attachment 259638 [details] [review] 0004-destroy-wl_buffer-by-register-to-wl_buffer_listener
Created attachment 259639 [details] [review] 0005-add-WAYLAND_CHECK_VERSION-for-event_queue-use
thanks sree. i have updated the patches according to your feedback.
Review of attachment 259636 [details] [review]: The signal could simply be "show-frame" that takes a clean GstBuffer as argument. i.e. GstFlowReturn (*show_frame) (GstBaseSink * base_sink, GstBuffer * buf); By clean GstBuffer argument, I mean the one with VA backed storage, i.e. the one obtained with gst_vaapi_plugin_base_get_input_buffer(). Next, the default object handler is mostly what we have already for gst_vaapisink_show_frame(). If the user handler returns GST_FLOW_NOT_LINKED, then the accumulator will stop execution, return FALSE and make the final signal handler return value be GST_FLOW_OK. That way, we can handle cases like - pre show-frame hook - the actual vaapisink show-frame - post show-frame hook
Review of attachment 259637 [details] [review]: If the VA render mode is incorrectly set from the driver to match overlay or textured mode, then this certainly has to be fixed on the driver side. Next, if the gst_vaapi_window_wayland_sync() mechanism doesn't work properly, this probably ought to be fixed there. We already keep one buffer in overlay, and this should be enough.
Review of attachment 259638 [details] [review]: ::: gst-libs/gst/vaapi/gstvaapiwindow_wayland.c @@ +537,3 @@ + // since there may still be future WL_BUFFER_RELEASE event when we try to destroy event_queue in gst_vaapi_window_wayland_destroy + // it potential can be resolved if wl_surface has WL_SURFACE_RELEACE event + // wl_proxy_set_queue((struct wl_proxy *)buffer, priv->event_queue); What about waiting for the completion of the frame redraw prior to destructing the event queue then?
seems you extends it to a new usage. the original purpose is to emit a signal after vaapisink shows a frame, something like a damage event. vaapisink still owns video rendering. (In reply to comment #25) > Review of attachment 259636 [details] [review]: > > The signal could simply be "show-frame" that takes a clean GstBuffer as > argument. i.e. GstFlowReturn (*show_frame) (GstBaseSink * base_sink, GstBuffer > * buf); > By clean GstBuffer argument, I mean the one with VA backed storage, i.e. the > one obtained with gst_vaapi_plugin_base_get_input_buffer(). > Next, the default object handler is mostly what we have already for > gst_vaapisink_show_frame(). > > If the user handler returns GST_FLOW_NOT_LINKED, then the accumulator will stop > execution, return FALSE and make the final signal handler return value be > GST_FLOW_OK. > > That way, we can handle cases like > - pre show-frame hook > - the actual vaapisink show-frame > - post show-frame hook
(In reply to comment #28) > the original purpose is to emit a signal after vaapisink shows a frame, > something like a damage event. vaapisink still owns video rendering. This doesn't change the purpose. vaapisink still renders the frame, but you now have the opportunity to notify upper layer when we are about to draw and after we are done.
(In reply to comment #26) this patch has few to do with overlay, but retains more video frame after it is committed to wayland server. in theory, one buffer seems not sufficient for overlay: the kept one buffer can be treated as back buffer of overlay, we haven't ref the one for front buffer of overlay yet. > Review of attachment 259637 [details] [review]: > > If the VA render mode is incorrectly set from the driver to match overlay or > textured mode, then this certainly has to be fixed on the driver side. Next, if > the gst_vaapi_window_wayland_sync() mechanism doesn't work properly, this > probably ought to be fixed there. We already keep one buffer in overlay, and > this should be enough.
(In reply to comment #27) > Review of attachment 259638 [details] [review]: I remembered I tried that before. it requires WL_BUFFER_RELEASE to be sent after a new one is used for compositing (not after a new one is committed). that breaks 3D usage in webkit, so I don't try that more. > > ::: gst-libs/gst/vaapi/gstvaapiwindow_wayland.c > @@ +537,3 @@ > + // since there may still be future WL_BUFFER_RELEASE event when we try to > destroy event_queue in gst_vaapi_window_wayland_destroy > + // it potential can be resolved if wl_surface has WL_SURFACE_RELEACE event > + // wl_proxy_set_queue((struct wl_proxy *)buffer, priv->event_queue); > > What about waiting for the completion of the frame redraw prior to destructing > the event queue then?
(In reply to comment #28) sorry, I don't catch your meaning. is it something like the following: static GstFlowReturn gst_vaapisink_show_frame_new(GstBaseSink *base_sink, GstBuffer *src_buffer) { // xx some work before emit 'show-frame' signal // xx emit 'show-frame' signal if (ret == GST_FLOW_NOT_LINKED) { } else { gst_vaapisink_show_frame(); } // xx some rest work } > This doesn't change the purpose. vaapisink still renders the frame, but you now > have the opportunity to notify upper layer when we are about to draw and after > we are done.
(In reply to comment #32) > (In reply to comment #28) > sorry, I don't catch your meaning. > is it something like the following: > > static GstFlowReturn > gst_vaapisink_show_frame_new(GstBaseSink *base_sink, GstBuffer *src_buffer) > { > // xx some work before emit 'show-frame' signal > > // xx emit 'show-frame' signal > > if (ret == GST_FLOW_NOT_LINKED) { > > } > else { > gst_vaapisink_show_frame(); > } > > // xx some rest work > > } In gst_vaapisink_show_frame(), code from: meta = gst_buffer_get_vaapi_video_meta(buffer); to: if (!success) goto error; is moved to static GstFlowReturn default_show_frame(GstBaseSink *base_sink, GstBuffer *buffer) { } this means that, gst_vaapisink_show_frame() can be reduced to: { // ... ret = gst_vaapi_plugin_base_get_input_buffer(GST_VAAPI_PLUGIN_BASE(sink), src_buffer, &buffer); if (ret != GST_FLOW_OK && ret != GST_FLOW_NOT_SUPPORTED) return ret; ret = g_signal_emit(...); /* Retain VA surface until the next one is displayed */ if (sink->use_overlay) gst_buffer_replace(&sink->video_buffer, buffer); gst_buffer_unref(buffer); return ret; } The GSignalAccumulator function for "show-frame" will check the handler_return. If it is GST_FLOW_NOT_LINKED, this means we want to prematurely abort the signal handlers emission. Thus, we return FALSE to signal that, and set the return_accu to GST_FLOW_OK (since this is not an error per se). Thanks.
(In reply to comment #27) > Review of attachment 259638 [details] [review]: > > ::: gst-libs/gst/vaapi/gstvaapiwindow_wayland.c > @@ +537,3 @@ > + // since there may still be future WL_BUFFER_RELEASE event when we try to > destroy event_queue in gst_vaapi_window_wayland_destroy > + // it potential can be resolved if wl_surface has WL_SURFACE_RELEACE event > + // wl_proxy_set_queue((struct wl_proxy *)buffer, priv->event_queue); > > What about waiting for the completion of the frame redraw prior to destructing > the event queue then? there are uncertain number of WL_BUFFER_RELEASE event in the future; if we try to wait until the last signal of WL_BUFFER_RELEASE, a list is required to track the committed-but-not-released wl_buffer. I don't think it deserve to do so. (there is <=1 frame callback to wait; however, the number of unreleased wl_buffer is uncertain)
Please open separate reports to address different bugs: - Patch0004: to be moved to "wayland: improve buffer release tracking" - Patch0002: to be moved to "vaapisink: add {pre,post}-rendering notifications"
(In reply to comment #34) > (In reply to comment #27) > > Review of attachment 259638 [details] [review] [details]: > > > > ::: gst-libs/gst/vaapi/gstvaapiwindow_wayland.c > > @@ +537,3 @@ > > + // since there may still be future WL_BUFFER_RELEASE event when we try to > > destroy event_queue in gst_vaapi_window_wayland_destroy > > + // it potential can be resolved if wl_surface has WL_SURFACE_RELEACE event > > + // wl_proxy_set_queue((struct wl_proxy *)buffer, priv->event_queue); > > > > What about waiting for the completion of the frame redraw prior to destructing > > the event queue then? > > there are uncertain number of WL_BUFFER_RELEASE event in the future; if we try > to wait until the last signal of WL_BUFFER_RELEASE, a list is required to track > the committed-but-not-released wl_buffer. I don't think it deserve to do so. > > (there is <=1 frame callback to wait; however, the number of unreleased > wl_buffer is uncertain) So, Weston really keeps an extra reference to the buffer and does not release it immediately even if the wl_surface buffer is replaced with a new one? IMHO, the number of unreleased wl_buffers should be less than 2. There is a way to fix that, and that would avoid extra work in sink elements along the way, but I would like to understand how we are getting stale wl_buffers if we routinely replace wl_surface buffers?
(In reply to comment #36) when wayalnd server (weston or webkit ui process) renders slower than video, there could be a stale wl_buffer. 1. vaapisink commit wl_buffer N 2. vaapisink commit wl_buffer N+1, N+2, before weston renders N. 3. N+1 wl_buffer is released upon N+2; 4. weston pick up N+2 and release N N+1 is release before N. > So, Weston really keeps an extra reference to the buffer and does not release > it immediately even if the wl_surface buffer is replaced with a new one? IMHO, > the number of unreleased wl_buffers should be less than 2. > > There is a way to fix that, and that would avoid extra work in sink elements > along the way, but I would like to understand how we are getting stale > wl_buffers if we routinely replace wl_surface buffers?
Created attachment 301558 [details] [review] test: add gst_vaapi_display_wayland_new_with_display in test-display Change-Id: I75f25a4068b49694297175961e3e8e9932e08280
Created attachment 301559 [details] [review] wayland: add foreign wl_display/wl_surface support wl_display can be set from application. It is not fatal when wl_display doesn't support shell/output interface, since shell_surface and opaque_region are not necessary for external wayland surfaces. Added these new methods gst_vaapi_window_wayland_new_with_surface(), gst_vaapi_window_wayland_get_surface() and gst_vaapi_window_wayland_is_foreign_surface() Co-author: changzhix.wei@intel.com vjaquez@igalia.com Change-Id: I90e3f48870e90ffb4720cf458558d2e0ca95066c Signed-off-by: Víctor Manuel Jáquez Leal <vjaquez@igalia.com>
Created attachment 301560 [details] [review] wayland: add guards for event_queue use wl_display_dispatch_queue_pending() is available from wayland version 1.0.2 on wards. So we need some version checking of wayland to maintain the backward compatibility. Change-Id: Iec63bb26b78f79b39f3a9f71f078a105c15f0b0b Signed-off-by: Víctor Manuel Jáquez Leal <vjaquez@igalia.com>
Created attachment 301561 [details] [review] sink: wayland: retain additional buffers after video rendering After wl_buffer is commited to Wayland server, it is kept using until next buffer is commited. We would retain 3 buffers after video rendering in media side: - one buffer for the one is used as texture in wayland compositing (front buffer) - one buffer for the one just committed (back buffer) - one buffer additional when wayland compositing is slower than media side Change-Id: I66ece1430a25a4864b6bd432e804a42af7fb7bcd Signed-off-by: Víctor Manuel Jáquez Leal <vjaquez@igalia.com>
I have rebased and (mostly) re-written these patches. They seem to work OK. I'll move the other two patches to their respective bug reports, as Gwenole said in comment #35.
Comment on attachment 259636 [details] [review] 0002-add-frame-rendered-signal moved to https://bugzilla.gnome.org/show_bug.cgi?id=747905
Comment on attachment 259638 [details] [review] 0004-destroy-wl_buffer-by-register-to-wl_buffer_listener This patch has been superseded by https://bugzilla.gnome.org/show_bug.cgi?id=747492
(In reply to Víctor Manuel Jáquez Leal from comment #40) > Created attachment 301560 [details] [review] [review] > wayland: add guards for event_queue use > > wl_display_dispatch_queue_pending() is available from wayland > version 1.0.2 on wards. So we need some version checking of wayland > to maintain the backward compatibility. > > Change-Id: Iec63bb26b78f79b39f3a9f71f078a105c15f0b0b > Signed-off-by: Víctor Manuel Jáquez Leal <vjaquez@igalia.com> I've digged in the wayland repository and wl_display_dispatch_queue_pending is the only symbol added after release 1.0, so there's no need to guard all the queue handling.
Review of attachment 301560 [details] [review]: Can't you just require wayland 1.2 ?
Review of attachment 301559 [details] [review]: I think you also need to export the GstVaapiDisplay GType so display can be set using GstContext. I'd like to have a test of a pipeline with two separate vaapidecode->vaapisink, this requires removing the cache (bug #747946) to be useful. The whole thing seems to just segfault in libwayland (which is very brittle) right now if you mix up the buffer from two separate display connections.
(In reply to Olivier Crête from comment #46) > Review of attachment 301560 [details] [review] [review]: > > Can't you just require wayland 1.2 ? Sree doesn't like the idea. But it makes sense for me, since we already dropped the support previous to gstreamer-1.2 (September 2013), and Wayland 1.2 comes from July 2013.
Today I found that sharing the wl_surface is not enough. We need to share the wl_display. waylandsink does it through the GstContext mechanism. I wrote an example for waylandsink as GTK overlay: https://bugzilla.gnome.org/show_bug.cgi?id=748322 It can be reused for vaapisink. But first we need find a way to share the wl_display.
Comment on attachment 301559 [details] [review] wayland: add foreign wl_display/wl_surface support We need a mean to share the wl_display.
Created attachment 302220 [details] [review] wayland: add foreign wl_display/wl_surface support wl_display can be set from application. It is not fatal when wl_display doesn't support shell/output interface, since shell_surface and opaque_region are not necessary for external wayland surfaces. Added these new methods gst_vaapi_window_wayland_new_with_surface(), gst_vaapi_window_wayland_get_surface() and gst_vaapi_window_wayland_is_foreign_surface() Co-author: changzhix.wei@intel.com vjaquez@igalia.com Change-Id: I90e3f48870e90ffb4720cf458558d2e0ca95066c Signed-off-by: Víctor Manuel Jáquez Leal <vjaquez@igalia.com>
Comment on attachment 302220 [details] [review] wayland: add foreign wl_display/wl_surface support I just uploaded a simple fix to avoid a precondition check failure. Still the patch needs work for the display sharing.
(In reply to Víctor Manuel Jáquez Leal from comment #48) > (In reply to Olivier Crête from comment #46) > > Review of attachment 301560 [details] [review] [review] [review]: > > > > Can't you just require wayland 1.2 ? > > Sree doesn't like the idea. But it makes sense for me, since we already > dropped the support previous to gstreamer-1.2 (September 2013), and Wayland > 1.2 comes from July 2013. Wayland >= 1.2 should be fine. Or anything else that provides wl_scaler actually. Other customers can hardly do anything viable without a recent enough libwayland/weston combination anyway. Besides, I think there were some critical memory leaks in wayland/weston before then, IIRC -- very vaguely though.
Comment on attachment 301560 [details] [review] wayland: add guards for event_queue use As the required wayland version has been bumped, this patch is not needed.
(In reply to Víctor Manuel Jáquez Leal from comment #52) > Comment on attachment 302220 [details] [review] [review] > wayland: add foreign wl_display/wl_surface support > > I just uploaded a simple fix to avoid a precondition check failure. Still > the patch needs work for the display sharing. I have lurked around to figure out how to process the GstContext with the Wayland Display, but is not trivial, since the whole context processing code needs to be modified. Though, this might be done anyway, since the GstGLContext is not, afaiu, handled correctly (bug #745233)
Is there anything left to be done here?
Running GNOME 3.22 + Wayland (Arch), with gstreamer 1.10.2 and libva 1.7.3, I get this for MPEG-2/AC3 MKV file (extracted from DVD): $ gst-play-1.0 test.mkv Press 'k' to see a list of keyboard shortcuts. Now playing /home/marcos/Downloads/test.mkv libva info: VA-API version 0.39.4 libva info: va_getDriverName() returns 0 libva info: Trying to open /usr/lib/dri/i965_drv_video.so libva info: Found init function __vaDriverInit_0_39 libva info: va_openDriver() returns 0 libva info: VA-API version 0.39.4 libva info: va_getDriverName() returns 0 libva info: Trying to open /usr/lib/dri/i965_drv_video.so libva info: Found init function __vaDriverInit_0_39 libva info: va_openDriver() returns 0 Redistribute latency... Redistribute latency... wl_surface@7: error 2: Failed to create a texture for surface 7 ERROR Internal error: could not render surface for file:///home/marcos/Downloads/test.mkv ERROR debug information: gstvaapisink.c(1457): gst_vaapisink_show_frame_unlocked (): /GstPlayBin:playbin/GstPlaySink:playsink/GstBin:vbin/GstVaapiSink:vaapisink0 Reached end of play list. (Totem fails too) I have an old Intel GM45 chip. Under Xorg session it works. Is it this bug?
(In reply to Marcos Mello from comment #57) > Running GNOME 3.22 + Wayland (Arch), with gstreamer 1.10.2 and libva 1.7.3, > I get this for MPEG-2/AC3 MKV file (extracted from DVD): > > $ gst-play-1.0 test.mkv > Press 'k' to see a list of keyboard shortcuts. > Now playing /home/marcos/Downloads/test.mkv > libva info: VA-API version 0.39.4 > libva info: va_getDriverName() returns 0 > libva info: Trying to open /usr/lib/dri/i965_drv_video.so > libva info: Found init function __vaDriverInit_0_39 > libva info: va_openDriver() returns 0 > libva info: VA-API version 0.39.4 > libva info: va_getDriverName() returns 0 > libva info: Trying to open /usr/lib/dri/i965_drv_video.so > libva info: Found init function __vaDriverInit_0_39 > libva info: va_openDriver() returns 0 > Redistribute latency... > Redistribute latency... > wl_surface@7: error 2: Failed to create a texture for surface 7 > ERROR Internal error: could not render surface for > file:///home/marcos/Downloads/test.mkv > ERROR debug information: gstvaapisink.c(1457): > gst_vaapisink_show_frame_unlocked (): > /GstPlayBin:playbin/GstPlaySink:playsink/GstBin:vbin/GstVaapiSink:vaapisink0 > Reached end of play list. > > (Totem fails too) > > I have an old Intel GM45 chip. Under Xorg session it works. > > Is it this bug? No. Please open another bug. But looking at the error, it looks like a problem in wayland.
Created attachment 352067 [details] [review] libs: window: wayland: scale when size of src and dst is different When the size of src_rect and dst_rect is different, we need to use vpp. Otherwise, the output is cropped according to the size of src rect.
Created attachment 352068 [details] [review] libs: display/window: wayland: add foreign wl_display/wl_surface support wl_display can be set from application. It is not fatal when wl_display doesn't support shell/output interface, since shell_surface and opaque_region are not necessary for external wayland surfaces. Also, use wl_subcompositor and wl_subsurface. See Appendix A. Wayland Protocol Specification as the following. "The aim of sub-surfaces is to offload some of the compositing work within a window from clients to the compositor. A prime example is a video player with decorations and video in separate wl_surface objects. This should allow the compositor to pass YUV video buffer processing to dedicated overlay hardware when possible." Added these new methods gst_vaapi_window_wayland_new_with_surface(), gst_vaapi_window_wayland_get_surface() and gst_vaapi_window_wayland_is_foreign_surface() Original-Patch-By: Víctor Manuel Jáquez Leal <victorx.jaquez@intel.com>
Created attachment 352069 [details] [review] vaapisink: implements gst_vaapisink_wayland_create_window_from_handle Implements gst_vaapisink_wayland_create_window_from_handle to support using external wl_surface.
Created attachment 352070 [details] [review] libs: window: implements gst_vaapi_window_set_render_rectangle Implements new vmethod gst_vaapi_window_set_render_rectangle, which is doing set the information of the rendered rectangle set by user. This is necessary on wayland at least to get exact information of external surface. And vaapisink calls this when gst_video_overlay_set_render_rectangle is called.
Created attachment 352071 [details] [review] libs: window: make safe when resize of window When resizing the window, gst_vaapi_window_set_size is likely to be called in different thread. So wee need to protect it.
Note that these patches are not going to work on the master. GstContext work(#bug 766704) should be done first.
*** Bug 795220 has been marked as a duplicate of this bug. ***
Created attachment 371096 [details] [review] libs: display,window: wayland: foreign wl_display/wl_surface support wl_display can be set from application. It is not fatal when wl_display doesn't support shell/output interface, since shell_surface and opaque_region are not necessary for external wayland surfaces. Also, use wl_subcompositor and wl_subsurface. See Appendix A. Wayland Protocol Specification as the following. """ The aim of sub-surfaces is to offload some of the compositing work within a window from clients to the compositor. A prime example is a video player with decorations and video in separate wl_surface objects. This should allow the compositor to pass YUV video buffer processing to dedicated overlay hardware when possible. """ Added new method gst_vaapi_window_wayland_new_with_surface() Original-Patch-By: Víctor Manuel Jáquez Leal <victorx.jaquez@intel.com>
Created attachment 371097 [details] [review] vaapisink: implements gst_vaapisink_wayland_create_window_from_handle() Implements gst_vaapisink_wayland_create_window_from_handle() to support using external wl_surface.
Created attachment 371098 [details] [review] libs: window: implements gst_vaapi_window_set_render_rectangle Implements new vmethod gst_vaapi_window_set_render_rectangle, which is doing set the information of the rendered rectangle set by user. This is necessary on wayland at least to get exact information of external surface. And vaapisink calls this when gst_video_overlay_set_render_rectangle is called.
Created attachment 371099 [details] [review] libs: window: make safe when resize of window When resizing the window, gst_vaapi_window_set_size is likely to be called in different thread. So wee need to protect it.
Created attachment 371100 [details] [review] libs: display: wayland: add gst_vaapi_display_wayland_new_with_va_display() Implements new API function so that users could create GstVaapiDisplay with their own VADisplay within a native display as backend.
Created attachment 371101 [details] [review] videocontext: support wl-display in "gst.vaapi.app.Display" Through "gst.vaapi.app.Display" context, users can set their own VADisplay and native display of their backend. So far we support only X11 display, from now we also support Wayland display. Attributes: - wl-display : pointer of struct wl_display .
Attachment 371100 [details] pushed as 1825d93 - libs: display: wayland: add gst_vaapi_display_wayland_new_with_va_display() Attachment 371101 [details] pushed as bfac678 - videocontext: support wl-display in "gst.vaapi.app.Display"
I'm giving a try to these patches. I make it work but I'm facing an issue when I change the src of the vaapisink element. When I switch a from one src to another a frame of the previous src remain rendered on top of the new frame. Do you ever see a similar issue in the past? This issue is not happening on the X11 counterpart.
(In reply to Matteo Valdina from comment #73) > I'm giving a try to these patches. > > I make it work but I'm facing an issue when I change the src of the > vaapisink element. > > When I switch a from one src to another a frame of the previous src remain > rendered on top of the new frame. > > Do you ever see a similar issue in the past? > > This issue is not happening on the X11 counterpart.
Hi, I would like to know if there is a need to implement the wp_viewport for the rescale in vaapisink or it will use the vpp?
Hi, About my initial issue, I figured out. It was an issue on my side. The only persistent issue is when I set a different size from the src. I set my desired dimension via gst_video_overlay_set_render_rectangle. This show for a fraction of a second the desired input but after that the surface become black. There is a TODO in the "gst_vaapi_window_wayland_set_render_rect". void gst_vaapi_window_wayland_set_render_rect (GstVaapiWindow * window, gint x, gint y, gint width, gint height) { GstVaapiWindowWaylandPrivate *const priv = GST_VAAPI_WINDOW_WAYLAND_GET_PRIVATE (window); if (priv->video_subsurface) wl_subsurface_set_position (priv->video_subsurface, x, y); /* TODO: something to scale maybe */ return; } My first thought is to add the wl_viewport to do the rescale. But I'm wondering if it should be implemented with a vaapostproc instead.
(In reply to Matteo Valdina from comment #75) > Hi, > I would like to know if there is a need to implement the wp_viewport for the > rescale in vaapisink or it will use the vpp? vaapisink uses VPP when it requires to rescale, since it is independent of the rendering server (X/drm/Wayland)
Thanks, I imagined that. I'm digging a little deeper in these patches. The first issue that I found was when there is a caps renegotiation the whole frame is rendered black. This was caused in the gstvaapisink.c function gst_vaapisink_set_caps. The current code (after the patch) was ... gst_vaapisink_ensure_window_size (sink, &win_width, &win_height); if (sink->window) { if (!sink->foreign_window || sink->fullscreen) gst_vaapi_window_set_size (sink->window, win_width, win_height); } else { ... The problem that when the caps are renegotiated (like change the src) it will skip assigning the window_vaapi_size and will assign the size of 0x0 to the sink->window_width and sink->window_height. ... gst_vaapisink_ensure_window_size (sink, &win_width, &win_height); if (sink->window) { if (!sink->foreign_window || sink->fullscreen) { gst_vaapi_window_set_size (sink->window, win_width, win_height); } else if (sink->foreign_window) { gst_vaapi_window_get_size (sink->window, &win_width, &win_height); } } else { ... this make it work but I don't know if it is good or not. The second problem is related to the VPP, after the first run it operate correctly but after that, it is not rescaled or it fails to re-create the surface for an invalid VADisplay.
If you post the patches, I'll gladly review them :)
Created attachment 373932 [details] [review] Minor cleanup.
Created attachment 373933 [details] [review] bs wayland window: Fixed frame_done sync. Restored syncronization of frame_done with the rendering. Without this sync it will override the rendering frame (last_frame) and put the rendering in a bad state.
Created attachment 373934 [details] [review] gstvaapisink: Set sink window_width & height. Set the window_width and window_height when there is a foreign_window. Previously in the wayland scenario, this size is zero so the dst/sink window will rescale the frame to 0x0 and so render nothing.
I posted my patches. About the VPP issue, I found out that If I re-create the VADisplay each time that vaapisink need a context it will work. Please note that in my case I use a VPP element in the pipeline.
Review of attachment 373932 [details] [review]: this is already fixed in commit * 91320901 libs: replace g_warning with GST_WARNING
Review of attachment 373933 [details] [review]: So this patch is basically a code review for attachment 371096 [details] [review].
Review of attachment 373934 [details] [review]: ::: gst/vaapi/gstvaapisink.c @@ -454,2 @@ if (!sink->foreign_window) - x11_event_mask |= ButtonPressMask | ButtonReleaseMask; This code style fix should be another commit since it is not related with this issue.
I have just realized that I have a local branch with patches (and a test) for this issue. I've just pushed it to my repository in github https://github.com/ceyusa/gstreamer-vaapi/tree/705821
-- GitLab Migration Automatic Message -- This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gstreamer-vaapi/issues/1.