Any comments on updated patch by link below:
https://patchwork.ffmpeg.org/project/ffmpeg/patch/20200306130954.8938-1-artem.ga...@gmail.com/
Thanks,
Artem.
On Sat, 1 Feb 2020 at 14:55, Mark Thompson <s...@jkqxz.net> wrote:
> 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".
_______________________________________________
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".
