After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 745516 - xvimagesink: race condition causes crash in XCB
xvimagesink: race condition causes crash in XCB
Status: RESOLVED OBSOLETE
Product: GStreamer
Classification: Platform
Component: gst-plugins-base
git master
Other Linux
: Normal normal
: git master
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2015-03-03 11:06 UTC by Vincent Penquerc'h
Modified: 2018-11-03 11:35 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
call XInitThreads to get rid of the crash (1.04 KB, patch)
2015-03-03 11:07 UTC, Vincent Penquerc'h
rejected Details | Review

Description Vincent Penquerc'h 2015-03-03 11:06:24 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)
Comment 1 Vincent Penquerc'h 2015-03-03 11:07:26 UTC
Created attachment 298412 [details] [review]
call XInitThreads to get rid of the crash
Comment 2 Olivier Crête 2015-03-03 16:14:19 UTC
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.
Comment 3 Vincent Penquerc'h 2015-03-04 09:10:07 UTC
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.
Comment 4 Sebastian Dröge (slomo) 2015-03-04 10:21:53 UTC
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.
Comment 5 Vincent Penquerc'h 2015-03-04 10:31:10 UTC
(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...
Comment 6 Vincent Penquerc'h 2015-03-04 10:35:47 UTC
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...
Comment 7 Sebastian Dröge (slomo) 2015-03-04 10:52:34 UTC
(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.
Comment 8 Vincent Penquerc'h 2015-03-04 10:54:44 UTC
(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 ?
Comment 9 Sebastian Dröge (slomo) 2015-03-04 10:56:24 UTC
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.
Comment 10 Vincent Penquerc'h 2015-03-12 14:13:08 UTC
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.
Comment 11 GStreamer system administrator 2018-11-03 11:35:21 UTC
-- 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.