[FFmpeg-devel] [PATCH v2] lavc/qsv: adding DX11 support
Mark Thompson
sw at jkqxz.net
Sat Feb 1 16:54:54 EET 2020
On 24/01/2020 19:37, Artem Galin wrote:
> On Fri, 24 Jan 2020 at 00:46, Mark Thompson <sw at jkqxz.net> wrote:
>
>> On 23/01/2020 15:18, Artem Galin wrote:
>>> This enables DX11 support for QSV with higher priority than DX9.
>>> In case of multiple GPUs configuration, DX9 API does not allow to get
>>> access to QSV device in some cases - headless.
>>> Implementation based on DX11 resolves that restriction by enumerating
>> list of available GPUs and finding device with QSV support.
>>>
>>> Signed-off-by: Artem Galin <artem.galin at gmail.com>
>>> ---
>>> libavcodec/qsv.c | 38 ++++----
>>> libavcodec/qsv_internal.h | 5 +
>>> libavcodec/version.h | 2 +-
>>> libavfilter/qsvvpp.c | 19 +---
>>> libavutil/hwcontext_d3d11va.c | 57 +++++++++++-
>>> libavutil/hwcontext_qsv.c | 169 +++++++++++++++++++++++++++++-----
>>> libavutil/hwcontext_qsv.h | 13 ++-
>>> libavutil/version.h | 2 +-
>>> 8 files changed, 242 insertions(+), 63 deletions(-)
>>
>> This should be split into separate commits for the three libraries you
>> touch.
>>
>> These changes are logically one piece of code and do not work
> independently.
I don't understand what you mean by this comment. libavutil does not depend on the other libraries, and incompatible changes to libavutil are not allowed.
>> ...
>>>
>>> +static int d3d11va_device_find_qsv_adapter(AVHWDeviceContext *ctx, UINT
>> creationFlags)
>>> +{
>>> + HRESULT hr;
>>> + IDXGIAdapter *adapter = NULL;
>>> + int adapter_id = 0;
>>> + int vendor_id = 0x8086;
>>> + IDXGIFactory2 *factory;
>>> + hr = mCreateDXGIFactory(&IID_IDXGIFactory2, (void **)&factory);
>>> + while (IDXGIFactory2_EnumAdapters(factory, adapter_id++, &adapter)
>> != DXGI_ERROR_NOT_FOUND)
>>> + {
>>> + ID3D11Device* device = NULL;
>>> + DXGI_ADAPTER_DESC adapter_desc;
>>> +
>>> + hr = mD3D11CreateDevice(adapter, D3D_DRIVER_TYPE_UNKNOWN, NULL,
>> creationFlags, NULL, 0, D3D11_SDK_VERSION, &device, NULL, NULL);
>>> + if (FAILED(hr)) {
>>> + av_log(ctx, AV_LOG_ERROR, "D3D11CreateDevice returned
>> error\n");
>>> + continue;
>>> + }
>>> +
>>> + hr = IDXGIAdapter2_GetDesc(adapter, &adapter_desc);
>>> + if (FAILED(hr)) {
>>> + av_log(ctx, AV_LOG_ERROR, "IDXGIAdapter2_GetDesc returned
>> error\n");
>>> + continue;
>>> + }
>>> +
>>> + if(device)
>>> + ID3D11Device_Release(device);
>>> +
>>> + if (adapter)
>>> + IDXGIAdapter_Release(adapter);
>>> +
>>> + if (adapter_desc.VendorId == vendor_id) {
>>> + IDXGIFactory2_Release(factory);
>>> + return adapter_id - 1;
>>> + }
>>> + }
>>> + IDXGIFactory2_Release(factory);
>>> + return -1;
>>> +}
>>> +
>>> static int d3d11va_device_create(AVHWDeviceContext *ctx, const char
>> *device,
>>> AVDictionary *opts, int flags)
>>> {
>>> @@ -519,7 +559,9 @@ static int d3d11va_device_create(AVHWDeviceContext
>> *ctx, const char *device,
>>> IDXGIAdapter *pAdapter = NULL;
>>> ID3D10Multithread *pMultithread;
>>> UINT creationFlags = D3D11_CREATE_DEVICE_VIDEO_SUPPORT;
>>> + int adapter = -1;
>>> int is_debug = !!av_dict_get(opts, "debug", NULL, 0);
>>> + int is_qsv = !!av_dict_get(opts, "d3d11va_qsv", NULL, 0);
>>
>> The only constraint you actually check here is the vendor ID, right? I
>> think it would make more sense to add code which goes looking for a device
>> given the vendor ID rather than hard-coding a special function doing this
>> specific case in here - compare with how VAAPI does exactly the same thing.
>>
>> Agree to change interface to support given vendor id.
>
>
>> (That is, make "ffmpeg -init_hw_device d3d11va=,vendor=0x8086" do what
>> you would expect rather than hacking in a special libmfx case here.)
>>
>
> Agree that your proposal to have option to choose vendor by given vendor id
> via command line is nice to have optionally and could be added later. This
> patch is about enabling DX11 for qsv at the moment.
The point of adding the option is then to use it in the libmfx code rather than dumping ad-hoc libmfx code in the D3D11 file.
>>> ...
>>> +#endif
>>> #if CONFIG_DXVA2
>>> { MFX_HANDLE_D3D9_DEVICE_MANAGER, AV_HWDEVICE_TYPE_DXVA2,
>> AV_PIX_FMT_DXVA2_VLD },
>>> #endif
>>> @@ -124,18 +131,21 @@ static int qsv_device_init(AVHWDeviceContext *ctx)
>>> int i;
>>>
>>> for (i = 0; supported_handle_types[i].handle_type; i++) {
>>> - err = MFXVideoCORE_GetHandle(hwctx->session,
>> supported_handle_types[i].handle_type,
>>> - &s->handle);
>>> - if (err == MFX_ERR_NONE) {
>>> - s->handle_type =
>> supported_handle_types[i].handle_type;
>>> - s->child_device_type =
>> supported_handle_types[i].device_type;
>>> - s->child_pix_fmt = supported_handle_types[i].pix_fmt;
>>> - break;
>>> + if (supported_handle_types[i].handle_type ==
>> hwctx->handle_type) {
>>> + err = MFXVideoCORE_GetHandle(hwctx->session,
>> supported_handle_types[i].handle_type,
>>> + &s->handle);
>>> + if (err == MFX_ERR_NONE) {
>>> + s->handle_type =
>> supported_handle_types[i].handle_type;
>>> + s->child_device_type =
>> supported_handle_types[i].device_type;
>>> + s->child_pix_fmt =
>> supported_handle_types[i].pix_fmt;
>>> + break;
>>> + }
>>> }
>>> }
>>> if (!s->handle) {
>>> av_log(ctx, AV_LOG_VERBOSE, "No supported hw handle could be
>> retrieved "
>>> "from the session\n");
>>> + return AVERROR_UNKNOWN;
>>
>> Why has this become an error when it wasn't previously?
>>
> I presume previously it was wrong, but never be the case. We can't go
> further if handle is null.
Well, it certainly breaks the software cases.
>>> ...
>>> @@ -492,7 +541,7 @@ static int
>> qsv_init_internal_session(AVHWFramesContext *ctx,
>>>
>>> err = MFXVideoVPP_Init(*session, &par);
>>> if (err != MFX_ERR_NONE) {
>>> - av_log(ctx, AV_LOG_VERBOSE, "Error opening the internal VPP
>> session."
>>> + av_log(ctx, AV_LOG_ERROR, "Error opening the internal VPP
>> session."
>>> "Surface upload/download will not be possible\n");
>>
>> Why is this now an error where it wasn't previously?
>>
> I presume previously it was wrong. It should be an error level.
Why? Surface upload/download is not a required feature.
>>> ...
>>>
>>> for (i = 0; i < hwctx->nb_surfaces; i++) {
>>> #if CONFIG_VAAPI
>>> - if (*(VASurfaceID*)hwctx->surfaces[i].Data.MemId ==
>>> - (VASurfaceID)(uintptr_t)src->data[3])
>>> - break;
>>> + if (AV_HWDEVICE_TYPE_VAAPI == type) {
>>> + if (*(VASurfaceID*)hwctx->surfaces[i].Data.MemId ==
>>> + (VASurfaceID)(uintptr_t)src->data[3])
>>> + break;
>>> + }
>>> +#endif
>>> +#if CONFIG_D3D11VA
>>> + if (AV_HWDEVICE_TYPE_D3D11VA == type) {
>>> + if ((hwctx->texture ==
>> (ID3D11Texture2D*)(uintptr_t)src->data[0]) &&
>>> + ((ID3D11Texture2D*)hwctx->surfaces[i].Data.MemId ==
>>> + (ID3D11Texture2D*)(uintptr_t)src->data[1]))
>>> + break;
>>> + }
>>> #endif
>>> #if CONFIG_DXVA2
>>> - if ((IDirect3DSurface9*)hwctx->surfaces[i].Data.MemId ==
>>> - (IDirect3DSurface9*)(uintptr_t)src->data[3])
>>> - break;
>>> + if (AV_HWDEVICE_TYPE_DXVA2 == type) {
>>> + if ((IDirect3DSurface9*)hwctx->surfaces[i].Data.MemId ==
>>> + (IDirect3DSurface9*)(uintptr_t)src->data[3])
>>> + break;
>>> + }
>>> #endif
>>> }
>>> if (i >= hwctx->nb_surfaces) {
>>> @@ -1074,7 +1167,7 @@ static void qsv_device_free(AVHWDeviceContext *ctx)
>>> av_freep(&priv);
>>> }
>>>
>>> -static mfxIMPL choose_implementation(const char *device)
>>> +static mfxIMPL choose_implementation(const char *device, enum
>> AVHWDeviceType child_device_type)
>>> {
>>> static const struct {
>>> const char *name;
>>> @@ -1103,6 +1196,10 @@ static mfxIMPL choose_implementation(const char
>> *device)
>>> impl = strtol(device, NULL, 0);
>>> }
>>>
>>> + if ( (child_device_type == AV_HWDEVICE_TYPE_D3D11VA) && (impl !=
>> MFX_IMPL_SOFTWARE) ) {
>>> + impl |= MFX_IMPL_VIA_D3D11;
>>> + }
>>
>> If this is needed it probably shouldn't be in this function. This is
>> specifically the string name -> implementation type mapping function.
>>
> In case of DX11, we always have to have MFX_IMPL_HARDWARE_* flag with
> combination with MFX_IMPL_VIA_D3D11. Otherwise it will lead to errors,
> because of unsupported DX11 handle type.
So why is this special for D3D11?
>>> ...
>>> @@ -1226,23 +1335,35 @@ static int qsv_device_create(AVHWDeviceContext
>> *ctx, const char *device,
>>> // possible, even when multiple devices and drivers are
>> available.
>>> av_dict_set(&child_device_opts, "kernel_driver", "i915", 0);
>>> av_dict_set(&child_device_opts, "driver", "iHD", 0);
>>> - } else if (CONFIG_DXVA2)
>>> + } else if (CONFIG_D3D11VA) {
>>> + child_device_type = AV_HWDEVICE_TYPE_D3D11VA;
>>> + av_dict_set(&child_device_opts, "d3d11va_qsv", "enabled", 0);
>>> + } else if (CONFIG_DXVA2) {
>>> child_device_type = AV_HWDEVICE_TYPE_DXVA2;
>>> - else {
>>> + } else {
>>> av_log(ctx, AV_LOG_ERROR, "No supported child device type is
>> enabled\n");
>>> return AVERROR(ENOSYS);
>>> }
>>
>> Allowing a user-set child device type seems like a good idea (see the
>> original patch).
>>
> I will be happy if you share the link to original patch.
I managed to find <http://ixia.jkqxz.net/~mrt/aa6effaae834d9d1734d5d52fff6a461/0001-qsv-Add-support-for-D3D11.patch>. I'm not sure if it's the most recent one though, Maxim might have something different?
>>>
>>> ret = av_hwdevice_ctx_create(&priv->child_device_ctx,
>> child_device_type,
>>> e ? e->value : NULL,
>> child_device_opts, 0);
>>> -
>>> av_dict_free(&child_device_opts);
>>> - if (ret < 0)
>>> - return ret;
>>> + if (ret < 0) {
>>> + if (CONFIG_DXVA2 && (child_device_type ==
>> AV_HWDEVICE_TYPE_D3D11VA)) {
>>> + // in case of d3d11va fail, try one more chance to create
>> device via dxva2
>>> + child_device_type = AV_HWDEVICE_TYPE_DXVA2;
>>> + child_device_opts = NULL;
>>
>> Leaks the dictionary.
>>
>>> + ret = av_hwdevice_ctx_create(&priv->child_device_ctx,
>> child_device_type,
>>> + e ? e->value : NULL, child_device_opts, 0);
>>> + }
>>> + if (ret < 0) {
>>> + return ret;
>>> + }
>>> + }
>>>
>>> child_device = (AVHWDeviceContext*)priv->child_device_ctx->data;
>>>
>>> - impl = choose_implementation(device);
>>> + impl = choose_implementation(device, child_device_type);
>>>
>>> return qsv_device_derive_from_child(ctx, impl, child_device, 0);
>>> }
>>> diff --git a/libavutil/hwcontext_qsv.h b/libavutil/hwcontext_qsv.h
>>> index b98d611cfc..3a322037e5 100644
>>> --- a/libavutil/hwcontext_qsv.h
>>> +++ b/libavutil/hwcontext_qsv.h
>>> @@ -34,6 +34,15 @@
>>> */
>>> typedef struct AVQSVDeviceContext {
>>> mfxSession session;
>>> + /**
>>> + * Need to store actual handle type that session uses
>>> + * MFXVideoCORE_GetHandle() function returns mfxHandleType
>>> + * always equal to MFX_HANDLE_D3D9_DEVICE_MANAGER
>>> + * even when MFX_HANDLE_D3D11_DEVICE was set as handle before by
>>> + * MFXVideoCORE_SetHandle() to mfx session.
>>> + * Fixed already but will be available only with latest driver.
>>> + */
>>> + mfxHandleType handle_type;
>>
>> This incompatibly changes the ABI because you've made this field required
>> where it didn't previously exist.
>>
>> Not having it at all is clearly best, because the session really does know
>> what the handle type is. If there are some broken drivers where the type
>> can't be retrieved maybe we could unconditionally use the existing D3D9
>> code on them, which at least wouldn't regress?
>>
> In fact until now, session always returns DXVA handle type on Windows. Now
> we have to differentiate between DXVA2 and DX11 sessions somehow. MFX
> Implementation with version 1.30 or higher support correct behavior.
> What is your suggestion? Using the D3D9 if not the latest driver even we
> are able to keep the device type?
If there isn't any other way then that sounds like it would work without breaking anything existing.
>>> } AVQSVDeviceContext;
>>>
>>> /**
>>> @@ -42,11 +51,11 @@ typedef struct AVQSVDeviceContext {
>>> typedef struct AVQSVFramesContext {
>>> mfxFrameSurface1 *surfaces;
>>> int nb_surfaces;
>>> -
>>> /**
>>> * A combination of MFX_MEMTYPE_* describing the frame pool.
>>> */
>>> - int frame_type;
>>> + int frame_type;
>>> + void *texture;
>>
>> Why do you need to add this? Each frame already contains the texture
>> pointer in data[0].
>>
>> I added texture member for the cases where AVFrame with data[0] is not
> available.
> It is just a only one case, for example where we use surfaces array:
>
> @@ -452,6 +456,7 @@ static AVBufferRef *qsv_create_mids(AVBufferRef
> *hw_frames_ref)
> for (i = 0; i < nb_surfaces; i++) {
> QSVMid *mid = &mids[i];
> mid->handle = frames_hwctx->surfaces[i].Data.MemId;+
> mid->texture = frames_hwctx->texture;
> mid->hw_frames_ref = hw_frames_ref1;
>
> Previously in DXVA2 MemId was D3D9 tesxture, for DX11 we have to store one
> more parameter index inside the texture.
> Where do you suggest to store texture and index if we have only one member
> MemId?
It's just a pointer, let it point to some other structure.
>> Also, in existing D3D11 code the texture need not be the same for all
>> frames in a frames context (it is when using the default creation with a
>> fixed pool size, but not otherwise). Are you enforcing this because libmfx
>> is unable to work with multiple textures, or is there some other reason?
>> (If the former then I think you need to be more careful in banning D3D11
>> frames contexts of this form earlier, because otherwise you're going to
>> cause weird problems by trying to access higher indices of non-array
>> textures.)
>>
> Could you help me to reproduce this unusual behavior where texture need not
> be the same for all frames in a frames context?
It's the default behaviour of a D3D11 frames context; it only makes a single array texture when you set a fixed size at the start (specifically for the decode use-case which requires a single array texture). So for example d3d11 + hwupload filter will do this.
>>> ...
- Mark
More information about the ffmpeg-devel
mailing list