[FFmpeg-devel] [PATCH v3 1/1] avutils/hwcontext: When deriving a hwdevice, search for existing device in both directions
Soft Works
softworkz at hotmail.com
Thu Nov 25 06:40:31 EET 2021
> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of Lynne
> Sent: Thursday, November 25, 2021 5:09 AM
> To: FFmpeg development discussions and patches <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
>
> 25 Nov 2021, 03:58 by softworkz at hotmail.com:
>
> >
> >
> >> -----Original Message-----
> >> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of
> >> Lynne
> >> Sent: Wednesday, November 24, 2021 12:27 PM
> >> To: FFmpeg development discussions and patches <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
> >>
> >> 19 Nov 2021, 17:24 by softworkz at hotmail.com:
> >>
> >> >
> >> >
> >> >> -----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)
> >> >
> >> > @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.
> >>
> >
> > Hi,
> >
> >> 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,
> >>
> >
> > thanks a lot for taking the time.
> >
> >
> >> except a small coding style
> >> nit,
> >> do not put brackets on one-line statements, look at `if
> >> (!child_device->internal->derived_devices[ctx->type]) {`,
> >>
> >
> > Fixed, new patch submitted.
> >
> >> and you should also add a comment to AV_HWDEVICE_TYPE_NB where
> >> it says "Not part of the API, do not use.".
> >>
> >
> > Can't find that place. Anyway I'm not sure whether it matters
> > in this case what it "public" api or not. My code is using it
> > just internally and both live in libavutil.
> >
>
> It's in AVHWDeviceType.
> Internal or not, it's in a public API header. If someone does decide to use
> it, it'll be really bad as we wouldn't be able to add a new hwcontext
> without also adding a new AV_HWDEVICE_TYPE_NB2.
> I'll add the note when I apply it.
Ahh - I had totally forgotten that my patch is adding that member. That's why
I had responded so weird :-)
I had seen the same approach in a number of other places (e.g. AV_PIX_FMT_NB, AV_SAMPLE_FMT_NB), that's why I thought it would be OK.
> >> Apart from that, I think if no one responds in a day or two, you
> >> should
> >> push it.
> >>
> >
> > I have no permission, so I need to wait and hope for somebody
> > feeling responsible or sympathizing with the submitted code. 😉
> >
>
> I'll push it later alongside the Vulkan changes when they're ready.
Perfect, thanks!
More information about the ffmpeg-devel
mailing list