[vlc-devel] [PATCH 1/2] egl: allow window provider to prevent eglTerminate in vlc_gl_t

[Alexandre Janniaux]

Hi,

Thank you for your second review,

While I agree that using a VLC variable for this isn't the best idea
one can have, I sort of disagree with you on using a flag set by
the window to the provider.

Calling eglTerminate without refcounting won't create UB, but will
mark the current objets for deletion. So the only side effect is
other context being invalidated. As you need to share the display
connection for this to be harmful, it will only kill, say, the QML
EGL context, and the MakeCurrent of QML will only fail, but that's
not really the point here.

The window implementation is ought to be independent of OpenGL, and
especially of particular implementation of OpenGL WSI. I think that
reversing this and adding flags in the window implementation to
determine whether EGL should be terminated is even more harmful that
a module variable.

The best solution would have been to never call eglTerminate in
vlc_gl_t modules, but I hardly see how it's doable without moving
the issue to libvlc client or making libvlc client aware of EGL,
which is a no-go.

Maybe I misunderstand what you meant, if so, please tell me so.

Regards,
--
Alexandre Janniaux
Videolabs


On Wed, Sep 25, 2019 at 12:47:03PM +0300, Rémi Denis-Courmont wrote:
> Hi,
>
> I don't think memory management / avoiding UB belongs in a VLC object flag. This looks more or less like action at a distance anti-pattern.
>
> IMO, the flag should be conveyed explicitly by the window to the provider.
>
> Le 25 septembre 2019 12:31:51 GMT+03:00, Alexandre Janniaux <ajanni at videolabs.io> a écrit :
> >If a window provider is using EGL with the same display, the EGL state
> >should be terminated from there instead of from the vlc_gl_t provider.
> >---
> > modules/video_output/opengl/egl.c | 13 ++++++++++++-
> > 1 file changed, 12 insertions(+), 1 deletion(-)
> >
> >diff --git a/modules/video_output/opengl/egl.c
> >b/modules/video_output/opengl/egl.c
> >index 53d770365ad..6d6ab05738f 100644
> >--- a/modules/video_output/opengl/egl.c
> >+++ b/modules/video_output/opengl/egl.c
> >@@ -56,6 +56,8 @@ typedef struct vlc_gl_sys_t
> > #endif
> >     PFNEGLCREATEIMAGEKHRPROC    eglCreateImageKHR;
> >     PFNEGLDESTROYIMAGEKHRPROC   eglDestroyImageKHR;
> >+
> >+    bool prevent_terminate;
> > } vlc_gl_sys_t;
> >
> > static int MakeCurrent (vlc_gl_t *gl)
> >@@ -185,7 +187,11 @@ static void Close(vlc_gl_t *gl)
> >             eglDestroyContext(sys->display, sys->context);
> >         if (sys->surface != EGL_NO_SURFACE)
> >             eglDestroySurface(sys->display, sys->surface);
> >-        eglTerminate(sys->display);
> >+
> >+        /* Kill the egl state only if we own it. */
> >+        if (!sys->prevent_terminate)
> >+            eglTerminate(sys->display);
> >+        eglReleaseThread();
> >     }
> > #ifdef USE_PLATFORM_X11
> >     if (sys->x11 != NULL)
> >@@ -224,6 +230,10 @@ static int Open(vlc_gl_t *gl, const struct gl_api
> >*api,
> >         = CreateWindowSurface;
> >     void *window;
> >
> >+    /* Window providers can prevent the EGL implementation from
> >destroying
> >+     * the associated EGL display. */
> >+    sys->prevent_terminate = var_GetBool(wnd,
> >"egl-prevent-terminate");
> >+
> > #ifdef USE_PLATFORM_X11
> >     sys->x11 = NULL;
> >
> >@@ -319,6 +329,7 @@ static int Open(vlc_gl_t *gl, const struct gl_api
> >*api,
> >     EGLint major, minor;
> >     if (eglInitialize(sys->display, &major, &minor) != EGL_TRUE)
> >         goto error;
> >+
> >     msg_Dbg(obj, "EGL version %s by %s",
> >             eglQueryString(sys->display, EGL_VERSION),
> >             eglQueryString(sys->display, EGL_VENDOR));
> >--
> >2.23.0
> >
> >_______________________________________________
> >vlc-devel mailing list
> >To unsubscribe or modify your subscription options:
> >https://mailman.videolan.org/listinfo/vlc-devel
>
> --
> Envoyé de mon appareil Android avec Courriel K-9 Mail. Veuillez excuser ma brièveté.

> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel