GNOME Bugzilla – Bug 745516
xvimagesink: race condition causes crash in XCB
Last modified: 2018-11-03 11:35:21 UTC
I could get a fairly reliable crash with: gst-play-1.0 dvd:///someisofile On an ISO file with a menu (125 MB), clicking on the menu to start a video. It is fixed by adding XInitThreads to _class_init, but this seems to maybe not be the best fix, since: - ximagesink does not have it, and does not crash - 432790c4ff89aa0f51c9dc9d68048b52f9e7c343 in -base hints it should not be our problem The original error: XIO: fatal IO error 11 (Resource temporarily unavailable) on X server ":0.0" after 1466 requests (1466 known processed) with 0 events remaining. And another one, much rarer version, which helped a lot: [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. gst-play-1.0: ../../src/xcb_io.c:179: dequeue_pending_request: Assertion `!xcb_xlib_unknown_req_in_deq' failed. Aborted (core dumped)
Created attachment 298412 [details] [review] call XInitThreads to get rid of the crash
Review of attachment 298412 [details] [review]: In the GL code, there is a if (g_getenv()) surrounding the XInitThreads call. Either this is correct and the check should be removed from the GL code, or the GL code is right and a g_getenv() is needed here. In the second case, gst-launch should probably set it for us.
The man page does not say whether calling it multiple times is safe or not but if it is, I think it's best to call it in the plugin's init function, without checking any env var. This way, one is sure that the first X function which is not XInitThreads is called only after XInitThreads, without having to ensure all plugin users know to call it. After all, you don't want these programs to have a list of init things to call for possibly obscure libs that some plugin might use.
Why do we need XInitThreads() for xvimagesink? Where are we using the same display connection from multiple threads? The GL code does that, yes. But an application has to call XInitThreads() *before* using any X11 API. So also before e.g. gtk_init(). Otherwise things break in horrible ways. That's why we do it there conditional on an environment variable to opt-in.
(In reply to Sebastian Dröge (slomo) from comment #4) > Why do we need XInitThreads() for xvimagesink? Where are we using the same > display connection from multiple threads? Do you mean it's not supposed to ? > The GL code does that, yes. But an application has to call XInitThreads() > *before* using any X11 API. So also before e.g. gtk_init(). Otherwise things > break in horrible ways. That's why we do it there conditional on an > environment variable to opt-in. If GTK/GDK uses X calls, it should call XInitThreads itself, no ? If any library or program that uses X calls calls XInitThreads, then you can be sure that it is called when needed, without a program that uses libraries that calls X but does not itself call X directly having to call XInitThreads itself. I can repeat that with parentheses if needed, it reads a bit confusing :) The presence of the env var hints that calling it more than once is bad though, but I see no mention of that in the manpage...
Looking at the code, there's an event thread, using the display for XCheckWindowEvent. Which is kinda expected given one of the errors I got. However, ximagesink does the same, and reliably does not crash...
(In reply to Vincent Penquerc'h from comment #5) > (In reply to Sebastian Dröge (slomo) from comment #4) > > Why do we need XInitThreads() for xvimagesink? Where are we using the same > > display connection from multiple threads? > > Do you mean it's not supposed to ? It's not, it should use a different connection per thread because of the X11 threading mess :) > > The GL code does that, yes. But an application has to call XInitThreads() > > *before* using any X11 API. So also before e.g. gtk_init(). Otherwise things > > break in horrible ways. That's why we do it there conditional on an > > environment variable to opt-in. > > If GTK/GDK uses X calls, it should call XInitThreads itself, no ? If any > library or program that uses X calls calls XInitThreads, then you can be > sure that it is called when needed, without a program that uses libraries > that calls X but does not itself call X directly having to call XInitThreads > itself. I can repeat that with parentheses if needed, it reads a bit > confusing :) GTK/GDK does not use threading with X11, so why would it have to call that? But if anything in your application does, you have to call that before gtk_init(). It's really a mess. > The presence of the env var hints that calling it more than once is bad > though, but I see no mention of that in the manpage... Calling it more than once is not a problem AFAIK, but calling it *after anything* used X11 APIs already is a big problem. That's why we can't just unconditonally call it in plugins.
(In reply to Sebastian Dröge (slomo) from comment #7) > GTK/GDK does not use threading with X11, so why would it have to call that? Hmm, that's a fair point... > Calling it more than once is not a problem AFAIK, but calling it *after > anything* used X11 APIs already is a big problem. That's why we can't just > unconditonally call it in plugins. Surely calling it unconditionally cannot be any worse that calling it conditionally if multiple calls are OK ?
If you don't call it and use threading, it will work most of the time unless a connection is used from multiple threads at once (IIRC). So without calling it, it works somewhat... with calling it too late, everything will completely fall apart always.
Very unfortunate. Should all these elements have a single env var they check then, as it doesn't seem like a good idea to have gst-launch, etc, set all of GL_CALL_XINIT, XV_CALL_XINIT, etc.
-- 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/gst-plugins-base/issues/167.