[vlc-devel] [PATCH 6/7] egl_display: improve support detection

[Romain Vimont]

On Wed, Mar 03, 2021 at 07:46:47PM +0200, Rémi Denis-Courmont wrote:
> Le keskiviikkona 3. maaliskuuta 2021, 19.36.39 EET Romain Vimont a écrit :
> > A module egl_display must only provide EGL displays where eglTerminate()
> > can be called, even if other modules are using EGL (typically because it
> > is internally refcounted).
> > 
> > This is always the case on Android, so egl_display was initially only
> > enabled on Android:
> > https://android.googlesource.com/platform/frameworks/native/+/master/opengl/
> > libs/EGL/egl_display.cpp
> > 
> > But this may also be the case on other platforms which support
> > EGL_KHR_display_reference:
> > https://www.khronos.org/registry/EGL/extensions/KHR/EGL_KHR_display_referenc
> > e.txt
> > 
> > Co-authored-by: Alexandre Janniaux <ajanni at videolabs.io>
> > ---
> >  modules/video_output/opengl/Makefile.am       |  2 +-
> >  .../video_output/opengl/egl_display_generic.c | 36 +++++++++++++++++++
> >  2 files changed, 37 insertions(+), 1 deletion(-)
> > 
> > diff --git a/modules/video_output/opengl/Makefile.am
> > b/modules/video_output/opengl/Makefile.am index 3bee347ef0..c7cbdedfb6
> > 100644
> > --- a/modules/video_output/opengl/Makefile.am
> > +++ b/modules/video_output/opengl/Makefile.am
> > @@ -92,7 +92,7 @@ endif # HAVE_GL
> >  libegl_display_generic_plugin_la_SOURCES =
> > video_output/opengl/egl_display_generic.c
> > libegl_display_generic_plugin_la_CPPFLAGS = $(AM_CPPFLAGS) $(EGL_FLAGS)
> > libegl_display_generic_plugin_la_LIBADD = $(EGL_LIBS)
> > -if HAVE_ANDROID
> > +if HAVE_EGL
> >  vout_LTLIBRARIES += libegl_display_generic_plugin.la
> >  endif
> > 
> > diff --git a/modules/video_output/opengl/egl_display_generic.c
> > b/modules/video_output/opengl/egl_display_generic.c index
> > dfa68125c4..cca13beec8 100644
> > --- a/modules/video_output/opengl/egl_display_generic.c
> > +++ b/modules/video_output/opengl/egl_display_generic.c
> > @@ -31,11 +31,47 @@
> > 
> >  #include "egl_display.h"
> > 
> > +static EGLenum GetPlatform(const char *extensions)
> > +{
> > +#ifdef EGL_KHR_platform_x11
> > +    if (vlc_gl_StrHasToken(extensions, "EGL_EXT_platform_x11"))
> > +        return EGL_PLATFORM_X11_KHR;
> > +#endif
> > +
> > +#ifdef EGL_KHR_platform_wayland
> > +    if (vlc_gl_StrHasToken(extensions, "EGL_EXT_platform_wayland"))
> > +        return EGL_PLATFORM_WAYLAND_KHR;
> > +#endif
> > +
> > +    return 0;
> > +}
> > +
> >  static vlc_egl_display_open_fn Open;
> >  static int
> >  Open(struct vlc_egl_display *display)
> >  {
> > +#ifdef __ANDROID__
> > +    /* The default display is refcounted on Android */
> >      display->display = eglGetDisplay(EGL_DEFAULT_DISPLAY);
> > +#elif defined(EGL_KHR_display_reference)
> > +    const char *extensions = eglQueryString(EGL_NO_DISPLAY,
> > EGL_EXTENSIONS); +
> > +    if (!vlc_gl_StrHasToken(extensions, "EGL_KHR_display_reference"))
> > +        return VLC_EGENERIC;
> > +
> > +    EGLenum platform = GetPlatform(extensions);
> > +    if (!platform)
> > +        return VLC_EGENERIC;
> 
> It's nonobvious why you care about the platform here.
> At least it needs an in-line comment IMO.

Because the call to eglGetPlatformDisplay() below expects a platform as
first parameter.

And it is necessary to call eglGetPlatformDisplay() to be able to pass
attributes (to request tracking references).

> > +
> > +    const EGLAttrib attribs[] = {
> > +        EGL_TRACK_REFERENCES_KHR, EGL_TRUE,
> > +        EGL_NONE,
> > +    };
> > +
> > +    display->display =
> > +        eglGetPlatformDisplay(platform, EGL_DEFAULT_DISPLAY, attribs);
> 
> Won't this fail badly with EGL < 1.5 ?

In theory, yes. However, the CI is happy:
https://code.videolan.org/rom1v/vlc/-/pipelines/74458

Do we want to support EGL < 1.5? If so, I'll change that.

Regards