From: "Xiang, Haihao" <haihao.xiang-at-intel.com@ffmpeg.org>
To: "ffmpeg-devel@ffmpeg.org" <ffmpeg-devel@ffmpeg.org>
Cc: "Galin, Artem" <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: Thu, 16 Jun 2022 14:58:34 +0000
Message-ID: <55618359b3ca326827a249f106d410ed681960a8.camel@intel.com> (raw)
In-Reply-To: <165511810200.13099.5468957818361638884@lain>
On Mon, 2022-06-13 at 13:01 +0200, Anton Khirnov wrote:
> 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.
Sure, I'll split this patch into smaller patches.
>
> > 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?
Right
> > +
> > + 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?
It is used to ensure the implementation is the expected one.
>
> > +
> > + 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?
I'll change it in the new version.
>
> > + 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.
Thanks, I'll follow your suggestion to change the code.
>
> > +#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.
I'm considering to use libva API to get the device info in the new version.
>
> > + //
> > + // }
> > +#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.
Sure.
>
> > +#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.
I'll remove this redundant code.
>
> > +
> > + *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.
I'll try to use the main hwdevice one for download & upload.
>
> > @@ -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.
>
Thanks for catching it, I'll use ctx instead NULL.
> 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.
User might want to use dxva2 but they don't know the default one is d3d11va for
oneVPL. I'm fine to use verbose level instead.
>
> > 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.
Sure, I'll add some comments.
Many thanks for your careful review and suggestion!
-Haihao
_______________________________________________
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-16 14:58 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
2022-06-16 14:58 ` Xiang, Haihao [this message]
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=55618359b3ca326827a249f106d410ed681960a8.camel@intel.com \
--to=haihao.xiang-at-intel.com@ffmpeg.org \
--cc=artem.galin@intel.com \
--cc=ffmpeg-devel@ffmpeg.org \
/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