GNOME Bugzilla – Bug 747492
vaapisink: wayland improvements
Last modified: 2015-05-18 13:37:26 UTC
The vaapisink has two problems when interacting with a wayland compositor: 1. the frame is release on the frame done callback. This is incorrect. The compositor may still use it until the buffer release callback is called. 2. wl_display_dispatch_queue() can block forever e.g. when the compositor exits. This can prevent a normal pipeline shutdown. Here are two patches to fix these issues. I'm submitting both here because they tuch the same code and would conflict otherwise.
Created attachment 301105 [details] [review] wayland: release the frame in the buffer release callback The wayland compositor may still use the buffer when the frame done callback is called.
Created attachment 301106 [details] [review] vaapisink: implement unlock/unlock_stop for wayland Otherwise wl_display_dispatch_queue() might prevent the pipeline from shutting down. This can happen e.g. if the wayland compositor exits while the pipeline is running.
I wrote something similar last week. Another problem is that wl_surface_frame never happens if the surface is not on a visible output, at least in Weston. My version didn't have the poll, I just tried to read from the socket, and if there was nothing, I just dropped the incoming frame.
(In reply to Michael Olbrich from comment #2) > Created attachment 301106 [details] [review] [review] > vaapisink: implement unlock/unlock_stop for wayland > > Otherwise wl_display_dispatch_queue() might prevent the pipeline from > shutting down. This can happen e.g. if the wayland compositor exits while > the pipeline is running. this patch shows this errors: gstvaapiwindow.c:524:3: error: non-void function 'gst_vaapi_window_unlock' should return a value [-Wreturn-type] g_return_if_fail (window != NULL); ^ /opt/gnome/jh/include/glib-2.0/glib/gmessages.h:373:3: note: expanded from macro 'g_return_if_fail' return; \ ^ gstvaapiwindow.c:545:3: error: non-void function 'gst_vaapi_window_unlock_stop' should return a value [-Wreturn-type] g_return_if_fail (window != NULL); ^ /opt/gnome/jh/include/glib-2.0/glib/gmessages.h:373:3: note: expanded from macro 'g_return_if_fail' return; \ ^
Review of attachment 301106 [details] [review]: ::: gst-libs/gst/vaapi/gstvaapiwindow.c @@ -512,0 +512,13 @@ + +/** + * gst_vaapi_window_unlock: ... 10 more ... you should use g_return_val_if_fail() @@ -512,0 +512,34 @@ + +/** + * gst_vaapi_window_unlock: ... 31 more ... ditto ::: gst/vaapi/gstvaapisink.c @@ +1536,3 @@ +{ + GstVaapiSink *const sink = GST_VAAPISINK_CAST (base_sink); + return !sink->window || gst_vaapi_window_unlock (sink->window); Though I love this expression, it is not common in gstreamer or gstreamer-vaapi. I prefer if (sink->window) return gst_vaapi_window_unlock (sink->window); return FALSE; The compiler should do the optimization ;) @@ +1543,3 @@ +{ + GstVaapiSink *const sink = GST_VAAPISINK_CAST (base_sink); + return !sink->window || gst_vaapi_window_unlock_stop (sink->window); ditto
Review of attachment 301105 [details] [review]: ::: gst-libs/gst/vaapi/gstvaapiwindow_wayland.c @@ -332,2 +332,2 @@ if (priv->frame == frame) - priv->frame = NULL; + priv->frame_busy = FALSE; Though it is not common in gstreamer-vaapi, I think, for this purposes, the use of g_atomic_int_set() is better. @@ -474,2 +490,3 @@ return FALSE; priv->frame = frame; + priv->frame_busy = TRUE; ditto
Review of attachment 301106 [details] [review]: ::: gst-libs/gst/vaapi/gstvaapiwindow_wayland.c @@ +201,3 @@ + break; + } + wl_display_read_events (wl_display); This whole thing is problematic because you could have more than one vaapisink that share the same wl_display, and you can have more than one thread going this thing at a time. So instead, I think you need a separate thread in GstVaapiWaylandDisplay that polls and reads from the socket, and then the various sinks can wait on that.. See how it is implemented in waylandsink for reference.
Created attachment 301396 [details] [review] vaapisink: implement unlock/unlock_stop for wayland I've fixed the broken assertions. I'm not 100% sure why I don't see those errors. Probably because gstreamer is built with --disable-debug? And I changed unlock (and lock) to: [...] if (sink->window) return gst_vaapi_window_unlock (sink->window); return TRUE; [...] Notice the 'TRUE'. That's what my original code did and I'm pretty sure that this should succeed if no handler exists.
(In reply to Víctor Manuel Jáquez Leal from comment #6) > Review of attachment 301105 [details] [review] [review]: > > ::: gst-libs/gst/vaapi/gstvaapiwindow_wayland.c > @@ -332,2 +332,2 @@ > if (priv->frame == frame) > - priv->frame = NULL; > + priv->frame_busy = FALSE; > > Though it is not common in gstreamer-vaapi, I think, for this purposes, the > use of g_atomic_int_set() is better. I'm not sure if that is a good idea. Currently frame_busy is defined as "guint frame_busy:1;" and I don't think it can be modified atomically. And It's only modified from the same thread anyways.
(In reply to Olivier Crête from comment #7) > Review of attachment 301106 [details] [review] [review]: > > ::: gst-libs/gst/vaapi/gstvaapiwindow_wayland.c > @@ +201,3 @@ > + break; > + } > + wl_display_read_events (wl_display); > > This whole thing is problematic because you could have more than one > vaapisink that share the same wl_display, and you can have more than one > thread going this thing at a time. So instead, I think you need a separate > thread in GstVaapiWaylandDisplay that polls and reads from the socket, and > then the various sinks can wait on that.. > > See how it is implemented in waylandsink for reference. Actually, the wayland API is designed to handle exactly this case: - With wl_display_prepare_read() all threads announce their intention to read. - wl_display_read_events() is thread save. On threads reads, the other wait for it to finish. - With wl_display_dispatch_queue_pending() each thread dispatches its own events.
++ on the patches then, I didn't understand the wayland thing correctly.
I'm sorry for taking so long to merge these patches, but I'm learning wayland. I want to merge bug #705821 and bug #747902 too, if you want to review them.
Review of attachment 301105 [details] [review]: LGTM. Thanks.
Review of attachment 301396 [details] [review]: Where does unlock/unlock_stop paradigm come from?
Review of attachment 301396 [details] [review]: Nevermind, from GstBaseSink. :)
Better names needed here though. {Lock/}Unlock should be reserved to the traditional resources locking paradigm. Something like unblock/unblock_cancel() but it's still too vague imho. Thanks.
Comment on attachment 301105 [details] [review] wayland: release the frame in the buffer release callback commit 7548c72a7d7db3e1d1d2bb2965a08d028907565a Author: Michael Olbrich <m.olbrich@pengutronix.de> Date: Tue Feb 3 16:52:06 2015 +0100 wayland: free frame in buffer release callback The Wayland compositor may still use the buffer when the frame done callback is called. This patch destroys the frame (which contains the buffer) until the release callback is called. The draw termination callback only controls the display queue dispatching. Signed-off-by: Víctor Manuel Jáquez Leal <vjaquez@igalia.com> https://bugzilla.gnome.org/show_bug.cgi?id=747492
Created attachment 303428 [details] [review] wayland: wl_display_dispatch_queue() can block forever. wl_display_dispatch_queue() might prevent the pipeline from shutting down. This can happen e.g. if the wayland compositor exits while the pipeline is running. This patch replaces it with these steps: - With wl_display_prepare_read() all threads announce their intention to read. - wl_display_read_events() is thread save. On threads reads, the other wait for it to finish. - With wl_display_dispatch_queue_pending() each thread dispatches its own events. wl_display_dispatch_queue_pending() was defined since wayland 1.0.2 Original-patch-by: Michael Olbrich <m.olbrich@pengutronix.de> * stripped out the unlock() unlock_stop() logic * stripped out the poll handling Signed-off-by: Víctor Manuel Jáquez Leal <victorx.jaquez@intel.com> https://bugzilla.gnome.org/show_bug.cgi?id=749078
Created attachment 303429 [details] [review] vaapisink: implement unlock/unlock_stop for wayland Otherwise wl_display_dispatch_queue() might prevent the pipeline from shutting down. This can happen e.g. if the wayland compositor exits while the pipeline is running. Changes: * renamed unlock()/unlock_stop() to unblock()/unblock_cancel() in gstvaapiwindow * splitted the patch removing wl_display_dispatch_queue() Signed-off-by: Víctor Manuel Jáquez Leal <victorx.jaquez@intel.com> https://bugzilla.gnome.org/show_bug.cgi?id=747492 https://bugzilla.gnome.org/show_bug.cgi?id=749078
I uploaded a second version of the Michael's patch, but I split it in two patches: one making the sync() non-blocking and the second one with the unlock()/unlock_stop() logic with the changes recommended by Gwenole. I use these patches in bug #749078 If you are OK with them, I'll push them along with those in bug #749078 next Monday.
Attachment 303428 [details] pushed as 522ec79 - wayland: wl_display_dispatch_queue() can block forever. Attachment 303429 [details] pushed as 11b9260 - vaapisink: implement unlock/unlock_stop for wayland