From: Anton Khirnov <anton@khirnov.net>
To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
Cc: Haihao Xiang <haihao.xiang@intel.com>, galinart <artem.galin@intel.com>
Subject: Re: [FFmpeg-devel] [PATCH v9 09/10] qsv: use a new method to create mfx session when using oneVPL
Date: Mon, 13 Jun 2022 13:01:42 +0200
Message-ID: <165511810200.13099.5468957818361638884@lain> (raw)
In-Reply-To: <20220602034118.22447-10-haihao.xiang@intel.com>
Quoting Xiang, Haihao (2022-06-02 05:41:17)
> From: Haihao Xiang <haihao.xiang@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@intel.com>
> Signed-off-by: galinart <artem.galin@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
_______________________________________________
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".
next prev parent reply other threads:[~2022-06-13 11:01 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-06-02 3:41 [FFmpeg-devel] [PATCH v9 00/10] make QSV works with the Intel's oneVPL Xiang, Haihao
2022-06-02 3:41 ` [FFmpeg-devel] [PATCH v9 01/10] configure: ensure --enable-libmfx uses libmfx 1.x Xiang, Haihao
2022-06-02 3:41 ` [FFmpeg-devel] [PATCH v9 02/10] configure: fix the check for MFX_CODEC_VP9 Xiang, Haihao
2022-06-02 3:41 ` [FFmpeg-devel] [PATCH v9 03/10] qsv: remove mfx/ prefix from mfx headers Xiang, Haihao
2022-06-02 3:41 ` [FFmpeg-devel] [PATCH v9 04/10] qsv: load user plugin for MFX_VERSION < 2.0 Xiang, Haihao
2022-06-02 3:41 ` [FFmpeg-devel] [PATCH v9 05/10] qsv: build audio related code when " Xiang, Haihao
2022-06-02 3:41 ` [FFmpeg-devel] [PATCH v9 06/10] qsvenc: support multi-frame encode " Xiang, Haihao
2022-06-02 3:41 ` [FFmpeg-devel] [PATCH v9 07/10] qsvenc: support MFX_RATECONTROL_LA_EXT " Xiang, Haihao
2022-06-02 3:41 ` [FFmpeg-devel] [PATCH v9 08/10] qsv: support OPAQUE memory " Xiang, Haihao
2022-06-13 9:40 ` Anton Khirnov
2022-06-16 8:33 ` Xiang, Haihao
2022-06-02 3:41 ` [FFmpeg-devel] [PATCH v9 09/10] qsv: use a new method to create mfx session when using oneVPL Xiang, Haihao
2022-06-13 11:01 ` Anton Khirnov [this message]
2022-06-16 14:58 ` Xiang, Haihao
2022-06-02 3:41 ` [FFmpeg-devel] [PATCH v9 10/10] configure: add --enable-libvpl option Xiang, Haihao
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=165511810200.13099.5468957818361638884@lain \
--to=anton@khirnov.net \
--cc=artem.galin@intel.com \
--cc=ffmpeg-devel@ffmpeg.org \
--cc=haihao.xiang@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Git Inbox Mirror of the ffmpeg-devel mailing list - see https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
This inbox may be cloned and mirrored by anyone:
git clone --mirror https://master.gitmailbox.com/ffmpegdev/0 ffmpegdev/git/0.git
# If you have public-inbox 1.1+ installed, you may
# initialize and index your mirror using the following commands:
public-inbox-init -V2 ffmpegdev ffmpegdev/ https://master.gitmailbox.com/ffmpegdev \
ffmpegdev@gitmailbox.com
public-inbox-index ffmpegdev
Example config snippet for mirrors.
AGPL code for this site: git clone https://public-inbox.org/public-inbox.git