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

[Xiang, Haihao]

On Sat, 2021-08-07 at 01:46 +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>
> ---
>  libavutil/hwcontext.c          | 16 ++++++++++++++++
>  libavutil/hwcontext_internal.h |  6 ++++++
>  libavutil/hwcontext_qsv.c      |  7 ++++++-
>  3 files changed, 28 insertions(+), 1 deletion(-)
> 
> diff --git a/libavutil/hwcontext.c b/libavutil/hwcontext.c
> index d13d0f7c9b..3714ce7553 100644
> --- a/libavutil/hwcontext.c
> +++ b/libavutil/hwcontext.c
> @@ -132,6 +132,7 @@ static void hwdevice_ctx_free(void *opaque, uint8_t *data)
>          ctx->free(ctx);
>  
>      av_buffer_unref(&ctx->internal->source_device);
> +    av_buffer_unref(&ctx->internal->derived_device);
>  
>      av_freep(&ctx->hwctx);
>      av_freep(&ctx->internal->priv);
> @@ -666,6 +667,20 @@ int av_hwdevice_ctx_create_derived_opts(AVBufferRef
> **dst_ref_ptr,
>          tmp_ref = tmp_ctx->internal->source_device;
>      }
>  
> +    tmp_ref = src_ref;
> +    while (tmp_ref) {
> +        tmp_ctx = (AVHWDeviceContext*)tmp_ref->data;
> +        if (tmp_ctx->type == type) {
> +            dst_ref = av_buffer_ref(tmp_ref);
> +            if (!dst_ref) {
> +                ret = AVERROR(ENOMEM);
> +                goto fail;
> +            }
> +            goto done;
> +        }
> +        tmp_ref = tmp_ctx->internal->derived_device;
> +    }
> +
>      dst_ref = av_hwdevice_ctx_alloc(type);
>      if (!dst_ref) {
>          ret = AVERROR(ENOMEM);
> @@ -683,6 +698,7 @@ int av_hwdevice_ctx_create_derived_opts(AVBufferRef
> **dst_ref_ptr,
>                                                              flags);
>              if (ret == 0) {
>                  dst_ctx->internal->source_device = av_buffer_ref(src_ref);
> +                tmp_ctx->internal->derived_device = av_buffer_ref(dst_ref);
>                  if (!dst_ctx->internal->source_device) {

Need to check tmp_ctx->internal->derived_device too. 

>                      ret = AVERROR(ENOMEM);
>                      goto fail;
> diff --git a/libavutil/hwcontext_internal.h b/libavutil/hwcontext_internal.h
> index e6266494ac..cfe525d20c 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;
> +
> +    /**
> +     * A reference to a device context which
> +     * was derived from this device.
> +     */
> +    AVBufferRef *derived_device;
>  };
>  
>  struct AVHWFramesInternal {
> diff --git a/libavutil/hwcontext_qsv.c b/libavutil/hwcontext_qsv.c
> index 08a6e0ee1c..27d96d116f 100644
> --- a/libavutil/hwcontext_qsv.c
> +++ b/libavutil/hwcontext_qsv.c
> @@ -1268,8 +1268,13 @@ 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_device =
> av_buffer_create((uint8_t*)ctx, sizeof(*ctx), 0, ctx, 0);

Need to check the new references here.

> +    }
>  
> -    return qsv_device_derive_from_child(ctx, impl, child_device, 0);
> +    return ret;
>  }
>  
>  const HWContextType ff_hwcontext_type_qsv = {