[FFmpeg-devel] [PATCH 0/3] Add derive-device function which searches for existing devices in both directions

[FFmpeg-devel] [PATCH 0/3] Add derive-device function which searches for existing devices in both directions

[ffmpegagent]

This is an updated version of: [PATCH v4 1/1] avutils/hwcontext: When
deriving a hwdevice, search for existing device in both directions

There has been an objection that the earlier patchset would change API
behavior, and that this change should be limited to ffmpeg cli.

To achieve this, the API behavior is left unchanged now and a new function
av_hwdevice_ctx_get_or_create_derived() is added and used by the hwupload
and hwmap filters.

softworkz (3):
  avutils/hwcontext: add derive-device function which searches for
    existing devices in both directions
  lavu: bump minor version and add doc/APIchanges entry for
    av_hwdevice_ctx_get_or_create_derived()
  avfilter/hwmap,hwupload: use new av_hwdevice_ctx_get_or_create_derived
    method

 doc/APIchanges                 |  3 ++
 libavfilter/vf_hwmap.c         |  4 +-
 libavfilter/vf_hwupload.c      |  2 +-
 libavutil/hwcontext.c          | 72 +++++++++++++++++++++++++++++++---
 libavutil/hwcontext.h          | 20 ++++++++++
 libavutil/hwcontext_internal.h |  6 +++
 libavutil/hwcontext_qsv.c      | 13 ++++--
 libavutil/version.h            |  4 +-
 8 files changed, 110 insertions(+), 14 deletions(-)


base-commit: eef652ca9c893a84c6430fcdd53eed186c299d82
Published-As: https://github.com/ffstaging/FFmpeg/releases/tag/pr-ffstaging-25%2Fsoftworkz%2Fderive_devices-v1
Fetch-It-Via: git fetch https://github.com/ffstaging/FFmpeg pr-ffstaging-25/softworkz/derive_devices-v1
Pull-Request: https://github.com/ffstaging/FFmpeg/pull/25
-- 
ffmpeg-codebot
_______________________________________________
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".

[FFmpeg-devel] [PATCH 1/3] avutils/hwcontext: add derive-device function which searches for existing devices in both directions

[softworkz]

From: softworkz <softworkz@hotmail.com>

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>
---
 libavutil/hwcontext.c          | 72 +++++++++++++++++++++++++++++++---
 libavutil/hwcontext.h          | 20 ++++++++++
 libavutil/hwcontext_internal.h |  6 +++
 libavutil/hwcontext_qsv.c      | 13 ++++--
 4 files changed, 102 insertions(+), 9 deletions(-)

diff --git a/libavutil/hwcontext.c b/libavutil/hwcontext.c
index ab9ad3703e..1aea7dd5c3 100644
--- a/libavutil/hwcontext.c
+++ b/libavutil/hwcontext.c
@@ -123,6 +123,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 */
@@ -133,6 +134,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);
@@ -644,10 +647,31 @@ fail:
     return ret;
 }
 
-int av_hwdevice_ctx_create_derived_opts(AVBufferRef **dst_ref_ptr,
-                                        enum AVHWDeviceType type,
-                                        AVBufferRef *src_ref,
-                                        AVDictionary *options, int flags)
+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;
+}
+
+static int hwdevice_ctx_create_derived(AVBufferRef **dst_ref_ptr,
+                                       enum AVHWDeviceType type,
+                                       AVBufferRef *src_ref,
+                                       AVDictionary *options, int flags,
+                                       int get_existing)
 {
     AVBufferRef *dst_ref = NULL, *tmp_ref;
     AVHWDeviceContext *dst_ctx, *tmp_ctx;
@@ -667,6 +691,18 @@ int av_hwdevice_ctx_create_derived_opts(AVBufferRef **dst_ref_ptr,
         tmp_ref = tmp_ctx->internal->source_device;
     }
 
+    if (get_existing) {
+        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);
@@ -688,6 +724,13 @@ int av_hwdevice_ctx_create_derived_opts(AVBufferRef **dst_ref_ptr,
                     ret = AVERROR(ENOMEM);
                     goto fail;
                 }
+                if (!tmp_ctx->internal->derived_devices[type]) {
+                    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;
@@ -712,12 +755,29 @@ fail:
     return ret;
 }
 
+int av_hwdevice_ctx_create_derived_opts(AVBufferRef **dst_ref_ptr,
+                                        enum AVHWDeviceType type,
+                                        AVBufferRef *src_ref,
+                                        AVDictionary *options, int flags)
+{
+    return hwdevice_ctx_create_derived(dst_ref_ptr, type, src_ref,
+                                       options, flags, 0);
+}
+
+int av_hwdevice_ctx_get_or_create_derived(AVBufferRef **dst_ref_ptr,
+                                          enum AVHWDeviceType type,
+                                          AVBufferRef *src_ref, int flags)
+{
+    return hwdevice_ctx_create_derived(dst_ref_ptr, type, src_ref,
+                                       NULL, flags, 1);
+}
+
 int av_hwdevice_ctx_create_derived(AVBufferRef **dst_ref_ptr,
                                    enum AVHWDeviceType type,
                                    AVBufferRef *src_ref, int flags)
 {
-    return av_hwdevice_ctx_create_derived_opts(dst_ref_ptr, type, src_ref,
-                                               NULL, flags);
+    return hwdevice_ctx_create_derived(dst_ref_ptr, type, src_ref,
+                                       NULL, flags, 0);
 }
 
 static void ff_hwframe_unmap(void *opaque, uint8_t *data)
diff --git a/libavutil/hwcontext.h b/libavutil/hwcontext.h
index c18b7e1e8b..3785811f98 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;
@@ -328,6 +329,25 @@ int av_hwdevice_ctx_create_derived(AVBufferRef **dst_ctx,
                                    enum AVHWDeviceType type,
                                    AVBufferRef *src_ctx, int flags);
 
+/**
+ * Create a new device of the specified type from an existing device.
+ *
+ * This function performs the same action as av_hwdevice_ctx_create_derived,
+ * however, if a derived device of the specified type already exists,
+ * it returns the existing instance.
+ *
+ * @param dst_ctx On success, a reference to the newly-created
+ *                AVHWDeviceContext.
+ * @param type    The type of the new device to create.
+ * @param src_ctx A reference to an existing AVHWDeviceContext which will be
+ *                used to create the new device.
+ * @param flags   Currently unused; should be set to zero.
+ * @return        Zero on success, a negative AVERROR code on failure.
+ */
+int av_hwdevice_ctx_get_or_create_derived(AVBufferRef **dst_ctx,
+                                          enum AVHWDeviceType type,
+                                          AVBufferRef *src_ctx, int flags);
+
 /**
  * Create a new device of the specified type from an existing device.
  *
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 66c0e38955..f24d06fe97 100644
--- a/libavutil/hwcontext_qsv.c
+++ b/libavutil/hwcontext_qsv.c
@@ -309,7 +309,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)
 {
 }
 
@@ -322,7 +322,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;
@@ -1649,8 +1649,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 = {
-- 
ffmpeg-codebot

_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH 1/3] avutils/hwcontext: add derive-device function which searches for existing devices in both directions

[Mark Thompson]

On 30/04/2022 21:07, softworkz wrote:
> From: softworkz <softworkz@hotmail.com>
> 
> 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>
> ---
>   libavutil/hwcontext.c          | 72 +++++++++++++++++++++++++++++++---
>   libavutil/hwcontext.h          | 20 ++++++++++
>   libavutil/hwcontext_internal.h |  6 +++
>   libavutil/hwcontext_qsv.c      | 13 ++++--
>   4 files changed, 102 insertions(+), 9 deletions(-)

Yeah, something like this seems fair.

Some general comments:

* Whenever you use derivation it creates a circular reference, so the instances can never be freed in the current implementation.

* The thread-safety properties of the hwcontext API have been lost - you can no longer operate on devices independently across threads (insofar as the underlying API allows that).
   Maybe there is an argument that derivation is something which should happen early on and therefore documenting it as thread-unsafe is ok, but when hwupload/hwmap can use it inside filtergraphs that just isn't going to happen (and will be violated in the FFmpeg utility if filters get threaded, as is being worked on).

* I'm not sure that it is reasonable to ignore options.  If an unrelated component derived a device before you with special options, you might get that device even if you have incompatible different options.

> diff --git a/libavutil/hwcontext.c b/libavutil/hwcontext.c
> index ab9ad3703e..1aea7dd5c3 100644
> --- a/libavutil/hwcontext.c
> +++ b/libavutil/hwcontext.c
> @@ -123,6 +123,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 */
> @@ -133,6 +134,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);
> @@ -644,10 +647,31 @@ fail:
>       return ret;
>   }
>   
> -int av_hwdevice_ctx_create_derived_opts(AVBufferRef **dst_ref_ptr,
> -                                        enum AVHWDeviceType type,
> -                                        AVBufferRef *src_ref,
> -                                        AVDictionary *options, int flags)
> +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;
> +}
> +
> +static int hwdevice_ctx_create_derived(AVBufferRef **dst_ref_ptr,
> +                                       enum AVHWDeviceType type,
> +                                       AVBufferRef *src_ref,
> +                                       AVDictionary *options, int flags,
> +                                       int get_existing)
>   {
>       AVBufferRef *dst_ref = NULL, *tmp_ref;
>       AVHWDeviceContext *dst_ctx, *tmp_ctx;
> @@ -667,6 +691,18 @@ int av_hwdevice_ctx_create_derived_opts(AVBufferRef **dst_ref_ptr,
>           tmp_ref = tmp_ctx->internal->source_device;
>       }
>   
> +    if (get_existing) {
> +        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);
> @@ -688,6 +724,13 @@ int av_hwdevice_ctx_create_derived_opts(AVBufferRef **dst_ref_ptr,
>                       ret = AVERROR(ENOMEM);
>                       goto fail;
>                   }
> +                if (!tmp_ctx->internal->derived_devices[type]) {

I wonder whether you only want to do this when the user made the new call, not the old one?

The semantics would perhaps feel clearer as "get or create the shared derived device" rather than "get the first device derived or create a new one if not".

> +                    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;
> @@ -712,12 +755,29 @@ fail:
>       return ret;
>   }
>   
> +int av_hwdevice_ctx_create_derived_opts(AVBufferRef **dst_ref_ptr,
> +                                        enum AVHWDeviceType type,
> +                                        AVBufferRef *src_ref,
> +                                        AVDictionary *options, int flags)
> +{
> +    return hwdevice_ctx_create_derived(dst_ref_ptr, type, src_ref,
> +                                       options, flags, 0);
> +}
> +
> +int av_hwdevice_ctx_get_or_create_derived(AVBufferRef **dst_ref_ptr,
> +                                          enum AVHWDeviceType type,
> +                                          AVBufferRef *src_ref, int flags)
> +{
> +    return hwdevice_ctx_create_derived(dst_ref_ptr, type, src_ref,
> +                                       NULL, flags, 1);
> +}
> +
>   int av_hwdevice_ctx_create_derived(AVBufferRef **dst_ref_ptr,
>                                      enum AVHWDeviceType type,
>                                      AVBufferRef *src_ref, int flags)
>   {
> -    return av_hwdevice_ctx_create_derived_opts(dst_ref_ptr, type, src_ref,
> -                                               NULL, flags);
> +    return hwdevice_ctx_create_derived(dst_ref_ptr, type, src_ref,
> +                                       NULL, flags, 0);
>   }
>   
>   static void ff_hwframe_unmap(void *opaque, uint8_t *data)
> diff --git a/libavutil/hwcontext.h b/libavutil/hwcontext.h
> index c18b7e1e8b..3785811f98 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

Can we avoid adding a non-constant constant to the user API?

av_hwdevice_iterate_types() exists for this purpose.

>   };
>   
>   typedef struct AVHWDeviceInternal AVHWDeviceInternal;
> @@ -328,6 +329,25 @@ int av_hwdevice_ctx_create_derived(AVBufferRef **dst_ctx,
>                                      enum AVHWDeviceType type,
>                                      AVBufferRef *src_ctx, int flags);
>   
> +/**
> + * Create a new device of the specified type from an existing device.
> + *
> + * This function performs the same action as av_hwdevice_ctx_create_derived,
> + * however, if a derived device of the specified type already exists,
> + * it returns the existing instance.
> + *
> + * @param dst_ctx On success, a reference to the newly-created
> + *                AVHWDeviceContext.
> + * @param type    The type of the new device to create.
> + * @param src_ctx A reference to an existing AVHWDeviceContext which will be
> + *                used to create the new device.
> + * @param flags   Currently unused; should be set to zero.
> + * @return        Zero on success, a negative AVERROR code on failure.
> + */
> +int av_hwdevice_ctx_get_or_create_derived(AVBufferRef **dst_ctx,
> +                                          enum AVHWDeviceType type,
> +                                          AVBufferRef *src_ctx, int flags);

Include the options here?  Not having them in the original call was an unfortunate omission, I think it would be better to include them here as well even if you don't use them immediately.

> +
>   /**
>    * Create a new device of the specified type from an existing device.
>    *
> 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 {

- Mark
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH 1/3] avutils/hwcontext: add derive-device function which searches for existing devices in both directions

[Soft Works]

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Mark
> Thompson
> Sent: Saturday, April 30, 2022 11:39 PM
> To: ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH 1/3] avutils/hwcontext: add derive-
> device function which searches for existing devices in both directions
> 
> On 30/04/2022 21:07, softworkz wrote:
> > From: softworkz <softworkz@hotmail.com>
> >
> > 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>
> > ---
> >   libavutil/hwcontext.c          | 72
> +++++++++++++++++++++++++++++++---
> >   libavutil/hwcontext.h          | 20 ++++++++++
> >   libavutil/hwcontext_internal.h |  6 +++
> >   libavutil/hwcontext_qsv.c      | 13 ++++--
> >   4 files changed, 102 insertions(+), 9 deletions(-)
> 
> Yeah, something like this seems fair.

:-)

> Some general comments:
> 
> * Whenever you use derivation it creates a circular reference, so the
> instances can never be freed in the current implementation.

It's been a while...I thought there wasn't, but looking at it now,
it seems you are right.

How would you solve it?


> * The thread-safety properties of the hwcontext API have been lost -
> you can no longer operate on devices independently across threads
> (insofar as the underlying API allows that).
>    Maybe there is an argument that derivation is something which
> should happen early on and therefore documenting it as thread-unsafe
> is ok, but when hwupload/hwmap can use it inside filtergraphs that
> just isn't going to happen (and will be violated in the FFmpeg utility
> if filters get threaded, as is being worked on).

From my understanding there will be a single separate thread which
handles all filtergraph operations.
I don't think it would even be possible (without massive changes)
to arbitrate filter processing in parallel.
But even if this would be implemented: the filtergraph setup (init,
uninit, query_formats, etc.) would surely happen on a single thread.


> * I'm not sure that it is reasonable to ignore options.  If an
> unrelated component derived a device before you with special options,
> you might get that device even if you have incompatible different
> options.

I understand what you mean, but this is outside the scope of 
this patchset, because when you would want to do this, it
would need to be implemented for derivation in general, not
in this patchset which only adds reverse-search to the
existing derivation functionality.


> > diff --git a/libavutil/hwcontext.c b/libavutil/hwcontext.c
> > index ab9ad3703e..1aea7dd5c3 100644
> > --- a/libavutil/hwcontext.c
> > +++ b/libavutil/hwcontext.c
> > @@ -123,6 +123,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 */
> > @@ -133,6 +134,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);
> > @@ -644,10 +647,31 @@ fail:
> >       return ret;
> >   }
> >
> > -int av_hwdevice_ctx_create_derived_opts(AVBufferRef **dst_ref_ptr,
> > -                                        enum AVHWDeviceType type,
> > -                                        AVBufferRef *src_ref,
> > -                                        AVDictionary *options, int
> flags)
> > +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;
> > +}
> > +
> > +static int hwdevice_ctx_create_derived(AVBufferRef **dst_ref_ptr,
> > +                                       enum AVHWDeviceType type,
> > +                                       AVBufferRef *src_ref,
> > +                                       AVDictionary *options, int
> flags,
> > +                                       int get_existing)
> >   {
> >       AVBufferRef *dst_ref = NULL, *tmp_ref;
> >       AVHWDeviceContext *dst_ctx, *tmp_ctx;
> > @@ -667,6 +691,18 @@ int
> av_hwdevice_ctx_create_derived_opts(AVBufferRef **dst_ref_ptr,
> >           tmp_ref = tmp_ctx->internal->source_device;
> >       }
> >
> > +    if (get_existing) {
> > +        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);
> > @@ -688,6 +724,13 @@ int
> av_hwdevice_ctx_create_derived_opts(AVBufferRef **dst_ref_ptr,
> >                       ret = AVERROR(ENOMEM);
> >                       goto fail;
> >                   }
> > +                if (!tmp_ctx->internal->derived_devices[type]) {
> 
> I wonder whether you only want to do this when the user made the new
> call, not the old one?
> 
> The semantics would perhaps feel clearer as "get or create the shared
> derived device" rather than "get the first device derived or create a
> new one if not".

I've been there for a moment, and then I thought that when the API
consumer would mix API calls, e.g. first without 'get' and second
with 'get', then the second call would not produce the expected 
result.

Let me know what you think, I have no strong opinion about this.


> > +                    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;
> > @@ -712,12 +755,29 @@ fail:
> >       return ret;
> >   }
> >
> > +int av_hwdevice_ctx_create_derived_opts(AVBufferRef **dst_ref_ptr,
> > +                                        enum AVHWDeviceType type,
> > +                                        AVBufferRef *src_ref,
> > +                                        AVDictionary *options, int
> flags)
> > +{
> > +    return hwdevice_ctx_create_derived(dst_ref_ptr, type, src_ref,
> > +                                       options, flags, 0);
> > +}
> > +
> > +int av_hwdevice_ctx_get_or_create_derived(AVBufferRef
> **dst_ref_ptr,
> > +                                          enum AVHWDeviceType type,
> > +                                          AVBufferRef *src_ref, int
> flags)
> > +{
> > +    return hwdevice_ctx_create_derived(dst_ref_ptr, type, src_ref,
> > +                                       NULL, flags, 1);
> > +}
> > +
> >   int av_hwdevice_ctx_create_derived(AVBufferRef **dst_ref_ptr,
> >                                      enum AVHWDeviceType type,
> >                                      AVBufferRef *src_ref, int
> flags)
> >   {
> > -    return av_hwdevice_ctx_create_derived_opts(dst_ref_ptr, type,
> src_ref,
> > -                                               NULL, flags);
> > +    return hwdevice_ctx_create_derived(dst_ref_ptr, type, src_ref,
> > +                                       NULL, flags, 0);
> >   }
> >
> >   static void ff_hwframe_unmap(void *opaque, uint8_t *data)
> > diff --git a/libavutil/hwcontext.h b/libavutil/hwcontext.h
> > index c18b7e1e8b..3785811f98 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
> 
> Can we avoid adding a non-constant constant to the user API?
> 
> av_hwdevice_iterate_types() exists for this purpose.

There was a reason why this can't be used. IIRC, it was that the
device count needs to be known at some other place where 
av_hwdevice_iterate_types() is not available.

Please see the previous discussion with Hendrik about this.


> >   };
> >
> >   typedef struct AVHWDeviceInternal AVHWDeviceInternal;
> > @@ -328,6 +329,25 @@ int av_hwdevice_ctx_create_derived(AVBufferRef
> **dst_ctx,
> >                                      enum AVHWDeviceType type,
> >                                      AVBufferRef *src_ctx, int
> flags);
> >
> > +/**
> > + * Create a new device of the specified type from an existing
> device.
> > + *
> > + * This function performs the same action as
> av_hwdevice_ctx_create_derived,
> > + * however, if a derived device of the specified type already
> exists,
> > + * it returns the existing instance.
> > + *
> > + * @param dst_ctx On success, a reference to the newly-created
> > + *                AVHWDeviceContext.
> > + * @param type    The type of the new device to create.
> > + * @param src_ctx A reference to an existing AVHWDeviceContext
> which will be
> > + *                used to create the new device.
> > + * @param flags   Currently unused; should be set to zero.
> > + * @return        Zero on success, a negative AVERROR code on
> failure.
> > + */
> > +int av_hwdevice_ctx_get_or_create_derived(AVBufferRef **dst_ctx,
> > +                                          enum AVHWDeviceType type,
> > +                                          AVBufferRef *src_ctx, int
> flags);
> 
> Include the options here?  Not having them in the original call was an
> unfortunate omission, I think it would be better to include them here
> as well even if you don't use them immediately.

I didn't see the options being used anywhere, that's why I thought
it would be the other way round (options param overload exists 
for legacy/compatibility reasons).

But sure, I'll change it then to include options.


Thanks for the quick review!


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".

Re: [FFmpeg-devel] [PATCH 1/3] avutils/hwcontext: add derive-device function which searches for existing devices in both directions

[Mark Thompson]

On 30/04/2022 23:42, Soft Works wrote:
> 
> 
>> -----Original Message-----
>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Mark
>> Thompson
>> Sent: Saturday, April 30, 2022 11:39 PM
>> To: ffmpeg-devel@ffmpeg.org
>> Subject: Re: [FFmpeg-devel] [PATCH 1/3] avutils/hwcontext: add derive-
>> device function which searches for existing devices in both directions
>>
>> On 30/04/2022 21:07, softworkz wrote:
>>> From: softworkz <softworkz@hotmail.com>
>>>
>>> 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>
>>> ---
>>>    libavutil/hwcontext.c          | 72
>> +++++++++++++++++++++++++++++++---
>>>    libavutil/hwcontext.h          | 20 ++++++++++
>>>    libavutil/hwcontext_internal.h |  6 +++
>>>    libavutil/hwcontext_qsv.c      | 13 ++++--
>>>    4 files changed, 102 insertions(+), 9 deletions(-)
>>
>> Yeah, something like this seems fair.
> 
> :-)
> 
>> Some general comments:
>>
>> * Whenever you use derivation it creates a circular reference, so the
>> instances can never be freed in the current implementation.
> 
> It's been a while...I thought there wasn't, but looking at it now,
> it seems you are right.
> 
> How would you solve it?

Hmm.  You do need both the source and derived device to be able to keep the other alive with this form, so the strict reference-counting structure isn't going to work.  Given that, I guess it's got to do something else but I've no idea what.

>> * The thread-safety properties of the hwcontext API have been lost -
>> you can no longer operate on devices independently across threads
>> (insofar as the underlying API allows that).
>>     Maybe there is an argument that derivation is something which
>> should happen early on and therefore documenting it as thread-unsafe
>> is ok, but when hwupload/hwmap can use it inside filtergraphs that
>> just isn't going to happen (and will be violated in the FFmpeg utility
>> if filters get threaded, as is being worked on).
> 
>  From my understanding there will be a single separate thread which
> handles all filtergraph operations.
> I don't think it would even be possible (without massive changes)
> to arbitrate filter processing in parallel.
> But even if this would be implemented: the filtergraph setup (init,
> uninit, query_formats, etc.) would surely happen on a single thread.

The ffmpeg utility creates filtergraphs dynamically when the first frame is available from their inputs, so I don't see why you wouldn't allow multiple of them to be created in parallel in that case.

If you create all devices at the beginning and then give references to them to the various filters which need them (so never manipulate devices dynamically within the graph) then it would be ok, but I think you've already rejected that approach.

>> * I'm not sure that it is reasonable to ignore options.  If an
>> unrelated component derived a device before you with special options,
>> you might get that device even if you have incompatible different
>> options.
> 
> I understand what you mean, but this is outside the scope of
> this patchset, because when you would want to do this, it
> would need to be implemented for derivation in general, not
> in this patchset which only adds reverse-search to the
> existing derivation functionality.

I'm not sure what you mean by that?  The feature already exists; here is a concrete example of where you would get the wrong result:

Start with VAAPI device A.

Component P derives Vulkan device B with some extension options X.

Component Q wants the same device as P, so it derives again with extension options X and gets B.

Everything works fine for a while.

Later, unrelated component R is inserted before P and Q.  It wants a Vulkan device C with extension options Y, so it derives that.

Now component Q is broken because it gets C instead of B and has the wrong extensions enabled.

>>> diff --git a/libavutil/hwcontext.c b/libavutil/hwcontext.c
>>> index ab9ad3703e..1aea7dd5c3 100644
>>> --- a/libavutil/hwcontext.c
>>> +++ b/libavutil/hwcontext.c
>>> @@ -123,6 +123,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 */
>>> @@ -133,6 +134,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);
>>> @@ -644,10 +647,31 @@ fail:
>>>        return ret;
>>>    }
>>>
>>> -int av_hwdevice_ctx_create_derived_opts(AVBufferRef **dst_ref_ptr,
>>> -                                        enum AVHWDeviceType type,
>>> -                                        AVBufferRef *src_ref,
>>> -                                        AVDictionary *options, int
>> flags)
>>> +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;
>>> +}
>>> +
>>> +static int hwdevice_ctx_create_derived(AVBufferRef **dst_ref_ptr,
>>> +                                       enum AVHWDeviceType type,
>>> +                                       AVBufferRef *src_ref,
>>> +                                       AVDictionary *options, int
>> flags,
>>> +                                       int get_existing)
>>>    {
>>>        AVBufferRef *dst_ref = NULL, *tmp_ref;
>>>        AVHWDeviceContext *dst_ctx, *tmp_ctx;
>>> @@ -667,6 +691,18 @@ int
>> av_hwdevice_ctx_create_derived_opts(AVBufferRef **dst_ref_ptr,
>>>            tmp_ref = tmp_ctx->internal->source_device;
>>>        }
>>>
>>> +    if (get_existing) {
>>> +        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);
>>> @@ -688,6 +724,13 @@ int
>> av_hwdevice_ctx_create_derived_opts(AVBufferRef **dst_ref_ptr,
>>>                        ret = AVERROR(ENOMEM);
>>>                        goto fail;
>>>                    }
>>> +                if (!tmp_ctx->internal->derived_devices[type]) {
>>
>> I wonder whether you only want to do this when the user made the new
>> call, not the old one?
>>
>> The semantics would perhaps feel clearer as "get or create the shared
>> derived device" rather than "get the first device derived or create a
>> new one if not".
> 
> I've been there for a moment, and then I thought that when the API
> consumer would mix API calls, e.g. first without 'get' and second
> with 'get', then the second call would not produce the expected
> result.
> 
> Let me know what you think, I have no strong opinion about this.

Can you explain your example further?

Making the shared device always opt-in seems better to me to avoid unexpected interactions.  (Like in the above example where a non-sharing component is added before everything else - when sharing is implicit this ends up being the first device derived and gets shared with others.)

>>> +                    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;
>>> @@ -712,12 +755,29 @@ fail:
>>>        return ret;
>>>    }
>>>
>>> +int av_hwdevice_ctx_create_derived_opts(AVBufferRef **dst_ref_ptr,
>>> +                                        enum AVHWDeviceType type,
>>> +                                        AVBufferRef *src_ref,
>>> +                                        AVDictionary *options, int
>> flags)
>>> +{
>>> +    return hwdevice_ctx_create_derived(dst_ref_ptr, type, src_ref,
>>> +                                       options, flags, 0);
>>> +}
>>> +
>>> +int av_hwdevice_ctx_get_or_create_derived(AVBufferRef
>> **dst_ref_ptr,
>>> +                                          enum AVHWDeviceType type,
>>> +                                          AVBufferRef *src_ref, int
>> flags)
>>> +{
>>> +    return hwdevice_ctx_create_derived(dst_ref_ptr, type, src_ref,
>>> +                                       NULL, flags, 1);
>>> +}
>>> +
>>>    int av_hwdevice_ctx_create_derived(AVBufferRef **dst_ref_ptr,
>>>                                       enum AVHWDeviceType type,
>>>                                       AVBufferRef *src_ref, int
>> flags)
>>>    {
>>> -    return av_hwdevice_ctx_create_derived_opts(dst_ref_ptr, type,
>> src_ref,
>>> -                                               NULL, flags);
>>> +    return hwdevice_ctx_create_derived(dst_ref_ptr, type, src_ref,
>>> +                                       NULL, flags, 0);
>>>    }
>>>
>>>    static void ff_hwframe_unmap(void *opaque, uint8_t *data)
>>> diff --git a/libavutil/hwcontext.h b/libavutil/hwcontext.h
>>> index c18b7e1e8b..3785811f98 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
>>
>> Can we avoid adding a non-constant constant to the user API?
>>
>> av_hwdevice_iterate_types() exists for this purpose.
> 
> There was a reason why this can't be used. IIRC, it was that the
> device count needs to be known at some other place where
> av_hwdevice_iterate_types() is not available.
> 
> Please see the previous discussion with Hendrik about this.

Where is that?  The only place I see this used is the array of derived devices.

Two alternative implementations without the constant spring to mind:

* A shorter array indexed by av_hwdevice_iterate_types() which would not have empty entries for devices not compatible with the current platform.

* An array of type/reference pairs.

>>>    };
>>>
>>>    typedef struct AVHWDeviceInternal AVHWDeviceInternal;
>>> @@ -328,6 +329,25 @@ int av_hwdevice_ctx_create_derived(AVBufferRef
>> **dst_ctx,
>>>                                       enum AVHWDeviceType type,
>>>                                       AVBufferRef *src_ctx, int
>> flags);
>>>
>>> +/**
>>> + * Create a new device of the specified type from an existing
>> device.
>>> + *
>>> + * This function performs the same action as
>> av_hwdevice_ctx_create_derived,
>>> + * however, if a derived device of the specified type already
>> exists,
>>> + * it returns the existing instance.
>>> + *
>>> + * @param dst_ctx On success, a reference to the newly-created
>>> + *                AVHWDeviceContext.
>>> + * @param type    The type of the new device to create.
>>> + * @param src_ctx A reference to an existing AVHWDeviceContext
>> which will be
>>> + *                used to create the new device.
>>> + * @param flags   Currently unused; should be set to zero.
>>> + * @return        Zero on success, a negative AVERROR code on
>> failure.
>>> + */
>>> +int av_hwdevice_ctx_get_or_create_derived(AVBufferRef **dst_ctx,
>>> +                                          enum AVHWDeviceType type,
>>> +                                          AVBufferRef *src_ctx, int
>> flags);
>>
>> Include the options here?  Not having them in the original call was an
>> unfortunate omission, I think it would be better to include them here
>> as well even if you don't use them immediately.
> 
> I didn't see the options being used anywhere, that's why I thought
> it would be the other way round (options param overload exists
> for legacy/compatibility reasons).
> 
> But sure, I'll change it then to include options.

- Mark
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH 1/3] avutils/hwcontext: add derive-device function which searches for existing devices in both directions

[Soft Works]

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Mark
> Thompson
> Sent: Monday, May 2, 2022 12:01 AM
> To: ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH 1/3] avutils/hwcontext: add derive-
> device function which searches for existing devices in both directions

[..]

> >> * Whenever you use derivation it creates a circular reference, so
> the
> >> instances can never be freed in the current implementation.
> >
> > It's been a while...I thought there wasn't, but looking at it now,
> > it seems you are right.
> >
> > How would you solve it?
> 
> Hmm.  You do need both the source and derived device to be able to
> keep the other alive with this form, so the strict reference-counting
> structure isn't going to work.  Given that, I guess it's got to do
> something else but I've no idea what.

Ok, before I come up with something that might find objection, I'd like
to ask all around:

Do we have some pattern for weak/shared referencing via AVBufferRef?

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".

Re: [FFmpeg-devel] [PATCH 1/3] avutils/hwcontext: add derive-device function which searches for existing devices in both directions

[Soft Works]

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Mark
> Thompson
> Sent: Monday, May 2, 2022 12:01 AM
> To: ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH 1/3] avutils/hwcontext: add derive-
> device function which searches for existing devices in both directions

[..]

> >> * The thread-safety properties of the hwcontext API have been lost
> -
> >> you can no longer operate on devices independently across threads
> >> (insofar as the underlying API allows that).
> >>     Maybe there is an argument that derivation is something which
> >> should happen early on and therefore documenting it as thread-
> unsafe
> >> is ok, but when hwupload/hwmap can use it inside filtergraphs that
> >> just isn't going to happen (and will be violated in the FFmpeg
> utility
> >> if filters get threaded, as is being worked on).
> >
> >  From my understanding there will be a single separate thread which
> > handles all filtergraph operations.
> > I don't think it would even be possible (without massive changes)
> > to arbitrate filter processing in parallel.
> > But even if this would be implemented: the filtergraph setup (init,
> > uninit, query_formats, etc.) would surely happen on a single thread.
> 
> The ffmpeg utility creates filtergraphs dynamically when the first
> frame is available from their inputs, so I don't see why you wouldn't
> allow multiple of them to be created in parallel in that case.
> 
> If you create all devices at the beginning and then give references to
> them to the various filters which need them (so never manipulate
> devices dynamically within the graph) then it would be ok, but I think
> you've already rejected that approach.

Please let's not break out of the scope of this patchset.
This patchset is not about re-doing device derivation. The only
small change that it does is that it returns an existing device
instead of creating a new one when such device already exists
in the same(!) chain.

The change it makes has really nothing to do with threading or 
anything around it.


> >> * I'm not sure that it is reasonable to ignore options.  If an
> >> unrelated component derived a device before you with special
> options,
> >> you might get that device even if you have incompatible different
> >> options.
> >
> > I understand what you mean, but this is outside the scope of
> > this patchset, because when you would want to do this, it
> > would need to be implemented for derivation in general, not
> > in this patchset which only adds reverse-search to the
> > existing derivation functionality.
> 
> I'm not sure what you mean by that?  The feature already exists; here
> is a concrete example of where you would get the wrong result:
> 
> Start with VAAPI device A.
> 
> Component P derives Vulkan device B with some extension options X.
> 
> Component Q wants the same device as P, so it derives again with
> extension options X and gets B.
> 
> Everything works fine for a while.
> 
> Later, unrelated component R is inserted before P and Q.  It wants a
> Vulkan device C with extension options Y, so it derives that.
> 
> Now component Q is broken because it gets C instead of B and has the
> wrong extensions enabled.

As per your request, this patchset's changes are now limited to
use ffmpeg cli. And there is currently no support for "extension
options" when deriving a device.

What I meant above is this: 

Assume this patchset wouldn't be applied, but a patchset would
be applied that allows to specify "extension options".
Then, even without this patchset, I could construct a similar 
example, where you would get the same device when deriving 
two times from the same device but with different extension
options.

That's why I said: yes, I understand the case you are talking
about. But it would require two separate patches, one for
enabling extension options and another one for matching 
extension options when deriving a device. This would make
sense, but:

It has nothing to do with this patchset.
(it could be done before or afterwards)

I'm open to discuss this (separately), because it opens up 
a range of questions how it could be done.

> >>> diff --git a/libavutil/hwcontext.c b/libavutil/hwcontext.c
> >>> index ab9ad3703e..1aea7dd5c3 100644
> >>> --- a/libavutil/hwcontext.c
> >>> +++ b/libavutil/hwcontext.c
> >>> @@ -123,6 +123,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 */
> >>> @@ -133,6 +134,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);
> >>> @@ -644,10 +647,31 @@ fail:
> >>>        return ret;
> >>>    }
> >>>
> >>> -int av_hwdevice_ctx_create_derived_opts(AVBufferRef
> **dst_ref_ptr,
> >>> -                                        enum AVHWDeviceType type,
> >>> -                                        AVBufferRef *src_ref,
> >>> -                                        AVDictionary *options,
> int
> >> flags)
> >>> +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;
> >>> +}
> >>> +
> >>> +static int hwdevice_ctx_create_derived(AVBufferRef **dst_ref_ptr,
> >>> +                                       enum AVHWDeviceType type,
> >>> +                                       AVBufferRef *src_ref,
> >>> +                                       AVDictionary *options, int
> >> flags,
> >>> +                                       int get_existing)
> >>>    {
> >>>        AVBufferRef *dst_ref = NULL, *tmp_ref;
> >>>        AVHWDeviceContext *dst_ctx, *tmp_ctx;
> >>> @@ -667,6 +691,18 @@ int
> >> av_hwdevice_ctx_create_derived_opts(AVBufferRef **dst_ref_ptr,
> >>>            tmp_ref = tmp_ctx->internal->source_device;
> >>>        }
> >>>
> >>> +    if (get_existing) {
> >>> +        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);
> >>> @@ -688,6 +724,13 @@ int
> >> av_hwdevice_ctx_create_derived_opts(AVBufferRef **dst_ref_ptr,
> >>>                        ret = AVERROR(ENOMEM);
> >>>                        goto fail;
> >>>                    }
> >>> +                if (!tmp_ctx->internal->derived_devices[type]) {
> >>
> >> I wonder whether you only want to do this when the user made the
> new
> >> call, not the old one?
> >>
> >> The semantics would perhaps feel clearer as "get or create the
> shared
> >> derived device" rather than "get the first device derived or create
> a
> >> new one if not".
> >
> > I've been there for a moment, and then I thought that when the API
> > consumer would mix API calls, e.g. first without 'get' and second
> > with 'get', then the second call would not produce the expected
> > result.
> >
> > Let me know what you think, I have no strong opinion about this.
> 
> Can you explain your example further?

Maybe we should get clear about what this patchset does exactly.
Let's look at the following derivation chain of devices:

A
├─ X
│  └─ Y
├─ B
│  └─ C
└─ V
   └─ W

The meaning is: 

- Y is derived from X, X is derived from A
- C is derived from B, B is derived from A
- W is derived from V, V is derived from A

In the existing implementation, each device "knows" its parent
(via the 'source_device' field).

When you call av_hwdevice_ctx_create_derived() and specify "C"
as the source device, then it will iterate the tree upwards,
so when B is of the requested type, it will return B or if
A is of the requested type, it will return A.
Otherwise, it will create a new device of the requested type
and sets C as its parent.

But it doesn't return X, Y, V or W (when any would match the
requested type).

This is the current behavior.


What this patchset does is that we also store the derived 
children for each device (derived_devices array).

In the example above, it means hat A has references to 
X, B and V. X to Y, B to C and V to W.

The behavior of the new function is as follows:

When you call av_hwdevice_ctx_get_or_create_derived() and specify "C"
as the source device, then it will iterate the tree upwards,
so when B is of the requested type, it will return B or if
A is of the requested type, it will return A (like before).

Additionally, it will also iterate all through other children 
of B and other children of A. Which means that if X, Y, V or W
matches the requested type, it would return it.

Otherwise, it will create a new device of the requested type
and sets C as its parent.

This is the behavior of the new function.
All that it changes is that it searches the full tree instead
of the parents only.


Now back to your original question:

>>> +                if (!tmp_ctx->internal->derived_devices[type]) {
>>
>> I wonder whether you only want to do this when the user made the new
>> call, not the old one?
>>
>> The semantics would perhaps feel clearer as "get or create the shared
>> derived device" rather than "get the first device derived or create a
>> new one if not".

The line of code you commented on, is about adding a newly created
derived device to the child collection of its parent.

>> I wonder whether you only want to do this when the user made the new
>> call, not the old one?

The difference between old and new is that old searches only parents
and new searches the full tree.
But should calling the old function also control whether a newly created
derived device is added to the child collection of its parent?

It might be confusing behavior when you first call the old function
and later call the new function but you would get the device only
when it's a parent but not when it's a down-tree-child of any parent.

And that's where I said:

> Let me know what you think, I have no strong opinion about this.

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".

Re: [FFmpeg-devel] [PATCH 1/3] avutils/hwcontext: add derive-device function which searches for existing devices in both directions

[Mark Thompson]

On 02/05/2022 09:14, Soft Works wrote:
>> -----Original Message-----
>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Mark
>> Thompson
>> Sent: Monday, May 2, 2022 12:01 AM
>> To: ffmpeg-devel@ffmpeg.org
>> Subject: Re: [FFmpeg-devel] [PATCH 1/3] avutils/hwcontext: add derive-
>> device function which searches for existing devices in both directions
> 
> [..]
> 
>>>> * The thread-safety properties of the hwcontext API have been lost
>> -
>>>> you can no longer operate on devices independently across threads
>>>> (insofar as the underlying API allows that).
>>>>      Maybe there is an argument that derivation is something which
>>>> should happen early on and therefore documenting it as thread-
>> unsafe
>>>> is ok, but when hwupload/hwmap can use it inside filtergraphs that
>>>> just isn't going to happen (and will be violated in the FFmpeg
>> utility
>>>> if filters get threaded, as is being worked on).
>>>
>>>   From my understanding there will be a single separate thread which
>>> handles all filtergraph operations.
>>> I don't think it would even be possible (without massive changes)
>>> to arbitrate filter processing in parallel.
>>> But even if this would be implemented: the filtergraph setup (init,
>>> uninit, query_formats, etc.) would surely happen on a single thread.
>>
>> The ffmpeg utility creates filtergraphs dynamically when the first
>> frame is available from their inputs, so I don't see why you wouldn't
>> allow multiple of them to be created in parallel in that case.
>>
>> If you create all devices at the beginning and then give references to
>> them to the various filters which need them (so never manipulate
>> devices dynamically within the graph) then it would be ok, but I think
>> you've already rejected that approach.
> 
> Please let's not break out of the scope of this patchset.
> This patchset is not about re-doing device derivation. The only
> small change that it does is that it returns an existing device
> instead of creating a new one when such device already exists
> in the same(!) chain.
> 
> The change it makes has really nothing to do with threading or
> anything around it.

The change makes existing thread-safe hwcontext APIs unsafe.  That is definitely not "nothing to do with threading", and has to be resolved since they can currently be called from contexts which expect thread-safety (such as making filtergraphs).

>>>> * I'm not sure that it is reasonable to ignore options.  If an
>>>> unrelated component derived a device before you with special
>> options,
>>>> you might get that device even if you have incompatible different
>>>> options.
>>>
>>> I understand what you mean, but this is outside the scope of
>>> this patchset, because when you would want to do this, it
>>> would need to be implemented for derivation in general, not
>>> in this patchset which only adds reverse-search to the
>>> existing derivation functionality.
>>
>> I'm not sure what you mean by that?  The feature already exists; here
>> is a concrete example of where you would get the wrong result:
>>
>> Start with VAAPI device A.
>>
>> Component P derives Vulkan device B with some extension options X.
>>
>> Component Q wants the same device as P, so it derives again with
>> extension options X and gets B.
>>
>> Everything works fine for a while.
>>
>> Later, unrelated component R is inserted before P and Q.  It wants a
>> Vulkan device C with extension options Y, so it derives that.
>>
>> Now component Q is broken because it gets C instead of B and has the
>> wrong extensions enabled.
> 
> As per your request, this patchset's changes are now limited to
> use ffmpeg cli. And there is currently no support for "extension
> options" when deriving a device.

Yes there is - see the "instance_extensions" and "device_extensions" options to Vulkan device derivation.  (Hence the VAAPI+Vulkan example.)

> What I meant above is this:
> 
> Assume this patchset wouldn't be applied, but a patchset would
> be applied that allows to specify "extension options".
> Then, even without this patchset, I could construct a similar
> example, where you would get the same device when deriving
> two times from the same device but with different extension
> options.

How?

The existing derivation setup always makes a new device, so you can't accidentally get an old one.

The existing way of reusing devices is to keep the reference and reuse it directly, which means you need to pass the reference around explicitly and there is no problem.

> That's why I said: yes, I understand the case you are talking
> about. But it would require two separate patches, one for
> enabling extension options and another one for matching
> extension options when deriving a device. This would make
> sense, but:
> 
> It has nothing to do with this patchset.
> (it could be done before or afterwards)
> 
> I'm open to discuss this (separately), because it opens up
> a range of questions how it could be done.
> 
>>>>> diff --git a/libavutil/hwcontext.c b/libavutil/hwcontext.c
>>>>> index ab9ad3703e..1aea7dd5c3 100644
>>>>> --- a/libavutil/hwcontext.c
>>>>> +++ b/libavutil/hwcontext.c
>>>>> @@ -123,6 +123,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 */
>>>>> @@ -133,6 +134,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);
>>>>> @@ -644,10 +647,31 @@ fail:
>>>>>         return ret;
>>>>>     }
>>>>>
>>>>> -int av_hwdevice_ctx_create_derived_opts(AVBufferRef
>> **dst_ref_ptr,
>>>>> -                                        enum AVHWDeviceType type,
>>>>> -                                        AVBufferRef *src_ref,
>>>>> -                                        AVDictionary *options,
>> int
>>>> flags)
>>>>> +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;
>>>>> +}
>>>>> +
>>>>> +static int hwdevice_ctx_create_derived(AVBufferRef **dst_ref_ptr,
>>>>> +                                       enum AVHWDeviceType type,
>>>>> +                                       AVBufferRef *src_ref,
>>>>> +                                       AVDictionary *options, int
>>>> flags,
>>>>> +                                       int get_existing)
>>>>>     {
>>>>>         AVBufferRef *dst_ref = NULL, *tmp_ref;
>>>>>         AVHWDeviceContext *dst_ctx, *tmp_ctx;
>>>>> @@ -667,6 +691,18 @@ int
>>>> av_hwdevice_ctx_create_derived_opts(AVBufferRef **dst_ref_ptr,
>>>>>             tmp_ref = tmp_ctx->internal->source_device;
>>>>>         }
>>>>>
>>>>> +    if (get_existing) {
>>>>> +        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);
>>>>> @@ -688,6 +724,13 @@ int
>>>> av_hwdevice_ctx_create_derived_opts(AVBufferRef **dst_ref_ptr,
>>>>>                         ret = AVERROR(ENOMEM);
>>>>>                         goto fail;
>>>>>                     }
>>>>> +                if (!tmp_ctx->internal->derived_devices[type]) {
>>>>
>>>> I wonder whether you only want to do this when the user made the
>> new
>>>> call, not the old one?
>>>>
>>>> The semantics would perhaps feel clearer as "get or create the
>> shared
>>>> derived device" rather than "get the first device derived or create
>> a
>>>> new one if not".
>>>
>>> I've been there for a moment, and then I thought that when the API
>>> consumer would mix API calls, e.g. first without 'get' and second
>>> with 'get', then the second call would not produce the expected
>>> result.
>>>
>>> Let me know what you think, I have no strong opinion about this.
>>
>> Can you explain your example further?
> 
> Maybe we should get clear about what this patchset does exactly.
> Let's look at the following derivation chain of devices:
> 
> A
> ├─ X
> │  └─ Y
> ├─ B
> │  └─ C
> └─ V
>     └─ W
> 
> The meaning is:
> 
> - Y is derived from X, X is derived from A
> - C is derived from B, B is derived from A
> - W is derived from V, V is derived from A
> 
> In the existing implementation, each device "knows" its parent
> (via the 'source_device' field).
> 
> When you call av_hwdevice_ctx_create_derived() and specify "C"
> as the source device, then it will iterate the tree upwards,
> so when B is of the requested type, it will return B or if
> A is of the requested type, it will return A.
> Otherwise, it will create a new device of the requested type
> and sets C as its parent.
> 
> But it doesn't return X, Y, V or W (when any would match the
> requested type).
> 
> This is the current behavior.
> 
> 
> What this patchset does is that we also store the derived
> children for each device (derived_devices array).
> 
> In the example above, it means hat A has references to
> X, B and V. X to Y, B to C and V to W.
> 
> The behavior of the new function is as follows:
> 
> When you call av_hwdevice_ctx_get_or_create_derived() and specify "C"
> as the source device, then it will iterate the tree upwards,
> so when B is of the requested type, it will return B or if
> A is of the requested type, it will return A (like before).
> 
> Additionally, it will also iterate all through other children
> of B and other children of A. Which means that if X, Y, V or W
> matches the requested type, it would return it.

No it doesn't?  Your new function find_derived_hwdevice_ctx() is called only for the original source device, and recurses into its (transitive) children.  It can't return any of X, Y, V or W when starting from C.

> Otherwise, it will create a new device of the requested type
> and sets C as its parent.
> 
> This is the behavior of the new function.
> All that it changes is that it searches the full tree instead
> of the parents only.
 >
> Now back to your original question:
> 
>>>> +                if (!tmp_ctx->internal->derived_devices[type]) {
>>>
>>> I wonder whether you only want to do this when the user made the new
>>> call, not the old one?
>>>
>>> The semantics would perhaps feel clearer as "get or create the shared
>>> derived device" rather than "get the first device derived or create a
>>> new one if not".
> 
> The line of code you commented on, is about adding a newly created
> derived device to the child collection of its parent.

Yes, exactly - I'm suggesting only sharing the device when asked to share devices.

>>> I wonder whether you only want to do this when the user made the new
>>> call, not the old one?
> 
> The difference between old and new is that old searches only parents
> and new searches the full tree.
> But should calling the old function also control whether a newly created
> derived device is added to the child collection of its parent?
> 
> It might be confusing behavior when you first call the old function
> and later call the new function but you would get the device only
> when it's a parent but not when it's a down-tree-child of any parent.
> 
> And that's where I said:
> 
>> Let me know what you think, I have no strong opinion about this.
> 
> Best regards,
> softworkz
- Mark
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH 1/3] avutils/hwcontext: add derive-device function which searches for existing devices in both directions

[Soft Works]

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Mark
> Thompson
> Sent: Tuesday, May 3, 2022 12:12 AM
> To: ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH 1/3] avutils/hwcontext: add derive-
> device function which searches for existing devices in both directions
> 
> On 02/05/2022 09:14, Soft Works wrote:
> >> -----Original Message-----
> >> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Mark
> >> Thompson
> >> Sent: Monday, May 2, 2022 12:01 AM
> >> To: ffmpeg-devel@ffmpeg.org
> >> Subject: Re: [FFmpeg-devel] [PATCH 1/3] avutils/hwcontext: add
> derive-
> >> device function which searches for existing devices in both
> directions
> >
> > [..]
> >
> >>>> * The thread-safety properties of the hwcontext API have been
> lost
> >> -
> >>>> you can no longer operate on devices independently across threads
> >>>> (insofar as the underlying API allows that).
> >>>>      Maybe there is an argument that derivation is something
> which
> >>>> should happen early on and therefore documenting it as thread-
> >> unsafe
> >>>> is ok, but when hwupload/hwmap can use it inside filtergraphs
> that
> >>>> just isn't going to happen (and will be violated in the FFmpeg
> >> utility
> >>>> if filters get threaded, as is being worked on).
> >>>
> >>>   From my understanding there will be a single separate thread
> which
> >>> handles all filtergraph operations.
> >>> I don't think it would even be possible (without massive changes)
> >>> to arbitrate filter processing in parallel.
> >>> But even if this would be implemented: the filtergraph setup
> (init,
> >>> uninit, query_formats, etc.) would surely happen on a single
> thread.
> >>
> >> The ffmpeg utility creates filtergraphs dynamically when the first
> >> frame is available from their inputs, so I don't see why you
> wouldn't
> >> allow multiple of them to be created in parallel in that case.
> >>
> >> If you create all devices at the beginning and then give references
> to
> >> them to the various filters which need them (so never manipulate
> >> devices dynamically within the graph) then it would be ok, but I
> think
> >> you've already rejected that approach.
> >
> > Please let's not break out of the scope of this patchset.
> > This patchset is not about re-doing device derivation. The only
> > small change that it does is that it returns an existing device
> > instead of creating a new one when such device already exists
> > in the same(!) chain.
> >
> > The change it makes has really nothing to do with threading or
> > anything around it.
> 
> The change makes existing thread-safe hwcontext APIs unsafe.  That is
> definitely not "nothing to do with threading", and has to be resolved
> since they can currently be called from contexts which expect thread-
> safety (such as making filtergraphs).

As I said, I'm not aware that filtergraph creation would be a context 
which requires thread-safety, even when considering frames which get 
pushed into a filtergraph (like from a decoder).

Could you please show a command line which causes a situation where
device_derive is being called from multiple threads?


> >>>> * I'm not sure that it is reasonable to ignore options.  If an
> >>>> unrelated component derived a device before you with special
> >> options,
> >>>> you might get that device even if you have incompatible different
> >>>> options.
> >>>
> >>> I understand what you mean, but this is outside the scope of
> >>> this patchset, because when you would want to do this, it
> >>> would need to be implemented for derivation in general, not
> >>> in this patchset which only adds reverse-search to the
> >>> existing derivation functionality.
> >>
> >> I'm not sure what you mean by that?  The feature already exists;
> here
> >> is a concrete example of where you would get the wrong result:
> >>
> >> Start with VAAPI device A.
> >>
> >> Component P derives Vulkan device B with some extension options X.
> >>
> >> Component Q wants the same device as P, so it derives again with
> >> extension options X and gets B.
> >>
> >> Everything works fine for a while.
> >>
> >> Later, unrelated component R is inserted before P and Q.  It wants
> a
> >> Vulkan device C with extension options Y, so it derives that.
> >>
> >> Now component Q is broken because it gets C instead of B and has
> the
> >> wrong extensions enabled.
> >
> > As per your request, this patchset's changes are now limited to
> > use ffmpeg cli. And there is currently no support for "extension
> > options" when deriving a device.
> 
> Yes there is - see the "instance_extensions" and "device_extensions"
> options to Vulkan device derivation.  (Hence the VAAPI+Vulkan
> example.)

OK, but when deriving a device via command line, it doesn't consider
such parameters in the current device_derive function, and hence it's
not something that can be burdened onto this patchset.


> > What I meant above is this:
> >
> > Assume this patchset wouldn't be applied, but a patchset would
> > be applied that allows to specify "extension options".
> > Then, even without this patchset, I could construct a similar
> > example, where you would get the same device when deriving
> > two times from the same device but with different extension
> > options.
> 
> How?

VAAPI >> QSV(Param1) >> OpenCL

Now, from OpenCL, you want to derive QSV but with different parameters
(Param2). You won't get a new device, you get the existing QSV device.


> The existing derivation setup always makes a new device, so you can't
> accidentally get an old one.

No, not always, see above.

> The existing way of reusing devices is to keep the reference and reuse
> it directly, which means you need to pass the reference around
> explicitly and there is no problem.

You can do this as API user, but this patch is about ffmpeg cli and as
per your request limited to ffmpeg cli usage.


> >> Can you explain your example further?
> >
> > Maybe we should get clear about what this patchset does exactly.
> > Let's look at the following derivation chain of devices:
> >
> > A
> > ├─ X
> > │  └─ Y
> > ├─ B
> > │  └─ C
> > └─ V
> >     └─ W
> >
> > The meaning is:
> >
> > - Y is derived from X, X is derived from A
> > - C is derived from B, B is derived from A
> > - W is derived from V, V is derived from A
> >
> > In the existing implementation, each device "knows" its parent
> > (via the 'source_device' field).
> >
> > When you call av_hwdevice_ctx_create_derived() and specify "C"
> > as the source device, then it will iterate the tree upwards,
> > so when B is of the requested type, it will return B or if
> > A is of the requested type, it will return A.
> > Otherwise, it will create a new device of the requested type
> > and sets C as its parent.
> >
> > But it doesn't return X, Y, V or W (when any would match the
> > requested type).
> >
> > This is the current behavior.
> >
> >
> > What this patchset does is that we also store the derived
> > children for each device (derived_devices array).
> >
> > In the example above, it means hat A has references to
> > X, B and V. X to Y, B to C and V to W.
> >
> > The behavior of the new function is as follows:
> >
> > When you call av_hwdevice_ctx_get_or_create_derived() and specify
> "C"
> > as the source device, then it will iterate the tree upwards,
> > so when B is of the requested type, it will return B or if
> > A is of the requested type, it will return A (like before).
> >
> > Additionally, it will also iterate all through other children
> > of B and other children of A. Which means that if X, Y, V or W
> > matches the requested type, it would return it.
> 
> No it doesn't?  Your new function find_derived_hwdevice_ctx() is
> called only for the original source device, and recurses into its
> (transitive) children.  It can't return any of X, Y, V or W when
> starting from C.

OK, that's my mistake. It's been a while...
What I described is the original behavior. There were reasons
to limit it this way.

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".

Re: [FFmpeg-devel] [PATCH 1/3] avutils/hwcontext: add derive-device function which searches for existing devices in both directions

[Mark Thompson]

On 02/05/2022 23:59, Soft Works wrote:
>> -----Original Message-----
>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Mark
>> Thompson
>> Sent: Tuesday, May 3, 2022 12:12 AM
>> To: ffmpeg-devel@ffmpeg.org
>> Subject: Re: [FFmpeg-devel] [PATCH 1/3] avutils/hwcontext: add derive-
>> device function which searches for existing devices in both directions
>>
>> On 02/05/2022 09:14, Soft Works wrote:
>>>> -----Original Message-----
>>>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
>> Mark
>>>> Thompson
>>>> Sent: Monday, May 2, 2022 12:01 AM
>>>> To: ffmpeg-devel@ffmpeg.org
>>>> Subject: Re: [FFmpeg-devel] [PATCH 1/3] avutils/hwcontext: add
>> derive-
>>>> device function which searches for existing devices in both
>> directions
>>>
>>> [..]
>>>
>>>>>> * The thread-safety properties of the hwcontext API have been
>> lost
>>>> -
>>>>>> you can no longer operate on devices independently across threads
>>>>>> (insofar as the underlying API allows that).
>>>>>>       Maybe there is an argument that derivation is something
>> which
>>>>>> should happen early on and therefore documenting it as thread-
>>>> unsafe
>>>>>> is ok, but when hwupload/hwmap can use it inside filtergraphs
>> that
>>>>>> just isn't going to happen (and will be violated in the FFmpeg
>>>> utility
>>>>>> if filters get threaded, as is being worked on).
>>>>>
>>>>>    From my understanding there will be a single separate thread
>> which
>>>>> handles all filtergraph operations.
>>>>> I don't think it would even be possible (without massive changes)
>>>>> to arbitrate filter processing in parallel.
>>>>> But even if this would be implemented: the filtergraph setup
>> (init,
>>>>> uninit, query_formats, etc.) would surely happen on a single
>> thread.
>>>>
>>>> The ffmpeg utility creates filtergraphs dynamically when the first
>>>> frame is available from their inputs, so I don't see why you
>> wouldn't
>>>> allow multiple of them to be created in parallel in that case.
>>>>
>>>> If you create all devices at the beginning and then give references
>> to
>>>> them to the various filters which need them (so never manipulate
>>>> devices dynamically within the graph) then it would be ok, but I
>> think
>>>> you've already rejected that approach.
>>>
>>> Please let's not break out of the scope of this patchset.
>>> This patchset is not about re-doing device derivation. The only
>>> small change that it does is that it returns an existing device
>>> instead of creating a new one when such device already exists
>>> in the same(!) chain.
>>>
>>> The change it makes has really nothing to do with threading or
>>> anything around it.
>>
>> The change makes existing thread-safe hwcontext APIs unsafe.  That is
>> definitely not "nothing to do with threading", and has to be resolved
>> since they can currently be called from contexts which expect thread-
>> safety (such as making filtergraphs).
> 
> As I said, I'm not aware that filtergraph creation would be a context
> which requires thread-safety, even when considering frames which get
> pushed into a filtergraph (like from a decoder).

User programs can do whatever they like within the API constraints.

> Could you please show a command line which causes a situation where
> device_derive is being called from multiple threads?

Given that the ffmpeg utility is currently entirely single-threaded, this will only happen once the intended plans for adding threading to it are complete.

With that, it will be true for something which makes two filtergraphs and uses derivation in both of them.  For example:, this currently makes two independent VAAPI devices, but equally could reuse the same device:

# ffmpeg -y -f kmsgrab -i /dev/dri/card0 -vf hwmap=derive_device=vaapi,scale_vaapi=format=nv12 -c:v h264_vaapi out1.mp4 -vf hwmap=derive_device=vaapi,scale_vaapi=w=1280:h=720:format=nv12 -c:v h264_vaapi out2.mp4

>>>>>> * I'm not sure that it is reasonable to ignore options.  If an
>>>>>> unrelated component derived a device before you with special
>>>> options,
>>>>>> you might get that device even if you have incompatible different
>>>>>> options.
>>>>>
>>>>> I understand what you mean, but this is outside the scope of
>>>>> this patchset, because when you would want to do this, it
>>>>> would need to be implemented for derivation in general, not
>>>>> in this patchset which only adds reverse-search to the
>>>>> existing derivation functionality.
>>>>
>>>> I'm not sure what you mean by that?  The feature already exists;
>> here
>>>> is a concrete example of where you would get the wrong result:
>>>>
>>>> Start with VAAPI device A.
>>>>
>>>> Component P derives Vulkan device B with some extension options X.
>>>>
>>>> Component Q wants the same device as P, so it derives again with
>>>> extension options X and gets B.
>>>>
>>>> Everything works fine for a while.
>>>>
>>>> Later, unrelated component R is inserted before P and Q.  It wants
>> a
>>>> Vulkan device C with extension options Y, so it derives that.
>>>>
>>>> Now component Q is broken because it gets C instead of B and has
>> the
>>>> wrong extensions enabled.
>>>
>>> As per your request, this patchset's changes are now limited to
>>> use ffmpeg cli. And there is currently no support for "extension
>>> options" when deriving a device.
>>
>> Yes there is - see the "instance_extensions" and "device_extensions"
>> options to Vulkan device derivation.  (Hence the VAAPI+Vulkan
>> example.)
> 
> OK, but when deriving a device via command line, it doesn't consider
> such parameters in the current device_derive function, and hence it's
> not something that can be burdened onto this patchset.

Then it sounds like you want this function to be part of the ffmpeg utility, not inside one of the libraries which have other users.

>>> What I meant above is this:
>>>
>>> Assume this patchset wouldn't be applied, but a patchset would
>>> be applied that allows to specify "extension options".
>>> Then, even without this patchset, I could construct a similar
>>> example, where you would get the same device when deriving
>>> two times from the same device but with different extension
>>> options.
>>
>> How?
> 
> VAAPI >> QSV(Param1) >> OpenCL
> 
> Now, from OpenCL, you want to derive QSV but with different parameters
> (Param2). You won't get a new device, you get the existing QSV device.

This doesn't make sense - remember that device derivation is unidirectional, and OpenCL is a leaf API which can only derive from other things.  The "derive" operation there has to be interpreted as "return the device this was derived from", in which case options don't make sense.

(Indeed, it seems like a good idea to disallow options in that case.  I will prepare a patch to that effect.)

>> The existing derivation setup always makes a new device, so you can't
>> accidentally get an old one.
> 
> No, not always, see above.
> 
>> The existing way of reusing devices is to keep the reference and reuse
>> it directly, which means you need to pass the reference around
>> explicitly and there is no problem.
> 
> You can do this as API user, but this patch is about ffmpeg cli and as
> per your request limited to ffmpeg cli usage.
> 
> 
>>>> Can you explain your example further?
>>>
>>> Maybe we should get clear about what this patchset does exactly.
>>> Let's look at the following derivation chain of devices:
>>>
>>> A
>>> ├─ X
>>> │  └─ Y
>>> ├─ B
>>> │  └─ C
>>> └─ V
>>>      └─ W
>>>
>>> The meaning is:
>>>
>>> - Y is derived from X, X is derived from A
>>> - C is derived from B, B is derived from A
>>> - W is derived from V, V is derived from A
>>>
>>> In the existing implementation, each device "knows" its parent
>>> (via the 'source_device' field).
>>>
>>> When you call av_hwdevice_ctx_create_derived() and specify "C"
>>> as the source device, then it will iterate the tree upwards,
>>> so when B is of the requested type, it will return B or if
>>> A is of the requested type, it will return A.
>>> Otherwise, it will create a new device of the requested type
>>> and sets C as its parent.
>>>
>>> But it doesn't return X, Y, V or W (when any would match the
>>> requested type).
>>>
>>> This is the current behavior.
>>>
>>>
>>> What this patchset does is that we also store the derived
>>> children for each device (derived_devices array).
>>>
>>> In the example above, it means hat A has references to
>>> X, B and V. X to Y, B to C and V to W.
>>>
>>> The behavior of the new function is as follows:
>>>
>>> When you call av_hwdevice_ctx_get_or_create_derived() and specify
>> "C"
>>> as the source device, then it will iterate the tree upwards,
>>> so when B is of the requested type, it will return B or if
>>> A is of the requested type, it will return A (like before).
>>>
>>> Additionally, it will also iterate all through other children
>>> of B and other children of A. Which means that if X, Y, V or W
>>> matches the requested type, it would return it.
>>
>> No it doesn't?  Your new function find_derived_hwdevice_ctx() is
>> called only for the original source device, and recurses into its
>> (transitive) children.  It can't return any of X, Y, V or W when
>> starting from C.
> 
> OK, that's my mistake. It's been a while...
> What I described is the original behavior. There were reasons
> to limit it this way.

Well, which one is is actually intended then?

- Mark
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH 1/3] avutils/hwcontext: add derive-device function which searches for existing devices in both directions

[Soft Works]

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Mark
> Thompson
> Sent: Tuesday, May 3, 2022 1:57 AM
> To: ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH 1/3] avutils/hwcontext: add derive-
> device function which searches for existing devices in both directions
> 
> On 02/05/2022 23:59, Soft Works wrote:
> >> -----Original Message-----
> >> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Mark
> >> Thompson
> >> Sent: Tuesday, May 3, 2022 12:12 AM
> >> To: ffmpeg-devel@ffmpeg.org
> >> Subject: Re: [FFmpeg-devel] [PATCH 1/3] avutils/hwcontext: add
> derive-
> >> device function which searches for existing devices in both
> directions
> >>
> >> On 02/05/2022 09:14, Soft Works wrote:
> >>>> -----Original Message-----
> >>>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> >> Mark
> >>>> Thompson
> >>>> Sent: Monday, May 2, 2022 12:01 AM
> >>>> To: ffmpeg-devel@ffmpeg.org
> >>>> Subject: Re: [FFmpeg-devel] [PATCH 1/3] avutils/hwcontext: add
> >> derive-
> >>>> device function which searches for existing devices in both
> >> directions
> >>>
> >>> [..]
> >>>
> >>>>>> * The thread-safety properties of the hwcontext API have been
> >> lost
> >>>> -
> >>>>>> you can no longer operate on devices independently across
> threads
> >>>>>> (insofar as the underlying API allows that).
> >>>>>>       Maybe there is an argument that derivation is something
> >> which
> >>>>>> should happen early on and therefore documenting it as thread-
> >>>> unsafe
> >>>>>> is ok, but when hwupload/hwmap can use it inside filtergraphs
> >> that
> >>>>>> just isn't going to happen (and will be violated in the FFmpeg
> >>>> utility
> >>>>>> if filters get threaded, as is being worked on).
> >>>>>
> >>>>>    From my understanding there will be a single separate thread
> >> which
> >>>>> handles all filtergraph operations.
> >>>>> I don't think it would even be possible (without massive
> changes)
> >>>>> to arbitrate filter processing in parallel.
> >>>>> But even if this would be implemented: the filtergraph setup
> >> (init,
> >>>>> uninit, query_formats, etc.) would surely happen on a single
> >> thread.
> >>>>
> >>>> The ffmpeg utility creates filtergraphs dynamically when the
> first
> >>>> frame is available from their inputs, so I don't see why you
> >> wouldn't
> >>>> allow multiple of them to be created in parallel in that case.
> >>>>
> >>>> If you create all devices at the beginning and then give
> references
> >> to
> >>>> them to the various filters which need them (so never manipulate
> >>>> devices dynamically within the graph) then it would be ok, but I
> >> think
> >>>> you've already rejected that approach.
> >>>
> >>> Please let's not break out of the scope of this patchset.
> >>> This patchset is not about re-doing device derivation. The only
> >>> small change that it does is that it returns an existing device
> >>> instead of creating a new one when such device already exists
> >>> in the same(!) chain.
> >>>
> >>> The change it makes has really nothing to do with threading or
> >>> anything around it.
> >>
> >> The change makes existing thread-safe hwcontext APIs unsafe.  That
> is
> >> definitely not "nothing to do with threading", and has to be
> resolved
> >> since they can currently be called from contexts which expect
> thread-
> >> safety (such as making filtergraphs).
> >
> > As I said, I'm not aware that filtergraph creation would be a
> context
> > which requires thread-safety, even when considering frames which get
> > pushed into a filtergraph (like from a decoder).
> 
> User programs can do whatever they like within the API constraints.
> 
> > Could you please show a command line which causes a situation where
> > device_derive is being called from multiple threads?
> 
> Given that the ffmpeg utility is currently entirely single-threaded,
> this will only happen once the intended plans for adding threading to
> it are complete.

As mentioned, I don't think that would be possible easily, and from 
what I have understood, the plan is to have separate threads for decoding,
encoding and filtering but not multiple threads for filtering.


> With that, it will be true for something which makes two filtergraphs
> and uses derivation in both of them.  For example:, this currently
> makes two independent VAAPI devices, but equally could reuse the same
> device:
> 
> # ffmpeg -y -f kmsgrab -i /dev/dri/card0 -vf
> hwmap=derive_device=vaapi,scale_vaapi=format=nv12 -c:v h264_vaapi
> out1.mp4 -vf
> hwmap=derive_device=vaapi,scale_vaapi=w=1280:h=720:format=nv12 -c:v
> h264_vaapi out2.mp4

Well, multi-threading is not an issue right now, and I don't expect it
to be then.

But on a more practical take: what do you want me to do?

Guard that function with a lock? I can do this, no problem. But
none of any of the device control function does any synchronization,
so why would exactly this - and only this function need synchronization?


> >>>>>> * I'm not sure that it is reasonable to ignore options.  If an
> >>>>>> unrelated component derived a device before you with special
> >>>> options,
> >>>>>> you might get that device even if you have incompatible
> different
> >>>>>> options.
> >>>>>
> >>>>> I understand what you mean, but this is outside the scope of
> >>>>> this patchset, because when you would want to do this, it
> >>>>> would need to be implemented for derivation in general, not
> >>>>> in this patchset which only adds reverse-search to the
> >>>>> existing derivation functionality.
> >>>>
> >>>> I'm not sure what you mean by that?  The feature already exists;
> >> here
> >>>> is a concrete example of where you would get the wrong result:
> >>>>
> >>>> Start with VAAPI device A.
> >>>>
> >>>> Component P derives Vulkan device B with some extension options
> X.
> >>>>
> >>>> Component Q wants the same device as P, so it derives again with
> >>>> extension options X and gets B.
> >>>>
> >>>> Everything works fine for a while.
> >>>>
> >>>> Later, unrelated component R is inserted before P and Q.  It
> wants
> >> a
> >>>> Vulkan device C with extension options Y, so it derives that.
> >>>>
> >>>> Now component Q is broken because it gets C instead of B and has
> >> the
> >>>> wrong extensions enabled.
> >>>
> >>> As per your request, this patchset's changes are now limited to
> >>> use ffmpeg cli. And there is currently no support for "extension
> >>> options" when deriving a device.
> >>
> >> Yes there is - see the "instance_extensions" and
> "device_extensions"
> >> options to Vulkan device derivation.  (Hence the VAAPI+Vulkan
> >> example.)
> >
> > OK, but when deriving a device via command line, it doesn't consider
> > such parameters in the current device_derive function, and hence
> it's
> > not something that can be burdened onto this patchset.
> 
> Then it sounds like you want this function to be part of the ffmpeg
> utility, not inside one of the libraries which have other users.

No. I say, matching device parameters when searching for an existing 
device isn't done right now, and it's outside the scope of this patchset.


> >>> What I meant above is this:
> >>>
> >>> Assume this patchset wouldn't be applied, but a patchset would
> >>> be applied that allows to specify "extension options".
> >>> Then, even without this patchset, I could construct a similar
> >>> example, where you would get the same device when deriving
> >>> two times from the same device but with different extension
> >>> options.
> >>
> >> How?
> >
> > VAAPI >> QSV(Param1) >> OpenCL
> >
> > Now, from OpenCL, you want to derive QSV but with different
> parameters
> > (Param2). You won't get a new device, you get the existing QSV
> device.
> 
> This doesn't make sense - remember that device derivation is
> unidirectional, and OpenCL is a leaf API which can only derive from
> other things.  The "derive" operation there has to be interpreted as
> "return the device this was derived from", in which case options don't
> make sense.
> 
> (Indeed, it seems like a good idea to disallow options in that case.
> I will prepare a patch to that effect.)

OK.

> >> No it doesn't?  Your new function find_derived_hwdevice_ctx() is
> >> called only for the original source device, and recurses into its
> >> (transitive) children.  It can't return any of X, Y, V or W when
> >> starting from C.
> >
> > OK, that's my mistake. It's been a while...
> > What I described is the original behavior. There were reasons
> > to limit it this way.
> 
> Well, which one is is actually intended then?

This patchset. It is used by us and by Intel for quite a while already.

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".

Re: [FFmpeg-devel] [PATCH 1/3] avutils/hwcontext: add derive-device function which searches for existing devices in both directions

[Mark Thompson]

On 03/05/2022 01:11, Soft Works wrote:
>> -----Original Message-----
>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Mark
>> Thompson
>> Sent: Tuesday, May 3, 2022 1:57 AM
>> To: ffmpeg-devel@ffmpeg.org
>> Subject: Re: [FFmpeg-devel] [PATCH 1/3] avutils/hwcontext: add derive-
>> device function which searches for existing devices in both directions
>>
>> On 02/05/2022 23:59, Soft Works wrote:
>>>> -----Original Message-----
>>>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
>> Mark
>>>> Thompson
>>>> Sent: Tuesday, May 3, 2022 12:12 AM
>>>> To: ffmpeg-devel@ffmpeg.org
>>>> Subject: Re: [FFmpeg-devel] [PATCH 1/3] avutils/hwcontext: add
>> derive-
>>>> device function which searches for existing devices in both
>> directions
>>>>
>>>> On 02/05/2022 09:14, Soft Works wrote:
>>>>>> -----Original Message-----
>>>>>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
>>>> Mark
>>>>>> Thompson
>>>>>> Sent: Monday, May 2, 2022 12:01 AM
>>>>>> To: ffmpeg-devel@ffmpeg.org
>>>>>> Subject: Re: [FFmpeg-devel] [PATCH 1/3] avutils/hwcontext: add
>>>> derive-
>>>>>> device function which searches for existing devices in both
>>>> directions
>>>>>
>>>>> [..]
>>>>>
>>>>>>>> * The thread-safety properties of the hwcontext API have been
>>>> lost
>>>>>> -
>>>>>>>> you can no longer operate on devices independently across
>> threads
>>>>>>>> (insofar as the underlying API allows that).
>>>>>>>>        Maybe there is an argument that derivation is something
>>>> which
>>>>>>>> should happen early on and therefore documenting it as thread-
>>>>>> unsafe
>>>>>>>> is ok, but when hwupload/hwmap can use it inside filtergraphs
>>>> that
>>>>>>>> just isn't going to happen (and will be violated in the FFmpeg
>>>>>> utility
>>>>>>>> if filters get threaded, as is being worked on).
>>>>>>>
>>>>>>>     From my understanding there will be a single separate thread
>>>> which
>>>>>>> handles all filtergraph operations.
>>>>>>> I don't think it would even be possible (without massive
>> changes)
>>>>>>> to arbitrate filter processing in parallel.
>>>>>>> But even if this would be implemented: the filtergraph setup
>>>> (init,
>>>>>>> uninit, query_formats, etc.) would surely happen on a single
>>>> thread.
>>>>>>
>>>>>> The ffmpeg utility creates filtergraphs dynamically when the
>> first
>>>>>> frame is available from their inputs, so I don't see why you
>>>> wouldn't
>>>>>> allow multiple of them to be created in parallel in that case.
>>>>>>
>>>>>> If you create all devices at the beginning and then give
>> references
>>>> to
>>>>>> them to the various filters which need them (so never manipulate
>>>>>> devices dynamically within the graph) then it would be ok, but I
>>>> think
>>>>>> you've already rejected that approach.
>>>>>
>>>>> Please let's not break out of the scope of this patchset.
>>>>> This patchset is not about re-doing device derivation. The only
>>>>> small change that it does is that it returns an existing device
>>>>> instead of creating a new one when such device already exists
>>>>> in the same(!) chain.
>>>>>
>>>>> The change it makes has really nothing to do with threading or
>>>>> anything around it.
>>>>
>>>> The change makes existing thread-safe hwcontext APIs unsafe.  That
>> is
>>>> definitely not "nothing to do with threading", and has to be
>> resolved
>>>> since they can currently be called from contexts which expect
>> thread-
>>>> safety (such as making filtergraphs).
>>>
>>> As I said, I'm not aware that filtergraph creation would be a
>> context
>>> which requires thread-safety, even when considering frames which get
>>> pushed into a filtergraph (like from a decoder).
>>
>> User programs can do whatever they like within the API constraints.
>>
>>> Could you please show a command line which causes a situation where
>>> device_derive is being called from multiple threads?
>>
>> Given that the ffmpeg utility is currently entirely single-threaded,
>> this will only happen once the intended plans for adding threading to
>> it are complete.
> 
> As mentioned, I don't think that would be possible easily, and from
> what I have understood, the plan is to have separate threads for decoding,
> encoding and filtering but not multiple threads for filtering.

As the summary in <https://lists.ffmpeg.org/pipermail/ffmpeg-devel/2022-April/294940.html> states, the intent is a separate thread for each instance of those things, including filtergraphs.

>> With that, it will be true for something which makes two filtergraphs
>> and uses derivation in both of them.  For example:, this currently
>> makes two independent VAAPI devices, but equally could reuse the same
>> device:
>>
>> # ffmpeg -y -f kmsgrab -i /dev/dri/card0 -vf
>> hwmap=derive_device=vaapi,scale_vaapi=format=nv12 -c:v h264_vaapi
>> out1.mp4 -vf
>> hwmap=derive_device=vaapi,scale_vaapi=w=1280:h=720:format=nv12 -c:v
>> h264_vaapi out2.mp4
> 
> Well, multi-threading is not an issue right now, and I don't expect it
> to be then.
> 
> But on a more practical take: what do you want me to do?
> 
> Guard that function with a lock? I can do this, no problem.

I'd like it to be designed in a way that avoids this sort of problem, as all the existing functions are.

>                                                             But
> none of any of the device control function does any synchronization,
> so why would exactly this - and only this function need synchronization?

The derived_devices field is modified post-creation, which gives you data races if there is no other synchronisation.

(Consider simultaneous derivation calls: currently that's completely fine - it makes no changes to the parent device and the derived devices are independent.  With your proposed patch there is undefined behaviour because of the simultaneous reading and writing of the derived_devices field.)

The only other mutable field is the buffer pool in a frames context, which has its own internal synchronisation.

Having thought about this a bit further, I suspect you are going to need an additional shared-devices object of some kind in order to get around the circular references and fix the memory leak problem.

That additional object will have to be mutable and accessible from multiple threads, so will probably need an internal lock (or careful lock-free design).

- Mark
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH 1/3] avutils/hwcontext: add derive-device function which searches for existing devices in both directions

[Soft Works]

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Mark
> Thompson
> Sent: Tuesday, May 3, 2022 10:23 PM
> To: ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH 1/3] avutils/hwcontext: add derive-
> device function which searches for existing devices in both directions
> 
> On 03/05/2022 01:11, Soft Works wrote:
> >> -----Original Message-----
> >> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Mark
> >> Thompson
> >> Sent: Tuesday, May 3, 2022 1:57 AM
> >> To: ffmpeg-devel@ffmpeg.org
> >> Subject: Re: [FFmpeg-devel] [PATCH 1/3] avutils/hwcontext: add
> derive-
> >> device function which searches for existing devices in both
> directions
> >>
> >> On 02/05/2022 23:59, Soft Works wrote:
> >>>> -----Original Message-----
> >>>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> >> Mark
> >>>> Thompson
> >>>> Sent: Tuesday, May 3, 2022 12:12 AM
> >>>> To: ffmpeg-devel@ffmpeg.org
> >>>> Subject: Re: [FFmpeg-devel] [PATCH 1/3] avutils/hwcontext: add
> >> derive-
> >>>> device function which searches for existing devices in both
> >> directions
> >>>>
> >>>> On 02/05/2022 09:14, Soft Works wrote:
> >>>>>> -----Original Message-----
> >>>>>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf
> Of
> >>>> Mark
> >>>>>> Thompson
> >>>>>> Sent: Monday, May 2, 2022 12:01 AM
> >>>>>> To: ffmpeg-devel@ffmpeg.org
> >>>>>> Subject: Re: [FFmpeg-devel] [PATCH 1/3] avutils/hwcontext: add
> >>>> derive-
> >>>>>> device function which searches for existing devices in both
> >>>> directions
> >>>>>
> >>>>> [..]
> >>>>>
> >>>>>>>> * The thread-safety properties of the hwcontext API have been
> >>>> lost
> >>>>>> -
> >>>>>>>> you can no longer operate on devices independently across
> >> threads
> >>>>>>>> (insofar as the underlying API allows that).
> >>>>>>>>        Maybe there is an argument that derivation is
> something
> >>>> which
> >>>>>>>> should happen early on and therefore documenting it as
> thread-
> >>>>>> unsafe
> >>>>>>>> is ok, but when hwupload/hwmap can use it inside filtergraphs
> >>>> that
> >>>>>>>> just isn't going to happen (and will be violated in the
> FFmpeg
> >>>>>> utility
> >>>>>>>> if filters get threaded, as is being worked on).
> >>>>>>>
> >>>>>>>     From my understanding there will be a single separate
> thread
> >>>> which
> >>>>>>> handles all filtergraph operations.
> >>>>>>> I don't think it would even be possible (without massive
> >> changes)
> >>>>>>> to arbitrate filter processing in parallel.
> >>>>>>> But even if this would be implemented: the filtergraph setup
> >>>> (init,
> >>>>>>> uninit, query_formats, etc.) would surely happen on a single
> >>>> thread.
> >>>>>>
> >>>>>> The ffmpeg utility creates filtergraphs dynamically when the
> >> first
> >>>>>> frame is available from their inputs, so I don't see why you
> >>>> wouldn't
> >>>>>> allow multiple of them to be created in parallel in that case.
> >>>>>>
> >>>>>> If you create all devices at the beginning and then give
> >> references
> >>>> to
> >>>>>> them to the various filters which need them (so never
> manipulate
> >>>>>> devices dynamically within the graph) then it would be ok, but
> I
> >>>> think
> >>>>>> you've already rejected that approach.
> >>>>>
> >>>>> Please let's not break out of the scope of this patchset.
> >>>>> This patchset is not about re-doing device derivation. The only
> >>>>> small change that it does is that it returns an existing device
> >>>>> instead of creating a new one when such device already exists
> >>>>> in the same(!) chain.
> >>>>>
> >>>>> The change it makes has really nothing to do with threading or
> >>>>> anything around it.
> >>>>
> >>>> The change makes existing thread-safe hwcontext APIs unsafe.
> That
> >> is
> >>>> definitely not "nothing to do with threading", and has to be
> >> resolved
> >>>> since they can currently be called from contexts which expect
> >> thread-
> >>>> safety (such as making filtergraphs).
> >>>
> >>> As I said, I'm not aware that filtergraph creation would be a
> >> context
> >>> which requires thread-safety, even when considering frames which
> get
> >>> pushed into a filtergraph (like from a decoder).
> >>
> >> User programs can do whatever they like within the API constraints.
> >>
> >>> Could you please show a command line which causes a situation
> where
> >>> device_derive is being called from multiple threads?
> >>
> >> Given that the ffmpeg utility is currently entirely single-
> threaded,
> >> this will only happen once the intended plans for adding threading
> to
> >> it are complete.
> >
> > As mentioned, I don't think that would be possible easily, and from
> > what I have understood, the plan is to have separate threads for
> decoding,
> > encoding and filtering but not multiple threads for filtering.
> 
> As the summary in <https://lists.ffmpeg.org/pipermail/ffmpeg-
> devel/2022-April/294940.html> states, the intent is a separate thread
> for each instance of those things, including filtergraphs.
> 
> >> With that, it will be true for something which makes two
> filtergraphs
> >> and uses derivation in both of them.  For example:, this currently
> >> makes two independent VAAPI devices, but equally could reuse the
> same
> >> device:
> >>
> >> # ffmpeg -y -f kmsgrab -i /dev/dri/card0 -vf
> >> hwmap=derive_device=vaapi,scale_vaapi=format=nv12 -c:v h264_vaapi
> >> out1.mp4 -vf
> >> hwmap=derive_device=vaapi,scale_vaapi=w=1280:h=720:format=nv12 -c:v
> >> h264_vaapi out2.mp4
> >
> > Well, multi-threading is not an issue right now, and I don't expect
> it
> > to be then.
> >
> > But on a more practical take: what do you want me to do?
> >
> > Guard that function with a lock? I can do this, no problem.
> 
> I'd like it to be designed in a way that avoids this sort of problem,
> as all the existing functions are.
> 
> >                                                             But
> > none of any of the device control function does any synchronization,
> > so why would exactly this - and only this function need
> synchronization?
> 
> The derived_devices field is modified post-creation, which gives you
> data races if there is no other synchronisation.
> 
> (Consider simultaneous derivation calls: currently that's completely
> fine - it makes no changes to the parent device and the derived
> devices are independent.  With your proposed patch there is undefined
> behaviour because of the simultaneous reading and writing of the
> derived_devices field.)
> 
> The only other mutable field is the buffer pool in a frames context,
> which has its own internal synchronisation.
> 
> Having thought about this a bit further, I suspect you are going to
> need an additional shared-devices object of some kind in order to get
> around the circular references and fix the memory leak problem.
> 
> That additional object will have to be mutable and accessible from
> multiple threads, so will probably need an internal lock (or careful
> lock-free design).

Yup, that sounds quite reasonable to me.
Very helpful comment, thanks!


_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH 1/3] avutils/hwcontext: add derive-device function which searches for existing devices in both directions

[Soft Works]

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Mark Thompson
> Sent: Tuesday, May 3, 2022 10:23 PM
> To: ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH 1/3] avutils/hwcontext: add
> derive-device function which searches for existing devices in both
> directions
> 
> On 03/05/2022 01:11, Soft Works wrote:
> >> -----Original Message-----
> >> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Mark
> >> Thompson
> >> Sent: Tuesday, May 3, 2022 1:57 AM
> >> To: ffmpeg-devel@ffmpeg.org
> >> Subject: Re: [FFmpeg-devel] [PATCH 1/3] avutils/hwcontext: add
> derive-
> >> device function which searches for existing devices in both
> directions
> >>
> >> On 02/05/2022 23:59, Soft Works wrote:
> >>>> -----Original Message-----
> >>>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf
> Of
> >> Mark
> >>>> Thompson
> >>>> Sent: Tuesday, May 3, 2022 12:12 AM
> >>>> To: ffmpeg-devel@ffmpeg.org
> >>>> Subject: Re: [FFmpeg-devel] [PATCH 1/3] avutils/hwcontext: add
> >> derive-
> >>>> device function which searches for existing devices in both
> >> directions
> >>>>
> >>>> On 02/05/2022 09:14, Soft Works wrote:
> >>>>>> -----Original Message-----
> >>>>>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf
> Of
> >>>> Mark
> >>>>>> Thompson
> >>>>>> Sent: Monday, May 2, 2022 12:01 AM
> >>>>>> To: ffmpeg-devel@ffmpeg.org
> >>>>>> Subject: Re: [FFmpeg-devel] [PATCH 1/3] avutils/hwcontext: add
> >>>> derive-
> >>>>>> device function which searches for existing devices in both
> >>>> directions
> >>>>>
> >>>>> [..]
> >>>>>
> >>>>>>>> * The thread-safety properties of the hwcontext API have
> been
> >>>> lost
> >>>>>> -
> >>>>>>>> you can no longer operate on devices independently across
> >> threads
> >>>>>>>> (insofar as the underlying API allows that).
> >>>>>>>>        Maybe there is an argument that derivation is
> something
> >>>> which
> >>>>>>>> should happen early on and therefore documenting it as
> thread-
> >>>>>> unsafe
> >>>>>>>> is ok, but when hwupload/hwmap can use it inside
> filtergraphs
> >>>> that
> >>>>>>>> just isn't going to happen (and will be violated in the
> FFmpeg
> >>>>>> utility
> >>>>>>>> if filters get threaded, as is being worked on).
> >>>>>>>
> >>>>>>>     From my understanding there will be a single separate
> thread
> >>>> which
> >>>>>>> handles all filtergraph operations.
> >>>>>>> I don't think it would even be possible (without massive
> >> changes)
> >>>>>>> to arbitrate filter processing in parallel.
> >>>>>>> But even if this would be implemented: the filtergraph setup
> >>>> (init,
> >>>>>>> uninit, query_formats, etc.) would surely happen on a single
> >>>> thread.
> >>>>>>
> >>>>>> The ffmpeg utility creates filtergraphs dynamically when the
> >> first
> >>>>>> frame is available from their inputs, so I don't see why you
> >>>> wouldn't
> >>>>>> allow multiple of them to be created in parallel in that case.
> >>>>>>
> >>>>>> If you create all devices at the beginning and then give
> >> references
> >>>> to
> >>>>>> them to the various filters which need them (so never
> manipulate
> >>>>>> devices dynamically within the graph) then it would be ok, but
> I
> >>>> think
> >>>>>> you've already rejected that approach.
> >>>>>
> >>>>> Please let's not break out of the scope of this patchset.
> >>>>> This patchset is not about re-doing device derivation. The only
> >>>>> small change that it does is that it returns an existing device
> >>>>> instead of creating a new one when such device already exists
> >>>>> in the same(!) chain.
> >>>>>
> >>>>> The change it makes has really nothing to do with threading or
> >>>>> anything around it.
> >>>>
> >>>> The change makes existing thread-safe hwcontext APIs unsafe.
> That
> >> is
> >>>> definitely not "nothing to do with threading", and has to be
> >> resolved
> >>>> since they can currently be called from contexts which expect
> >> thread-
> >>>> safety (such as making filtergraphs).
> >>>
> >>> As I said, I'm not aware that filtergraph creation would be a
> >> context
> >>> which requires thread-safety, even when considering frames which
> get
> >>> pushed into a filtergraph (like from a decoder).
> >>
> >> User programs can do whatever they like within the API
> constraints.
> >>
> >>> Could you please show a command line which causes a situation
> where
> >>> device_derive is being called from multiple threads?
> >>
> >> Given that the ffmpeg utility is currently entirely single-
> threaded,
> >> this will only happen once the intended plans for adding threading
> to
> >> it are complete.
> >
> > As mentioned, I don't think that would be possible easily, and from
> > what I have understood, the plan is to have separate threads for
> decoding,
> > encoding and filtering but not multiple threads for filtering.
> 
> As the summary in <https://lists.ffmpeg.org/pipermail/ffmpeg-
> devel/2022-April/294940.html> states, the intent is a separate thread
> for each instance of those things, including filtergraphs.
> 
> >> With that, it will be true for something which makes two
> filtergraphs
> >> and uses derivation in both of them.  For example:, this currently
> >> makes two independent VAAPI devices, but equally could reuse the
> same
> >> device:
> >>
> >> # ffmpeg -y -f kmsgrab -i /dev/dri/card0 -vf
> >> hwmap=derive_device=vaapi,scale_vaapi=format=nv12 -c:v h264_vaapi
> >> out1.mp4 -vf
> >> hwmap=derive_device=vaapi,scale_vaapi=w=1280:h=720:format=nv12 -
> c:v
> >> h264_vaapi out2.mp4
> >
> > Well, multi-threading is not an issue right now, and I don't expect
> it
> > to be then.
> >
> > But on a more practical take: what do you want me to do?
> >
> > Guard that function with a lock? I can do this, no problem.


Hi Mark,

I have submitted a new patchset based on your suggestions:
https://patchwork.ffmpeg.org/project/ffmpeg/list/?series=6655


> I'd like it to be designed in a way that avoids this sort of problem,
> as all the existing functions are.

Understood and accepted.


> > none of any of the device control function does any
> synchronization,
> > so why would exactly this - and only this function need
> synchronization?
> 
> The derived_devices field is modified post-creation, which gives you
> data races if there is no other synchronisation.
> 
> (Consider simultaneous derivation calls: currently that's completely
> fine - it makes no changes to the parent device and the derived
> devices are independent.  With your proposed patch there is undefined
> behaviour because of the simultaneous reading and writing of the
> derived_devices field.)
> 
> The only other mutable field is the buffer pool in a frames context,
> which has its own internal synchronisation.

I had thought there must be other cases of race conditions because
none of the other functions are employing any synchronization patterns,
but - at least with regards to device ctx - I realized that you are right
in that there aren't any races (in "regular" use cases at least).


> Having thought about this a bit further, I suspect you are going to
> need an additional shared-devices object of some kind in order to get
> around the circular references and fix the memory leak problem.
> 
> That additional object will have to be mutable and accessible from
> multiple threads, so will probably need an internal lock (or careful
> lock-free design).


I think I have found a safe and simple solution to both requirements:
weak-references and synchronization.


Weak References
===============

The use of AVBuffer and AVBufferRef is a well-established pattern, so it's
surely not an option to make any fundamental changes or start using a 
different pattern for a specific use case. Unfortunately, its  implementation
doesn't leave room for adapting it as a weak-reference pattern.

So came to the following pattern:

- When a device context is created it is registered in a global array
  (with thread-safe access) and gets a registration id (1, 2, 3, ...)
  => see register_hw_device()

- The registration id is the "weak reference"

- When a device context is freed, it gets unregistered 
  (removed from the array)
  => see unregister_hw_device()

- Now, a device context can reference other device contexts just 
  through that number (of course not knowing whether the corresponding
  device is still "alive")

- But this can be be determined by querying from the array which can return an
  AVBufferRef to that device on success´
  => see get_registered_hw_device()


Synchronization
===============

- All register and unregister calls are locked by a mutex
- Querying for a device by registration id is locked in the same way
  and returns an AVBufferRef to the device context, making sure
  that the returned device context will remain valid and not be
  freed


Specifics
=========

I had one problem to make it work as described above: How to return
an AVBufferRef to the device context from get_registered_hw_device()?

Currently, an AVBufferRef can only be created from another AVBufferRef.
But neither the device context nor the registration array can store
an AVBufferRef to the context, because that additional reference would
cause the device context to never be freed at all. 
Neither could we use the AVBufferRef that is supplied for 
av_hwdevice_ctx_init(), because it could be released at some time while
another AVBufferRef is still holding a reference.

That's why I chose to use and store AVBuffer itself. AVBuffer itself 
is not suitable for use as a weak reference, because it's not possible
to determine from the pointer once and whether it is av_free'd or not.

Though, in the context of hwdevice we don't have that problem due
to the use of hwdevice_ctx_free() as a custom callback for freeing
the AVBuffer, so we can safely use AVBuffer to store as a reference
in the global device context registration array.

The reference count is stored in AVBuffer anyway, and that leaves us
with just one requirement:

Being able to create an AVBufferRef from an AVBuffer directly instead
of from another AVBufferRef.
This is done in PATCH 1/4: add av_ref_from_buffer() function



I hope this makes sense to you and fulfills the requirements you 
are asking for.
Please let me know what you think.

Thanks,
sw
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH 1/3] avutils/hwcontext: add derive-device function which searches for existing devices in both directions

[Soft Works]

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Mark
> Thompson
> Sent: Monday, May 2, 2022 12:01 AM
> To: ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH 1/3] avutils/hwcontext: add derive-
> device function which searches for existing devices in both directions

[..]

> > There was a reason why this can't be used. IIRC, it was that the
> > device count needs to be known at some other place where
> > av_hwdevice_iterate_types() is not available.
> >
> > Please see the previous discussion with Hendrik about this.
> 
> Where is that?  The only place I see this used is the array of derived
> devices.

Sorry, it was with Lynne, not Hendrik:

https://www.mail-archive.com/ffmpeg-devel@ffmpeg.org/msg127768.html

> Two alternative implementations without the constant spring to mind:
> 
> * A shorter array indexed by av_hwdevice_iterate_types() which would
> not have empty entries for devices not compatible with the current
> platform.
> 
> * An array of type/reference pairs.

I would prefer not to over-engineer this. Everybody else seemed to
be OK with the current approach.

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".

[FFmpeg-devel] [PATCH 2/3] lavu: bump minor version and add doc/APIchanges entry for av_hwdevice_ctx_get_or_create_derived()

[softworkz]

From: softworkz <softworkz@hotmail.com>

Signed-off-by: softworkz <softworkz@hotmail.com>
---
 doc/APIchanges      | 3 +++
 libavutil/version.h | 4 ++--
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/doc/APIchanges b/doc/APIchanges
index 1a9f0a303e..3d467bd3d6 100644
--- a/doc/APIchanges
+++ b/doc/APIchanges
@@ -14,6 +14,9 @@ libavutil:     2021-04-27
 
 API changes, most recent first:
 
+2022-04-30 - xxxxxxxxxx - lavu 57.25.100 - hwcontext.h
+  Add av_hwdevice_ctx_get_or_create_derived().
+
 2022-03-16 - xxxxxxxxxx - all libraries - version_major.h
   Add lib<name>/version_major.h as new installed headers, which only
   contain the major version number (and corresponding API deprecation
diff --git a/libavutil/version.h b/libavutil/version.h
index 6735c20090..dd7d20a9fa 100644
--- a/libavutil/version.h
+++ b/libavutil/version.h
@@ -79,8 +79,8 @@
  */
 
 #define LIBAVUTIL_VERSION_MAJOR  57
-#define LIBAVUTIL_VERSION_MINOR  24
-#define LIBAVUTIL_VERSION_MICRO 101
+#define LIBAVUTIL_VERSION_MINOR  25
+#define LIBAVUTIL_VERSION_MICRO 100
 
 #define LIBAVUTIL_VERSION_INT   AV_VERSION_INT(LIBAVUTIL_VERSION_MAJOR, \
                                                LIBAVUTIL_VERSION_MINOR, \
-- 
ffmpeg-codebot

_______________________________________________
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".

[FFmpeg-devel] [PATCH 3/3] avfilter/hwmap, hwupload: use new av_hwdevice_ctx_get_or_create_derived method

[softworkz]

From: softworkz <softworkz@hotmail.com>

Signed-off-by: softworkz <softworkz@hotmail.com>
---
 libavfilter/vf_hwmap.c    | 4 ++--
 libavfilter/vf_hwupload.c | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/libavfilter/vf_hwmap.c b/libavfilter/vf_hwmap.c
index 2e03dfc1fe..b79cf6732c 100644
--- a/libavfilter/vf_hwmap.c
+++ b/libavfilter/vf_hwmap.c
@@ -82,8 +82,8 @@ static int hwmap_config_output(AVFilterLink *outlink)
                 goto fail;
             }
 
-            err = av_hwdevice_ctx_create_derived(&device, type,
-                                                 hwfc->device_ref, 0);
+            err = av_hwdevice_ctx_get_or_create_derived(&device, type,
+                                                        hwfc->device_ref, 0);
             if (err < 0) {
                 av_log(avctx, AV_LOG_ERROR, "Failed to created derived "
                        "device context: %d.\n", err);
diff --git a/libavfilter/vf_hwupload.c b/libavfilter/vf_hwupload.c
index dbc41734cc..41ee0e43c4 100644
--- a/libavfilter/vf_hwupload.c
+++ b/libavfilter/vf_hwupload.c
@@ -51,7 +51,7 @@ static int hwupload_query_formats(AVFilterContext *avctx)
         /* We already have a specified device. */
     } else if (avctx->hw_device_ctx) {
         if (ctx->device_type) {
-            err = av_hwdevice_ctx_create_derived(
+            err = av_hwdevice_ctx_get_or_create_derived(
                 &ctx->hwdevice_ref,
                 av_hwdevice_find_type_by_name(ctx->device_type),
                 avctx->hw_device_ctx, 0);
-- 
ffmpeg-codebot
_______________________________________________
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".

[FFmpeg-devel] [PATCH v2 0/4] Add derive-device function which searches for existing devices in both directions

[ffmpegagent]

This is an updated version of: [PATCH v4 1/1] avutils/hwcontext: When
deriving a hwdevice, search for existing device in both directions

There has been an objection that the earlier patchset would change API
behavior, and that this change should be limited to ffmpeg cli.

To achieve this, the API behavior is left unchanged now and a new function
av_hwdevice_ctx_get_or_create_derived() is added and used by the hwupload
and hwmap filters.

v2: Implemented concept for "weak references" to avoid circular reference
lockup.

softworkz (4):
  avutil/buffer: add av_ref_from_buffer() function
  avutils/hwcontext: add derive-device function which searches for
    existing devices in both directions
  lavu: bump minor version and add doc/APIchanges entry for
    av_hwdevice_ctx_get_or_create_derived()
  avfilter/hwmap,hwupload: use new av_hwdevice_ctx_get_or_create_derived
    method

 doc/APIchanges                 |   6 ++
 libavfilter/vf_hwmap.c         |   4 +-
 libavfilter/vf_hwupload.c      |   2 +-
 libavutil/buffer.c             |  16 ++++
 libavutil/buffer.h             |   8 ++
 libavutil/hwcontext.c          | 167 +++++++++++++++++++++++++++++++--
 libavutil/hwcontext.h          |  20 ++++
 libavutil/hwcontext_internal.h |  11 +++
 libavutil/hwcontext_qsv.c      |  11 ++-
 libavutil/version.h            |   4 +-
 10 files changed, 234 insertions(+), 15 deletions(-)


base-commit: 9ab20b1614194280b862d98dfcdb7b1bcff03329
Published-As: https://github.com/ffstaging/FFmpeg/releases/tag/pr-ffstaging-25%2Fsoftworkz%2Fderive_devices-v2
Fetch-It-Via: git fetch https://github.com/ffstaging/FFmpeg pr-ffstaging-25/softworkz/derive_devices-v2
Pull-Request: https://github.com/ffstaging/FFmpeg/pull/25

Range-diff vs v1:

 -:  ---------- > 1:  e9c920c237 avutil/buffer: add av_ref_from_buffer() function
 1:  85ff784b6f ! 2:  bec8ccfcc2 avutils/hwcontext: add derive-device function which searches for existing devices in both directions
     @@ Commit message
          Signed-off-by: softworkz <softworkz@hotmail.com>
      
       ## libavutil/hwcontext.c ##
     -@@ libavutil/hwcontext.c: static const AVClass hwdevice_ctx_class = {
     - static void hwdevice_ctx_free(void *opaque, uint8_t *data)
     +@@
     + #include "buffer.h"
     + #include "common.h"
     + #include "hwcontext.h"
     ++
     ++#include <stdatomic.h>
     ++
     ++#include "buffer_internal.h"
     + #include "hwcontext_internal.h"
     + #include "imgutils.h"
     + #include "log.h"
     + #include "mem.h"
     + #include "pixdesc.h"
     + #include "pixfmt.h"
     ++#include "thread.h"
     + 
     + static const HWContextType * const hw_table[] = {
     + #if CONFIG_CUDA
     +@@ libavutil/hwcontext.c: static const char *const hw_type_names[] = {
     +     [AV_HWDEVICE_TYPE_VULKAN] = "vulkan",
     + };
     + 
     ++#define DEVICE_REGISTRY_SIZE 1024
     ++
     ++static AVMutex mutex;
     ++static int is_mutex_initialized = 0;
     ++static int max_device_reg_id = 1;
     ++static AVBuffer *hw_device_registry[DEVICE_REGISTRY_SIZE];
     ++
     ++static int register_hw_device(const AVBufferRef *ref)
     ++{
     ++    AVHWDeviceContext *ctx = (AVHWDeviceContext*)ref->data;
     ++    const int reg_id = max_device_reg_id;
     ++
     ++    if (ctx == NULL)
     ++        return AVERROR(EINVAL);
     ++
     ++    if (!is_mutex_initialized) {
     ++        int ret;
     ++        ret = ff_mutex_init(&mutex, NULL);
     ++        if (ret) {
     ++            av_log(ctx, AV_LOG_ERROR, "hwcontext: mutex initialization failed! Error code: %d\n", ret);
     ++            return AVERROR(EINVAL);
     ++        }
     ++
     ++        is_mutex_initialized = 1;
     ++    }
     ++
     ++    ff_mutex_lock(&mutex);
     ++
     ++    for (int i = 0; i < max_device_reg_id; ++i) {
     ++        if (hw_device_registry[i] != NULL && hw_device_registry[i] == ref->buffer) {
     ++            ff_mutex_unlock(&mutex);
     ++            return i;
     ++        }
     ++    }
     ++
     ++    if (max_device_reg_id >= DEVICE_REGISTRY_SIZE) {
     ++        ff_mutex_unlock(&mutex);
     ++        av_log(ctx, AV_LOG_ERROR, "Device registry limit (%d) reached. Please check for excessive device creation.", DEVICE_REGISTRY_SIZE);
     ++        return AVERROR(ENOMEM);
     ++    }
     ++
     ++    hw_device_registry[reg_id] = ref->buffer;
     ++    max_device_reg_id++;
     ++
     ++    ff_mutex_unlock(&mutex);
     ++
     ++    return reg_id;
     ++}
     ++
     ++static void unregister_hw_device(const AVHWDeviceContext *ctx)
     ++{
     ++    if (ctx == NULL || !is_mutex_initialized)
     ++        return;
     ++
     ++    ff_mutex_lock(&mutex);
     ++
     ++    hw_device_registry[ctx->internal->registered_device_id] = NULL;
     ++
     ++    ff_mutex_unlock(&mutex);
     ++}
     ++
     ++static AVBufferRef *get_registered_hw_device(int registered_id)
     ++{
     ++    if (registered_id <= 0 || registered_id >= max_device_reg_id)
     ++        return NULL;
     ++
     ++    ff_mutex_lock(&mutex);
     ++
     ++    if (hw_device_registry[registered_id] != NULL && hw_device_registry[registered_id]->data != NULL) {
     ++        AVBufferRef *ref = av_ref_from_buffer(hw_device_registry[registered_id]);
     ++        return ref;
     ++    }
     ++
     ++    ff_mutex_unlock(&mutex);
     ++
     ++    return NULL;
     ++}
     ++
     + enum AVHWDeviceType av_hwdevice_find_type_by_name(const char *name)
     + {
     +     int type;
     +@@ libavutil/hwcontext.c: static void hwdevice_ctx_free(void *opaque, uint8_t *data)
       {
           AVHWDeviceContext *ctx = (AVHWDeviceContext*)data;
     -+    int i;
       
     ++    unregister_hw_device(ctx);
     ++
           /* uninit might still want access the hw context and the user
            * free() callback might destroy it, so uninit has to be called first */
     -@@ libavutil/hwcontext.c: static void hwdevice_ctx_free(void *opaque, uint8_t *data)
     -         ctx->free(ctx);
     +     if (ctx->internal->hw_type->device_uninit)
     +@@ libavutil/hwcontext.c: int av_hwdevice_ctx_create(AVBufferRef **pdevice_ref, enum AVHWDeviceType type,
     +                            const char *device, AVDictionary *opts, int flags)
     + {
     +     AVBufferRef *device_ref = NULL;
     +-    AVHWDeviceContext *device_ctx;
     ++    AVHWDeviceContext *device_ctx = NULL;
     +     int ret = 0;
       
     -     av_buffer_unref(&ctx->internal->source_device);
     -+    for (i = 0; i < AV_HWDEVICE_TYPE_NB; i++)
     -+        av_buffer_unref(&ctx->internal->derived_devices[i]);
     +     device_ref = av_hwdevice_ctx_alloc(type);
     +@@ libavutil/hwcontext.c: int av_hwdevice_ctx_create(AVBufferRef **pdevice_ref, enum AVHWDeviceType type,
     +     if (ret < 0)
     +         goto fail;
       
     -     av_freep(&ctx->hwctx);
     -     av_freep(&ctx->internal->priv);
     -@@ libavutil/hwcontext.c: fail:
     ++    ret = register_hw_device(device_ref);
     ++    if (ret < 0)
     ++        goto fail;
     ++
     +     ret = av_hwdevice_ctx_init(device_ref);
     +     if (ret < 0)
     +         goto fail;
     + 
     ++    device_ctx->internal->registered_device_id = ret;
     ++
     +     *pdevice_ref = device_ref;
     +     return 0;
     + fail:
     ++    unregister_hw_device(device_ctx);
     +     av_buffer_unref(&device_ref);
     +     *pdevice_ref = NULL;
           return ret;
       }
       
     @@ libavutil/hwcontext.c: fail:
      -                                        AVDictionary *options, int flags)
      +static AVBufferRef* find_derived_hwdevice_ctx(AVBufferRef *src_ref, enum AVHWDeviceType type)
      +{
     -+    AVBufferRef *tmp_ref;
     ++    AVBufferRef *derived_ref;
      +    AVHWDeviceContext *src_ctx;
      +    int i;
      +
     @@ libavutil/hwcontext.c: fail:
      +        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;
     ++        if (src_ctx->internal->derived_device_ids[i]) {
     ++            AVBufferRef *tmp_ref = get_registered_hw_device(src_ctx->internal->derived_device_ids[i]);
     ++
     ++            if (tmp_ref) {
     ++                derived_ref = find_derived_hwdevice_ctx(tmp_ref, type);
     ++
     ++                if (tmp_ref != derived_ref)
     ++                    av_buffer_unref(&tmp_ref);
     ++
     ++                if (derived_ref)
     ++                    return derived_ref;
     ++            }
      +        }
      +
      +    return NULL;
     @@ libavutil/hwcontext.c: int av_hwdevice_ctx_create_derived_opts(AVBufferRef **dst
                           ret = AVERROR(ENOMEM);
                           goto fail;
                       }
     -+                if (!tmp_ctx->internal->derived_devices[type]) {
     -+                    tmp_ctx->internal->derived_devices[type] = av_buffer_ref(dst_ref);
     -+                    if (!tmp_ctx->internal->derived_devices[type]) {
     -+                        ret = AVERROR(ENOMEM);
     -+                        goto fail;
     -+                    }
     -+                }
     ++                if (!tmp_ctx->internal->derived_device_ids[type])
     ++                    tmp_ctx->internal->derived_device_ids[type] = dst_ctx->internal->registered_device_id;
     ++
                       ret = av_hwdevice_ctx_init(dst_ref);
                       if (ret < 0)
                           goto fail;
     @@ libavutil/hwcontext_internal.h: struct AVHWDeviceInternal {
           AVBufferRef *source_device;
      +
      +    /**
     -+     * An array of reference to device contexts which
     ++     * An array of device registration ids from device contexts which
      +     * were derived from this device.
      +     */
     -+    AVBufferRef *derived_devices[AV_HWDEVICE_TYPE_NB];
     ++    int derived_device_ids[AV_HWDEVICE_TYPE_NB];
     ++
     ++    /**
     ++     * ID under wich the hw context is registered internally.
     ++     */
     ++    int registered_device_id;
       };
       
       struct AVHWFramesInternal {
     @@ libavutil/hwcontext_qsv.c: static int qsv_device_create(AVHWDeviceContext *ctx,
      +    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);
     ++        child_device->internal->derived_device_ids[ctx->type] = ctx->internal->registered_device_id;
      +    }
       
      -    return qsv_device_derive_from_child(ctx, impl, child_device, 0);
 2:  8c6a1a8b8c ! 3:  a2c8e79e8a lavu: bump minor version and add doc/APIchanges entry for av_hwdevice_ctx_get_or_create_derived()
     @@ doc/APIchanges: libavutil:     2021-04-27
       
       API changes, most recent first:
       
     -+2022-04-30 - xxxxxxxxxx - lavu 57.25.100 - hwcontext.h
     ++2022-05-21 - xxxxxxxxxx - lavu 57.25.100 - hwcontext.h
      +  Add av_hwdevice_ctx_get_or_create_derived().
     ++
     ++2022-04-30 - xxxxxxxxxx - lavu 57.25.100 - buffer.h
     ++  Add av_ref_from_buffer().
      +
       2022-03-16 - xxxxxxxxxx - all libraries - version_major.h
         Add lib<name>/version_major.h as new installed headers, which only
 3:  023597a557 = 4:  d0957afde6 avfilter/hwmap,hwupload: use new av_hwdevice_ctx_get_or_create_derived method

-- 
ffmpeg-codebot
_______________________________________________
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".

[FFmpeg-devel] [PATCH v2 1/4] avutil/buffer: add av_ref_from_buffer() function

[softworkz]

From: softworkz <softworkz@hotmail.com>

This allows to create AVBufferRef from AVBuffer directly.

Signed-off-by: softworkz <softworkz@hotmail.com>
---
 libavutil/buffer.c | 16 ++++++++++++++++
 libavutil/buffer.h |  8 ++++++++
 2 files changed, 24 insertions(+)

diff --git a/libavutil/buffer.c b/libavutil/buffer.c
index 54590be566..f9df0ad6ea 100644
--- a/libavutil/buffer.c
+++ b/libavutil/buffer.c
@@ -100,6 +100,22 @@ AVBufferRef *av_buffer_allocz(size_t size)
     return ret;
 }
 
+AVBufferRef *av_ref_from_buffer(AVBuffer *buf)
+{
+    AVBufferRef *ref = av_mallocz(sizeof(*ref));
+
+    if (!ref)
+        return NULL;
+
+    ref->buffer = buf;
+    ref->data   = buf->data;
+    ref->size   = buf->size;
+
+    atomic_fetch_add_explicit(&buf->refcount, 1, memory_order_relaxed);
+
+    return ref;
+}
+
 AVBufferRef *av_buffer_ref(const AVBufferRef *buf)
 {
     AVBufferRef *ret = av_mallocz(sizeof(*ret));
diff --git a/libavutil/buffer.h b/libavutil/buffer.h
index e1ef5b7f07..491734b9ca 100644
--- a/libavutil/buffer.h
+++ b/libavutil/buffer.h
@@ -139,6 +139,14 @@ AVBufferRef *av_buffer_create(uint8_t *data, size_t size,
  */
 void av_buffer_default_free(void *opaque, uint8_t *data);
 
+/**
+ * Create a new reference to an AVBuffer.
+ *
+ * @return a new AVBufferRef referring to the same AVBuffer as buf or NULL on
+ * failure.
+ */
+AVBufferRef *av_ref_from_buffer(AVBuffer *buf);
+
 /**
  * Create a new reference to an AVBuffer.
  *
-- 
ffmpeg-codebot

_______________________________________________
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".

[FFmpeg-devel] [PATCH v2 2/4] avutils/hwcontext: add derive-device function which searches for existing devices in both directions

[softworkz]

From: softworkz <softworkz@hotmail.com>

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>
---
 libavutil/hwcontext.c          | 167 +++++++++++++++++++++++++++++++--
 libavutil/hwcontext.h          |  20 ++++
 libavutil/hwcontext_internal.h |  11 +++
 libavutil/hwcontext_qsv.c      |  11 ++-
 4 files changed, 199 insertions(+), 10 deletions(-)

diff --git a/libavutil/hwcontext.c b/libavutil/hwcontext.c
index ab9ad3703e..43e39cb2d3 100644
--- a/libavutil/hwcontext.c
+++ b/libavutil/hwcontext.c
@@ -22,12 +22,17 @@
 #include "buffer.h"
 #include "common.h"
 #include "hwcontext.h"
+
+#include <stdatomic.h>
+
+#include "buffer_internal.h"
 #include "hwcontext_internal.h"
 #include "imgutils.h"
 #include "log.h"
 #include "mem.h"
 #include "pixdesc.h"
 #include "pixfmt.h"
+#include "thread.h"
 
 static const HWContextType * const hw_table[] = {
 #if CONFIG_CUDA
@@ -80,6 +85,84 @@ static const char *const hw_type_names[] = {
     [AV_HWDEVICE_TYPE_VULKAN] = "vulkan",
 };
 
+#define DEVICE_REGISTRY_SIZE 1024
+
+static AVMutex mutex;
+static int is_mutex_initialized = 0;
+static int max_device_reg_id = 1;
+static AVBuffer *hw_device_registry[DEVICE_REGISTRY_SIZE];
+
+static int register_hw_device(const AVBufferRef *ref)
+{
+    AVHWDeviceContext *ctx = (AVHWDeviceContext*)ref->data;
+    const int reg_id = max_device_reg_id;
+
+    if (ctx == NULL)
+        return AVERROR(EINVAL);
+
+    if (!is_mutex_initialized) {
+        int ret;
+        ret = ff_mutex_init(&mutex, NULL);
+        if (ret) {
+            av_log(ctx, AV_LOG_ERROR, "hwcontext: mutex initialization failed! Error code: %d\n", ret);
+            return AVERROR(EINVAL);
+        }
+
+        is_mutex_initialized = 1;
+    }
+
+    ff_mutex_lock(&mutex);
+
+    for (int i = 0; i < max_device_reg_id; ++i) {
+        if (hw_device_registry[i] != NULL && hw_device_registry[i] == ref->buffer) {
+            ff_mutex_unlock(&mutex);
+            return i;
+        }
+    }
+
+    if (max_device_reg_id >= DEVICE_REGISTRY_SIZE) {
+        ff_mutex_unlock(&mutex);
+        av_log(ctx, AV_LOG_ERROR, "Device registry limit (%d) reached. Please check for excessive device creation.", DEVICE_REGISTRY_SIZE);
+        return AVERROR(ENOMEM);
+    }
+
+    hw_device_registry[reg_id] = ref->buffer;
+    max_device_reg_id++;
+
+    ff_mutex_unlock(&mutex);
+
+    return reg_id;
+}
+
+static void unregister_hw_device(const AVHWDeviceContext *ctx)
+{
+    if (ctx == NULL || !is_mutex_initialized)
+        return;
+
+    ff_mutex_lock(&mutex);
+
+    hw_device_registry[ctx->internal->registered_device_id] = NULL;
+
+    ff_mutex_unlock(&mutex);
+}
+
+static AVBufferRef *get_registered_hw_device(int registered_id)
+{
+    if (registered_id <= 0 || registered_id >= max_device_reg_id)
+        return NULL;
+
+    ff_mutex_lock(&mutex);
+
+    if (hw_device_registry[registered_id] != NULL && hw_device_registry[registered_id]->data != NULL) {
+        AVBufferRef *ref = av_ref_from_buffer(hw_device_registry[registered_id]);
+        return ref;
+    }
+
+    ff_mutex_unlock(&mutex);
+
+    return NULL;
+}
+
 enum AVHWDeviceType av_hwdevice_find_type_by_name(const char *name)
 {
     int type;
@@ -124,6 +207,8 @@ static void hwdevice_ctx_free(void *opaque, uint8_t *data)
 {
     AVHWDeviceContext *ctx = (AVHWDeviceContext*)data;
 
+    unregister_hw_device(ctx);
+
     /* uninit might still want access the hw context and the user
      * free() callback might destroy it, so uninit has to be called first */
     if (ctx->internal->hw_type->device_uninit)
@@ -612,7 +697,7 @@ int av_hwdevice_ctx_create(AVBufferRef **pdevice_ref, enum AVHWDeviceType type,
                            const char *device, AVDictionary *opts, int flags)
 {
     AVBufferRef *device_ref = NULL;
-    AVHWDeviceContext *device_ctx;
+    AVHWDeviceContext *device_ctx = NULL;
     int ret = 0;
 
     device_ref = av_hwdevice_ctx_alloc(type);
@@ -632,22 +717,58 @@ int av_hwdevice_ctx_create(AVBufferRef **pdevice_ref, enum AVHWDeviceType type,
     if (ret < 0)
         goto fail;
 
+    ret = register_hw_device(device_ref);
+    if (ret < 0)
+        goto fail;
+
     ret = av_hwdevice_ctx_init(device_ref);
     if (ret < 0)
         goto fail;
 
+    device_ctx->internal->registered_device_id = ret;
+
     *pdevice_ref = device_ref;
     return 0;
 fail:
+    unregister_hw_device(device_ctx);
     av_buffer_unref(&device_ref);
     *pdevice_ref = NULL;
     return ret;
 }
 
-int av_hwdevice_ctx_create_derived_opts(AVBufferRef **dst_ref_ptr,
-                                        enum AVHWDeviceType type,
-                                        AVBufferRef *src_ref,
-                                        AVDictionary *options, int flags)
+static AVBufferRef* find_derived_hwdevice_ctx(AVBufferRef *src_ref, enum AVHWDeviceType type)
+{
+    AVBufferRef *derived_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_device_ids[i]) {
+            AVBufferRef *tmp_ref = get_registered_hw_device(src_ctx->internal->derived_device_ids[i]);
+
+            if (tmp_ref) {
+                derived_ref = find_derived_hwdevice_ctx(tmp_ref, type);
+
+                if (tmp_ref != derived_ref)
+                    av_buffer_unref(&tmp_ref);
+
+                if (derived_ref)
+                    return derived_ref;
+            }
+        }
+
+    return NULL;
+}
+
+static int hwdevice_ctx_create_derived(AVBufferRef **dst_ref_ptr,
+                                       enum AVHWDeviceType type,
+                                       AVBufferRef *src_ref,
+                                       AVDictionary *options, int flags,
+                                       int get_existing)
 {
     AVBufferRef *dst_ref = NULL, *tmp_ref;
     AVHWDeviceContext *dst_ctx, *tmp_ctx;
@@ -667,6 +788,18 @@ int av_hwdevice_ctx_create_derived_opts(AVBufferRef **dst_ref_ptr,
         tmp_ref = tmp_ctx->internal->source_device;
     }
 
+    if (get_existing) {
+        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);
@@ -688,6 +821,9 @@ int av_hwdevice_ctx_create_derived_opts(AVBufferRef **dst_ref_ptr,
                     ret = AVERROR(ENOMEM);
                     goto fail;
                 }
+                if (!tmp_ctx->internal->derived_device_ids[type])
+                    tmp_ctx->internal->derived_device_ids[type] = dst_ctx->internal->registered_device_id;
+
                 ret = av_hwdevice_ctx_init(dst_ref);
                 if (ret < 0)
                     goto fail;
@@ -712,12 +848,29 @@ fail:
     return ret;
 }
 
+int av_hwdevice_ctx_create_derived_opts(AVBufferRef **dst_ref_ptr,
+                                        enum AVHWDeviceType type,
+                                        AVBufferRef *src_ref,
+                                        AVDictionary *options, int flags)
+{
+    return hwdevice_ctx_create_derived(dst_ref_ptr, type, src_ref,
+                                       options, flags, 0);
+}
+
+int av_hwdevice_ctx_get_or_create_derived(AVBufferRef **dst_ref_ptr,
+                                          enum AVHWDeviceType type,
+                                          AVBufferRef *src_ref, int flags)
+{
+    return hwdevice_ctx_create_derived(dst_ref_ptr, type, src_ref,
+                                       NULL, flags, 1);
+}
+
 int av_hwdevice_ctx_create_derived(AVBufferRef **dst_ref_ptr,
                                    enum AVHWDeviceType type,
                                    AVBufferRef *src_ref, int flags)
 {
-    return av_hwdevice_ctx_create_derived_opts(dst_ref_ptr, type, src_ref,
-                                               NULL, flags);
+    return hwdevice_ctx_create_derived(dst_ref_ptr, type, src_ref,
+                                       NULL, flags, 0);
 }
 
 static void ff_hwframe_unmap(void *opaque, uint8_t *data)
diff --git a/libavutil/hwcontext.h b/libavutil/hwcontext.h
index c18b7e1e8b..3785811f98 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;
@@ -328,6 +329,25 @@ int av_hwdevice_ctx_create_derived(AVBufferRef **dst_ctx,
                                    enum AVHWDeviceType type,
                                    AVBufferRef *src_ctx, int flags);
 
+/**
+ * Create a new device of the specified type from an existing device.
+ *
+ * This function performs the same action as av_hwdevice_ctx_create_derived,
+ * however, if a derived device of the specified type already exists,
+ * it returns the existing instance.
+ *
+ * @param dst_ctx On success, a reference to the newly-created
+ *                AVHWDeviceContext.
+ * @param type    The type of the new device to create.
+ * @param src_ctx A reference to an existing AVHWDeviceContext which will be
+ *                used to create the new device.
+ * @param flags   Currently unused; should be set to zero.
+ * @return        Zero on success, a negative AVERROR code on failure.
+ */
+int av_hwdevice_ctx_get_or_create_derived(AVBufferRef **dst_ctx,
+                                          enum AVHWDeviceType type,
+                                          AVBufferRef *src_ctx, int flags);
+
 /**
  * Create a new device of the specified type from an existing device.
  *
diff --git a/libavutil/hwcontext_internal.h b/libavutil/hwcontext_internal.h
index e6266494ac..adb649cde4 100644
--- a/libavutil/hwcontext_internal.h
+++ b/libavutil/hwcontext_internal.h
@@ -109,6 +109,17 @@ struct AVHWDeviceInternal {
      * context it was derived from.
      */
     AVBufferRef *source_device;
+
+    /**
+     * An array of device registration ids from device contexts which
+     * were derived from this device.
+     */
+    int derived_device_ids[AV_HWDEVICE_TYPE_NB];
+
+    /**
+     * ID under wich the hw context is registered internally.
+     */
+    int registered_device_id;
 };
 
 struct AVHWFramesInternal {
diff --git a/libavutil/hwcontext_qsv.c b/libavutil/hwcontext_qsv.c
index 66c0e38955..423b891ec5 100644
--- a/libavutil/hwcontext_qsv.c
+++ b/libavutil/hwcontext_qsv.c
@@ -309,7 +309,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)
 {
 }
 
@@ -322,7 +322,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;
@@ -1649,8 +1649,13 @@ 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_device_ids[ctx->type] = ctx->internal->registered_device_id;
+    }
 
-    return qsv_device_derive_from_child(ctx, impl, child_device, 0);
+    return ret;
 }
 
 const HWContextType ff_hwcontext_type_qsv = {
-- 
ffmpeg-codebot

_______________________________________________
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".

[FFmpeg-devel] [PATCH v2 3/4] lavu: bump minor version and add doc/APIchanges entry for av_hwdevice_ctx_get_or_create_derived()

[softworkz]

From: softworkz <softworkz@hotmail.com>

Signed-off-by: softworkz <softworkz@hotmail.com>
---
 doc/APIchanges      | 6 ++++++
 libavutil/version.h | 4 ++--
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/doc/APIchanges b/doc/APIchanges
index 1a9f0a303e..92b6d70ac0 100644
--- a/doc/APIchanges
+++ b/doc/APIchanges
@@ -14,6 +14,12 @@ libavutil:     2021-04-27
 
 API changes, most recent first:
 
+2022-05-21 - xxxxxxxxxx - lavu 57.25.100 - hwcontext.h
+  Add av_hwdevice_ctx_get_or_create_derived().
+
+2022-04-30 - xxxxxxxxxx - lavu 57.25.100 - buffer.h
+  Add av_ref_from_buffer().
+
 2022-03-16 - xxxxxxxxxx - all libraries - version_major.h
   Add lib<name>/version_major.h as new installed headers, which only
   contain the major version number (and corresponding API deprecation
diff --git a/libavutil/version.h b/libavutil/version.h
index 6735c20090..dd7d20a9fa 100644
--- a/libavutil/version.h
+++ b/libavutil/version.h
@@ -79,8 +79,8 @@
  */
 
 #define LIBAVUTIL_VERSION_MAJOR  57
-#define LIBAVUTIL_VERSION_MINOR  24
-#define LIBAVUTIL_VERSION_MICRO 101
+#define LIBAVUTIL_VERSION_MINOR  25
+#define LIBAVUTIL_VERSION_MICRO 100
 
 #define LIBAVUTIL_VERSION_INT   AV_VERSION_INT(LIBAVUTIL_VERSION_MAJOR, \
                                                LIBAVUTIL_VERSION_MINOR, \
-- 
ffmpeg-codebot

_______________________________________________
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".

[FFmpeg-devel] [PATCH v2 4/4] avfilter/hwmap, hwupload: use new av_hwdevice_ctx_get_or_create_derived method

[softworkz]

From: softworkz <softworkz@hotmail.com>

Signed-off-by: softworkz <softworkz@hotmail.com>
---
 libavfilter/vf_hwmap.c    | 4 ++--
 libavfilter/vf_hwupload.c | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/libavfilter/vf_hwmap.c b/libavfilter/vf_hwmap.c
index 2e03dfc1fe..b79cf6732c 100644
--- a/libavfilter/vf_hwmap.c
+++ b/libavfilter/vf_hwmap.c
@@ -82,8 +82,8 @@ static int hwmap_config_output(AVFilterLink *outlink)
                 goto fail;
             }
 
-            err = av_hwdevice_ctx_create_derived(&device, type,
-                                                 hwfc->device_ref, 0);
+            err = av_hwdevice_ctx_get_or_create_derived(&device, type,
+                                                        hwfc->device_ref, 0);
             if (err < 0) {
                 av_log(avctx, AV_LOG_ERROR, "Failed to created derived "
                        "device context: %d.\n", err);
diff --git a/libavfilter/vf_hwupload.c b/libavfilter/vf_hwupload.c
index dbc41734cc..41ee0e43c4 100644
--- a/libavfilter/vf_hwupload.c
+++ b/libavfilter/vf_hwupload.c
@@ -51,7 +51,7 @@ static int hwupload_query_formats(AVFilterContext *avctx)
         /* We already have a specified device. */
     } else if (avctx->hw_device_ctx) {
         if (ctx->device_type) {
-            err = av_hwdevice_ctx_create_derived(
+            err = av_hwdevice_ctx_get_or_create_derived(
                 &ctx->hwdevice_ref,
                 av_hwdevice_find_type_by_name(ctx->device_type),
                 avctx->hw_device_ctx, 0);
-- 
ffmpeg-codebot
_______________________________________________
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".

[FFmpeg-devel] [PATCH v3 0/4] Add derive-device function which searches for existing devices in both directions

[ffmpegagent]

This is an updated version of: [PATCH v4 1/1] avutils/hwcontext: When
deriving a hwdevice, search for existing device in both directions

There has been an objection that the earlier patchset would change API
behavior, and that this change should be limited to ffmpeg cli.

To achieve this, the API behavior is left unchanged now and a new function
av_hwdevice_ctx_get_or_create_derived() is added and used by the hwupload
and hwmap filters.

v2: Implemented concept for "weak references" to avoid circular reference
lockup.

v3: rebased due to conflicts

softworkz (4):
  avutil/buffer: add av_ref_from_buffer() function
  avutils/hwcontext: add derive-device function which searches for
    existing devices in both directions
  lavu: bump minor version and add doc/APIchanges entry for
    av_hwdevice_ctx_get_or_create_derived()
  avfilter/hwmap,hwupload: use new av_hwdevice_ctx_get_or_create_derived
    method

 doc/APIchanges                 |   6 ++
 libavfilter/vf_hwmap.c         |   4 +-
 libavfilter/vf_hwupload.c      |   2 +-
 libavutil/buffer.c             |  16 ++++
 libavutil/buffer.h             |   8 ++
 libavutil/hwcontext.c          | 167 +++++++++++++++++++++++++++++++--
 libavutil/hwcontext.h          |  20 ++++
 libavutil/hwcontext_internal.h |  11 +++
 libavutil/hwcontext_qsv.c      |  11 ++-
 libavutil/version.h            |   2 +-
 10 files changed, 233 insertions(+), 14 deletions(-)


base-commit: f7d510b33ff33d2f5cb096017ee1c00f624cc138
Published-As: https://github.com/ffstaging/FFmpeg/releases/tag/pr-ffstaging-25%2Fsoftworkz%2Fderive_devices-v3
Fetch-It-Via: git fetch https://github.com/ffstaging/FFmpeg pr-ffstaging-25/softworkz/derive_devices-v3
Pull-Request: https://github.com/ffstaging/FFmpeg/pull/25

Range-diff vs v2:

 1:  e9c920c237 = 1:  b17a72426c avutil/buffer: add av_ref_from_buffer() function
 2:  bec8ccfcc2 = 2:  5cd017dfa7 avutils/hwcontext: add derive-device function which searches for existing devices in both directions
 3:  a2c8e79e8a ! 3:  03528fce43 lavu: bump minor version and add doc/APIchanges entry for av_hwdevice_ctx_get_or_create_derived()
     @@ doc/APIchanges: libavutil:     2021-04-27
       
       API changes, most recent first:
       
     -+2022-05-21 - xxxxxxxxxx - lavu 57.25.100 - hwcontext.h
     ++2022-07-01 - xxxxxxxxxx - lavu 57.31.100 - hwcontext.h
      +  Add av_hwdevice_ctx_get_or_create_derived().
      +
     -+2022-04-30 - xxxxxxxxxx - lavu 57.25.100 - buffer.h
     ++2022-07-01 - xxxxxxxxxx - lavu 57.31.100 - buffer.h
      +  Add av_ref_from_buffer().
      +
     - 2022-03-16 - xxxxxxxxxx - all libraries - version_major.h
     -   Add lib<name>/version_major.h as new installed headers, which only
     -   contain the major version number (and corresponding API deprecation
     + 2022-07-xx - xxxxxxxxxx - lavu 57.30.100 - frame.h
     +   Add AVFrame.duration, deprecate AVFrame.pkt_duration.
     + 
      
       ## libavutil/version.h ##
      @@
        */
       
       #define LIBAVUTIL_VERSION_MAJOR  57
     --#define LIBAVUTIL_VERSION_MINOR  24
     --#define LIBAVUTIL_VERSION_MICRO 101
     -+#define LIBAVUTIL_VERSION_MINOR  25
     -+#define LIBAVUTIL_VERSION_MICRO 100
     +-#define LIBAVUTIL_VERSION_MINOR  30
     ++#define LIBAVUTIL_VERSION_MINOR  31
     + #define LIBAVUTIL_VERSION_MICRO 100
       
       #define LIBAVUTIL_VERSION_INT   AV_VERSION_INT(LIBAVUTIL_VERSION_MAJOR, \
     -                                                LIBAVUTIL_VERSION_MINOR, \
 4:  d0957afde6 = 4:  7ee28b7c25 avfilter/hwmap,hwupload: use new av_hwdevice_ctx_get_or_create_derived method

-- 
ffmpeg-codebot
_______________________________________________
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".

[FFmpeg-devel] [PATCH v3 1/4] avutil/buffer: add av_ref_from_buffer() function

[softworkz]

From: softworkz <softworkz@hotmail.com>

This allows to create AVBufferRef from AVBuffer directly.

Signed-off-by: softworkz <softworkz@hotmail.com>
---
 libavutil/buffer.c | 16 ++++++++++++++++
 libavutil/buffer.h |  8 ++++++++
 2 files changed, 24 insertions(+)

diff --git a/libavutil/buffer.c b/libavutil/buffer.c
index 54590be566..f9df0ad6ea 100644
--- a/libavutil/buffer.c
+++ b/libavutil/buffer.c
@@ -100,6 +100,22 @@ AVBufferRef *av_buffer_allocz(size_t size)
     return ret;
 }
 
+AVBufferRef *av_ref_from_buffer(AVBuffer *buf)
+{
+    AVBufferRef *ref = av_mallocz(sizeof(*ref));
+
+    if (!ref)
+        return NULL;
+
+    ref->buffer = buf;
+    ref->data   = buf->data;
+    ref->size   = buf->size;
+
+    atomic_fetch_add_explicit(&buf->refcount, 1, memory_order_relaxed);
+
+    return ref;
+}
+
 AVBufferRef *av_buffer_ref(const AVBufferRef *buf)
 {
     AVBufferRef *ret = av_mallocz(sizeof(*ret));
diff --git a/libavutil/buffer.h b/libavutil/buffer.h
index e1ef5b7f07..491734b9ca 100644
--- a/libavutil/buffer.h
+++ b/libavutil/buffer.h
@@ -139,6 +139,14 @@ AVBufferRef *av_buffer_create(uint8_t *data, size_t size,
  */
 void av_buffer_default_free(void *opaque, uint8_t *data);
 
+/**
+ * Create a new reference to an AVBuffer.
+ *
+ * @return a new AVBufferRef referring to the same AVBuffer as buf or NULL on
+ * failure.
+ */
+AVBufferRef *av_ref_from_buffer(AVBuffer *buf);
+
 /**
  * Create a new reference to an AVBuffer.
  *
-- 
ffmpeg-codebot

_______________________________________________
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".

[FFmpeg-devel] [PATCH v3 2/4] avutils/hwcontext: add derive-device function which searches for existing devices in both directions

[softworkz]

From: softworkz <softworkz@hotmail.com>

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>
---
 libavutil/hwcontext.c          | 167 +++++++++++++++++++++++++++++++--
 libavutil/hwcontext.h          |  20 ++++
 libavutil/hwcontext_internal.h |  11 +++
 libavutil/hwcontext_qsv.c      |  11 ++-
 4 files changed, 199 insertions(+), 10 deletions(-)

diff --git a/libavutil/hwcontext.c b/libavutil/hwcontext.c
index ab9ad3703e..43e39cb2d3 100644
--- a/libavutil/hwcontext.c
+++ b/libavutil/hwcontext.c
@@ -22,12 +22,17 @@
 #include "buffer.h"
 #include "common.h"
 #include "hwcontext.h"
+
+#include <stdatomic.h>
+
+#include "buffer_internal.h"
 #include "hwcontext_internal.h"
 #include "imgutils.h"
 #include "log.h"
 #include "mem.h"
 #include "pixdesc.h"
 #include "pixfmt.h"
+#include "thread.h"
 
 static const HWContextType * const hw_table[] = {
 #if CONFIG_CUDA
@@ -80,6 +85,84 @@ static const char *const hw_type_names[] = {
     [AV_HWDEVICE_TYPE_VULKAN] = "vulkan",
 };
 
+#define DEVICE_REGISTRY_SIZE 1024
+
+static AVMutex mutex;
+static int is_mutex_initialized = 0;
+static int max_device_reg_id = 1;
+static AVBuffer *hw_device_registry[DEVICE_REGISTRY_SIZE];
+
+static int register_hw_device(const AVBufferRef *ref)
+{
+    AVHWDeviceContext *ctx = (AVHWDeviceContext*)ref->data;
+    const int reg_id = max_device_reg_id;
+
+    if (ctx == NULL)
+        return AVERROR(EINVAL);
+
+    if (!is_mutex_initialized) {
+        int ret;
+        ret = ff_mutex_init(&mutex, NULL);
+        if (ret) {
+            av_log(ctx, AV_LOG_ERROR, "hwcontext: mutex initialization failed! Error code: %d\n", ret);
+            return AVERROR(EINVAL);
+        }
+
+        is_mutex_initialized = 1;
+    }
+
+    ff_mutex_lock(&mutex);
+
+    for (int i = 0; i < max_device_reg_id; ++i) {
+        if (hw_device_registry[i] != NULL && hw_device_registry[i] == ref->buffer) {
+            ff_mutex_unlock(&mutex);
+            return i;
+        }
+    }
+
+    if (max_device_reg_id >= DEVICE_REGISTRY_SIZE) {
+        ff_mutex_unlock(&mutex);
+        av_log(ctx, AV_LOG_ERROR, "Device registry limit (%d) reached. Please check for excessive device creation.", DEVICE_REGISTRY_SIZE);
+        return AVERROR(ENOMEM);
+    }
+
+    hw_device_registry[reg_id] = ref->buffer;
+    max_device_reg_id++;
+
+    ff_mutex_unlock(&mutex);
+
+    return reg_id;
+}
+
+static void unregister_hw_device(const AVHWDeviceContext *ctx)
+{
+    if (ctx == NULL || !is_mutex_initialized)
+        return;
+
+    ff_mutex_lock(&mutex);
+
+    hw_device_registry[ctx->internal->registered_device_id] = NULL;
+
+    ff_mutex_unlock(&mutex);
+}
+
+static AVBufferRef *get_registered_hw_device(int registered_id)
+{
+    if (registered_id <= 0 || registered_id >= max_device_reg_id)
+        return NULL;
+
+    ff_mutex_lock(&mutex);
+
+    if (hw_device_registry[registered_id] != NULL && hw_device_registry[registered_id]->data != NULL) {
+        AVBufferRef *ref = av_ref_from_buffer(hw_device_registry[registered_id]);
+        return ref;
+    }
+
+    ff_mutex_unlock(&mutex);
+
+    return NULL;
+}
+
 enum AVHWDeviceType av_hwdevice_find_type_by_name(const char *name)
 {
     int type;
@@ -124,6 +207,8 @@ static void hwdevice_ctx_free(void *opaque, uint8_t *data)
 {
     AVHWDeviceContext *ctx = (AVHWDeviceContext*)data;
 
+    unregister_hw_device(ctx);
+
     /* uninit might still want access the hw context and the user
      * free() callback might destroy it, so uninit has to be called first */
     if (ctx->internal->hw_type->device_uninit)
@@ -612,7 +697,7 @@ int av_hwdevice_ctx_create(AVBufferRef **pdevice_ref, enum AVHWDeviceType type,
                            const char *device, AVDictionary *opts, int flags)
 {
     AVBufferRef *device_ref = NULL;
-    AVHWDeviceContext *device_ctx;
+    AVHWDeviceContext *device_ctx = NULL;
     int ret = 0;
 
     device_ref = av_hwdevice_ctx_alloc(type);
@@ -632,22 +717,58 @@ int av_hwdevice_ctx_create(AVBufferRef **pdevice_ref, enum AVHWDeviceType type,
     if (ret < 0)
         goto fail;
 
+    ret = register_hw_device(device_ref);
+    if (ret < 0)
+        goto fail;
+
     ret = av_hwdevice_ctx_init(device_ref);
     if (ret < 0)
         goto fail;
 
+    device_ctx->internal->registered_device_id = ret;
+
     *pdevice_ref = device_ref;
     return 0;
 fail:
+    unregister_hw_device(device_ctx);
     av_buffer_unref(&device_ref);
     *pdevice_ref = NULL;
     return ret;
 }
 
-int av_hwdevice_ctx_create_derived_opts(AVBufferRef **dst_ref_ptr,
-                                        enum AVHWDeviceType type,
-                                        AVBufferRef *src_ref,
-                                        AVDictionary *options, int flags)
+static AVBufferRef* find_derived_hwdevice_ctx(AVBufferRef *src_ref, enum AVHWDeviceType type)
+{
+    AVBufferRef *derived_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_device_ids[i]) {
+            AVBufferRef *tmp_ref = get_registered_hw_device(src_ctx->internal->derived_device_ids[i]);
+
+            if (tmp_ref) {
+                derived_ref = find_derived_hwdevice_ctx(tmp_ref, type);
+
+                if (tmp_ref != derived_ref)
+                    av_buffer_unref(&tmp_ref);
+
+                if (derived_ref)
+                    return derived_ref;
+            }
+        }
+
+    return NULL;
+}
+
+static int hwdevice_ctx_create_derived(AVBufferRef **dst_ref_ptr,
+                                       enum AVHWDeviceType type,
+                                       AVBufferRef *src_ref,
+                                       AVDictionary *options, int flags,
+                                       int get_existing)
 {
     AVBufferRef *dst_ref = NULL, *tmp_ref;
     AVHWDeviceContext *dst_ctx, *tmp_ctx;
@@ -667,6 +788,18 @@ int av_hwdevice_ctx_create_derived_opts(AVBufferRef **dst_ref_ptr,
         tmp_ref = tmp_ctx->internal->source_device;
     }
 
+    if (get_existing) {
+        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);
@@ -688,6 +821,9 @@ int av_hwdevice_ctx_create_derived_opts(AVBufferRef **dst_ref_ptr,
                     ret = AVERROR(ENOMEM);
                     goto fail;
                 }
+                if (!tmp_ctx->internal->derived_device_ids[type])
+                    tmp_ctx->internal->derived_device_ids[type] = dst_ctx->internal->registered_device_id;
+
                 ret = av_hwdevice_ctx_init(dst_ref);
                 if (ret < 0)
                     goto fail;
@@ -712,12 +848,29 @@ fail:
     return ret;
 }
 
+int av_hwdevice_ctx_create_derived_opts(AVBufferRef **dst_ref_ptr,
+                                        enum AVHWDeviceType type,
+                                        AVBufferRef *src_ref,
+                                        AVDictionary *options, int flags)
+{
+    return hwdevice_ctx_create_derived(dst_ref_ptr, type, src_ref,
+                                       options, flags, 0);
+}
+
+int av_hwdevice_ctx_get_or_create_derived(AVBufferRef **dst_ref_ptr,
+                                          enum AVHWDeviceType type,
+                                          AVBufferRef *src_ref, int flags)
+{
+    return hwdevice_ctx_create_derived(dst_ref_ptr, type, src_ref,
+                                       NULL, flags, 1);
+}
+
 int av_hwdevice_ctx_create_derived(AVBufferRef **dst_ref_ptr,
                                    enum AVHWDeviceType type,
                                    AVBufferRef *src_ref, int flags)
 {
-    return av_hwdevice_ctx_create_derived_opts(dst_ref_ptr, type, src_ref,
-                                               NULL, flags);
+    return hwdevice_ctx_create_derived(dst_ref_ptr, type, src_ref,
+                                       NULL, flags, 0);
 }
 
 static void ff_hwframe_unmap(void *opaque, uint8_t *data)
diff --git a/libavutil/hwcontext.h b/libavutil/hwcontext.h
index c18b7e1e8b..3785811f98 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;
@@ -328,6 +329,25 @@ int av_hwdevice_ctx_create_derived(AVBufferRef **dst_ctx,
                                    enum AVHWDeviceType type,
                                    AVBufferRef *src_ctx, int flags);
 
+/**
+ * Create a new device of the specified type from an existing device.
+ *
+ * This function performs the same action as av_hwdevice_ctx_create_derived,
+ * however, if a derived device of the specified type already exists,
+ * it returns the existing instance.
+ *
+ * @param dst_ctx On success, a reference to the newly-created
+ *                AVHWDeviceContext.
+ * @param type    The type of the new device to create.
+ * @param src_ctx A reference to an existing AVHWDeviceContext which will be
+ *                used to create the new device.
+ * @param flags   Currently unused; should be set to zero.
+ * @return        Zero on success, a negative AVERROR code on failure.
+ */
+int av_hwdevice_ctx_get_or_create_derived(AVBufferRef **dst_ctx,
+                                          enum AVHWDeviceType type,
+                                          AVBufferRef *src_ctx, int flags);
+
 /**
  * Create a new device of the specified type from an existing device.
  *
diff --git a/libavutil/hwcontext_internal.h b/libavutil/hwcontext_internal.h
index e6266494ac..adb649cde4 100644
--- a/libavutil/hwcontext_internal.h
+++ b/libavutil/hwcontext_internal.h
@@ -109,6 +109,17 @@ struct AVHWDeviceInternal {
      * context it was derived from.
      */
     AVBufferRef *source_device;
+
+    /**
+     * An array of device registration ids from device contexts which
+     * were derived from this device.
+     */
+    int derived_device_ids[AV_HWDEVICE_TYPE_NB];
+
+    /**
+     * ID under wich the hw context is registered internally.
+     */
+    int registered_device_id;
 };
 
 struct AVHWFramesInternal {
diff --git a/libavutil/hwcontext_qsv.c b/libavutil/hwcontext_qsv.c
index 56dffa1f25..09f6b1289f 100644
--- a/libavutil/hwcontext_qsv.c
+++ b/libavutil/hwcontext_qsv.c
@@ -307,7 +307,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)
 {
 }
 
@@ -320,7 +320,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;
@@ -1658,8 +1658,13 @@ 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_device_ids[ctx->type] = ctx->internal->registered_device_id;
+    }
 
-    return qsv_device_derive_from_child(ctx, impl, child_device, 0);
+    return ret;
 }
 
 const HWContextType ff_hwcontext_type_qsv = {
-- 
ffmpeg-codebot

_______________________________________________
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".

[FFmpeg-devel] [PATCH v3 3/4] lavu: bump minor version and add doc/APIchanges entry for av_hwdevice_ctx_get_or_create_derived()

[softworkz]

From: softworkz <softworkz@hotmail.com>

Signed-off-by: softworkz <softworkz@hotmail.com>
---
 doc/APIchanges      | 6 ++++++
 libavutil/version.h | 2 +-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/doc/APIchanges b/doc/APIchanges
index b3563cd528..ba21f60953 100644
--- a/doc/APIchanges
+++ b/doc/APIchanges
@@ -14,6 +14,12 @@ libavutil:     2021-04-27
 
 API changes, most recent first:
 
+2022-07-01 - xxxxxxxxxx - lavu 57.31.100 - hwcontext.h
+  Add av_hwdevice_ctx_get_or_create_derived().
+
+2022-07-01 - xxxxxxxxxx - lavu 57.31.100 - buffer.h
+  Add av_ref_from_buffer().
+
 2022-07-xx - xxxxxxxxxx - lavu 57.30.100 - frame.h
   Add AVFrame.duration, deprecate AVFrame.pkt_duration.
 
diff --git a/libavutil/version.h b/libavutil/version.h
index ee4c531b80..e9eefcdb2c 100644
--- a/libavutil/version.h
+++ b/libavutil/version.h
@@ -79,7 +79,7 @@
  */
 
 #define LIBAVUTIL_VERSION_MAJOR  57
-#define LIBAVUTIL_VERSION_MINOR  30
+#define LIBAVUTIL_VERSION_MINOR  31
 #define LIBAVUTIL_VERSION_MICRO 100
 
 #define LIBAVUTIL_VERSION_INT   AV_VERSION_INT(LIBAVUTIL_VERSION_MAJOR, \
-- 
ffmpeg-codebot

_______________________________________________
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".

[FFmpeg-devel] [PATCH v3 4/4] avfilter/hwmap, hwupload: use new av_hwdevice_ctx_get_or_create_derived method

[softworkz]

From: softworkz <softworkz@hotmail.com>

Signed-off-by: softworkz <softworkz@hotmail.com>
---
 libavfilter/vf_hwmap.c    | 4 ++--
 libavfilter/vf_hwupload.c | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/libavfilter/vf_hwmap.c b/libavfilter/vf_hwmap.c
index 2e03dfc1fe..b79cf6732c 100644
--- a/libavfilter/vf_hwmap.c
+++ b/libavfilter/vf_hwmap.c
@@ -82,8 +82,8 @@ static int hwmap_config_output(AVFilterLink *outlink)
                 goto fail;
             }
 
-            err = av_hwdevice_ctx_create_derived(&device, type,
-                                                 hwfc->device_ref, 0);
+            err = av_hwdevice_ctx_get_or_create_derived(&device, type,
+                                                        hwfc->device_ref, 0);
             if (err < 0) {
                 av_log(avctx, AV_LOG_ERROR, "Failed to created derived "
                        "device context: %d.\n", err);
diff --git a/libavfilter/vf_hwupload.c b/libavfilter/vf_hwupload.c
index dbc41734cc..41ee0e43c4 100644
--- a/libavfilter/vf_hwupload.c
+++ b/libavfilter/vf_hwupload.c
@@ -51,7 +51,7 @@ static int hwupload_query_formats(AVFilterContext *avctx)
         /* We already have a specified device. */
     } else if (avctx->hw_device_ctx) {
         if (ctx->device_type) {
-            err = av_hwdevice_ctx_create_derived(
+            err = av_hwdevice_ctx_get_or_create_derived(
                 &ctx->hwdevice_ref,
                 av_hwdevice_find_type_by_name(ctx->device_type),
                 avctx->hw_device_ctx, 0);
-- 
ffmpeg-codebot
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH v3 0/4] Add derive-device function which searches for existing devices in both directions

[Soft Works]

> -----Original Message-----
> From: ffmpegagent <ffmpegagent@gmail.com>
> Sent: Friday, July 22, 2022 1:40 AM
> To: ffmpeg-devel@ffmpeg.org
> Cc: Mark Thompson <sw@jkqxz.net>; Soft Works <softworkz@hotmail.com>;
> softworkz <softworkz@hotmail.com>
> Subject: [PATCH v3 0/4] Add derive-device function which searches for
> existing devices in both directions
> 
> This is an updated version of: [PATCH v4 1/1] avutils/hwcontext: When
> deriving a hwdevice, search for existing device in both directions
> 
> There has been an objection that the earlier patchset would change
> API
> behavior, and that this change should be limited to ffmpeg cli.
> 
> To achieve this, the API behavior is left unchanged now and a new
> function
> av_hwdevice_ctx_get_or_create_derived() is added and used by the
> hwupload
> and hwmap filters.
> 
> v2: Implemented concept for "weak references" to avoid circular
> reference
> lockup.
> 
> v3: rebased due to conflicts
> 
> softworkz (4):
>   avutil/buffer: add av_ref_from_buffer() function
>   avutils/hwcontext: add derive-device function which searches for
>     existing devices in both directions
>   lavu: bump minor version and add doc/APIchanges entry for
>     av_hwdevice_ctx_get_or_create_derived()
>   avfilter/hwmap,hwupload: use new
> av_hwdevice_ctx_get_or_create_derived
>     method

Hi,

@Mark Thompson - would you mind to take a look at this?

You were the only one who was objecting and I had reworked 
this in the sense of our discussion (hopefully).

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".