On Mon, 2021-10-11 at 04:19 +0000, Soft Works wrote:
> The test /libavutil/tests/hwdevice checks that when deriving a device
> from a source device and then deriving back to the type of the source
> device, the result is matching the original source device, i.e. the
> derivation mechanism doesn't create a new device in this case.
>
> Previously, this test was usually passed, but only due to two different
> kind of flaws:
>
> 1. The test covers only a single level of derivation (and back)
>
> It derives device Y from device X and then Y back to the type of X and
> checks whether the result matches X.
>
> What it doesn't check for, are longer chains of derivation like:
>
> CUDA1 > OpenCL2 > CUDA3 and then back to OpenCL4
>
> In that case, the second derivation returns the first device (CUDA3 ==
> CUDA1), but when deriving OpenCL4, hwcontext.c was creating a new
> OpenCL4 context instead of returning OpenCL2, because there was no link
> from CUDA1 to OpenCL2 (only backwards from OpenCL2 to CUDA1)
>
> If the test would check for two levels of derivation, it would have
> failed.
>
> This patch fixes those (yet untested) cases by introducing forward
> references (derived_device) in addition to the existing back references
> (source_device).
>
> 2. hwcontext_qsv didn't properly set the source_device
>
> In case of QSV, hwcontext_qsv creates a source context internally
> (vaapi, dxva2 or d3d11va) without calling av_hwdevice_ctx_create_derived
> and without setting source_device.
>
> This way, the hwcontext test ran successful, but what practically
> happened, was that - for example - deriving vaapi from qsv didn't return
> the original underlying vaapi device and a new one was created instead:
> Exactly what the test is intended to detect and prevent. It just
> couldn't do so, because the original device was hidden (= not set as the
> source_device of the QSV device).
>
> This patch properly makes these setting and fixes all derivation
> scenarios.
>
> (at a later stage, /libavutil/tests/hwdevice should be extended to check
> longer derivation chains as well)
>
> Signed-off-by: softworkz <softwo...@hotmail.com>
> ---
> v3: avoid double-release as suggested by Haihao
>
> libavutil/hwcontext.c | 38 ++++++++++++++++++++++++++++++++++
> libavutil/hwcontext.h | 1 +
> libavutil/hwcontext_internal.h | 6 ++++++
> libavutil/hwcontext_qsv.c | 16 ++++++++++----
> 4 files changed, 57 insertions(+), 4 deletions(-)
>
> diff --git a/libavutil/hwcontext.c b/libavutil/hwcontext.c
> index 31c7840dba..1a50635018 100644
> --- a/libavutil/hwcontext.c
> +++ b/libavutil/hwcontext.c
> @@ -122,6 +122,7 @@ static const AVClass hwdevice_ctx_class = {
> static void hwdevice_ctx_free(void *opaque, uint8_t *data)
> {
> AVHWDeviceContext *ctx = (AVHWDeviceContext*)data;
> + int i;
>
> /* uninit might still want access the hw context and the user
> * free() callback might destroy it, so uninit has to be called first */
> @@ -132,6 +133,8 @@ static void hwdevice_ctx_free(void *opaque, uint8_t *data)
> ctx->free(ctx);
>
> av_buffer_unref(&ctx->internal->source_device);
> + for (i = 0; i < AV_HWDEVICE_TYPE_NB; i++)
> + av_buffer_unref(&ctx->internal->derived_devices[i]);
>
> av_freep(&ctx->hwctx);
> av_freep(&ctx->internal->priv);
> @@ -643,6 +646,26 @@ fail:
> return ret;
> }
>
> +static AVBufferRef* find_derived_hwdevice_ctx(AVBufferRef *src_ref, enum
> AVHWDeviceType type)
> +{
> + AVBufferRef *tmp_ref;
> + AVHWDeviceContext *src_ctx;
> + int i;
> +
> + src_ctx = (AVHWDeviceContext*)src_ref->data;
> + if (src_ctx->type == type)
> + return src_ref;
> +
> + for (i = 0; i < AV_HWDEVICE_TYPE_NB; i++)
> + if (src_ctx->internal->derived_devices[i]) {
> + tmp_ref = find_derived_hwdevice_ctx(src_ctx->internal-
> >derived_devices[i], type);
> + if (tmp_ref)
> + return tmp_ref;
> + }
> +
> + return NULL;
> +}
> +
> int av_hwdevice_ctx_create_derived_opts(AVBufferRef **dst_ref_ptr,
> enum AVHWDeviceType type,
> AVBufferRef *src_ref,
> @@ -666,6 +689,16 @@ int av_hwdevice_ctx_create_derived_opts(AVBufferRef
> **dst_ref_ptr,
> tmp_ref = tmp_ctx->internal->source_device;
> }
>
> + tmp_ref = find_derived_hwdevice_ctx(src_ref, type);
> + if (tmp_ref) {
> + dst_ref = av_buffer_ref(tmp_ref);
> + if (!dst_ref) {
> + ret = AVERROR(ENOMEM);
> + goto fail;
> + }
> + goto done;
> + }
> +
> dst_ref = av_hwdevice_ctx_alloc(type);
> if (!dst_ref) {
> ret = AVERROR(ENOMEM);
> @@ -687,6 +720,11 @@ int av_hwdevice_ctx_create_derived_opts(AVBufferRef
> **dst_ref_ptr,
> ret = AVERROR(ENOMEM);
> goto fail;
> }
> + tmp_ctx->internal->derived_devices[type] =
> av_buffer_ref(dst_ref);
> + if (!tmp_ctx->internal->derived_devices[type]) {
> + ret = AVERROR(ENOMEM);
> + goto fail;
> + }
> ret = av_hwdevice_ctx_init(dst_ref);
> if (ret < 0)
> goto fail;
> diff --git a/libavutil/hwcontext.h b/libavutil/hwcontext.h
> index 04d19d89c2..56077963e6 100644
> --- a/libavutil/hwcontext.h
> +++ b/libavutil/hwcontext.h
> @@ -37,6 +37,7 @@ enum AVHWDeviceType {
> AV_HWDEVICE_TYPE_OPENCL,
> AV_HWDEVICE_TYPE_MEDIACODEC,
> AV_HWDEVICE_TYPE_VULKAN,
> + AV_HWDEVICE_TYPE_NB, ///< number of hw device types
> };
>
> typedef struct AVHWDeviceInternal AVHWDeviceInternal;
> diff --git a/libavutil/hwcontext_internal.h b/libavutil/hwcontext_internal.h
> index e6266494ac..f6fb67c491 100644
> --- a/libavutil/hwcontext_internal.h
> +++ b/libavutil/hwcontext_internal.h
> @@ -109,6 +109,12 @@ struct AVHWDeviceInternal {
> * context it was derived from.
> */
> AVBufferRef *source_device;
> +
> + /**
> + * An array of reference to device contexts which
> + * were derived from this device.
> + */
> + AVBufferRef *derived_devices[AV_HWDEVICE_TYPE_NB];
> };
>
> struct AVHWFramesInternal {
> diff --git a/libavutil/hwcontext_qsv.c b/libavutil/hwcontext_qsv.c
> index c18747f7eb..7b559e2b47 100644
> --- a/libavutil/hwcontext_qsv.c
> +++ b/libavutil/hwcontext_qsv.c
> @@ -223,7 +223,7 @@ static void qsv_frames_uninit(AVHWFramesContext *ctx)
> av_buffer_unref(&s->child_frames_ref);
> }
>
> -static void qsv_pool_release_dummy(void *opaque, uint8_t *data)
> +static void qsv_release_dummy(void *opaque, uint8_t *data)
> {
> }
>
> @@ -236,9 +236,9 @@ static AVBufferRef *qsv_pool_alloc(void *opaque, size_t
> size)
> if (s->nb_surfaces_used < hwctx->nb_surfaces) {
> s->nb_surfaces_used++;
> av_buffer_create((uint8_t*)(s->handle_pairs_internal + s-
> >nb_surfaces_used - 1),
> - sizeof(*s->handle_pairs_internal),
> qsv_pool_release_dummy, NULL, 0);
> + sizeof(*s->handle_pairs_internal),
> qsv_release_dummy, NULL, 0);
> 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;
> @@ -1528,8 +1528,16 @@ 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 = {
LGTM,
-Haihao
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
