[FFmpeg-devel] [PATCH v9 09/10] qsv: use a new method to create mfx session when using oneVPL
Anton Khirnov
anton at khirnov.net
Mon Jun 13 14:01:42 EEST 2022
Quoting Xiang, Haihao (2022-06-02 05:41:17)
> From: Haihao Xiang <haihao.xiang at intel.com>
>
> In oneVPL, MFXLoad() and MFXCreateSession() are required to create a
> workable mfx session[1]
>
> Add config filters for D3D9/D3D11 session (galinart)
>
> The default device is changed to d3d11va for oneVPL when both d3d11va
> and dxva2 are enabled on Microsoft Windows
>
> This is in preparation for oneVPL support
>
> TODO: Add a config filter for device on Linux when enumerating all of
> available implementations once we may get the corresponding device info
> via a VADisplay handle
>
> [1] https://spec.oneapi.io/versions/latest/elements/oneVPL/source/programming_guide/VPL_prg_session.html#onevpl-dispatcher
>
> Co-authored-by: galinart <artem.galin at intel.com>
> Signed-off-by: galinart <artem.galin at intel.com>
> ---
> libavcodec/qsv.c | 191 ++++++++++--
> libavcodec/qsv_internal.h | 1 +
> libavcodec/qsvdec.c | 10 +
> libavcodec/qsvenc.h | 3 +
> libavcodec/qsvenc_h264.c | 1 -
> libavcodec/qsvenc_hevc.c | 1 -
> libavcodec/qsvenc_jpeg.c | 1 -
> libavcodec/qsvenc_mpeg2.c | 1 -
> libavcodec/qsvenc_vp9.c | 1 -
> libavfilter/qsvvpp.c | 113 +++++++-
> libavfilter/qsvvpp.h | 5 +
> libavfilter/vf_deinterlace_qsv.c | 14 +-
> libavfilter/vf_scale_qsv.c | 12 +-
> libavutil/hwcontext_qsv.c | 478 ++++++++++++++++++++++++++++---
> libavutil/hwcontext_qsv.h | 1 +
Could you please split the API changes into their own patch? That makes
this whole thing easier to understand and review.
> diff --git a/libavutil/hwcontext_qsv.c b/libavutil/hwcontext_qsv.c
> index 21a2a805f8..aaa62f0bcc 100644
> --- a/libavutil/hwcontext_qsv.c
> +++ b/libavutil/hwcontext_qsv.c
> +#if QSV_ONEVPL
> +
> +static int qsv_create_mfx_session(void *ctx,
> + mfxHDL handle,
> + mfxHandleType handle_type,
> + mfxIMPL implementation,
> + mfxVersion *pver,
> + mfxSession *psession,
> + void **ploader)
> +{
> + mfxStatus sts;
> + mfxLoader loader = NULL;
> + mfxSession session = NULL;
> + mfxConfig cfg;
> + mfxVersion ver;
> + mfxVariant impl_value;
> + uint32_t impl_idx = 0;
> +
> + av_log(ctx, AV_LOG_VERBOSE,
> + "Use Intel(R) oneVPL to create MFX session, API version is "
> + "%d.%d, the required implementation version is %d.%d\n",
> + MFX_VERSION_MAJOR, MFX_VERSION_MINOR, pver->Major, pver->Minor);
> +
> + if (handle_type != MFX_HANDLE_VA_DISPLAY &&
> + handle_type != MFX_HANDLE_D3D9_DEVICE_MANAGER &&
> + handle_type != MFX_HANDLE_D3D11_DEVICE) {
> + av_log(ctx, AV_LOG_ERROR,
> + "Invalid MFX device handle\n");
> + return AVERROR(EXDEV);
> + }
> +
> + *psession = NULL;
> + *ploader = NULL;
> + loader = MFXLoad();
> +
> + if (!loader) {
> + av_log(ctx, AV_LOG_ERROR, "Error creating a MFX loader\n");
> + goto fail;
> + }
> +
> + /* Create configurations for implementation */
> + cfg = MFXCreateConfig(loader);
Am I understanding correctly that this config object is attached to the
loader and owned/freed by it?
> +
> + if (!cfg) {
> + av_log(ctx, AV_LOG_ERROR, "Error creating a MFX configuration\n");
> + goto fail;
> + }
> +
> + impl_value.Type = MFX_VARIANT_TYPE_U32;
> + impl_value.Data.U32 = (implementation == MFX_IMPL_SOFTWARE) ?
> + MFX_IMPL_TYPE_SOFTWARE : MFX_IMPL_TYPE_HARDWARE;
> + sts = MFXSetConfigFilterProperty(cfg,
> + (const mfxU8 *)"mfxImplDescription.Impl", impl_value);
> +
> + if (sts != MFX_ERR_NONE) {
> + av_log(ctx, AV_LOG_ERROR, "Error adding a MFX configuration "
> + "property: %d.\n", sts);
> + goto fail;
> + }
> +
> + impl_value.Type = MFX_VARIANT_TYPE_U32;
> +
> + if (MFX_HANDLE_VA_DISPLAY == handle_type)
> + impl_value.Data.U32 = MFX_ACCEL_MODE_VIA_VAAPI;
> + else if (MFX_HANDLE_D3D9_DEVICE_MANAGER == handle_type)
> + impl_value.Data.U32 = MFX_ACCEL_MODE_VIA_D3D9;
> + else
> + impl_value.Data.U32 = MFX_ACCEL_MODE_VIA_D3D11;
> +
> + sts = MFXSetConfigFilterProperty(cfg,
> + (const mfxU8 *)"mfxImplDescription.AccelerationMode", impl_value);
> +
> + if (sts != MFX_ERR_NONE) {
> + av_log(ctx, AV_LOG_ERROR, "Error adding a MFX configuration"
> + "AccelerationMode property: %d.\n", sts);
> + goto fail;
> + }
> +
> + impl_value.Type = MFX_VARIANT_TYPE_U16;
> + impl_value.Data.U16 = 0x8086;
> + sts = MFXSetConfigFilterProperty(cfg,
> + (const mfxU8 *)"mfxExtendedDeviceId.VendorID", impl_value);
Is it necessary to hardcode a specific vendor id?
> +
> + if (sts != MFX_ERR_NONE) {
> + av_log(ctx, AV_LOG_ERROR, "Error adding a MFX configuration"
> + "VendorID property: %d.\n", sts);
> + goto fail;
> + }
> +
> +#if CONFIG_D3D11VA
> + if (MFX_HANDLE_D3D11_DEVICE == handle_type) {
> + if (handle) {
Why not merge the two conditions and save an indentation level?
> + IDXGIAdapter* pDXGIAdapter;
> + DXGI_ADAPTER_DESC adapterDesc;
> + IDXGIDevice* pDXGIDevice = NULL;
> + HRESULT hr;
> + ID3D11Device* device = handle;
> +
> + hr = ID3D11Device_QueryInterface(device, &IID_IDXGIDevice, (void**)&pDXGIDevice);
> +
> + if (SUCCEEDED(hr)) {
> + hr = IDXGIDevice_GetAdapter(pDXGIDevice, &pDXGIAdapter);
> +
> + if (FAILED(hr)) {
> + av_log(ctx, AV_LOG_ERROR, "Error IDXGIDevice_GetAdapter %d\n", hr);
> + goto fail;
> + }
> +
> + hr = IDXGIAdapter_GetDesc(pDXGIAdapter, &adapterDesc);
> +
> + if (FAILED(hr)) {
> + av_log(ctx, AV_LOG_ERROR, "Error IDXGIAdapter_GetDesc %d\n", hr);
> + goto fail;
> + }
> + } else {
> + av_log(ctx, AV_LOG_ERROR, "Error ID3D11Device_QueryInterface %d\n", hr);
> + goto fail;
> + }
> +
> + impl_value.Type = MFX_VARIANT_TYPE_U16;
> + impl_value.Data.U16 = adapterDesc.DeviceId;
> + sts = MFXSetConfigFilterProperty(cfg,
> + (const mfxU8 *)"mfxExtendedDeviceId.DeviceID", impl_value);
> +
> + if (sts != MFX_ERR_NONE) {
> + av_log(ctx, AV_LOG_ERROR, "Error adding a MFX configuration"
> + "DeviceID property: %d.\n", sts);
> + goto fail;
> + }
> +
> + impl_value.Type = MFX_VARIANT_TYPE_PTR;
> + impl_value.Data.Ptr = &adapterDesc.AdapterLuid;
> + sts = MFXSetConfigFilterProperty(cfg,
> + (const mfxU8 *)"mfxExtendedDeviceId.DeviceLUID", impl_value);
> +
> + if (sts != MFX_ERR_NONE) {
> + av_log(ctx, AV_LOG_ERROR, "Error adding a MFX configuration"
> + "DeviceLUID property: %d.\n", sts);
> + goto fail;
> + }
> +
> + impl_value.Type = MFX_VARIANT_TYPE_U32;
> + impl_value.Data.U32 = 0x0001;
> + sts = MFXSetConfigFilterProperty(cfg,
> + (const mfxU8 *)"mfxExtendedDeviceId.LUIDDeviceNodeMask", impl_value);
> +
> + if (sts != MFX_ERR_NONE) {
> + av_log(ctx, AV_LOG_ERROR, "Error adding a MFX configuration"
> + "LUIDDeviceNodeMask property: %d.\n", sts);
> + goto fail;
> + }
> + }
> + }
> +#endif
> +#if CONFIG_DXVA2
> + if (MFX_HANDLE_D3D9_DEVICE_MANAGER == handle_type) {
> + if (handle) {
> + IDirect3DDeviceManager9* devmgr = handle;
> + IDirect3DDevice9Ex *device = NULL;
> + HANDLE device_handle = 0;
> + IDirect3D9Ex *d3d9ex = NULL;
> + LUID luid;
> + D3DDEVICE_CREATION_PARAMETERS params;
> + HRESULT hr = IDirect3DDeviceManager9_OpenDeviceHandle(devmgr, &device_handle);
> +
> + if (SUCCEEDED(hr)) {
> + hr = IDirect3DDeviceManager9_LockDevice(devmgr, device_handle, &device, TRUE);
> +
> + if (FAILED(hr)) {
> + av_log(ctx, AV_LOG_ERROR, "Error LockDevice %d\n", hr);
> + goto fail;
> + }
> +
> + hr = IDirect3DDevice9Ex_GetCreationParameters(device, ¶ms);
> +
> + if (FAILED(hr)) {
> + av_log(ctx, AV_LOG_ERROR, "Error IDirect3DDevice9_GetCreationParameters %d\n", hr);
> + IDirect3DDeviceManager9_UnlockDevice(devmgr, device_handle, FALSE);
> + goto fail;
> + }
> +
> + hr = IDirect3DDevice9Ex_GetDirect3D(device, &d3d9ex);
> +
> + if (FAILED(hr)) {
> + av_log(ctx, AV_LOG_ERROR, "Error IDirect3DDevice9Ex_GetAdapterLUID %d\n", hr);
> + IDirect3DDeviceManager9_UnlockDevice(devmgr, device_handle, FALSE);
> + goto fail;
> + }
> +
> + hr = IDirect3D9Ex_GetAdapterLUID(d3d9ex, params.AdapterOrdinal, &luid);
> +
> + if (FAILED(hr)) {
> + av_log(ctx, AV_LOG_ERROR, "Error IDirect3DDevice9Ex_GetAdapterLUID %d\n", hr);
> + IDirect3DDeviceManager9_UnlockDevice(devmgr, device_handle, FALSE);
> + goto fail;
> + }
> +
> + hr = IDirect3DDeviceManager9_UnlockDevice(devmgr, device_handle, FALSE);
> +
> + if (FAILED(hr)) {
> + av_log(ctx, AV_LOG_ERROR, "Error IDirect3DDeviceManager9_UnlockDevice %d\n", hr);
> + goto fail;
> + }
> +
> + impl_value.Type = MFX_VARIANT_TYPE_PTR;
> + impl_value.Data.Ptr = &luid;
> + sts = MFXSetConfigFilterProperty(cfg,
> + (const mfxU8 *)"mfxExtendedDeviceId.DeviceLUID", impl_value);
> +
> + if (sts != MFX_ERR_NONE) {
> + av_log(ctx, AV_LOG_ERROR, "Error adding a MFX configuration"
> + "DeviceLUID property: %d.\n", sts);
> + goto fail;
> + }
> + } else {
> + av_log(ctx, AV_LOG_ERROR, "Error OpenDeviceHandle %d\n", hr);
> + goto fail;
> + }
> + }
> + }
Move each of these blocks into their own functions, they are logically
separate and having them here makes qsv_create_mfx_session() absurdly
long and hard to read.
This will also allow you to deduplicate all those separate
IDirect3DDeviceManager9_UnlockDevice() calls.
> +#endif
> +#if CONFIG_VAAPI
> + // TODO: add config filter for device
> + // Without device info, oneVPL loads the default implementation which doesn't
> + // matter on a single GPU. But on multiple GPUs, the default implementation might
> + // not work with the given device. We'll add the config filter for device once we
> + // can get device info via a VADisplay handle
> + // if (MFX_HANDLE_VA_DISPLAY == handle_type) {
I think it'd be better to error out here if the handle is provided,
rather than opening something other than what the user requested.
> + //
> + // }
> +#endif
> +
> + impl_value.Type = MFX_VARIANT_TYPE_U32;
> + impl_value.Data.U32 = pver->Version;
> + sts = MFXSetConfigFilterProperty(cfg,
> + (const mfxU8 *)"mfxImplDescription.ApiVersion.Version",
> + impl_value);
> +
> + if (sts != MFX_ERR_NONE) {
> + av_log(ctx, AV_LOG_ERROR, "Error adding a MFX configuration "
> + "property: %d.\n", sts);
> + goto fail;
> + }
> +
> + while (1) {
> + /* Enumerate all implementations */
> + mfxImplDescription *impl_desc;
> +
> + sts = MFXEnumImplementations(loader, impl_idx,
> + MFX_IMPLCAPS_IMPLDESCSTRUCTURE,
> + (mfxHDL *)&impl_desc);
> +
> + /* Failed to find an available implementation */
> + if (sts == MFX_ERR_NOT_FOUND)
> + break;
> + else if (sts != MFX_ERR_NONE) {
> + impl_idx++;
> + continue;
> + }
> +
> + sts = MFXCreateSession(loader, impl_idx, &session);
> + MFXDispReleaseImplDescription(loader, impl_desc);
> +
> + if (sts == MFX_ERR_NONE)
> + break;
> +
> + impl_idx++;
> + }
This loop could also be a separate function.
> +#else
> +
> +static int qsv_create_mfx_session(void *ctx,
> + mfxHDL handle,
> + mfxHandleType handle_type,
> + mfxIMPL implementation,
> + mfxVersion *pver,
> + mfxSession *psession,
> + void **ploader)
> +{
> + mfxVersion ver;
> + mfxStatus sts;
> + mfxSession session = NULL;
> +
> + av_log(ctx, AV_LOG_VERBOSE,
> + "Use Intel(R) Media SDK to create MFX session, API version is "
> + "%d.%d, the required implementation version is %d.%d\n",
> + MFX_VERSION_MAJOR, MFX_VERSION_MINOR, pver->Major, pver->Minor);
> +
> + if (handle_type != MFX_HANDLE_VA_DISPLAY &&
> + handle_type != MFX_HANDLE_D3D9_DEVICE_MANAGER &&
> + handle_type != MFX_HANDLE_D3D11_DEVICE) {
> + av_log(ctx, AV_LOG_ERROR,
> + "Invalid MFX device handle\n");
> + return AVERROR(EXDEV);
> + }
Why is this check here? This function does not even do anything with
handle_type. Same in the other implementation.
> +
> + *ploader = NULL;
> + *psession = NULL;
> + ver = *pver;
> + sts = MFXInit(implementation, &ver, &session);
> +
nit: typically we have no empty lines between a function call and its
associated error check, there also wasn't one in the code you moved here
> @@ -764,6 +1162,9 @@ static int qsv_frames_init(AVHWFramesContext *ctx)
> s->session_download = NULL;
> s->session_upload = NULL;
>
> + s->loader_download = NULL;
> + s->loader_upload = NULL;
> +
> s->session_download_init = 0;
> s->session_upload_init = 0;
>
> @@ -1063,6 +1464,7 @@ static int qsv_internal_session_check_init(AVHWFramesContext *ctx, int upload)
> QSVFramesContext *s = ctx->internal->priv;
> atomic_int *inited = upload ? &s->session_upload_init : &s->session_download_init;
> mfxSession *session = upload ? &s->session_upload : &s->session_download;
> + void **loader = upload ? &s->loader_upload : &s->loader_download;
Do you actually need two separate loaders here? Can the main hwdevice
one be reused? I would expect it to not have any processing state.
> @@ -1629,6 +2011,16 @@ static int qsv_device_create(AVHWDeviceContext *ctx, const char *device,
> }
> } else if (CONFIG_VAAPI) {
> child_device_type = AV_HWDEVICE_TYPE_VAAPI;
> +#if QSV_ONEVPL
> + } else if (CONFIG_D3D11VA) { // Use D3D11 by default if d3d11va is enabled
> + av_log(NULL, AV_LOG_WARNING,
> + "WARNING: defaulting child_device_type to AV_HWDEVICE_TYPE_D3D11VA for "
> + "oneVPL. Please explicitly set child device type via \"-init_hw_device\" "
> + "option if needed.\n");
Any reason not to log to ctx? I know there's an existing av_log here
that logs to NULL, but IMO that is a bug and should be changes. Same for
the other log you're adding below.
Also, is there a strong reason to print a warning to all users?
Presumably if the user didn't specify a device type, they want the
default one, so this message could be verbose level.
> diff --git a/libavutil/hwcontext_qsv.h b/libavutil/hwcontext_qsv.h
> index 42e34d0dda..2485daa899 100644
> --- a/libavutil/hwcontext_qsv.h
> +++ b/libavutil/hwcontext_qsv.h
> @@ -34,6 +34,7 @@
> */
> typedef struct AVQSVDeviceContext {
> mfxSession session;
> + void *loader;
This should be documented - who sets and owns this, what are the API
users supposed to use this for.
--
Anton Khirnov
More information about the ffmpeg-devel
mailing list