On Sat, 2021-08-07 at 01:46 +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>
> ---
> libavutil/hwcontext.c | 16 ++++++++++++++++
> libavutil/hwcontext_internal.h | 6 ++++++
> libavutil/hwcontext_qsv.c | 7 ++++++-
> 3 files changed, 28 insertions(+), 1 deletion(-)
>
> diff --git a/libavutil/hwcontext.c b/libavutil/hwcontext.c
> index d13d0f7c9b..3714ce7553 100644
> --- a/libavutil/hwcontext.c
> +++ b/libavutil/hwcontext.c
> @@ -132,6 +132,7 @@ static void hwdevice_ctx_free(void *opaque, uint8_t *data)
> ctx->free(ctx);
>
> av_buffer_unref(&ctx->internal->source_device);
> + av_buffer_unref(&ctx->internal->derived_device);
>
> av_freep(&ctx->hwctx);
> av_freep(&ctx->internal->priv);
> @@ -666,6 +667,20 @@ int av_hwdevice_ctx_create_derived_opts(AVBufferRef
> **dst_ref_ptr,
> tmp_ref = tmp_ctx->internal->source_device;
> }
>
> + tmp_ref = src_ref;
> + while (tmp_ref) {
> + tmp_ctx = (AVHWDeviceContext*)tmp_ref->data;
> + if (tmp_ctx->type == type) {
> + dst_ref = av_buffer_ref(tmp_ref);
> + if (!dst_ref) {
> + ret = AVERROR(ENOMEM);
> + goto fail;
> + }
> + goto done;
> + }
> + tmp_ref = tmp_ctx->internal->derived_device;
> + }
> +
> dst_ref = av_hwdevice_ctx_alloc(type);
> if (!dst_ref) {
> ret = AVERROR(ENOMEM);
> @@ -683,6 +698,7 @@ int av_hwdevice_ctx_create_derived_opts(AVBufferRef
> **dst_ref_ptr,
> flags);
> if (ret == 0) {
> dst_ctx->internal->source_device = av_buffer_ref(src_ref);
> + tmp_ctx->internal->derived_device = av_buffer_ref(dst_ref);
> if (!dst_ctx->internal->source_device) {
Need to check tmp_ctx->internal->derived_device too.
> ret = AVERROR(ENOMEM);
> goto fail;
> diff --git a/libavutil/hwcontext_internal.h b/libavutil/hwcontext_internal.h
> index e6266494ac..cfe525d20c 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;
> +
> + /**
> + * A reference to a device context which
> + * was derived from this device.
> + */
> + AVBufferRef *derived_device;
> };
>
> struct AVHWFramesInternal {
> diff --git a/libavutil/hwcontext_qsv.c b/libavutil/hwcontext_qsv.c
> index 08a6e0ee1c..27d96d116f 100644
> --- a/libavutil/hwcontext_qsv.c
> +++ b/libavutil/hwcontext_qsv.c
> @@ -1268,8 +1268,13 @@ static int qsv_device_create(AVHWDeviceContext *ctx,
> const char *device,
> child_device = (AVHWDeviceContext*)priv->child_device_ctx->data;
>
> impl = choose_implementation(device);
> + ret = qsv_device_derive_from_child(ctx, impl, child_device, 0);
> + if (ret >= 0) {
> + ctx->internal->source_device = av_buffer_ref(priv->child_device_ctx);
> + child_device->internal->derived_device =
> av_buffer_create((uint8_t*)ctx, sizeof(*ctx), 0, ctx, 0);
Need to check the new references here.
> + }
>
> - return qsv_device_derive_from_child(ctx, impl, child_device, 0);
> + return ret;
> }
>
> const HWContextType ff_hwcontext_type_qsv = {
_______________________________________________
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".
