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

[Xiang, Haihao]

On Tue, 2021-08-10 at 09:52 +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>
> ---
> v2: allow storing multiple derived devices in a device ctx; add checks for oom
>  libavutil/hwcontext.c          | 38 ++++++++++++++++++++++++++++++++++
>  libavutil/hwcontext.h          |  1 +
>  libavutil/hwcontext_internal.h |  6 ++++++
>  libavutil/hwcontext_qsv.c      | 10 ++++++++-
>  4 files changed, 54 insertions(+), 1 deletion(-)
> 
> diff --git a/libavutil/hwcontext.c b/libavutil/hwcontext.c
> index d13d0f7c9b..7f4e541553 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 0;

I prefer to use NULL instead of 0 because the return type is pointer to
AVBufferRef. 

The remaining looks good to me and it really fixes a few issues for me. 

Thanks
Haihao

> +}
> +
>  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 08a6e0ee1c..b26910f95d 100644
> --- a/libavutil/hwcontext_qsv.c
> +++ b/libavutil/hwcontext_qsv.c
> @@ -1268,8 +1268,16 @@ static int qsv_device_create(AVHWDeviceContext *ctx,
> const char *device,
>      child_device = (AVHWDeviceContext*)priv->child_device_ctx->data;
>  
>      impl = choose_implementation(device);
> +    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), 0, 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 = {