GNOME Bugzilla – Bug 733661
glimagesink navigation interface causes hangs with X11/GMainLoop event thread
Last modified: 2014-09-09 11:55:53 UTC
My understanding of the problem: 1. Navigation event sent from the X11 thread attempts to flush_start/stop and lock basesink's preroll lock. 2. glimagesink may or may not be waiting on a redraw (via g_main_context_invoke) to complete which is waiting on the current X11/GMainContext operation (navigation event) which is called with basesink's preroll lock taken. Reproduce: gst-launch-1.0 playbin video-sink="navseek ! glimagesink" and mash the left/right keys. irc discussion < ystreet00> slomo: the hang could be from the navigation event being sent from a non-streaming thread < slomo> ystreet00: we're always sending navigation events from non-streaming threads < slomo> ystreet00: do you have backtraces? < ystreet00> slomo: ya :) http://paste.debian.net/111386/ < ystreet00> slomo: it seems to block on the preroll lock in basesink < slomo> ystreet00: 290 stack frames \o/ < ystreet00> slomo: reproduce by mashing the left/right keys on your keyboard :) < slomo> ystreet00: interesting backtrace though < slomo> that shouldn't happen :) < slomo> especially that flushing causes a deadlock... < ystreet00> you don't say :) < slomo> oh < slomo> so < slomo> ystreet00: the problem is that glimagesink is rendering something and waiting for the result from the X11 thread... and the X11 thread is sending the seek and waiting for glimagesink to finish < slomo> maybe we shouldn't do anything complicated from the X11 thread and just dispatch that to some new thread instead < slomo> however starting a new thread for every navigation event might be a bit crazy < ystreet00> slomo: this whole rendering thread thing needs looking at anyway < slomo> ystreet00: why? what else is weird? :) < slomo> ystreet00: and please put this deadlock in bugzilla with the backlog ↑ so we don't forget :) backtrace: (gdb) t a a bt
+ Trace 233873
Thread 14 (Thread 0x7fffc75f5700 (LWP 22271))
Created attachment 284623 [details] [review] [PATCH] GstGlWindow: Introduce navigation thread This RFC patch introduces a navigation thread in GstGLWindow to dispatch navigation events. GstGlWindow_x11 thread is changed to invoke the navigation thread for navigation dispatching, instead of emiting the events itself. The deadlock should be avoided this way. The navigation thread is currently part of GstGLWindow and not implemented in separate subclasses / backends yet. Perhaps this is needed? Also commit 4dacc4ba introduced get_surface_dimensions method. The problem for the X11 backend, is that now this function is called from the navigation thread, resulting in xlib aborting due to multithreaded access: [xcb] Unknown request in queue while dequeuing [xcb] Most likely this is a multi-threaded client and XInitThreads has not been called [xcb] Aborting, sorry about that. The call to gst_gl_window_get_surface_dimensions has been commented for now. I am not sure if this is considered a gst-launch problem (gst-launch not calling XInitThreads), or a design problem of this navigation thread solution. Also the commit that introduced get_surface_dimensions (4dacc4ba) could also be reverted for now if needed.
Review of attachment 284623 [details] [review]: Looks good, some comments below. (In reply to comment #1) > The navigation thread is currently part of GstGLWindow and not implemented in > separate subclasses / backends yet. Perhaps this is needed? It will be needed at some point :) > Also commit 4dacc4ba introduced get_surface_dimensions method. The problem for > the X11 backend, is that now this function is called from the navigation > thread, resulting in xlib aborting due to multithreaded access: For x11 it would make sense to store the window dimensions in the subclass and return the cached values in get_surface_dimensions (). The other backends will most likely be fine with all of this. If you have the GST_GL_XINITHTHREADS environment variable set, then XInitThreads() will be called on plugin initialization. However it would be nice to avoid requiring XInitThreads in the basic gst-launch case.
This needs to be fixed before 1.5.1 as it's causing a regression btw
(In reply to comment #2) thanks for reviewing > Review of attachment 284623 [details] [review]: > > Looks good, some comments below. > > (In reply to comment #1) > > The navigation thread is currently part of GstGLWindow and not implemented in > > separate subclasses / backends yet. Perhaps this is needed? > > It will be needed at some point :) ok. So should I work on it (i.e. making only dummy and x11 subclass implementation for now) or leave it in GstGLWindow for first inclusion? > > > Also commit 4dacc4ba introduced get_surface_dimensions method. The problem for > > the X11 backend, is that now this function is called from the navigation > > thread, resulting in xlib aborting due to multithreaded access: > > For x11 it would make sense to store the window dimensions in the subclass and > return the cached values in get_surface_dimensions (). The other backends will > most likely be fine with all of this. makes sense, I 'll try this.
Created attachment 285414 [details] [review] [PATCH] GstGlWindow: Introduce navigation thread v1 -> v2 - gst_gl_window_x11_get_surface_dimensions now uses values retrieved from x11 thread, to avoid xlib multithreaded access abort. - cleaned up some leftover debugging navigation thread is still not implemented in subclasses.
Review of attachment 285414 [details] [review]: ::: gst-libs/gst/gl/gstglwindow.c @@ +242,3 @@ + window->priv->navigation_thread = g_thread_new ("gstglnavigation", + (GThreadFunc) gst_gl_window_navigation_thread, window); I'm not sure if it matters but this is racy with event sending from backends. The thread could have not been created yet when the backends try to send events through it. You might want to look at the gst_gl_context_create in gstglcontext.c for some inspiration. @@ +347,2 @@ /** + * gst_gl_window_run: run_navigation
Created attachment 285724 [details] [review] GstGLWindow: Introduce navigation thread v2 -> v3: Added Gmutex and Gcond for creation / destruction of navigation thread
commit 3c3b78508fb7f283606db661c4e838daa4b3d865 Author: Vasilis Liaskovitis <vliaskov@gmail.com> Date: Tue Sep 9 12:01:47 2014 +0200 GstGLWindow: Introduce navigation thread This thread dispatches navigation events. It is needed to avoid deadlocks between window backend threads that emit navigation events (e.g. X11/GMainLoop thread) and consumers of navigation events such as glimagesink, see https://bugzilla.gnome.org/show_bug.cgi?id=733661 GstGlWindow_x11 thread is changed to invoke the navigation thread for navigation dispatching, instead of emiting the event itself. Othe backends beside X11 do not dispatch navigation events yet, but should use this thread when dispatching these events in the future. The navigation thread is currently part of GstGLWindow and not implemented in separate subclasses / backends. This will be needed in the future. gst_gl_window_x11_get_surface_dimensions is also changed to use a cached value of the window's width, height. These values are now retrieved in the X11 thread, function gst_gl_window_x11_handle_event. This change is needed because otherwise the XGetWindowAttributes gets called from the navigation thread, leading to xlib aborting due to multithreaded access (if XInitThreads is not called before, as is the case for gst-launch)