[vlc-devel] [PATCH 1/2] interop_dxva2: load extension from vlc_gl_t

[Alexandre Janniaux]

Hi,

On Tue, Feb 16, 2021 at 08:05:44AM +0100, Steve Lhomme wrote:
> On 2021-02-15 19:05, Alexandre Janniaux wrote:
> > The only users of those extensions are interop modules, so only
> > interop_dxva2 for extension related to WGL integration.
> >
> > By moving the loading of those extension to the interop, we can have
> > them working even when the OpenGL platform context is provided by the
> > vglmem (vgl.c) module callbacks, allowing hardware acceleration even
> > when using the libvlc OpenGL callbacks.
> >
> > Filters would likely be using only OpenGL extensions that are already
> > available, but could be using the same mechanism in the worst-case
> > scenario.
> >
> > Fixes #25234
> > ---
> >   modules/video_output/Makefile.am            |  2 +-
> >   modules/video_output/opengl/interop_dxva2.c | 30 ++++++++++++++++++---
> >   2 files changed, 28 insertions(+), 4 deletions(-)
> >
> > diff --git a/modules/video_output/Makefile.am b/modules/video_output/Makefile.am
> > index 375da4b28d..86ed27d85d 100644
> > --- a/modules/video_output/Makefile.am
> > +++ b/modules/video_output/Makefile.am
> > @@ -121,7 +121,7 @@ libdirect3d9_plugin_la_LDFLAGS = $(AM_LDFLAGS) -rpath '$(voutdir)'
> >   libglinterop_dxva2_plugin_la_SOURCES = video_output/opengl/interop_dxva2.c \
> >   	video_output/opengl/interop.h
> >   libglinterop_dxva2_plugin_la_CFLAGS = $(AM_CFLAGS) $(GL_CFLAGS)
> > -libglinterop_dxva2_plugin_la_LIBADD = libd3d9_common.la
> > +libglinterop_dxva2_plugin_la_LIBADD = libd3d9_common.la -lopengl32
> >   if HAVE_WIN32_DESKTOP
> >   vout_LTLIBRARIES += $(LTLIBdirect3d9)
> > diff --git a/modules/video_output/opengl/interop_dxva2.c b/modules/video_output/opengl/interop_dxva2.c
> > index 2b32e93272..225db1d430 100644
> > --- a/modules/video_output/opengl/interop_dxva2.c
> > +++ b/modules/video_output/opengl/interop_dxva2.c
> > @@ -71,6 +71,8 @@ vlc_module_begin ()
> >   vlc_module_end ()
> >   struct wgl_vt {
> > +    PFNWGLGETEXTENSIONSSTRINGEXTPROC     GetExtensionsStringEXT;
> > +    PFNWGLGETEXTENSIONSSTRINGARBPROC     GetExtensionsStringARB;
> >       PFNWGLDXSETRESOURCESHAREHANDLENVPROC DXSetResourceShareHandleNV;
> >       PFNWGLDXOPENDEVICENVPROC             DXOpenDeviceNV;
> >       PFNWGLDXCLOSEDEVICENVPROC            DXCloseDeviceNV;
> > @@ -422,15 +424,37 @@ GLConvOpen(vlc_object_t *obj)
> >           return VLC_EGENERIC;
> >       }
> > -    if (interop->gl->ext != VLC_GL_EXT_WGL || !interop->gl->wgl.getExtensionsString)
> > +    if (interop->gl->surface == NULL)
> >           return VLC_EGENERIC;
> > -    const char *wglExt = interop->gl->wgl.getExtensionsString(interop->gl);
> > +    HDC hGLDC = NULL;
> > +    switch (interop->gl->surface->type)
> > +    {
> > +        case VOUT_WINDOW_TYPE_HWND:
> > +        case VOUT_WINDOW_TYPE_DUMMY:
>
> The wgl version only allowed VOUT_WINDOW_TYPE_HWND. I suppose it is safe
> because there is a check on interop->gl->surface ? If so why limit to the
> type of window at all ?

Indeed, I had the switch because of a previous version of the
patch where I was using GetDC() for the HWND type but it's of
no use now.

> Also if wglGetExtensionsStringEXT is available this is created but will
> never be used at all. May it's easier to handle the
> wglGetExtensionsStringEXT case first and if it's not there try the HDC way ?

wglGetCurrentDC() won't create an object anyway, but checking
that there is a DC current to the thread and bound to an OpenGL
context prevent using the interop when one OpenGL context has
been setup with another API (think ANGLE here for instance).

It does not prevent using it when OpenGL context has been setup
by two different API within the same thread, but I'm not sure
how to handle this and we don't have this case for now since the
callback/the OpenGL are setup in one of our thread anyway, and
it still better than the previous situation where none of the
interop were usable because none of the extensions were loaded
by the vglmem module.

In this particular case, we could just check wglGetCurrentContext
instead and implement a QueryPlatformExtension just like Thomas
did in the past, but that's quite equivalent and this way doesn't
need an API addition.

> > +            hGLDC = wglGetCurrentDC();
> > +            break;
> > +        default: return VLC_EGENERIC;
> > +    }
> > +
> > +    if (hGLDC == NULL)
> > +        return VLC_EGENERIC;
> > +
> > +    struct wgl_vt vt;
> > +    vt.GetExtensionsStringEXT =
> > +        vlc_gl_GetProcAddress(interop->gl, "wglGetExtensionsStringEXT");
> > +    vt.GetExtensionsStringARB =
> > +        vlc_gl_GetProcAddress(interop->gl, "wglGetExtensionsStringARB");
>
> This seems like what the LOAD_EXT macro defined below is doing. Maybe move
> the macro up and use it, for consistency. You already move the struct it's
> writing to anyway.

Not sure we need a macro for two function call TBH, though
casting might be missing.

> > +
> > +    const char *wglExt = NULL;
> > +    if (vt.GetExtensionsStringEXT)
> > +        wglExt = vt.GetExtensionsStringEXT();
> > +    else if (vt.GetExtensionsStringARB)
> > +        wglExt = vt.GetExtensionsStringARB(hGLDC);
> >       if (wglExt == NULL || !vlc_gl_StrHasToken(wglExt, "WGL_NV_DX_interop"))
> >           return VLC_EGENERIC;
> > -    struct wgl_vt vt;
> >   #define LOAD_EXT(name, type) do { \
> >       vt.name = (type) vlc_gl_GetProcAddress(interop->gl, "wgl" #name); \
> >       if (!vt.name) { \
> > --
> > 2.30.1
> >
> > _______________________________________________
> > vlc-devel mailing list
> > To unsubscribe or modify your subscription options:
> > https://mailman.videolan.org/listinfo/vlc-devel
> >
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel