Re: [FFmpeg-devel] [PATCH v7 09/10] qsv: use a new method to create mfx session when using oneVPL

[Hendrik Leppkes <h.leppkes@gmail.com>]

On Fri, Mar 11, 2022 at 2:43 PM Xiang, Haihao
<haihao.xiang-at-intel.com@ffmpeg.org> wrote:
>
> On Fri, 2022-03-11 at 09:35 +0100, Hendrik Leppkes wrote:
> > On Fri, Mar 11, 2022 at 9:18 AM Xiang, Haihao
> > <haihao.xiang-at-intel.com@ffmpeg.org> wrote:
> > > diff --git a/libavutil/hwcontext_d3d11va.c b/libavutil/hwcontext_d3d11va.c
> > > index 8ab96bad25..e0e820f164 100644
> > > --- a/libavutil/hwcontext_d3d11va.c
> > > +++ b/libavutil/hwcontext_d3d11va.c
> > > @@ -525,6 +525,13 @@ static void d3d11va_device_uninit(AVHWDeviceContext
> > > *hwdev)
> > >      }
> > >  }
> > >
> > > +static void d3d11va_device_free(AVHWDeviceContext *ctx)
> > > +{
> > > +    AVD3D11VADeviceContext *hwctx = ctx->hwctx;
> > > +
> > > +    av_free(hwctx->device_name);
> > > +}
> > > +
> > >  static int d3d11va_device_create(AVHWDeviceContext *ctx, const char
> > > *device,
> > >                                   AVDictionary *opts, int flags)
> > >  {
> > > @@ -537,6 +544,8 @@ static int d3d11va_device_create(AVHWDeviceContext *ctx,
> > > const char *device,
> > >      int is_debug       = !!av_dict_get(opts, "debug", NULL, 0);
> > >      int ret;
> > >
> > > +    ctx->free = d3d11va_device_free;
> > > +
> > >      // (On UWP we can't check this.)
> > >  #if !HAVE_UWP
> > >      if (!LoadLibrary("d3d11_1sdklayers.dll"))
> > > @@ -561,6 +570,10 @@ static int d3d11va_device_create(AVHWDeviceContext
> > > *ctx, const char *device,
> > >              if (FAILED(IDXGIFactory2_EnumAdapters(pDXGIFactory, adapter,
> > > &pAdapter)))
> > >                  pAdapter = NULL;
> > >              IDXGIFactory2_Release(pDXGIFactory);
> > > +
> > > +            device_hwctx->device_name = av_strdup(device);
> > > +            if (!device_hwctx->device_name)
> > > +                return AVERROR(ENOMEM);
> > >          }
> > >      }
> > >
> > > diff --git a/libavutil/hwcontext_d3d11va.h b/libavutil/hwcontext_d3d11va.h
> > > index 77d2d72f1b..41a315b9e6 100644
> > > --- a/libavutil/hwcontext_d3d11va.h
> > > +++ b/libavutil/hwcontext_d3d11va.h
> > > @@ -94,6 +94,11 @@ typedef struct AVD3D11VADeviceContext {
> > >      void (*lock)(void *lock_ctx);
> > >      void (*unlock)(void *lock_ctx);
> > >      void *lock_ctx;
> > > +
> > > +    /**
> > > +     * The string for the used adapter
> > > +     */
> > > +    char *device_name;
> > >  } AVD3D11VADeviceContext;
> > >
> > >  /**
> > > diff --git a/libavutil/hwcontext_dxva2.c b/libavutil/hwcontext_dxva2.c
> > > index 53d00fa815..6967357093 100644
> > > --- a/libavutil/hwcontext_dxva2.c
> > > +++ b/libavutil/hwcontext_dxva2.c
> > > @@ -431,6 +431,7 @@ static void dxva2_device_free(AVHWDeviceContext *ctx)
> > >          dlclose(priv->dxva2lib);
> > >
> > >      av_freep(&ctx->user_opaque);
> > > +    av_free(hwctx->device_name);
> > >  }
> > >
> > >  static int dxva2_device_create9(AVHWDeviceContext *ctx, UINT adapter)
> > > @@ -571,6 +572,13 @@ static int dxva2_device_create(AVHWDeviceContext *ctx,
> > > const char *device,
> > >          return AVERROR_UNKNOWN;
> > >      }
> > >
> > > +    if (device) {
> > > +        hwctx->device_name = av_strdup(device);
> > > +
> > > +        if (!hwctx->device_name)
> > > +            return AVERROR(ENOMEM);
> > > +    }
> > > +
> > >      return 0;
> > >  }
> > >
> > > diff --git a/libavutil/hwcontext_dxva2.h b/libavutil/hwcontext_dxva2.h
> > > index e1b79bc0de..253ddbed51 100644
> > > --- a/libavutil/hwcontext_dxva2.h
> > > +++ b/libavutil/hwcontext_dxva2.h
> > > @@ -38,6 +38,10 @@
> > >   */
> > >  typedef struct AVDXVA2DeviceContext {
> > >      IDirect3DDeviceManager9 *devmgr;
> > > +    /**
> > > +     * The string for the used adapter
> > > +     */
> > > +    char *device_name;
> > >  } AVDXVA2DeviceContext;
> > >
> > >  /**
> >
> > Why are these device names required? I would think deriving a child
> > device would use the actual device, eg. ID3D11Device or
> > IDirect3DDeviceManager9 (and whatever for VAAPI), and not some string
> > (that may or may not even be set).
> > It feels quite a bit icky to store these in the context just for qsv
> > to do... what with?
>
> Yes, it is a little ugly here. MediaSDK or oneVPL application creates mfx
> session and the device (dxva2, d3d11va or vaapi), then pass this device to the
> SDK through MFXVideoCORE_SetHandle(). implementation is introduced in oneVPL (
> https://spec.oneapi.io/versions/latest/elements/oneVPL/source/API_ref/VPL_disp_api_struct.html#structmfx_impl_description
> ) and user must select an available implementation before the creation of mfx
> session, however the device handle is unknown in the SDK when selecting an
> available implementation, the SDK provides a method to select implementation via
> the given adapter (on Windows) or DRI device node (on Linux). The default
> implementation will be selected if child device name is unknown.
>

First of all, whoever made that API should get a stern message.
Expecting to properly interoperate with the dominant platform APIs
should be a primary goal, and it sounds like it was somehow shoehorned
in after the fact.

For D3D11 for example, you can get the IDXGIAdapter a device was
created from, isn't there enough information in there to pass-on
without storing a string field?
IDXGIAdapter::GetDesc has tons of identification information to
identify the device in use.

D3D9 probably has something similar, haven't checked right now.

- Hendrik
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".