GNOME Bugzilla – Bug 752743
gl: add support for egl+x11+swrast on osx
Last modified: 2015-08-16 13:39:40 UTC
mesa provides software driver softpipe and llvmpipe that both works on osx when using egl+x11 The following patches allow to build GstGL with egl on osx.
Created attachment 307933 [details] [review] gl: move GL_NUM_EXTENSIONS def after gl.h
Created attachment 307934 [details] [review] gl: add internal helper _get_default_gl_platform
Created attachment 307935 [details] [review] gl: support cgl, egl and glx within a same build
Review of attachment 307933 [details] [review]: Looks good.
Review of attachment 307934 [details] [review]: Looks ok.
Review of attachment 307935 [details] [review]: If you're doing funky stuff like loading mesa on OSX, you can always pass the opengl library's to configure and it will use them instead. ::: configure.ac @@ +761,3 @@ + *-*darwin*) + GL_CFLAGS="-Wno-typedef-redefinition" + GL_OBJCFLAGS=$GL_CFLAGS Not a fan of globally disabling the typedef redefinition warning on OS X. @@ +1035,3 @@ fi + + if test "x$NEED_EGL" != "xno"; then Also need to test that $HAVE_EGL is "yes" as well. ::: gst-libs/gst/gl/gstglcontext.c @@ +124,3 @@ +#ifdef __APPLE__ + if (!module_opengl && (_get_default_gl_platform () == GST_GL_PLATFORM_GLX)) + module_opengl = g_module_open ("libGL.1.dylib", G_MODULE_BIND_LAZY); Don't we -framework OpenGL which links against libGL.dylib? Thus, the g_module_open (NULL) will find the opengl functions anyway later? Or am i misunderstanding how -framework works? Also, what's happening with the link command when including glx and cgl? Aren't two GL libraries being linked in? Or is the mesa code linked against libGL.dylib as well? This all sounds like a recipe for disaster honestly.
Review of attachment 307934 [details] [review]: This will now only try one context_new() before failing completely.
Comment on attachment 307933 [details] [review] gl: move GL_NUM_EXTENSIONS def after gl.h commit 90a1ff1383335b4e57019a710be9165b98a50a7c Author: Julien Isorce <julien.isorce@gmail.com> Date: Mon Jul 6 00:45:45 2015 +0100 gl: move GL_NUM_EXTENSIONS definition after gl.h https://bugzilla.gnome.org/show_bug.cgi?id=752743
(In reply to Matthew Waters from comment #7) > Review of attachment 307934 [details] [review] [review]: > > This will now only try one context_new() before failing completely. I think it is even better to do so :) gst_gl_context_cocoa_new, gst_gl_context_egl_new etc ... should not fail really otherwise something really wrong happened and we should not try to create another type of context (because these functions only allocate the object if I am not mistaken). Also if a specific block is built we expect it to work. But maybe I am missing something ?
(In reply to Matthew Waters from comment #6) > Review of attachment 307935 [details] [review] [review]: > > If you're doing funky stuff like loading mesa on OSX, you can always pass > the opengl library's to configure and it will use them instead. > > ::: configure.ac > @@ +761,3 @@ > + *-*darwin*) > + GL_CFLAGS="-Wno-typedef-redefinition" > + GL_OBJCFLAGS=$GL_CFLAGS > > Not a fan of globally disabling the typedef redefinition warning on OS X. Right, I'll add it only if darwin+egl > > @@ +1035,3 @@ > fi > + > + if test "x$NEED_EGL" != "xno"; then > > Also need to test that $HAVE_EGL is "yes" as well. Right. > > ::: gst-libs/gst/gl/gstglcontext.c > @@ +124,3 @@ > +#ifdef __APPLE__ > + if (!module_opengl && (_get_default_gl_platform () == > GST_GL_PLATFORM_GLX)) > + module_opengl = g_module_open ("libGL.1.dylib", G_MODULE_BIND_LAZY); > > Don't we -framework OpenGL which links against libGL.dylib? Thus, the > g_module_open (NULL) will find the opengl functions anyway later? Or am i > misunderstanding how -framework works? Also, what's happening with the link > command when including glx and cgl? Aren't two GL libraries being linked in? > Or is the mesa code linked against libGL.dylib as well? This all sounds > like a recipe for disaster honestly. You are right, this line does not make sense and also glx already work without it. It should be GST_GL_PLATFORM_EGL, not GST_GL_PLATFORM_GLX. Thx!
Created attachment 308044 [details] [review] gl: support cgl, egl and glx within a same build On osx, with the same build, gst-launch-1.0 videotestsrc ! glimagesink works with: GST_GL_PLATFORM=egl GST_GL_WINDOW=x11 GST_GL_API=gles2 GST_GL_PLATFORM=egl GST_GL_WINDOW=x11 GST_GL_API=opengl GST_GL_PLATFORM=glx GST_GL_WINDOW=x11 GST_GL_API=opengl GST_GL_PLATFORM=cgl GST_GL_WINDOW=cocoa GST_GL_API=opengl GST_GL_PLATFORM=cgl GST_GL_WINDOW=cocoa GST_GL_API=opengl3
(In reply to Julien Isorce from comment #9) > (In reply to Matthew Waters from comment #7) > > Review of attachment 307934 [details] [review] [review] [review]: > > > > This will now only try one context_new() before failing completely. > > I think it is even better to do so :) gst_gl_context_cocoa_new, > gst_gl_context_egl_new etc ... should not fail really otherwise something > really wrong happened and we should not try to create another type of > context (because these functions only allocate the object if I am not > mistaken). Also if a specific block is built we expect it to work. > But maybe I am missing something ? That's not how I see it. Context specific _new() implementations should fail for example, if it cannot work with the GstGLDisplay type. While that's against the guidelines of _new() operation, gst_gl_context_new is a bit special in that it's just a selector for choosing an appropriate context implementation. Currently the implementations do not fail and you can very easily get into situations where you're trying to create a glx context on wayland without overriding with all of GST_GL_PLATFORM, GST_GL_WINDOW, GST_GL_API env variables. The other case where this would be useful is with application provided GstGLDisplay's for the same reason without having to explicitly override GstGLDisplay::create-context. The defaults should just work™. In your case the same is true, if you only supply GST_GL_WINDOW=x11 on OS X but don't provide GST_GL_PLATFORM, it will attempt to create a CGL context with a X11 display handle and/or create an x11 window which isn't going to work very well. Same for x11/wayland + glx/egl. You can probably also install mesa on win32 and have the same problem.
Review of attachment 308044 [details] [review]: ::: configure.ac @@ +831,3 @@ + case "$host" in + *-*darwin*) + GL_CFLAGS="-Wno-typedef-redefinition" Again, not a fan of disabling the typedef redefinition on osx. Should also hide behind the $HAVE_EGL/$NEED_EGL check like the hunk below. ::: gst-libs/gst/gl/gstglcontext.c @@ +124,3 @@ +#ifdef __APPLE__ + if (!module_opengl && (_get_default_gl_platform () == GST_GL_PLATFORM_EGL)) + module_opengl = g_module_open ("libGL.dylib", G_MODULE_BIND_LAZY); You should probably remove the && _get_default_gl_platform() == to always get the dylib on OS X. The g_module_open call below is targetted to loadable modules which OS X differentiates between that and shared libraries. See http://www.finkproject.org/doc/porting/porting.en.html#shared.lib-and-mod
Created attachment 308051 [details] [review] glcontext: pass display to implementations
This allows for example GST_GL_WINDOW=wayland gst-launch to fail creating a glx context and then create an egl context. Theoretically it should also work for GST_GL_WINDOW=x11 on OS X to fail creating a cgl context and then create a GLX context instead. The same is probably needed for the window implemetations as well.
Created attachment 308055 [details] [review] glwindow: pass display to implementations _new() The window part. The x11 window impl already had something like this :)
(In reply to Matthew Waters from comment #13) > Review of attachment 308044 [details] [review] [review]: > > ::: configure.ac > @@ +831,3 @@ > + case "$host" in > + *-*darwin*) > + GL_CFLAGS="-Wno-typedef-redefinition" > > Again, not a fan of disabling the typedef redefinition on osx. I have no other solution (clang is not happy on osx) except reducing the scope: dnl check if we can include both GL and GLES2 at the same time if test "x$HAVE_GL" = "xyes" -a "x$HAVE_GLES2" = "xyes"; then GL_INCLUDES=" + # ifdef __APPLE_ + # pragma GCC diagnostic push + # pragma GCC diagnostic ignored "-Wtypedef-redefinition" + # endif # ifndef GL_GLEXT_PROTOTYPES ... # endif + # ifdef __APPLE_ + # pragma GCC reset_options + # pragma GCC diagnostic pop + # endif " And do the same in gstglapi.h (also checking we do that only if gl and gles are both going to be included) > Should also > hide behind the $HAVE_EGL/$NEED_EGL check like the hunk below. Right. Also depending if we go for the pragma push/pop it won't be needed. > > ::: gst-libs/gst/gl/gstglcontext.c > @@ +124,3 @@ > +#ifdef __APPLE__ > + if (!module_opengl && (_get_default_gl_platform () == > GST_GL_PLATFORM_EGL)) > + module_opengl = g_module_open ("libGL.dylib", G_MODULE_BIND_LAZY); > > You should probably remove the && _get_default_gl_platform() == to always > get the dylib on OS X. The g_module_open call below is targetted to > loadable modules which OS X differentiates between that and shared > libraries. See > http://www.finkproject.org/doc/porting/porting.en.html#shared.lib-and-mod Ah ok I'll try it
(In reply to Matthew Waters from comment #12) > In your case the same is true, if you only supply GST_GL_WINDOW=x11 on OS X > but don't provide GST_GL_PLATFORM, it will attempt to create a CGL context > with a X11 display handle and/or create an x11 window which isn't going to > work very well. > > Same for x11/wayland + glx/egl. > You can probably also install mesa on win32 and have the same problem. Great, sounds good with your 2 attached patches.
(side note: about a _new implementation that can fail, if you ever want to implement one, implement the interface GInitable, it will make this more binding friendly)
(In reply to Julien Isorce from comment #17) > (In reply to Matthew Waters from comment #13) > > ::: gst-libs/gst/gl/gstglcontext.c > > @@ +124,3 @@ > > +#ifdef __APPLE__ > > + if (!module_opengl && (_get_default_gl_platform () == > > GST_GL_PLATFORM_EGL)) > > + module_opengl = g_module_open ("libGL.dylib", G_MODULE_BIND_LAZY); > > > > You should probably remove the && _get_default_gl_platform() == to always > > get the dylib on OS X. The g_module_open call below is targetted to > > loadable modules which OS X differentiates between that and shared > > libraries. See > > http://www.finkproject.org/doc/porting/porting.en.html#shared.lib-and-mod > > Ah ok I'll try it If I remove the "&& _get_default_gl_platform() ==" then when using cgl it fails with: ERROR: from element /GstPipeline:pipeline0/GstGLImageSinkBin:glimagesinkbin0/GstGLImageSink:sink: glGetString not defined or returned invalid value Maybe it would be better to have gstglcontext in this function to get its platform. It would require to change api of gst_gl_context_default_get_proc_address to pass the gstglcontext pointer or its platform. Not sure how exactly yet because of gst_gl_context_get_proc_address_with_platform deps.
(In reply to Nicolas Dufresne (stormer) from comment #19) > (side note: about a _new implementation that can fail, if you ever want to > implement one, implement the interface GInitable, it will make this more > binding friendly) However that pulls in GIO for one interface which isn't great. (In reply to Julien Isorce from comment #20) > If I remove the "&& _get_default_gl_platform() ==" > then when using cgl it fails with: > ERROR: from element > /GstPipeline:pipeline0/GstGLImageSinkBin:glimagesinkbin0/GstGLImageSink:sink: > glGetString not defined or returned invalid value > > Maybe it would be better to have gstglcontext in this function to get its > platform. > It would require to change api of gst_gl_context_default_get_proc_address to > pass the gstglcontext pointer or its platform. Not sure how exactly yet > because of gst_gl_context_get_proc_address_with_platform deps. Are there two competing opengl libraries installed that you're trying to select between depending on the GL platform in use? I don't think that's going to be easy to solve generically without resorting to hacks that will only work in specific configurations. I'd say leave this to the compile time choice of the opengl/gles2 library to use (already exists). Or find some nice way to bake this into the gl platform specific context implementations rather than overloading the default implementation. By the time you're in the default_get_proc_address, the context specific GstGLContext get_proc_address() has potentially been run (most start with the default implementation first though).
Created attachment 308198 [details] [review] gl: support cgl, egl and glx within a same build Moved dylib loading from gstglcontext.c to egl/gstglcontext_egl.c .
About -Wno-typedef-redefinition it is not needed. I tried to removed it to print out the exact errors but nothing showed-up :)
Review of attachment 308198 [details] [review]: Looks ok. Feel free to push after addressing the comment. ::: gst-libs/gst/gl/egl/gstglcontext_egl.c @@ +641,3 @@ + } +#endif +#if GST_GL_HAVE_GLES2 There's also GST_GL_LIBGLESV2_MODULE_NAME which you want to consider here like the OPENGL case.
mhh sorry something wrong happened when I pushed
Created attachment 308200 [details] [review] gl: support cgl, egl and glx within a same build Here is the patch I pushed commit 5e4b94c1bbf530db49b56915154f4d9fabd09bec Author: Julien Isorce <julien.isorce@gmail.com> Date: Mon Jul 6 00:52:06 2015 +0100 gl: support cgl, egl and glx within a same build On osx, with the same build, gst-launch-1.0 videotestsrc ! glimagesink works with: GST_GL_PLATFORM=egl GST_GL_WINDOW=x11 GST_GL_API=gles2 GST_GL_PLATFORM=egl GST_GL_WINDOW=x11 GST_GL_API=opengl GST_GL_PLATFORM=glx GST_GL_WINDOW=x11 GST_GL_API=opengl GST_GL_PLATFORM=cgl GST_GL_WINDOW=cocoa GST_GL_API=opengl GST_GL_PLATFORM=cgl GST_GL_WINDOW=cocoa GST_GL_API=opengl3 https://bugzilla.gnome.org/show_bug.cgi?id=752743
The commit appears ok in the git repo but not here http://cgit.freedesktop.org/gstreamer/gst-plugins-bad/commit/?id=5e4b94c1bbf530db49b56915154f4d9fabd09bec
commit 3b89d8a23c76aaa7cb79a53a807e87638f9b9917 Author: Matthew Waters <matthew@centricular.com> Date: Fri Jul 24 17:00:27 2015 +1000 glwindow: pass display to implementation's _new() So they have to opportunity to fail if they cannot handle the display connection. https://bugzilla.gnome.org/show_bug.cgi?id=752743 commit dbcae77e02a465fa5813cf1bd97c1264d43b1ffe Author: Matthew Waters <matthew@centricular.com> Date: Fri Jul 24 16:11:38 2015 +1000 glcontext: pass display to implentation's _new() This allows the context to fail creation based on incompatible display type's. e.g. glx context with an wayland display handle. https://bugzilla.gnome.org/show_bug.cgi?id=752743
(In reply to Julien Isorce from comment #27) > The commit appears ok in the git repo but not here > http://cgit.freedesktop.org/gstreamer/gst-plugins-bad/commit/ > ?id=5e4b94c1bbf530db49b56915154f4d9fabd09bec It shows ok now :)