diff mbox series

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

Message ID MN2PR04MB59818D411FD9040330ADDD37BAF79@MN2PR04MB5981.namprd04.prod.outlook.com
State New
Headers show
Series [FFmpeg-devel,v2] avutils/hwcontext: When deriving a hwdevice, search for existing device in both directions
Related show

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished
andriy/PPC64_make success Make finished
andriy/PPC64_make_fate success Make fate finished

Commit Message

Soft Works Aug. 10, 2021, 9:52 a.m. UTC
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>
---
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(-)

Comments

Soft Works Aug. 11, 2021, 5:22 p.m. UTC | #1
> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Soft Works
> Sent: Tuesday, 10 August 2021 11:53
> To: ffmpeg-devel@ffmpeg.org
> Subject: [FFmpeg-devel] [PATCH v2] avutils/hwcontext: When deriving a
> hwdevice, search for existing device in both directions
> 

[..]

> Signed-off-by: softworkz <softworkz@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

Now, that Haihao's patch has been merged,
(https://patchwork.ffmpeg.org/project/ffmpeg/list/?series=4532)

this patch is required to make all related derivation scenarios
work, and for some that are working, it will avoid duplicate 
instantiation of hw device contexts.

This version of the patch addresses the (valid) concerns
from @Hendrik by storing multiple derived device contexts
in cases where more than one device is derived from another
device.

It also adds the missing checks as pointed out by Haihao.

Would be great when someone could take a look at the code.


The combination of this and Haihao's patch is quite a milestone
(in the QSV area) as it will enable several scenarios that 
haven't been working before.

From a quick lock, this will also close the following tickets:


openCL cannot be run simultaneously with h264_qsv
https://trac.ffmpeg.org/ticket/6077 

Intel QSV: "No device available for encoder" message is given
https://trac.ffmpeg.org/ticket/6492 

overlay_qsv fails to create qsvvpp
https://trac.ffmpeg.org/ticket/7808 

overlay_qsv cannot burn graphic subtitles on windows
https://trac.ffmpeg.org/ticket/8609 

overlay_qsv fails with "Inputs with different underlying QSV 
devices are forbidden"
https://trac.ffmpeg.org/ticket/8995


Kind regards,
softworkz
Haihao Xiang Aug. 12, 2021, 8:04 a.m. UTC | #2
On Wed, 2021-08-11 at 17:22 +0000, Soft Works wrote:
> > -----Original Message-----
> > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> > Soft Works
> > Sent: Tuesday, 10 August 2021 11:53
> > To: ffmpeg-devel@ffmpeg.org
> > Subject: [FFmpeg-devel] [PATCH v2] avutils/hwcontext: When deriving a
> > hwdevice, search for existing device in both directions
> > 
> 
> [..]
> 
> > Signed-off-by: softworkz <softworkz@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
> 
> Now, that Haihao's patch has been merged,
> (https://patchwork.ffmpeg.org/project/ffmpeg/list/?series=4532)
> 
> this patch is required to make all related derivation scenarios
> work, and for some that are working, it will avoid duplicate 
> instantiation of hw device contexts.
> 
> This version of the patch addresses the (valid) concerns
> from @Hendrik by storing multiple derived device contexts
> in cases where more than one device is derived from another
> device.
> 
> It also adds the missing checks as pointed out by Haihao.
> 
> Would be great when someone could take a look at the code.
> 

I will give a try and update you when done.

Thanks
Haihao

> The combination of this and Haihao's patch is quite a milestone
> (in the QSV area) as it will enable several scenarios that 
> haven't been working before.
> 
> From a quick lock, this will also close the following tickets:
> 
> 
> openCL cannot be run simultaneously with h264_qsv
> https://trac.ffmpeg.org/ticket/6077 
> 
> Intel QSV: "No device available for encoder" message is given
> https://trac.ffmpeg.org/ticket/6492 
> 
> overlay_qsv fails to create qsvvpp
> https://trac.ffmpeg.org/ticket/7808 
> 
> overlay_qsv cannot burn graphic subtitles on windows
> https://trac.ffmpeg.org/ticket/8609 
> 
> overlay_qsv fails with "Inputs with different underlying QSV 
> devices are forbidden"
> https://trac.ffmpeg.org/ticket/8995
> 
> 
> Kind regards,
> 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".
Haihao Xiang Aug. 13, 2021, 5:21 a.m. UTC | #3
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@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 = {
Haihao Xiang Aug. 13, 2021, 6:29 a.m. UTC | #4
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)
> 

This change causes a regression when running the command below:

$> ffmpeg -y -hwaccel qsv -c:v h264_qsv -i input.h264 -filter_complex
"split[s0][s1]" -map "[s0]" -c:v h264_qsv out0.h264 -map "[s1]" -c:v h264_qsv
out1.h264

...

video:143kB audio:0kB subtitle:0kB other streams:0kB global headers:0kB muxing
overhead: unknown
corrupted size vs. prev_size in fastbins
Aborted

Thanks
Haihao


> Signed-off-by: softworkz <softworkz@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;
> +}
> +
>  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 = {
Haihao Xiang Aug. 19, 2021, 7:36 a.m. UTC | #5
On Fri, 2021-08-13 at 06:29 +0000, Xiang, Haihao wrote:
> 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)
> > 
> 
> This change causes a regression when running the command below:
> 
> $> ffmpeg -y -hwaccel qsv -c:v h264_qsv -i input.h264 -filter_complex
> "split[s0][s1]" -map "[s0]" -c:v h264_qsv out0.h264 -map "[s1]" -c:v h264_qsv
> out1.h264
> 
> ...
> 
> video:143kB audio:0kB subtitle:0kB other streams:0kB global headers:0kB muxing
> overhead: unknown
> corrupted size vs. prev_size in fastbins
> Aborted
> 

Hi Softworks,

+        child_device->internal->derived_devices[ctx->type] =
av_buffer_create((uint8_t*)ctx, sizeof(*ctx), 0, ctx, 0);

The above change introduces a new AVBufferRef for ctx. The first AVBufferRef for
ctx is created when function av_hwdevice_ctx_alloc is called. So there are two
different AVBufferRefs referring to the same ctx, then ctx will be double-freed 

The change below is a bit ugly, but it may fix this double-free issue. 

+static void qsv_ctx_free(void *opaque, uint8_t *ctx)
+{
+    // Do nothing here
+    // ctx is freed in hwdevice_ctx_free
+}
+
 static int qsv_device_create(AVHWDeviceContext *ctx, const char *device,
                              AVDictionary *opts, int flags)
 {
@@ -1271,7 +1277,7 @@ static int qsv_device_create(AVHWDeviceContext *ctx, const
char *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);
+        child_device->internal->derived_devices[ctx->type] =
av_buffer_create((uint8_t*)ctx, sizeof(*ctx), qsv_ctx_free, ctx, 0);
         if (!child_device->internal->derived_devices[ctx->type]) {
             return AVERROR(ENOMEM);
         }


Thanks
Haihao
Soft Works Aug. 19, 2021, 7:51 a.m. UTC | #6
> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Xiang,
> Haihao
> Sent: Thursday, 19 August 2021 09:37
> To: ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH v2] avutils/hwcontext: When deriving a
> hwdevice, search for existing device in both directions
> 
> On Fri, 2021-08-13 at 06:29 +0000, Xiang, Haihao wrote:
> > 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.
> > >

[..]

> >
> > video:143kB audio:0kB subtitle:0kB other streams:0kB global headers:0kB
> muxing
> > overhead: unknown
> > corrupted size vs. prev_size in fastbins
> > Aborted
> >
> 
> Hi Softworks,
> 
> +        child_device->internal->derived_devices[ctx->type] =
> av_buffer_create((uint8_t*)ctx, sizeof(*ctx), 0, ctx, 0);
> 
> The above change introduces a new AVBufferRef for ctx. The first AVBufferRef
> for
> ctx is created when function av_hwdevice_ctx_alloc is called. So there are
> two
> different AVBufferRefs referring to the same ctx, then ctx will be double-
> freed
> 
> The change below is a bit ugly, but it may fix this double-free issue.
> 
> +static void qsv_ctx_free(void *opaque, uint8_t *ctx)
> +{
> +    // Do nothing here
> +    // ctx is freed in hwdevice_ctx_free
> +}
> +
>  static int qsv_device_create(AVHWDeviceContext *ctx, const char *device,
>                               AVDictionary *opts, int flags)
>  {
> @@ -1271,7 +1277,7 @@ static int qsv_device_create(AVHWDeviceContext *ctx,
> const
> char *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);
> +        child_device->internal->derived_devices[ctx->type] =
> av_buffer_create((uint8_t*)ctx, sizeof(*ctx), qsv_ctx_free, ctx, 0);
>          if (!child_device->internal->derived_devices[ctx->type]) {
>              return AVERROR(ENOMEM);
>          }

Hi Haihao,

sorry for getting back late and thanks for your suggested fix.
I'll apply it and submit a new patch version.

Best regards,
softworkz
Chen, Wenbin Sept. 8, 2021, 5:57 a.m. UTC | #7
Hi softworkz:

Any updates on this patch?
I want to submit pathes to qsv, but they depends on this patch

Thanks
Wenbin
> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Soft
> Works
> Sent: Thursday, August 19, 2021 3:52 PM
> To: FFmpeg development discussions and patches <ffmpeg-
> devel@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH v2] avutils/hwcontext: When deriving a
> hwdevice, search for existing device in both directions
> 
> 
> 
> > -----Original Message-----
> > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Xiang,
> > Haihao
> > Sent: Thursday, 19 August 2021 09:37
> > To: ffmpeg-devel@ffmpeg.org
> > Subject: Re: [FFmpeg-devel] [PATCH v2] avutils/hwcontext: When deriving a
> > hwdevice, search for existing device in both directions
> >
> > On Fri, 2021-08-13 at 06:29 +0000, Xiang, Haihao wrote:
> > > 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.
> > > >
> 
> [..]
> 
> > >
> > > video:143kB audio:0kB subtitle:0kB other streams:0kB global headers:0kB
> > muxing
> > > overhead: unknown
> > > corrupted size vs. prev_size in fastbins
> > > Aborted
> > >
> >
> > Hi Softworks,
> >
> > +        child_device->internal->derived_devices[ctx->type] =
> > av_buffer_create((uint8_t*)ctx, sizeof(*ctx), 0, ctx, 0);
> >
> > The above change introduces a new AVBufferRef for ctx. The first
> AVBufferRef
> > for
> > ctx is created when function av_hwdevice_ctx_alloc is called. So there are
> > two
> > different AVBufferRefs referring to the same ctx, then ctx will be double-
> > freed
> >
> > The change below is a bit ugly, but it may fix this double-free issue.
> >
> > +static void qsv_ctx_free(void *opaque, uint8_t *ctx)
> > +{
> > +    // Do nothing here
> > +    // ctx is freed in hwdevice_ctx_free
> > +}
> > +
> >  static int qsv_device_create(AVHWDeviceContext *ctx, const char *device,
> >                               AVDictionary *opts, int flags)
> >  {
> > @@ -1271,7 +1277,7 @@ static int qsv_device_create(AVHWDeviceContext
> *ctx,
> > const
> > char *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);
> > +        child_device->internal->derived_devices[ctx->type] =
> > av_buffer_create((uint8_t*)ctx, sizeof(*ctx), qsv_ctx_free, ctx, 0);
> >          if (!child_device->internal->derived_devices[ctx->type]) {
> >              return AVERROR(ENOMEM);
> >          }
> 
> Hi Haihao,
> 
> sorry for getting back late and thanks for your suggested fix.
> I'll apply it and submit a new patch version.
> 
> Best regards,
> 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".
Soft Works Sept. 8, 2021, 6:24 a.m. UTC | #8
> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Chen, Wenbin
> Sent: Wednesday, 8 September 2021 07:58
> To: FFmpeg development discussions and patches <ffmpeg-
> devel@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH v2] avutils/hwcontext: When
> deriving a hwdevice, search for existing device in both directions
> 
> Hi softworkz:
> 
> Any updates on this patch?
> I want to submit pathes to qsv, but they depends on this patch
> 
> Thanks
> Wenbin

Hi Wenbin,

Haihao has discovered a regression but I haven't looked into it
yet. Will do within the next few days.

softworkz
Chen, Wenbin Sept. 8, 2021, 7:13 a.m. UTC | #9
> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Soft
> Works
> Sent: Wednesday, September 8, 2021 2:25 PM
> To: FFmpeg development discussions and patches <ffmpeg-
> devel@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH v2] avutils/hwcontext: When deriving a
> hwdevice, search for existing device in both directions
> 
> 
> 
> > -----Original Message-----
> > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> > Chen, Wenbin
> > Sent: Wednesday, 8 September 2021 07:58
> > To: FFmpeg development discussions and patches <ffmpeg-
> > devel@ffmpeg.org>
> > Subject: Re: [FFmpeg-devel] [PATCH v2] avutils/hwcontext: When
> > deriving a hwdevice, search for existing device in both directions
> >
> > Hi softworkz:
> >
> > Any updates on this patch?
> > I want to submit pathes to qsv, but they depends on this patch
> >
> > Thanks
> > Wenbin
> 
> Hi Wenbin,
> 
> Haihao has discovered a regression but I haven't looked into it
> yet. Will do within the next few days.
> 
> softworkz

Got it
Thank you

> _______________________________________________
> 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".
diff mbox series

Patch

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;
+}
+
 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 = {