On 24/01/2020 19:37, Artem Galin wrote: > On Fri, 24 Jan 2020 at 00:46, Mark Thompson <s...@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.ga...@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 _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".