diff mbox series

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

Message ID DM8P223MB036578CDD5AEA447DD2DE424BA629@DM8P223MB0365.NAMP223.PROD.OUTLOOK.COM
State New
Headers show
Series [FFmpeg-devel,v4,1/1] avutils/hwcontext: When deriving a hwdevice, search for existing device in both directions
Related show

Checks

Context Check Description
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished
andriy/make_ppc success Make finished
andriy/make_fate_ppc success Make fate finished

Commit Message

Soft Works Nov. 25, 2021, 2:41 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>
---
v4: Fixed braces code style

 libavutil/hwcontext.c          | 38 ++++++++++++++++++++++++++++++++++
 libavutil/hwcontext.h          |  1 +
 libavutil/hwcontext_internal.h |  6 ++++++
 libavutil/hwcontext_qsv.c      | 13 +++++++++---
 4 files changed, 55 insertions(+), 3 deletions(-)

Comments

Anton Khirnov Nov. 25, 2021, 4:39 p.m. UTC | #1
Quoting Soft Works (2021-11-25 03:41:32)
> @@ -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;
> +                }

This means that once you derive a device of a certain type, you can
never truly close it without also killing the parent device. That
strikes me as
- potentially troublesome
- a behavior change

Also, I don't see it as completely obvious that all derivations should
always return the same child instance.
Soft Works Nov. 25, 2021, 5:02 p.m. UTC | #2
> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Anton
> Khirnov
> Sent: Thursday, November 25, 2021 5:40 PM
> To: ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH v4 1/1] avutils/hwcontext: When deriving a
> hwdevice, search for existing device in both directions
> 
> Quoting Soft Works (2021-11-25 03:41:32)
> > @@ -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;
> > +                }
> 
> This means that once you derive a device of a certain type, you can
> never truly close it without also killing the parent device. That
> strikes me as
> - potentially troublesome
> - a behavior change
> 
> Also, I don't see it as completely obvious that all derivations should
> always return the same child instance.


It creates the behavior that everybody wants and expects who is working 
with HW devices derivation.

softworkz
Anton Khirnov Nov. 26, 2021, 4:15 p.m. UTC | #3
Quoting Soft Works (2021-11-25 18:02:54)
> 
> 
> > -----Original Message-----
> > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Anton
> > Khirnov
> > Sent: Thursday, November 25, 2021 5:40 PM
> > To: ffmpeg-devel@ffmpeg.org
> > Subject: Re: [FFmpeg-devel] [PATCH v4 1/1] avutils/hwcontext: When deriving a
> > hwdevice, search for existing device in both directions
> > 
> > Quoting Soft Works (2021-11-25 03:41:32)
> > > @@ -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;
> > > +                }
> > 
> > This means that once you derive a device of a certain type, you can
> > never truly close it without also killing the parent device. That
> > strikes me as
> > - potentially troublesome
> > - a behavior change
> > 
> > Also, I don't see it as completely obvious that all derivations should
> > always return the same child instance.
> 
> 
> It creates the behavior that everybody wants and expects who is working 
> with HW devices derivation.

What qualifies you to speak for "everybody"? I would expect to hear some
practical arguments for such a strong claim.
Soft Works Nov. 26, 2021, 6:15 p.m. UTC | #4
> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Anton
> Khirnov
> Sent: Friday, November 26, 2021 5:15 PM
> To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH v4 1/1] avutils/hwcontext: When deriving a
> hwdevice, search for existing device in both directions
> 
> Quoting Soft Works (2021-11-25 18:02:54)
> >
> >
> > > -----Original Message-----
> > > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Anton
> > > Khirnov
> > > Sent: Thursday, November 25, 2021 5:40 PM
> > > To: ffmpeg-devel@ffmpeg.org
> > > Subject: Re: [FFmpeg-devel] [PATCH v4 1/1] avutils/hwcontext: When
> deriving a
> > > hwdevice, search for existing device in both directions
> > >
> > > Quoting Soft Works (2021-11-25 03:41:32)
> > > > @@ -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;
> > > > +                }
> > >
> > > This means that once you derive a device of a certain type, you can
> > > never truly close it without also killing the parent device. That
> > > strikes me as
> > > - potentially troublesome
> > > - a behavior change
> > >
> > > Also, I don't see it as completely obvious that all derivations should
> > > always return the same child instance.
> >
> >
> > It creates the behavior that everybody wants and expects who is working
> > with HW devices derivation.
> 
> What qualifies you to speak for "everybody"? I would expect to hear some
> practical arguments for such a strong claim.

I meant "everybody who has been dealing with multi-level hw context 
derivation and talked about it here".

All of them had given specific examples of problems they were running into
or considering problematic:

- Haihao Xiang
- Wenbin Chen
- Guangxin Xu
- Lynne
- Mark Thompson
  (https://www.mail-archive.com/ffmpeg-devel@ffmpeg.org/msg117373.html)

Do you have any example that would be negatively affected by this patch?

Thanks,
softworkz
Soft Works Nov. 26, 2021, 6:43 p.m. UTC | #5
> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Anton
> Khirnov
> Sent: Friday, November 26, 2021 5:15 PM
> To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH v4 1/1] avutils/hwcontext: When deriving a
> hwdevice, search for existing device in both directions
> 
> Quoting Soft Works (2021-11-25 18:02:54)
> >
> >
> > > -----Original Message-----
> > > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Anton
> > > Khirnov
> > > Sent: Thursday, November 25, 2021 5:40 PM
> > > To: ffmpeg-devel@ffmpeg.org
> > > Subject: Re: [FFmpeg-devel] [PATCH v4 1/1] avutils/hwcontext: When
> deriving a
> > > hwdevice, search for existing device in both directions
> > >
> > > Quoting Soft Works (2021-11-25 03:41:32)
> > > > @@ -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;
> > > > +                }
> > >
> > > This means that once you derive a device of a certain type, you can
> > > never truly close it without also killing the parent device. That

Maybe I'm missing something, but hw device contexts are refcounted.
What happens in hwdevice_ctx_free() is this:

av_buffer_unref(&ctx->internal->source_device);
Anton Khirnov Nov. 26, 2021, 7:12 p.m. UTC | #6
Quoting Soft Works (2021-11-26 19:43:58)
> Maybe I'm missing something, but hw device contexts are refcounted.
> What happens in hwdevice_ctx_free() is this:
> 
> av_buffer_unref(&ctx->internal->source_device);

IIUC this only happens after the parent device is freed. My concern is
the following situation:
- the caller creates a parent hwdevice
- the caller derives a child from it, which may acquire some additional
  resources beyond what the parent holds
- the caller unrefs all his references to the child, but the child does
  not get freed because the parent still holds a reference to it

Since av_hwdevice_ctx_create_derived() has a flags parameter, we might
want to introduce a flag to control this behavior.
Soft Works Nov. 26, 2021, 7:29 p.m. UTC | #7
> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Anton
> Khirnov
> Sent: Friday, November 26, 2021 8:12 PM
> To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH v4 1/1] avutils/hwcontext: When deriving a
> hwdevice, search for existing device in both directions
> 
> Quoting Soft Works (2021-11-26 19:43:58)
> > Maybe I'm missing something, but hw device contexts are refcounted.
> > What happens in hwdevice_ctx_free() is this:
> >
> > av_buffer_unref(&ctx->internal->source_device);
> 
> IIUC this only happens after the parent device is freed. My concern is
> the following situation:
> - the caller creates a parent hwdevice
> - the caller derives a child from it, which may acquire some additional
>   resources beyond what the parent holds
> - the caller unrefs all his references to the child, but the child does
>   not get freed because the parent still holds a reference to it
> 
> Since av_hwdevice_ctx_create_derived() has a flags parameter, we might
> want to introduce a flag to control this behavior.

I understand what you mean. I'm just not sure whether a practical case
with such a requirement exists. Should that turn out to be required,
such flag can be added at any time, IMO.

Kind regards,
softworkz
Soft Works Nov. 26, 2021, 7:42 p.m. UTC | #8
> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Soft Works
> Sent: Friday, November 26, 2021 8:29 PM
> To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH v4 1/1] 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 Anton
> > Khirnov
> > Sent: Friday, November 26, 2021 8:12 PM
> > To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
> > Subject: Re: [FFmpeg-devel] [PATCH v4 1/1] avutils/hwcontext: When deriving
> a
> > hwdevice, search for existing device in both directions
> >
> > Quoting Soft Works (2021-11-26 19:43:58)
> > > Maybe I'm missing something, but hw device contexts are refcounted.
> > > What happens in hwdevice_ctx_free() is this:
> > >
> > > av_buffer_unref(&ctx->internal->source_device);
> >
> > IIUC this only happens after the parent device is freed. My concern is
> > the following situation:
> > - the caller creates a parent hwdevice
> > - the caller derives a child from it, which may acquire some additional
> >   resources beyond what the parent holds
> > - the caller unrefs all his references to the child, but the child does
> >   not get freed because the parent still holds a reference to it

BTW, that's the current behavior anyway. The only change that this patch 
is adding is to find _all_ existing derived contexts, by iterating through
the whole tree of derivations instead of just following the parent direction.

softworkz
diff mbox series

Patch

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 268be9f8a1..3c140e4c8b 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,7 +236,7 @@  static AVBufferRef *qsv_pool_alloc(void *opaque, size_t size)
     if (s->nb_surfaces_used < hwctx->nb_surfaces) {
         s->nb_surfaces_used++;
         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;
@@ -1526,8 +1526,15 @@  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 = {