GNOME Bugzilla – Bug 731525
multiple glimagesink elements aborts due to no XInitThreads.
Last modified: 2014-06-28 08:30:44 UTC
Pipeline: videotestsrc ! tee name=t ! queue ! glimagesink t. ! queue ! glimagesink Output: [xcb] Most likely this is a multi-threaded client and XInitThreads has not been called [xcb] Aborting, sorry about that. gst-launch-1.0: xcb_io.c:179: dequeue_pending_request: Assertion `!xcb_xlib_unknown_req_in_deq' failed. The issue here is that the glx contexts use the display's x11 connection to create their contexts. If we separate the display connection per context, then context sharing breaks on intel drivers. egl/x11 works fine. The solution is to, as the error message suggests, call XInitThreads() however we would need to do that before any other GUI toolkit attempts to access X. For Gtk+ that is before gtk_init is called which is typically one of the first things an app does. This poses a problem as the libgstgl/libgstopengl is typically loaded (by GstElementFactory) after gtk_init is called and will cause segfaults due to the use of XInitThreads() after X11 resources have been created. Proposed Solution: 1. Punt responsibility to the application to call XInitThreads before calling any toolkit functions and 2. add an XInitThreads call to plugin_init in libgstopengl for the gst-launch case.
Do you mean that "gst_gl_ensure_display" make them use the same GstGLDisplay instance ? Maybe we could ensure thread safety ourself: renaming "gst_gl_display_get_handle": to "gst_gl_display_acquire_handle" and adding "gst_gl_display_release_handle". The caller would be responsible to release it when done. The bad thing is that it adds a locking at client level, so 2 glXSwapBuffers (x11_display_handle, window_handle) would never happen at the same time within the same pipeline. But I think it would be the same result with XInitThreads, plus it adds the constrain to all pipelines. Anyway I guess at some point there is a locking mechanism at server level if not using the same connection. Good thing is that we avoid 1 and 2. Also when using only one glimagesink, the same problem will happen when having one one GLThread per streaming thread and having gleffects ! queue ! glimagesink. In the end I would prefer to go for a solution where we have the hand on the locking. (Just a note, the glimage_sink->display_name is never used)
(In reply to comment #1) > Do you mean that "gst_gl_ensure_display" make them use the same GstGLDisplay > instance ? In order to share glx contexts with intel linux drivers, the X Display connection has to the same, it does not matter how they are the same, just that they are. It is an implementation detail that that X Display for glx contexts comes from the GstGLDisplay. > Maybe we could ensure thread safety ourself: renaming > "gst_gl_display_get_handle": > to "gst_gl_display_acquire_handle" and adding "gst_gl_display_release_handle". > > The caller would be responsible to release it when done. I thought about this and determined it infeasible. The problem is that it would require app/toolkit buy-in due to the thread separation between the app/toolkit's main thread and GStreamer's streaming threads. Take clutter for example, in order for us to share contexts with clutter, the display connection has to be the same (for intel linux drivers). So a good app provides the clutter X display to libgstgl using GstContext. Now imagine that clutter, without knowing about libgstgl or any of its GstGLDisplay synchronization, performs it's own glX calls at a similar time as libgstgl. If they overlap (without XInitThreads) BOOM, xcb will abort the app. Not good. > The bad thing is that it adds a locking at client level, so 2 glXSwapBuffers > (x11_display_handle, window_handle) would never happen at the same time within > the same pipeline. No two glX calls can be running at the same time. > But I think it would be the same result with XInitThreads, plus it adds the > constrain to all pipelines. Anyway I guess at some point there is a locking > mechanism at server level if not using the same connection. > Good thing is that we avoid 1 and 2. The other bad thing is that all of the other display systems are already thread-safe enough to ignore this issue so we would be doing this only for X11. > Also when using only one glimagesink, the same problem will happen when having > one one GLThread per streaming thread and having gleffects ! queue ! > glimagesink. Yes, very possible. However mitigated by the timing issues of the ALLOCATION query. > In the end I would prefer to go for a solution where we have the hand on the > locking. See above
I'm curious why egl/x11 works fine whereas not for glx. Would it be possible to fix glx ? Or is there a deep incompatibility ? Oki I have not considered the case sharing with external gl context, and when you are required to pass the X11 Display connection from the external tool kit (through gst_gl_ensure_display /gst_gl_display_x11_new_with_display). So in that case that means only those functions are concerned: gst_gl_context_glx_create_context/gst_gl_context_glx_destroy_context (creation/destruction) Others like gst_gl_context_glx_swap_buffers are not called because its bound to the external context managed by the UI tool kit. So we could pass the external locking mechanism. Cairo will pass cairo_device_acquire/release, clutter will pass clutter_threads_add_idle_full (clutter_threads_enter/leave are deprecated). Passed through gstcontext like for the X11 connection. For cairo it's simple to wrap it with gst_gl_display_acquire_handle/release. It's also possible for clutter but it requires more abstraction like the following: I think for a GstGLContext that wraps an external GL context, we should still be able to call gst_gl_context_thread_add on it. For the clutter case it will internally call clutter_threads_add_idle_full+a_blocking_mecanism. We could define a func prototype and then the user needs to implement them and pass to gstgl through gstcontext like for X11 connection. (for well known UI tool kit we could predefine some implementations to help the user) Then in gst_gl_context_create we would need to call gst_gl_context_thread_add (external_gstglcontext, ), so basically we create the gstgl internal context into the external thread, and then we create the gl thread and make the internal gl context current in there as usual. Anyway I think at some point it will be useful to have gst_gl_context_thread_add support on wrapped external gl context. Also because it will enable the possibility that gstgl do not create any internal GL context, just use the external one. It's just to be flexible. Especially if the driver does not support context sharing, so we should provide this possibility too even if it's not the optimal way.
(In reply to comment #3) > I'm curious why egl/x11 works fine whereas not for glx. Would it be possible to > fix glx ? Or is there a deep incompatibility ? The following points as I've iterated above, contribute to the incompatibility. 1. X Display's are not thread safe 2. Context sharing for glx on Intel drivers requires the display to be the same 3. These multiple contexts share the same display connection, which are accessed from different threads. 4. xcb aborts the app. The way to fix is to call XInitThreads() to provide the synchronisation structures to let the X server deal with multiple concurrent access to an X display connection from multiple threads. That's how every other display server out there deals with this (without the function call). > Oki I have not considered the case sharing with external gl context, and when > you are required to pass the X11 Display connection from the external tool kit > (through gst_gl_ensure_display /gst_gl_display_x11_new_with_display). So in > that case that means only those functions are concerned: > gst_gl_context_glx_create_context/gst_gl_context_glx_destroy_context > (creation/destruction) > Others like gst_gl_context_glx_swap_buffers are not called because its bound to > the external context managed by the UI tool kit. No, that's wrong. It's all glX functions called by GstGLContextGLX can potentially abort the app. > So we could pass the external locking mechanism. Cairo will pass > cairo_device_acquire/release, clutter will pass clutter_threads_add_idle_full > (clutter_threads_enter/leave are deprecated). Passed through gstcontext like > for the X11 connection. > For cairo it's simple to wrap it with gst_gl_display_acquire_handle/release. > It's also possible for clutter but it requires more abstraction like the > following: > > I think for a GstGLContext that wraps an external GL context, we should still > be able to call gst_gl_context_thread_add on it. For the clutter case it will > internally call clutter_threads_add_idle_full+a_blocking_mecanism. We could > define a func prototype and then the user needs to implement them and pass to > gstgl through gstcontext like for X11 connection. (for well known UI tool kit > we could predefine some implementations to help the user) And get an optional dependency on clutter or cairo, no thanks. > Then in gst_gl_context_create we would need to call gst_gl_context_thread_add > (external_gstglcontext, ), so basically we create the gstgl internal context > into the external thread, and then we create the gl thread and make the > internal gl context current in there as usual. > > Anyway I think at some point it will be useful to have > gst_gl_context_thread_add support on wrapped external gl context. Also because > it will enable the possibility that gstgl do not create any internal GL > context, just use the external one. It's just to be flexible. Especially if the > driver does not support context sharing, so we should provide this possibility > too even if it's not the optimal way. The entire gst_gl_context_thread_add should disappear entirely and is not at all relevant to this discussion.
I have no other solution :) I just want to make sure we explore other ideas before to apply a such requirement. Also do you think it will be agreed to call XInitThreads in apps that uses cluttersink like Totem ?
clutter-gst already calls XInitThreads in it's plugin_init function.
(In reply to comment #6) > clutter-gst already calls XInitThreads in it's plugin_init function. ... which might be too late already.
In 0.10, we made it mandatory, if an application wants to share it's X11 display connection, that connection need to be made threadsafe. For clutter, it means initialising X11 threading before anything else, and explicitly. A plugin cannot do that "as it might be too late". The problem with the idea about using idle, is that the idle will run in the main thread, which is the same thread that controls the pipeline. During synchronous state transition, that used to lead to deadlocks (no solution).
Also Nicolas just told me that a FBO could internally make some call to X. So that definitely breaks my solution as it was only for creation/destruction. I just had a look at Qt and they mention it too "Also, under X11, it's necessary to set the Qt::AA_X11InitThreads application attribute" (http://qt-project.org/doc/qt-4.8/qglframebufferobject.html). This remark disappears in qt-5 doc (http://qt-project.org/doc/qt-5/qopenglframebufferobject.html) and looking at the source code it's not clear when it's called and would require to actually test and break on XInitThreads. Any chance to fail properly if the app did not call XInitThreads ? Maybe re-calling XInitThreads in gstgl when using an external x11 connection, if it returns non-zero then it could mean it has been called before, otherwise we fail anyway. Nothing is mentioned about the case: the user call XInitThreads before any X call and then re-call XInitThreads later.
One more note, XInitThreads iirc set a global variable, so only the display created afterward will be affected. Existing display will remain non-threadsafe.
(In reply to comment #7) > (In reply to comment #6) > > clutter-gst already calls XInitThreads in it's plugin_init function. > > ... which might be too late already. It is... (In reply to comment #9) > (http://qt-project.org/doc/qt-5/qopenglframebufferobject.html) and looking at > the source code it's not clear when it's called and would require to actually > test and break on XInitThreads. http://qt-project.org/doc/qt-5/qt.html#ApplicationAttribute-enum It's still there. > Any chance to fail properly if the app did not call XInitThreads ? Maybe > re-calling XInitThreads in gstgl when using an external x11 connection, if it > returns non-zero then it could mean it has been called before, otherwise we > fail anyway. Nothing is mentioned about the case: the user call XInitThreads > before any X call and then re-call XInitThreads later. XInitThreads can be called multiple times, only the first call has any effect. Calling it multiple times will still report success with every call. (In reply to comment #10) > One more note, XInitThreads iirc set a global variable, so only the display > created afterward will be affected. Existing display will remain > non-threadsafe. It's even more dangerous than that, calling XInitThreads after gtk_init for example will segfault on a mutex lock in X (that presumably was not allocated at display creation time). I don't think we can really do anything about this other than defining that XInitThreads must be called for libgstgl to work with multiple contexts and/or external displays with at least glx. We could say for any use of X11 to be on the safe side :).
Why is this a blocker? And what's the plan here now?
I guess the plan is what Matthew suggested in the first comment.
So his 1) solution? And the 2) solution to make gst-launch happy and the few other corner cases?
This is 1) completed: commit 1d16cd50a3f07198e2519adec8e52b2b5b21ea14 Author: Matthew Waters <ystreet00@gmail.com> Date: Tue Jun 24 23:33:30 2014 +1000 gl: add a scary note about initializing thread support for the winsys We cannot do it as the winsys may crash if we initialize too late. Example, GLX contexts with Intel drivers: Intel requires the X Display to be the same in order to share GL resources across GL contexts. These GL contexts are generally accessed from different threads. Without winsys support it is nearly impossible to guarentee that concurrent access will not occur. This concurrent access could result in crashes or abortion by the winsys (xcb). https://bugzilla.gnome.org/show_bug.cgi?id=731525 and 2) protected by an opt-in environment variable commit 5409a3ea2f5bb76e11b922834c6877bffc463924 Author: Matthew Waters <ystreet00@gmail.com> Date: Tue Jun 24 23:51:24 2014 +1000 gl: XInitThreads when env variable is set This is too allow gst-launch debugging with multiple GL contexts as well as avoiding segfaulting innocent gtk+ apps that have not called XInitThreads. https://bugzilla.gnome.org/show_bug.cgi?id=731525