GNOME Bugzilla – Bug 741054
gl/cocoa: Simplify NSApplication initialisation for console apps
Last modified: 2018-11-03 11:33:35 UTC
This fixes a deadlock and makes failure to initialise the NSApp explicitly fail. I looked at how GDK deals with this on Sebastian's suggestion, and that looks a bit more involved -- we would need to replace the GLib poll function with a Cocoa event poll function, and move fd polling to a separate thread. In my mind, this is quite a lot of complexity for a bit of a corner case. Either way, this patch should make things better, not worse, while we try to find a more robust solution. The GDK solution, for the curious: https://git.gnome.org/browse/gtk+/tree/gdk/quartz/gdkeventloop-quartz.c#n12
Created attachment 292038 [details] [review] gl/cocoa: Simplify NSApplication initialisation for console apps This eliminates a possible deadlock in GL context initialisation -- if the GLib mainloop was creating an object (trying to get a GType registry lock) when a streaming thread asked for a GL context, we would try to create the context in class init (also holding the GType registry lock), we would deadlock because we were trying to wait for an idle callback to be called in the mainloop, which was currently blocked on the GType lock. Instead, we now schedule the idle callback and hope it gets called before we need to create the actual context. If there's a delay there, we introduce a 1s wait for the the callback to be called. If that fails, we bail. While this seems a bit of a regression to the previous state that was fixed by 8d92b6a3, this makes things a bit more robust, and properly fails if NSApplication initialisation is not completed by the time we need it.
Review of attachment 292038 [details] [review]: While this fixes one deadlock, it causes other problems :( Using the GDK approach also seems not wise inside libgstgl as we would severely modify the default main context behaviour... after a main loop is potentially running on it already. Not sure what to do about this. Note that using the approach from osxvideosink can't be used here although it is "perfect" there... the osxvideosink approach will cause deadlocks and assertions if any of the GL related code uses the GCD. Our own usage of it can be removed relatively easily and replaced by calling selectors instead like in osxvideosink, but there's nothing that prevents other code to use it. The problem with the GCD is that we can't seem to fake that the "main thread" is a different one for it, while for NSApplication that part works. Maybe GMainContext/GMainLoop should directly do the stuff GDK does when running on OSX/iOS. When the default main context is acquired from the NSApplication main thread. That would solve all our problems at once. ::: gst-libs/gst/gl/cocoa/gstglcontext_cocoa.m @@ +159,1 @@ } else if (g_main_context_acquire (context)) { Not your fault, but this is racy :) Consider the following standard situation: 1) gst_element_set_state(playbin, GST_STATE_PLAYING) 2) g_main_loop_run(loop) Now between 1) and 2) we could get to the above code if we're fast enough and successfully acquire the default main context... while actually everything would be ok. @@ +199,3 @@ __block NSOpenGLContext *glContext = nil; + if (!g_atomic_int_get (&nsapp_init_done)) { This will break if: - class_init() is run from some arbitrary thread (e.g. streaming thread in playbin), thus not the NS main thread - No GLib main loop is running on the default main context - We have a real Cocoa application That is, the default case if you develop a real Cocoa application and don't accidentially call class_init() from the main thread. Previously this probably would've resulted in a GST_WARNING() too @@ +202,3 @@ + /* Probably not scheduled in the idle callback yet */ + GST_WARNING ("NSApp not yet initialised, trying to wait"); + g_usleep (1 * G_USEC_PER_SEC); 1 second seems to be a bit big :)
On a related note, create_context() must currently be called from a thread different to the main thread. As it synchronously dispatches a block to the main queue/thread... which would deadlock if we are already on that thread. Simple workaround is to not dispatch anything if we are already on the NSApplication main thread.
*** Bug 741323 has been marked as a duplicate of this bug. ***
The patch from bug #741450 is now included in cerbero, and we conditionally enable it via #defines in the cerbero build. This should improve the situation a lot for us until GLib has merged a proper solution.
The latest changes in gst/gl/cocoa fix the remaining dead locks when not using cerbero (ex: gst-uninstalled + macports). Especially the dead lock mentioned in #1. As a side note, currently MacPorts provide gst-1.4.5 so they are quite up-to-date: https://www.macports.org/ports.php?by=name&substr=gstreamer1 . Should we keep this bug open until bug #741450 is merged ?
Yes, without fixing it in GLib there's nothing we can do it make it work really reliable in all cases :)
This patch broke something btw, see http://cgit.freedesktop.org/gstreamer/gstreamer/commit/?id=f02de0e9a1cd1c0ffe6935e884f726e5da526c5e Shouldn't usually be a problem except in special cases...
-- 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/149.