[FFmpeg-devel,v3,1/1] avutils/hwcontext: When deriving a hwdevice, search for existing device in both directions

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@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(-)

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@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

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@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 <softworkz@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.

Thanks,
softworkz

> > -----Original Message-----
> > From: ffmpeg-devel <ffmpeg-devel-bounces@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 <softworkz@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@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

19 Nov 2021, 17:24 by softworkz@hotmail.com:

>
>
>> -----Original Message-----
>> From: ffmpeg-devel <ffmpeg-devel-bounces@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 <softworkz@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.