GNOME Bugzilla – Bug 784779
gl: public gstgl headers should not include GL headers
Last modified: 2017-08-22 11:21:52 UTC
It should be up to the user to include the gl headers. Currently gstreamer-gl.pc does not have gl.pc in its "Requires:" or "Requires.private".
Created attachment 355315 [details] [review] gl: do not include GL headers in public gstgl headers
Created attachment 355316 [details] [review] pkgconfig: missing GL_CFLAGS in gstreamer-gl-uninstalled.pc.in
Review of attachment 355315 [details] [review]: The gstglfuncs include hunk below is a blocker. ::: gst-libs/gst/gl/gstglformat.c @@ +36,3 @@ + +#include "gstglcontext.h" +#include "gstglfuncs.h" Why the change from #include <> to #include "" ? ::: gst-libs/gst/gl/gstglframebuffer.h @@ +24,2 @@ #include <gst/gl/gstgl_fwd.h> +#include <gst/gl/gstglfuncs.h> Won't this cause the GL headers to be required again? ::: gst-libs/gst/gl/gstglmemorypbo.c @@ +31,3 @@ +#include "gstglcontext.h" +#include "gstglfuncs.h" +#include "gstglutils.h" Why the change from #include <> to #include "" ? ::: tests/check/libs/gstglmemory.c @@ +37,3 @@ +#if defined(EGLint) || defined(EGLBoolean) || defined(EGLAPI) +#error egl headers should not be included +#endif If you want do this, these should be in their own separate test
Review of attachment 355315 [details] [review]: Thx for the review! ::: gst-libs/gst/gl/gstglformat.c @@ +36,3 @@ + +#include "gstglcontext.h" +#include "gstglfuncs.h" Good question, well I hesitated to change it. I saw inconsistencies in https://cgit.freedesktop.org/gstreamer/gst-plugins-bad/tree/gst-libs/gst/gl/gstglbasefilter.c#n21 versus https://cgit.freedesktop.org/gstreamer/gst-plugins-bad/tree/gst-libs/gst/gl/gstgldisplay.c . I pointed these 2 only as example just to show that current state in gstgl is not uniform. I have not counted who win currently between #include <> versus #include "". But in general, within one lib, headers from .c should be included with "". So only use include <> in .h and in .c if it includes from another lib unit. This is also what gstreamer seems to do in general for the few gst libs I checked. ::: gst-libs/gst/gl/gstglframebuffer.h @@ +24,2 @@ #include <gst/gl/gstgl_fwd.h> +#include <gst/gl/gstglfuncs.h> My mistake, thx! ::: gst-libs/gst/gl/gstglmemorypbo.c @@ +31,3 @@ +#include "gstglcontext.h" +#include "gstglfuncs.h" +#include "gstglutils.h" Ditto. ::: tests/check/libs/gstglmemory.c @@ +37,3 @@ +#if defined(EGLint) || defined(EGLBoolean) || defined(EGLAPI) +#error egl headers should not be included +#if defined(GLint) || defined(GLAPI) || defined(GL_GLEXT_VERSION) You are right, I will make a dedicated test for it.
Comment on attachment 355316 [details] [review] pkgconfig: missing GL_CFLAGS in gstreamer-gl-uninstalled.pc.in commit e21375d40caaa715779d71c628160c749f2a222d Author: Julien Isorce <jisorce@oblong.com> Date: Fri Jul 7 16:33:42 2017 +0100 pkgconfig: missing GL_CFLAGS in gstreamer-gl-uninstalled.pc.in Already present in gstreamer-gl.pc.in https://bugzilla.gnome.org/show_bug.cgi?id=784779
Created attachment 355741 [details] [review] gl: do not include GL headers in public gstgl headers Addressed all remarks. The test could have been mostly empty and just include some headers with the define checks. But I added some constructors checks. Also it is missing checks for other platforms and window systems. For example the test currently does not include gstglcontext_glx.h. But this can be done later. The most important part was the 'common' gstgl headers.
Review of attachment 355741 [details] [review]: Ok there seems to a mismatch on the expected behaviour. gst/gl/gl.h is the top-level header for everybody to include and is also the single include used for gobject-introspection as #include-ing random gstgl headers has gotten us into trouble before (the glmemory includes are specially crafted) so changing that behaviour to require this scenario for not using GL types/functions probably isn't wise. I think the best route is that gst/gl/gstglfuncs.h should not be #include-d in gst/gl/gl.h which probably also has the effect of removing most of the header changes introduced by this patch. gst/gl/gstglfuncs.h should only be #include-d in places where we need the actual GL types/enums/functions/etc in addition to the gst/gl/gl.h #include. If we want backwards compatibility, we could follow mesa's GL_GLEXT_LEGACY/GL_GLEXT_PROTOTYPES defines. Personally, I don't think it's necessary but I could be wrong. The added typedef's are also problematic if the type doesn't match the system provided headers and/or GL/gl.h or GLES2/gl2.h aren't in system paths. The added test looks good. ::: gst-libs/gst/gl/egl/gsteglimage.h @@ +35,3 @@ #define GST_EGL_IMAGE(obj) (GST_EGL_IMAGE_CAST(obj)) +typedef void *EGLImageKHR; This may cause redefinition type warnings. What's the plan for dealing with these? ::: gst-libs/gst/gl/egl/gstglcontext_egl.h @@ +46,3 @@ +typedef void *EGLSurface; +typedef void *EGLContext; + This may cause redefinition type warnings unfortunately. What's the plan for dealing with these? ::: gst-libs/gst/gl/egl/gstgldisplay_egl.h @@ +39,3 @@ +typedef void *EGLDisplay; + This may cause redefinition type warnings. What's the plan for dealing with these?
Created attachment 356520 [details] [review] gl: remove include of gstglfuncs.h from gst/gl/gl.h
Comment on attachment 356520 [details] [review] gl: remove include of gstglfuncs.h from gst/gl/gl.h Ok, this looks good to me now :)
(In reply to Matthew Waters (ystreet00) from comment #7) > Review of attachment 355741 [details] [review] [review]: > > Ok there seems to a mismatch on the expected behaviour. gst/gl/gl.h is the > top-level header for everybody to include and is also the single include > used for gobject-introspection as #include-ing random gstgl headers has > gotten us into trouble before (the glmemory includes are specially crafted) > so changing that behaviour to require this scenario for not using GL > types/functions probably isn't wise. > > I think the best route is that gst/gl/gstglfuncs.h should not be #include-d > in gst/gl/gl.h Done in patch i just attached patch "gl: remove include of gstglfuncs.h from gst/gl/gl.h" I keep it split so that we can see the diff. But the plan is to squash it. > The added typedef's are also problematic if the type doesn't match the > system provided headers and/or GL/gl.h or GLES2/gl2.h aren't in system paths. typedef void *EGLImageKHR; typedef void *EGLSurface; typedef void *EGLContext; typedef void *EGLDisplay; Yup I am not sure, note that this is only for egl and are not platform dependent, i.e. not in eglplatform.h . So this should be always void* . > +typedef void *EGLImageKHR; > > This may cause redefinition type warnings. What's the plan for dealing with > these? The best I can do is to guard it with #ifndef / #endif > The added test looks good. Cool! \o/
Created attachment 356716 [details] [review] Revert some unneeded changes from initial patch "gl: do not include GL headers in public gstgl headers" This patch revert all unecessary changes from "gl: do not include GL headers in public gstgl headers", at least in gst-plugins-bad/ext/gl. But in gst-libs/gst/gl I keept some of them because they really make it cleaner. Also a client app might want to *not* include all gstgl headers by only including gstglcontext.h for example.
Review of attachment 356716 [details] [review]: This looks fine however: (In reply to Julien Isorce from comment #11) > Also a client app might want to *not* include all gstgl headers by only > including gstglcontext.h for example. This is explicitly a non-goal and is not something we are supporting.
(In reply to Matthew Waters (ystreet00) from comment #12) > Review of attachment 356716 [details] [review] [review]: > > This looks fine however: Great! > > (In reply to Julien Isorce from comment #11) > > Also a client app might want to *not* include all gstgl headers by only > > including gstglcontext.h for example. > > This is explicitly a non-goal and is not something we are supporting. Ack
Created attachment 356747 [details] [review] gl: do not include GL headers in public gstgl headers Squashed all together. Thx Matthew for the reviews and suggestions. Last missing bit to resolve is about the typedef.
Created attachment 357732 [details] [review] gl: do not include GL headers in public gstgl headers Expose gpointer instead of typedef redefinition as discussed offline.
Review of attachment 357732 [details] [review]: Looks ok to me. Feel free to push after changing the comment. Are you going to look into the other platform-specific headers? ::: tests/check/libs/gstglheaders.c @@ +26,3 @@ + +/* This test check that public gstgl headers does not include any + * GL headers. Except for gst/gl/gstglfuncs.h and gst/gl/gl.h */ Comment is not quite correct anymore. gst/gl/gl.h is included
(In reply to Matthew Waters (ystreet00) from comment #16) > Review of attachment 357732 [details] [review] [review]: > > Looks ok to me. Feel free to push after changing the comment. > > Are you going to look into the other platform-specific headers? Like for example XLib.h included from gstgl_window_x11.h ? not any time soon so feel free to go ahead. > > ::: tests/check/libs/gstglheaders.c > @@ +26,3 @@ > + > +/* This test check that public gstgl headers does not include any > + * GL headers. Except for gst/gl/gstglfuncs.h and gst/gl/gl.h */ > > Comment is not quite correct anymore. > > gst/gl/gl.h is included Done. Thx for the review.
Comment on attachment 357732 [details] [review] gl: do not include GL headers in public gstgl headers commit 2fd84a6c867641b53529de727ac72549b299a6f8 Author: Julien Isorce <jisorce@oblong.com> Date: Fri Jul 7 16:15:12 2017 +0100 gl: do not include GL headers in public gstgl headers Except for gst/gl/gstglfuncs.h It is up to the client app to include these headers. It is coherent with the fact that gstreamer-gl.pc does not require any egl.pc/gles.pc. I.e. it is the responsability of the app to search these headers within its build setup. For example gstreamer-vaapi includes explicitly EGL/egl.h and search for it in its configure.ac. For example with this patch, if an app includes the headers gst/gl/egl/gstglcontext_egl.h gst/gl/egl/gstgldisplay_egl.h gst/gl/egl/gstglmemoryegl.h it will *no longer* automatically include EGL/egl.h and GLES2/gl2.h. Which is good because the app might want to use the gstgl api only without the need to bother about gl headers. Also added a test: cd tests/check && make libs/gstglheaders.check https://bugzilla.gnome.org/show_bug.cgi?id=784779
This appears to have caused some build breakage: http://build.gnome.org/continuous/buildmaster/builds/2017/08/22/19/build/log-gst-plugins-bad.txt