[vlc-devel] [PATCH] video_output: refactor vlc_gl_api into opengl providers
Alexandre Janniaux
ajanni at videolabs.io
Mon Sep 21 15:22:24 CEST 2020
Hi,
On Mon, Sep 21, 2020 at 02:49:12PM +0200, Thomas Guillem wrote:
>
>
> On Mon, Sep 21, 2020, at 12:58, Alexandre Janniaux wrote:
> > This is part 4/ of the RFC OpenGL filter refactor implementation aiming
> > at reworking the OpenGL providers to be the sole linker to OpenGL
> > libraries.
> >
> > Currently, OpenGL providers are linking the provider libraries like EGL,
> > GLX, or CoreVideo but OpenGL is already sometimes needed to those kind
> > of modules. But in the general case, OpenGL / OpenGL ES must be linked
> > to the clinets modules, using the VLC OpenGL vtable because they need to
> > load it themselves and it's not always done in a dynamic way through
> > GetProcAddress mechanisms.
> >
> > Instead, link OpenGL / OpenGL ES to the provider modules bringing the
> > OpenGL features themselves. In particular, on Apple systems, it avoid
> > the need to link OpenGL framework to every OpenGL users, thus needing
> > special handling in buildsystem, and on other systems exposing both
> > OpenGL and OpenGL ES, it prepares to have different targets for both
> > variants in the future too.
> >
> > It also prepares defining tools for automatically testing shaders even
> > though they would be generated dynamically during runtime, and defining
> > layers for debugging purpose.
> >
> > This is also fixing the current issues on Windows where the functions
> > are dynamically loaded through WGL, but not found instead of being
> > dynamically linked like the Windows environment expects.
> >
> > The last pain point for independence stem from the OpenGL headers
> > themselves which are different between OpenGL and OpenGL ES. Note that
> > this patchset doesn't do the necessary refactor to avoid using `gl`
> > pointer in OpenGL code that would only use the `api` field, so there are
> > multiple places in which some modules actually have access to different
> > api pointer, though it's the same. Given the complexity for testing this
> > code, and the fact it fixes the Windows GL provider too, I evaluated
> > that merging it first was an acceptable tradeoff and it is left for a
> > future refactor.
> >
> > Tested on iOS, MacOSX (macosx.m), Windows and Linux.
> > ---
> > include/vlc_opengl.h | 2 ++
> > modules/video_output/Makefile.am | 26 ++++++++++++++++-------
> > modules/video_output/caopengllayer.m | 25 ++++++++++++++++------
> > modules/video_output/ios.m | 6 ++++++
> > modules/video_output/macosx.m | 11 ++++++++++
> > modules/video_output/opengl/Makefile.am | 16 ++++++--------
> > modules/video_output/opengl/egl.c | 19 +++++++++++++++++
> > modules/video_output/opengl/vout_helper.c | 18 ++++++----------
> > modules/video_output/wayland/Makefile.am | 3 ++-
> > modules/video_output/win32/wgl.c | 11 ++++++++--
> > modules/video_output/xcb/Makefile.am | 3 ++-
> > 11 files changed, 102 insertions(+), 38 deletions(-)
> >
> > diff --git a/include/vlc_opengl.h b/include/vlc_opengl.h
> > index 79e165e72b..05b5c5b242 100644
> > --- a/include/vlc_opengl.h
> > +++ b/include/vlc_opengl.h
> > @@ -53,6 +53,8 @@ struct vlc_gl_t
> > void*(*get_proc_address)(vlc_gl_t *, const char *);
> > void (*destroy)(vlc_gl_t *);
> >
> > + const struct vlc_gl_api *api;
> > +
> > enum {
> > VLC_GL_EXT_DEFAULT,
> > VLC_GL_EXT_EGL,
> > diff --git a/modules/video_output/Makefile.am
> > b/modules/video_output/Makefile.am
> > index c42fdc474a..4ebbcd8d87 100644
> > --- a/modules/video_output/Makefile.am
> > +++ b/modules/video_output/Makefile.am
> > @@ -27,13 +27,17 @@ libglinterop_cvpx_plugin_la_LDFLAGS = $(AM_LDFLAGS)
> > -rpath '$(voutdir)' \
> > -Wl,-framework,Foundation,-framework,CoreVideo
> >
> > if HAVE_OSX
> > -libvout_macosx_plugin_la_SOURCES = video_output/macosx.m
> > +libvout_macosx_plugin_la_SOURCES = video_output/macosx.m \
> > + video_output/opengl/gl_api.c \
> > + video_output/opengl/gl_api.h
> > libvout_macosx_plugin_la_CFLAGS = $(AM_CFLAGS) -DHAVE_GL_CORE_SYMBOLS
> > libvout_macosx_plugin_la_LIBADD = libvlc_opengl.la
> > libvout_macosx_plugin_la_LDFLAGS = $(AM_LDFLAGS) -rpath '$(voutdir)' \
> > -Wl,-framework,OpenGL,-framework,Cocoa
> >
> > -libcaopengllayer_plugin_la_SOURCES = video_output/caopengllayer.m
> > +libcaopengllayer_plugin_la_SOURCES = video_output/caopengllayer.m \
> > + video_output/opengl/gl_api.c \
> > + video_output/opengl/gl_api.h
> > libcaopengllayer_plugin_la_CFLAGS = $(AM_CFLAGS) -DHAVE_GL_CORE_SYMBOLS
> > libcaopengllayer_plugin_la_LIBADD = libvlc_opengl.la
> > libcaopengllayer_plugin_la_LDFLAGS = $(AM_LDFLAGS) -rpath '$(voutdir)' \
> > @@ -47,7 +51,9 @@ if HAVE_IOS
> > libglinterop_cvpx_plugin_la_CFLAGS = $(AM_CFLAGS) -DUSE_OPENGL_ES2
> > endif
> >
> > -libvout_ios_plugin_la_SOURCES = video_output/ios.m
> > +libvout_ios_plugin_la_SOURCES = video_output/ios.m \
> > + video_output/opengl/gl_api.c \
> > + video_output/opengl/gl_api.h
> > libvout_ios_plugin_la_CFLAGS = $(AM_CFLAGS) $(OPENGL_COMMONCFLAGS)
> > -DUSE_OPENGL_ES2
> > libvout_ios_plugin_la_LIBADD = libvlc_opengles.la
> > libvout_ios_plugin_la_LDFLAGS = $(AM_LDFLAGS) -rpath '$(voutdir)' \
> > @@ -143,13 +149,15 @@ libglwin32_plugin_la_SOURCES = \
> > video_output/win32/events.c video_output/win32/events.h \
> > video_output/win32/sensors.cpp \
> > video_output/win32/win32touch.c video_output/win32/win32touch.h
> > -libwgl_plugin_la_SOURCES = video_output/win32/wgl.c
> > +libwgl_plugin_la_SOURCES = video_output/win32/wgl.c \
> > + video_output/opengl/gl_api.c \
> > + video_output/opengl/gl_api.h
> >
> > libglwin32_plugin_la_LIBADD = libchroma_copy.la -lopengl32 -lgdi32
> > $(LIBCOM) -luuid libvlc_opengl.la
> > libwgl_plugin_la_LIBADD = -lopengl32 -lgdi32 libvlc_opengl.la
> >
> > -libglwin32_plugin_la_CFLAGS = $(AM_CFLAGS) $(OPENGL_COMMONCFLAGS)
> > -DHAVE_GL_CORE_SYMBOLS
> > -libwgl_plugin_la_CFLAGS = $(AM_CFLAGS) $(OPENGL_COMMONCFLAGS)
> > +libglwin32_plugin_la_CFLAGS = $(AM_CFLAGS) $(OPENGL_COMMONCFLAGS)
> > +libwgl_plugin_la_CFLAGS = $(AM_CFLAGS) $(OPENGL_COMMONCFLAGS)
> > -DHAVE_GL_CORE_SYMBOLS
> >
> > libglwin32_plugin_la_LDFLAGS = $(AM_LDFLAGS) -rpath '$(voutdir)'
> > libwgl_plugin_la_LDFLAGS = $(AM_LDFLAGS) -rpath '$(voutdir)'
> > @@ -173,7 +181,8 @@ if HAVE_WIN32_DESKTOP
> > vout_LTLIBRARIES += libwinhibit_plugin.la
> > endif
> >
> > -libegl_win32_plugin_la_SOURCES = video_output/opengl/egl.c
> > +libegl_win32_plugin_la_SOURCES = video_output/opengl/egl.c \
> > + video_output/opengl/gl_api.h video_output/opengl/gl_api.c
> > libegl_win32_plugin_la_CPPFLAGS = $(AM_CPPFLAGS) -DUSE_PLATFORM_WIN32=1
> > libegl_win32_plugin_la_CFLAGS = $(AM_CFLAGS) $(EGL_CFLAGS)
> > libegl_win32_plugin_la_LIBADD = $(EGL_LIBS)
> > @@ -208,7 +217,8 @@ endif
> >
> > ### Android ###
> >
> > -libegl_android_plugin_la_SOURCES = video_output/opengl/egl.c
> > +libegl_android_plugin_la_SOURCES = video_output/opengl/egl.c \
> > + video_output/opengl/gl_api.h video_output/opengl/gl_api.c
> > libegl_android_plugin_la_CFLAGS = $(AM_CFLAGS) $(EGL_CFLAGS)
> > -DUSE_PLATFORM_ANDROID=1
> > libegl_android_plugin_la_LIBADD = $(EGL_LIBS)
> >
> > diff --git a/modules/video_output/caopengllayer.m
> > b/modules/video_output/caopengllayer.m
> > index 587817d153..5b17ae9e5e 100644
> > --- a/modules/video_output/caopengllayer.m
> > +++ b/modules/video_output/caopengllayer.m
> > @@ -43,6 +43,7 @@
> > #include "opengl/filter_draw.h"
> > #include "opengl/renderer.h"
> > #include "opengl/vout_helper.h"
> > +#include "opengl/gl_api.h"
> >
> > #define OSX_SIERRA_AND_HIGHER (NSAppKitVersionNumber >= 1485)
> >
> > @@ -94,6 +95,7 @@ struct vout_display_sys_t {
> > VLCCAOpenGLLayer *cgLayer;
> >
> > vlc_gl_t *gl;
> > + struct vlc_gl_api api;
> > vout_display_opengl_t *vgl;
> >
> > vout_display_place_t place;
> > @@ -199,12 +201,23 @@ static int Open (vout_display_t *vd, const
> > vout_display_cfg_t *cfg,
> > glsys->cgLayer = sys->cgLayer;
> >
> > const vlc_fourcc_t *subpicture_chromas;
> > - if (!OpenglLock(sys->gl)) {
> > - sys->vgl = vout_display_opengl_New(fmt,
> > &subpicture_chromas,
> > - sys->gl,
> > &cfg->viewpoint, context);
> > - OpenglUnlock(sys->gl);
> > - } else
> > - sys->vgl = NULL;
> > + sys->vgl = NULL;
> > +
> > + if (OpenglLock(sys->gl) != VLC_SUCCESS)
> > + goto bailout;
> > +
> > + if (vlc_gl_api_Init(&sys->api, sys->gl) != VLC_SUCCESS)
> > + {
> > + msg_Err(vd, "Error while initializing opengl api.");
> > + goto bailout;
> > + }
> > + sys->gl->api = &sys->api;
> > +
> > + sys->vgl = vout_display_opengl_New(fmt, &subpicture_chromas,
> > + sys->gl, &cfg->viewpoint,
> > + context);
> > + OpenglUnlock(sys->gl);
> > +
> > if (!sys->vgl) {
> > msg_Err(vd, "Error while initializing opengl display.");
> > goto bailout;
> > diff --git a/modules/video_output/ios.m b/modules/video_output/ios.m
> > index 07c8355d45..fd6be2face 100644
> > --- a/modules/video_output/ios.m
> > +++ b/modules/video_output/ios.m
> > @@ -48,6 +48,7 @@
> > #import "opengl/filter_draw.h"
> > #import "opengl/renderer.h"
> > #import "opengl/vout_helper.h"
> > +#import "opengl/gl_api.h"
> >
> > /**
> > * Forward declarations
> > @@ -124,6 +125,7 @@ struct vout_display_sys_t
> > VLCOpenGLES2VideoView *glESView;
> >
> > vlc_gl_t *gl;
> > + struct vlc_gl_api api;
> >
> > vout_window_t *embed;
> > };
> > @@ -209,6 +211,10 @@ static int Open(vout_display_t *vd, const
> > vout_display_cfg_t *cfg,
> > if (vlc_gl_MakeCurrent(sys->gl) != VLC_SUCCESS)
> > goto bailout;
> >
> > + if (vlc_gl_api_Init(&sys->api, sys->gl) != VLC_SUCCESS)
> > + goto bailout;
> > + sys->gl->api = &sys->api;
>
> So, you are creating a vlc_gl_api_t using a vlc_gl_t, and then storing it in the vlc_gl_t. This seems weird.
> What about a function like vlc_gl_InitApi(vlc_gl_t *gl) that fetch the api and store it in gl->api. Then you could remove the api pointer in all modules (since they already have a gl pointer).
Why not, but as I've mentioned in the commit message, the current
patch is pretty painful to test (there's at least nine opengl
providers currently, I've skipped some of them in the tests because
they were enough factorized with others to give a reliable result
but it's a huge amount of work to put everything in place).
I'd hope we can move additional cleaning to a latter patchset.
Regards,
--
Alexandre Janniaux
Videolabs
More information about the vlc-devel
mailing list