19 Nov 2021, 17:24 by softwo...@hotmail.com: > > >> -----Original Message----- >> From: ffmpeg-devel <ffmpeg-devel-boun...@ffmpeg.org> On Behalf Of >> Xiang, Haihao >> Sent: Monday, October 18, 2021 6:48 AM >> To: ffmpeg-devel@ffmpeg.org >> Subject: Re: [FFmpeg-devel] [PATCH v3 1/1] avutils/hwcontext: When >> deriving a hwdevice, search for existing device in both directions >> >> On Mon, 2021-10-11 at 04:19 +0000, Soft Works wrote: >> > The test /libavutil/tests/hwdevice checks that when deriving a >> device >> > from a source device and then deriving back to the type of the >> source >> > device, the result is matching the original source device, i.e. the >> > derivation mechanism doesn't create a new device in this case. >> > >> > Previously, this test was usually passed, but only due to two >> different >> > kind of flaws: >> > >> > 1. The test covers only a single level of derivation (and back) >> > >> > It derives device Y from device X and then Y back to the type of X >> and >> > checks whether the result matches X. >> > >> > What it doesn't check for, are longer chains of derivation like: >> > >> > CUDA1 > OpenCL2 > CUDA3 and then back to OpenCL4 >> > >> > In that case, the second derivation returns the first device (CUDA3 >> == >> > CUDA1), but when deriving OpenCL4, hwcontext.c was creating a new >> > OpenCL4 context instead of returning OpenCL2, because there was no >> link >> > from CUDA1 to OpenCL2 (only backwards from OpenCL2 to CUDA1) >> > >> > If the test would check for two levels of derivation, it would have >> > failed. >> > >> > This patch fixes those (yet untested) cases by introducing forward >> > references (derived_device) in addition to the existing back >> references >> > (source_device). >> > >> > 2. hwcontext_qsv didn't properly set the source_device >> > >> > In case of QSV, hwcontext_qsv creates a source context internally >> > (vaapi, dxva2 or d3d11va) without calling >> av_hwdevice_ctx_create_derived >> > and without setting source_device. >> > >> > This way, the hwcontext test ran successful, but what practically >> > happened, was that - for example - deriving vaapi from qsv didn't >> return >> > the original underlying vaapi device and a new one was created >> instead: >> > Exactly what the test is intended to detect and prevent. It just >> > couldn't do so, because the original device was hidden (= not set >> as the >> > source_device of the QSV device). >> > >> > This patch properly makes these setting and fixes all derivation >> > scenarios. >> > >> > (at a later stage, /libavutil/tests/hwdevice should be extended to >> check >> > longer derivation chains as well) >> > >> > Signed-off-by: softworkz <softwo...@hotmail.com> >> > --- >> > v3: avoid double-release as suggested by Haihao >> > >> > libavutil/hwcontext.c | 38 >> ++++++++++++++++++++++++++++++++++ >> > libavutil/hwcontext.h | 1 + >> > libavutil/hwcontext_internal.h | 6 ++++++ >> > libavutil/hwcontext_qsv.c | 16 ++++++++++---- >> > 4 files changed, 57 insertions(+), 4 deletions(-) >> > >> > diff --git a/libavutil/hwcontext.c b/libavutil/hwcontext.c >> > index 31c7840dba..1a50635018 100644 >> > --- a/libavutil/hwcontext.c >> > +++ b/libavutil/hwcontext.c >> > @@ -122,6 +122,7 @@ static const AVClass hwdevice_ctx_class = { >> > static void hwdevice_ctx_free(void *opaque, uint8_t *data) >> > { >> > AVHWDeviceContext *ctx = (AVHWDeviceContext*)data; >> > + int i; >> > >> > /* uninit might still want access the hw context and the user >> > * free() callback might destroy it, so uninit has to be >> called first */ >> > @@ -132,6 +133,8 @@ static void hwdevice_ctx_free(void *opaque, >> uint8_t *data) >> > ctx->free(ctx); >> > >> > av_buffer_unref(&ctx->internal->source_device); >> > + for (i = 0; i < AV_HWDEVICE_TYPE_NB; i++) >> > + av_buffer_unref(&ctx->internal->derived_devices[i]); >> > >> > av_freep(&ctx->hwctx); >> > av_freep(&ctx->internal->priv); >> > @@ -643,6 +646,26 @@ fail: >> > return ret; >> > } >> > >> > +static AVBufferRef* find_derived_hwdevice_ctx(AVBufferRef >> *src_ref, enum >> > AVHWDeviceType type) >> > +{ >> > + AVBufferRef *tmp_ref; >> > + AVHWDeviceContext *src_ctx; >> > + int i; >> > + >> > + src_ctx = (AVHWDeviceContext*)src_ref->data; >> > + if (src_ctx->type == type) >> > + return src_ref; >> > + >> > + for (i = 0; i < AV_HWDEVICE_TYPE_NB; i++) >> > + if (src_ctx->internal->derived_devices[i]) { >> > + tmp_ref = find_derived_hwdevice_ctx(src_ctx->internal- >> > >derived_devices[i], type); >> > + if (tmp_ref) >> > + return tmp_ref; >> > + } >> > + >> > + return NULL; >> > +} >> > + >> > int av_hwdevice_ctx_create_derived_opts(AVBufferRef **dst_ref_ptr, >> > enum AVHWDeviceType type, >> > AVBufferRef *src_ref, >> > @@ -666,6 +689,16 @@ int >> av_hwdevice_ctx_create_derived_opts(AVBufferRef >> > **dst_ref_ptr, >> > tmp_ref = tmp_ctx->internal->source_device; >> > } >> > >> > + tmp_ref = find_derived_hwdevice_ctx(src_ref, type); >> > + if (tmp_ref) { >> > + dst_ref = av_buffer_ref(tmp_ref); >> > + if (!dst_ref) { >> > + ret = AVERROR(ENOMEM); >> > + goto fail; >> > + } >> > + goto done; >> > + } >> > + >> > dst_ref = av_hwdevice_ctx_alloc(type); >> > if (!dst_ref) { >> > ret = AVERROR(ENOMEM); >> > @@ -687,6 +720,11 @@ int >> av_hwdevice_ctx_create_derived_opts(AVBufferRef >> > **dst_ref_ptr, >> > ret = AVERROR(ENOMEM); >> > goto fail; >> > } >> > + tmp_ctx->internal->derived_devices[type] = >> > av_buffer_ref(dst_ref); >> > + if (!tmp_ctx->internal->derived_devices[type]) { >> > + ret = AVERROR(ENOMEM); >> > + goto fail; >> > + } >> > ret = av_hwdevice_ctx_init(dst_ref); >> > if (ret < 0) >> > goto fail; >> > diff --git a/libavutil/hwcontext.h b/libavutil/hwcontext.h >> > index 04d19d89c2..56077963e6 100644 >> > --- a/libavutil/hwcontext.h >> > +++ b/libavutil/hwcontext.h >> > @@ -37,6 +37,7 @@ enum AVHWDeviceType { >> > AV_HWDEVICE_TYPE_OPENCL, >> > AV_HWDEVICE_TYPE_MEDIACODEC, >> > AV_HWDEVICE_TYPE_VULKAN, >> > + AV_HWDEVICE_TYPE_NB, ///< number of hw device types >> > }; >> > >> > typedef struct AVHWDeviceInternal AVHWDeviceInternal; >> > diff --git a/libavutil/hwcontext_internal.h >> b/libavutil/hwcontext_internal.h >> > index e6266494ac..f6fb67c491 100644 >> > --- a/libavutil/hwcontext_internal.h >> > +++ b/libavutil/hwcontext_internal.h >> > @@ -109,6 +109,12 @@ struct AVHWDeviceInternal { >> > * context it was derived from. >> > */ >> > AVBufferRef *source_device; >> > + >> > + /** >> > + * An array of reference to device contexts which >> > + * were derived from this device. >> > + */ >> > + AVBufferRef *derived_devices[AV_HWDEVICE_TYPE_NB]; >> > }; >> > >> > struct AVHWFramesInternal { >> > diff --git a/libavutil/hwcontext_qsv.c b/libavutil/hwcontext_qsv.c >> > index c18747f7eb..7b559e2b47 100644 >> > --- a/libavutil/hwcontext_qsv.c >> > +++ b/libavutil/hwcontext_qsv.c >> > @@ -223,7 +223,7 @@ static void qsv_frames_uninit(AVHWFramesContext >> *ctx) >> > av_buffer_unref(&s->child_frames_ref); >> > } >> > >> > -static void qsv_pool_release_dummy(void *opaque, uint8_t *data) >> > +static void qsv_release_dummy(void *opaque, uint8_t *data) >> > { >> > } >> > >> > @@ -236,9 +236,9 @@ static AVBufferRef *qsv_pool_alloc(void >> *opaque, size_t >> > size) >> > if (s->nb_surfaces_used < hwctx->nb_surfaces) { >> > s->nb_surfaces_used++; >> > av_buffer_create((uint8_t*)(s->handle_pairs_internal + s- >> > >nb_surfaces_used - 1), >> > - sizeof(*s->handle_pairs_internal), >> > qsv_pool_release_dummy, NULL, 0); >> > + sizeof(*s->handle_pairs_internal), >> > qsv_release_dummy, NULL, 0); >> > return av_buffer_create((uint8_t*)(s->surfaces_internal + >> s- >> > >nb_surfaces_used - 1), >> > - sizeof(*hwctx->surfaces), >> > qsv_pool_release_dummy, NULL, 0); >> > + sizeof(*hwctx->surfaces), >> qsv_release_dummy, >> > NULL, 0); >> > } >> > >> > return NULL; >> > @@ -1528,8 +1528,16 @@ static int >> qsv_device_create(AVHWDeviceContext *ctx, >> > const char *device, >> > child_device = (AVHWDeviceContext*)priv->child_device_ctx- >> >data; >> > >> > impl = choose_implementation(device, child_device_type); >> > + ret = qsv_device_derive_from_child(ctx, impl, child_device, >> 0); >> > + if (ret >= 0) { >> > + ctx->internal->source_device = av_buffer_ref(priv- >> >child_device_ctx); >> > + child_device->internal->derived_devices[ctx->type] = >> > av_buffer_create((uint8_t*)ctx, sizeof(*ctx), qsv_release_dummy, >> ctx, 0); >> > + if (!child_device->internal->derived_devices[ctx->type]) { >> > + return AVERROR(ENOMEM); >> > + } >> > + } >> > >> > - return qsv_device_derive_from_child(ctx, impl, child_device, >> 0); >> > + return ret; >> > } >> > >> > const HWContextType ff_hwcontext_type_qsv = { >> >> LGTM, >> >> -Haihao >> > > @Hendrik: You had some concerns regarding the initial version, which I have > addressed. Could you please check whether it looks good to you now? > > @Wenbin, @Xu: Could you confirm whether this patch eliminates the need for > the other workarounds you currently have in place on cartwheel? > > For making things work like "qsv->vaapi->vulkan->vaapi->qsv pipeline" > (https://github.com/intel-media-ci/cartwheel-ffmpeg/commit/564169857f552d585f827dbc1387b6abf6526139) > > @Lynne, @haasm, @Wu: This patch might also be relevant for working Vulkan > context derivation as it prevents the creation of new hardware contexts > in non-trivial derivation chains. > > I think this patch is essential for properly working with derived hw contexts > and I'd be glad to hear some more thoughts about it. >
This is indeed relevant to working Vulkan derivation, especially with VAAPI. I've hit this limitation before. I've reviewed the patch, it looks good, except a small coding style nit, do not put brackets on one-line statements, look at `if (!child_device->internal->derived_devices[ctx->type]) {`, and you should also add a comment to AV_HWDEVICE_TYPE_NB where it says "Not part of the API, do not use.". Apart from that, I think if no one responds in a day or two, you should push it. _______________________________________________ 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".