[PATCH 0/3] 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. 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... 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
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
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
-----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
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
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
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
On 30/04/2022 21:07, softworkz wrote: source link 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
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
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
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
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
On 30/04/2022 21:07, softworkz wrote: source link 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
-----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
-----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
[..]
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 thread-safety properties of the hwcontext API have been lost
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
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
-----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
-----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
On 02/05/2022 09:14, Soft Works wrote: directions
[..]
* The thread-safety properties of the hwcontext API have been
-
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
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
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
lost that thread. 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
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
-----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
On 02/05/2022 09:14, Soft Works wrote: directions
[..]
* The thread-safety properties of the hwcontext API have been
-
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
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
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
lost that thread. 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
-----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
-----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
-----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
On 02/05/2022 09:14, Soft Works wrote: directions
[..]
> * The thread-safety properties of the hwcontext API have been
lost
-
> you can no longer operate on devices independently across
> (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
On 02/05/2022 23:59, Soft Works wrote: directions threads 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
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
-----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
-----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
On 02/05/2022 09:14, Soft Works wrote: directions
[..]
>> * The thread-safety properties of the hwcontext API have been
lost
- >> you can no longer operate on devices independently across
>> (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
On 02/05/2022 23:59, Soft Works wrote: directions threads 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
-----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
-----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
-----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
On 02/05/2022 23:59, Soft Works wrote: directions
On 02/05/2022 09:14, Soft Works wrote:
> -----Original Message----- > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf
Of
> 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
Mark directions
[..]
>>> * The thread-safety properties of the hwcontext API have been
> - >>> you can no longer operate on devices independently across
lost 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
> 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
>> 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
> > 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
which thread. 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-
On 03/05/2022 01:11, Soft Works wrote: directions thread- thread 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!
-----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
-----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
On 02/05/2022 23:59, Soft Works wrote:
-----Original Message----- From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf
Of
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
On 02/05/2022 09:14, Soft Works wrote:
> -----Original Message----- > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf
Of
> 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
Mark directions
[..]
>>> * The thread-safety properties of the hwcontext API have
been lost
> - >>> you can no longer operate on devices independently across
Mark directions 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
> 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
>> 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
> > 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
which thread. 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-
On 03/05/2022 01:11, Soft Works wrote: directions thread- thread 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
-----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
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
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
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... 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
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
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
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
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
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... 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
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
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
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
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
-----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
participants (4)
-
ffmpegagent -
Mark Thompson -
Soft Works -
softworkz