[FFmpeg-devel] [PATCH v3 1/1] avutils/hwcontext: When deriving a hwdevice, search for existing device in both directions
Chen, Wenbin
wenbin.chen at intel.com
Mon Nov 22 08:05:08 EET 2021
> > -----Original Message-----
> > From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of
> > Xiang, Haihao
> > Sent: Monday, October 18, 2021 6:48 AM
> > To: ffmpeg-devel at 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 <softworkz at 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)
This fixes the problem on qsv->vaapi part. This patch helps cartwheel to reduce
2~3 workaround patches. Thanks : )
>
> @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.
>
> Thanks,
> softworkz
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
More information about the ffmpeg-devel
mailing list