GNOME Bugzilla – Bug 706553
context creation is not thread safe
Last modified: 2013-10-01 20:41:36 UTC
I can get a trace as follows almost every time with GST_DEBUG=gl*:9 make libs/gstglmemory.gdb. Most of the time it will hang with the glmixer element although I have seen it less with glfilter subclasses as well albiet, less often. gstglmixer.c:2141:gst_gl_mixer_change_state:<glmosaic> stopping collectpads 0:01:05.319824401 4500 0x795cf0 INFO glwindow gstglwindow.c:174:gst_gl_window_finalize: send quit gl window loop 0:01:05.319849823 4500 0x795cf0 LOG glwindow gstglwindow_x11.c:739:gst_gl_window_x11_quit: sending quit 0:01:05.319874547 4500 0x795cf0 LOG glwindow gstglwindow_x11.c:743:gst_gl_window_x11_quit: quit sent 0:01:05.319883067 4500 0x795cf0 INFO glwindow gstglwindow.c:301:gst_gl_window_quit: quit sent to gl window loop 0:01:05.319900039 4500 0x67c680 DEBUG glwindow gstglwindow.c:281:gst_gl_window_run: running ^C Program received signal SIGINT, Interrupt. pthread_cond_wait@@GLIBC_2.3.2 () at ../nptl/sysdeps/unix/sysv/linux/x86_64/pthread_cond_wait.S:185 185 ../nptl/sysdeps/unix/sysv/linux/x86_64/pthread_cond_wait.S: No such file or directory. (gdb) thread apply all bt
+ Trace 232406
Well, this looks like it is simply because of the render_lock. It is not hold during quit() and the waiting, and also there's a small race between gst_gl_window_run() and quit(). Which leads to the above behaviour, run happening after quitting. run() should probably check if the window was quit already before starting the loop.
(In reply to comment #1) > Well, this looks like it is simply because of the render_lock. It is not hold > during quit() and the waiting, and also there's a small race between > gst_gl_window_run() and quit(). Which leads to the above behaviour, run > happening after quitting. run() should probably check if the window was quit > already before starting the loop. I have thought of two possible solutions: 1. A counter that counts the number of quit/run invocations have happened and determining whether to run/quit based on that. Just an extension of the quit flag idea. However that might be a little too complex. 2. unlock the mutex as the first thing in the run loop. Would require a loop that 'remembers' functions while not running and an async send_message. Ideally I'd like to move to GMainLoop for everything however Cocoa doesn't seem too keen on integration (from looking at clutter, gdk and the Cocoa documentation). Win32 looks pretty easy to do.
Just a note that this also applies to the following code fragment. gst_gl_context_create () gst_gl_window_send_message () /* loop may not have started yet */ and by extension, pretty much anything that relies on the loop actually running after context_create() is called. IMHO this reduces the usefulness of the run/quit counter/flag option and leaves us with unlocking the render_lock when the loop actually runs. Also, all of this could be moot when we stop creating threads to marshall GL calls into.
(In reply to comment #3) > Just a note that this also applies to the following code fragment. > > gst_gl_context_create () > gst_gl_window_send_message () /* loop may not have started yet */ > > and by extension, pretty much anything that relies on the loop actually running > after context_create() is called. > > IMHO this reduces the usefulness of the run/quit counter/flag option and leaves > us with unlocking the render_lock when the loop actually runs. Ack, do you see a clean solution for this that doesn't introduce new race conditions? :) > Also, all of this could be moot when we stop creating threads to marshall GL > calls into. That will probably be always there, no? Like for the video sink. Even if we don't do it for some contexts.
(In reply to comment #4) > (In reply to comment #3) > > IMHO this reduces the usefulness of the run/quit counter/flag option and leaves > > us with unlocking the render_lock when the loop actually runs. > > Ack, do you see a clean solution for this that doesn't introduce new race > conditions? :) Clean? not really. Solutions? A few. 1. move to GMainLoop however this requires support from the backends. Requires lifting the GMainLoop integration from clutter or gdk for Cocoa. The win32 backend is easy to implement and the others (x11 and wayland) already use GMainLoop. 2. add support for submitting functions before the main loop has started and run them at the beginning of the loop. GMainLoop allows this. Would require a gst_gl_window_send_message_async that doesn't wait for completion. 3. add api so that we can say 'yes we are going to run you really soon so you should buffer all functions you get and run them in the loop later' 4. spinlock by calling gst_gl_window_send_message with a function that changes some value that we check. Personally, I think option 1 would be the best in the long run however I don't have OS X anywhere so would be unable to work on this. Option 2 would be next on my list. Option 3 and 4 would be last resorts. > > Also, all of this could be moot when we stop creating threads to marshall GL > > calls into. > > That will probably be always there, no? Like for the video sink. Even if we > don't do it for some contexts. Hmm, now that I think about it, yes. Even if we move the thread creation up into glimagesink, the problem will still be there just not GstGLContext's problem anymore :).
(In reply to comment #5) > (In reply to comment #4) > > (In reply to comment #3) > > > IMHO this reduces the usefulness of the run/quit counter/flag option and leaves > > > us with unlocking the render_lock when the loop actually runs. > > > > Ack, do you see a clean solution for this that doesn't introduce new race > > conditions? :) > > Clean? not really. Solutions? A few. > > 1. move to GMainLoop however this requires support from the backends. Requires > lifting the GMainLoop integration from clutter or gdk for Cocoa. The win32 > backend is easy to implement and the others (x11 and wayland) already use > GMainLoop. Do you think it is possible at all to integrate Cocoa with GMainLoop? I think I saw some code somewhere doing that but it wasn't beautiful at all. > 2. add support for submitting functions before the main loop has started and > run them at the beginning of the loop. GMainLoop allows this. Would require a > gst_gl_window_send_message_async that doesn't wait for completion. Can we make them async without waiting for completion at all? > 3. add api so that we can say 'yes we are going to run you really soon so you > should buffer all functions you get and run them in the loop later' > 4. spinlock by calling gst_gl_window_send_message with a function that changes > some value that we check. Instead of a spinlock you can always use a GCond. > > Personally, I think option 1 would be the best in the long run however I don't > have OS X anywhere so would be unable to work on this. Agreed, that seems the most cleanest solution > Option 2 would be next on my list. Option 3 and 4 would be last resorts.
(In reply to comment #6) > (In reply to comment #5) > > (In reply to comment #4) > > Clean? not really. Solutions? A few. > > > > 1. move to GMainLoop however this requires support from the backends. Requires > > lifting the GMainLoop integration from clutter or gdk for Cocoa. The win32 > > backend is easy to implement and the others (x11 and wayland) already use > > GMainLoop. > > Do you think it is possible at all to integrate Cocoa with GMainLoop? I think I > saw some code somewhere doing that but it wasn't beautiful at all. It looks like it is possible from the gdk and clutter backends although not pretty. It seems that in a couple of places it relies on the default GMainLoop however that may be easy to change. From a quick glance they seem almost identical so should work :). I'll run a diff on them later to see what is different. > > 2. add support for submitting functions before the main loop has started and > > run them at the beginning of the loop. GMainLoop allows this. Would require a > > gst_gl_window_send_message_async that doesn't wait for completion. > > Can we make them async without waiting for completion at all? With GMainContext? yes, simply call g_main_context_invoke. With cocoa, you would do performSelector:@selector(funcname) onThread:thread_id withObject:arg waitUntilDone:NO. That may break the autorelease pool alloc/release idiom we currently use though :/. > > 3. add api so that we can say 'yes we are going to run you really soon so you > > should buffer all functions you get and run them in the loop later' > > 4. spinlock by calling gst_gl_window_send_message with a function that changes > > some value that we check. > > Instead of a spinlock you can always use a GCond. We are using a GCond and its not working :) > > > > Personally, I think option 1 would be the best in the long run however I don't > > have OS X anywhere so would be unable to work on this. > > Agreed, that seems the most cleanest solution
(In reply to comment #7) > (In reply to comment #6) > > (In reply to comment #5) > > > 2. add support for submitting functions before the main loop has started and > > > run them at the beginning of the loop. GMainLoop allows this. Would require a > > > gst_gl_window_send_message_async that doesn't wait for completion. > > > > Can we make them async without waiting for completion at all? > > With GMainContext? yes, simply call g_main_context_invoke. With cocoa, you > would do performSelector:@selector(funcname) onThread:thread_id withObject:arg > waitUntilDone:NO. That may break the autorelease pool alloc/release idiom we > currently use though :/. No I meant if the callers of send_message() currently require invocation to be synchronous or if we can easily make them async. > > > 3. add api so that we can say 'yes we are going to run you really soon so you > > > should buffer all functions you get and run them in the loop later' > > > 4. spinlock by calling gst_gl_window_send_message with a function that changes > > > some value that we check. > > > > Instead of a spinlock you can always use a GCond. > > We are using a GCond and its not working :) That was my point :) If we can't make it working with a GCond a spinlock won't make it work either.
(In reply to comment #8) > (In reply to comment #7) > > (In reply to comment #6) > > > (In reply to comment #5) > > No I meant if the callers of send_message() currently require invocation to be > synchronous or if we can easily make them async. We can probably make most of them async pretty easily. > > > Instead of a spinlock you can always use a GCond. > > > > We are using a GCond and its not working :) > > That was my point :) If we can't make it working with a GCond a spinlock won't > make it work either. Ah :)
(In reply to comment #9) > (In reply to comment #8) > > (In reply to comment #7) > > > (In reply to comment #6) > > > > (In reply to comment #5) > > > > No I meant if the callers of send_message() currently require invocation to be > > synchronous or if we can easily make them async. > > We can probably make most of them async pretty easily. I'm all for making things async, but then you need to make sure that the callback is *always* called at some point so the data passed to it can be freed. Maybe modelling the API around GAsyncResult (see the GIO APIs) would make sense too then, might also be too much for this though :)
(In reply to comment #10) > (In reply to comment #9) > > (In reply to comment #8) > > > (In reply to comment #7) > > > No I meant if the callers of send_message() currently require invocation to be > > > synchronous or if we can easily make them async. > > > > We can probably make most of them async pretty easily. > > I'm all for making things async, but then you need to make sure that the > callback is *always* called at some point so the data passed to it can be > freed. Maybe modelling the API around GAsyncResult (see the GIO APIs) would > make sense too then, might also be too much for this though :) I thought about trying that earlier but got caught up in other things that were more pressing :). OpenGL is inherently async anyway in most cases so yes, it might be too much which was one of the reasons I didn't attempt this earlier.
Created attachment 255655 [details] [review] window: add send_message_async vmethod This implements option 2. Simply because it was the easiest. Also available (with other minor fixes) here: https://github.com/ystreet/gst-plugins-gl/tree/context
commit 064b11db319b3cbd2a0c63937d609963524e12c4 Author: Matthew Waters <ystreet00@gmail.com> Date: Wed Sep 25 12:26:57 2013 +1000 window: add send_message_async vmethod - provide a default synchronous send_message - make context creation threadsafe again