GNOME Bugzilla – Bug 746012
gl: add GstGLDisplayCocoa to handle NSApp initialization
Last modified: 2015-03-14 08:41:23 UTC
Currently it's in GstGLContextCocoa. It initializes the display environment so it make sense to create gstgldisplay_cocoa.m/GstGLDisplayCocoa and move all this code into. See TODO in current gstglcontext_cocoa.m . In additon it will be easier for us to handle the setup of the internal custom NSApp loop that we run for debuging application like gst-launch. (While waiting for bug 741450)
Created attachment 299073 [details] [review] gl: add GstGLDisplayCocoa This patch also resolves the acquire(gmaincontext)/release case. Indeed if NSApp is still nil at this point it means that we can assume this is a debugging application (so we can allow us to wait a bit more for a glib loop to run in the main thread). Otherwise in a real application the user would have already initialize NSApp. Also it now fails the pipeline if we could not initialize NSApp. Note that in any case, not initializating the NSApp will make gstgl wait indefinitely and deadlock. So better to check and fails the pipeline in the worst case.
I forgot to mention that I removed the handling of g_source_remove because it is safe to call the callback at any time. So let the gmaincontext remove it. Also I realized I should return NULL in gstgldisplay.c if it fails to create GstGLDisplay. Instead of falling back to dummy display. And check more properly this case.
I don't like this, it makes everything more complicated again just to work around a bug that is better solved correctly inside GLib.
[16:46] <capOM> The goal was actually to make it less complicated. And it is not to work around a bug it is because it should not be handled by GstGLContext, but display. Because it is not 1:1 with GLContext. Then I just added in the patch a way to workaround acquire/release. [16:47] <slomo> but there is not really a GstGLDisplayCocoa [16:47] <slomo> there is nothing like a display there [16:47] <slomo> so it seems weird to just add a display for the workaround thing [16:47] <capOM> Well the apple doc says it initialize the display env [16:48] <slomo> yes, but there's only a single "display" [16:48] <slomo> you don't even have a handle or anything for it [16:49] <slomo> and once that bug is fixed in glib in one way or another, the GstGLDisplayCocoa would just be empty [16:51] <capOM> Yeah it is an overall display. And yep it would be empty as all the code currently in gstglcontextcocoa is under this ifndef [16:51] <capOM> well if glib has the fix soon I will be more than happy [16:51] <slomo> it probably won't be soon :) but we have a fix in cerbero [16:52] <capOM> The thing is I am not using cerbero but mac ports with gst-uninstalled [16:52] <slomo> so if you say the code is simpler now, and also better... just push it and i look away ;) [16:52] <slomo> but please don't make things more complicated, and also GstGLDisplayCocoa shouldn't be public API [17:02] <capOM> oups I was distracted, ok I'll make it not public at least. And it is roughly same code as current, except it is in a separate file. Later even if this file will be empty, it could still be a place to do some stuff that are not 1:1 to a context when shared with several context. [17:03] <slomo> ok :)
Created attachment 299305 [details] [review] gl: add GstGLDisplayCocoa As there is only one overall display, I made GstGLDisplayCocoa a kind of singleton. So gst_gl_display_cocoa_new just gives you the same instance all the time. It supports to reset it once ref count goes to 0. With the recent changes (glupload is back :) ) it needs a bit more testing.
commit 802bf799988ee5d2906550691b89f266ec0eb88f Author: Julien Isorce <j.isorce@samsung.com> Date: Wed Mar 11 00:06:55 2015 +0000 gl: add GstGLDisplayCocoa https://bugzilla.gnome.org/show_bug.cgi?id=74601