| Message ID | MN2PR04MB59818D411FD9040330ADDD37BAF79@MN2PR04MB5981.namprd04.prod.outlook.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [FFmpeg-devel,v2] avutils/hwcontext: When deriving a hwdevice, search for existing device in both directions | expand |
| Context | Check | Description |
|---|---|---|
| andriy/x86_make | success | Make finished |
| andriy/x86_make_fate | success | Make fate finished |
| andriy/PPC64_make | success | Make finished |
| andriy/PPC64_make_fate | success | Make fate finished |
> -----Original Message----- > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of > Soft Works > Sent: Tuesday, 10 August 2021 11:53 > To: ffmpeg-devel@ffmpeg.org > Subject: [FFmpeg-devel] [PATCH v2] avutils/hwcontext: When deriving a > hwdevice, search for existing device in both directions > [..] > Signed-off-by: softworkz <softworkz@hotmail.com> > --- > v2: allow storing multiple derived devices in a device ctx; add > checks for oom > libavutil/hwcontext.c | 38 > ++++++++++++++++++++++++++++++++++ > libavutil/hwcontext.h | 1 + > libavutil/hwcontext_internal.h | 6 ++++++ > libavutil/hwcontext_qsv.c | 10 ++++++++- > 4 files changed, 54 insertions(+), 1 deletion(-) > > diff --git a/libavutil/hwcontext.c b/libavutil/hwcontext.c > index d13d0f7c9b..7f4e541553 100644 > --- a/libavutil/hwcontext.c > +++ b/libavutil/hwcontext.c Now, that Haihao's patch has been merged, (https://patchwork.ffmpeg.org/project/ffmpeg/list/?series=4532) this patch is required to make all related derivation scenarios work, and for some that are working, it will avoid duplicate instantiation of hw device contexts. This version of the patch addresses the (valid) concerns from @Hendrik by storing multiple derived device contexts in cases where more than one device is derived from another device. It also adds the missing checks as pointed out by Haihao. Would be great when someone could take a look at the code. The combination of this and Haihao's patch is quite a milestone (in the QSV area) as it will enable several scenarios that haven't been working before. From a quick lock, this will also close the following tickets: openCL cannot be run simultaneously with h264_qsv https://trac.ffmpeg.org/ticket/6077 Intel QSV: "No device available for encoder" message is given https://trac.ffmpeg.org/ticket/6492 overlay_qsv fails to create qsvvpp https://trac.ffmpeg.org/ticket/7808 overlay_qsv cannot burn graphic subtitles on windows https://trac.ffmpeg.org/ticket/8609 overlay_qsv fails with "Inputs with different underlying QSV devices are forbidden" https://trac.ffmpeg.org/ticket/8995 Kind regards, softworkz
On Wed, 2021-08-11 at 17:22 +0000, Soft Works wrote: > > -----Original Message----- > > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of > > Soft Works > > Sent: Tuesday, 10 August 2021 11:53 > > To: ffmpeg-devel@ffmpeg.org > > Subject: [FFmpeg-devel] [PATCH v2] avutils/hwcontext: When deriving a > > hwdevice, search for existing device in both directions > > > > [..] > > > Signed-off-by: softworkz <softworkz@hotmail.com> > > --- > > v2: allow storing multiple derived devices in a device ctx; add > > checks for oom > > libavutil/hwcontext.c | 38 > > ++++++++++++++++++++++++++++++++++ > > libavutil/hwcontext.h | 1 + > > libavutil/hwcontext_internal.h | 6 ++++++ > > libavutil/hwcontext_qsv.c | 10 ++++++++- > > 4 files changed, 54 insertions(+), 1 deletion(-) > > > > diff --git a/libavutil/hwcontext.c b/libavutil/hwcontext.c > > index d13d0f7c9b..7f4e541553 100644 > > --- a/libavutil/hwcontext.c > > +++ b/libavutil/hwcontext.c > > Now, that Haihao's patch has been merged, > (https://patchwork.ffmpeg.org/project/ffmpeg/list/?series=4532) > > this patch is required to make all related derivation scenarios > work, and for some that are working, it will avoid duplicate > instantiation of hw device contexts. > > This version of the patch addresses the (valid) concerns > from @Hendrik by storing multiple derived device contexts > in cases where more than one device is derived from another > device. > > It also adds the missing checks as pointed out by Haihao. > > Would be great when someone could take a look at the code. > I will give a try and update you when done. Thanks Haihao > The combination of this and Haihao's patch is quite a milestone > (in the QSV area) as it will enable several scenarios that > haven't been working before. > > From a quick lock, this will also close the following tickets: > > > openCL cannot be run simultaneously with h264_qsv > https://trac.ffmpeg.org/ticket/6077 > > Intel QSV: "No device available for encoder" message is given > https://trac.ffmpeg.org/ticket/6492 > > overlay_qsv fails to create qsvvpp > https://trac.ffmpeg.org/ticket/7808 > > overlay_qsv cannot burn graphic subtitles on windows > https://trac.ffmpeg.org/ticket/8609 > > overlay_qsv fails with "Inputs with different underlying QSV > devices are forbidden" > https://trac.ffmpeg.org/ticket/8995 > > > Kind regards, > softworkz > > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
On Tue, 2021-08-10 at 09:52 +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 <softworkz@hotmail.com> > --- > v2: allow storing multiple derived devices in a device ctx; add checks for oom > libavutil/hwcontext.c | 38 ++++++++++++++++++++++++++++++++++ > libavutil/hwcontext.h | 1 + > libavutil/hwcontext_internal.h | 6 ++++++ > libavutil/hwcontext_qsv.c | 10 ++++++++- > 4 files changed, 54 insertions(+), 1 deletion(-) > > diff --git a/libavutil/hwcontext.c b/libavutil/hwcontext.c > index d13d0f7c9b..7f4e541553 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 0; I prefer to use NULL instead of 0 because the return type is pointer to AVBufferRef. The remaining looks good to me and it really fixes a few issues for me. Thanks Haihao > +} > + > 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 08a6e0ee1c..b26910f95d 100644 > --- a/libavutil/hwcontext_qsv.c > +++ b/libavutil/hwcontext_qsv.c > @@ -1268,8 +1268,16 @@ 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_devices[ctx->type] = > av_buffer_create((uint8_t*)ctx, sizeof(*ctx), 0, 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 = {
On Tue, 2021-08-10 at 09:52 +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) > This change causes a regression when running the command below: $> ffmpeg -y -hwaccel qsv -c:v h264_qsv -i input.h264 -filter_complex "split[s0][s1]" -map "[s0]" -c:v h264_qsv out0.h264 -map "[s1]" -c:v h264_qsv out1.h264 ... video:143kB audio:0kB subtitle:0kB other streams:0kB global headers:0kB muxing overhead: unknown corrupted size vs. prev_size in fastbins Aborted Thanks Haihao > Signed-off-by: softworkz <softworkz@hotmail.com> > --- > v2: allow storing multiple derived devices in a device ctx; add checks for oom > libavutil/hwcontext.c | 38 ++++++++++++++++++++++++++++++++++ > libavutil/hwcontext.h | 1 + > libavutil/hwcontext_internal.h | 6 ++++++ > libavutil/hwcontext_qsv.c | 10 ++++++++- > 4 files changed, 54 insertions(+), 1 deletion(-) > > diff --git a/libavutil/hwcontext.c b/libavutil/hwcontext.c > index d13d0f7c9b..7f4e541553 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 0; > +} > + > 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 08a6e0ee1c..b26910f95d 100644 > --- a/libavutil/hwcontext_qsv.c > +++ b/libavutil/hwcontext_qsv.c > @@ -1268,8 +1268,16 @@ 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_devices[ctx->type] = > av_buffer_create((uint8_t*)ctx, sizeof(*ctx), 0, 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 = {
On Fri, 2021-08-13 at 06:29 +0000, Xiang, Haihao wrote: > On Tue, 2021-08-10 at 09:52 +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) > > > > This change causes a regression when running the command below: > > $> ffmpeg -y -hwaccel qsv -c:v h264_qsv -i input.h264 -filter_complex > "split[s0][s1]" -map "[s0]" -c:v h264_qsv out0.h264 -map "[s1]" -c:v h264_qsv > out1.h264 > > ... > > video:143kB audio:0kB subtitle:0kB other streams:0kB global headers:0kB muxing > overhead: unknown > corrupted size vs. prev_size in fastbins > Aborted > Hi Softworks, + child_device->internal->derived_devices[ctx->type] = av_buffer_create((uint8_t*)ctx, sizeof(*ctx), 0, ctx, 0); The above change introduces a new AVBufferRef for ctx. The first AVBufferRef for ctx is created when function av_hwdevice_ctx_alloc is called. So there are two different AVBufferRefs referring to the same ctx, then ctx will be double-freed The change below is a bit ugly, but it may fix this double-free issue. +static void qsv_ctx_free(void *opaque, uint8_t *ctx) +{ + // Do nothing here + // ctx is freed in hwdevice_ctx_free +} + static int qsv_device_create(AVHWDeviceContext *ctx, const char *device, AVDictionary *opts, int flags) { @@ -1271,7 +1277,7 @@ static int qsv_device_create(AVHWDeviceContext *ctx, const char *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_devices[ctx->type] = av_buffer_create((uint8_t*)ctx, sizeof(*ctx), 0, ctx, 0); + child_device->internal->derived_devices[ctx->type] = av_buffer_create((uint8_t*)ctx, sizeof(*ctx), qsv_ctx_free, ctx, 0); if (!child_device->internal->derived_devices[ctx->type]) { return AVERROR(ENOMEM); } Thanks Haihao
> -----Original Message----- > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Xiang, > Haihao > Sent: Thursday, 19 August 2021 09:37 > To: ffmpeg-devel@ffmpeg.org > Subject: Re: [FFmpeg-devel] [PATCH v2] avutils/hwcontext: When deriving a > hwdevice, search for existing device in both directions > > On Fri, 2021-08-13 at 06:29 +0000, Xiang, Haihao wrote: > > On Tue, 2021-08-10 at 09:52 +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. > > > [..] > > > > video:143kB audio:0kB subtitle:0kB other streams:0kB global headers:0kB > muxing > > overhead: unknown > > corrupted size vs. prev_size in fastbins > > Aborted > > > > Hi Softworks, > > + child_device->internal->derived_devices[ctx->type] = > av_buffer_create((uint8_t*)ctx, sizeof(*ctx), 0, ctx, 0); > > The above change introduces a new AVBufferRef for ctx. The first AVBufferRef > for > ctx is created when function av_hwdevice_ctx_alloc is called. So there are > two > different AVBufferRefs referring to the same ctx, then ctx will be double- > freed > > The change below is a bit ugly, but it may fix this double-free issue. > > +static void qsv_ctx_free(void *opaque, uint8_t *ctx) > +{ > + // Do nothing here > + // ctx is freed in hwdevice_ctx_free > +} > + > static int qsv_device_create(AVHWDeviceContext *ctx, const char *device, > AVDictionary *opts, int flags) > { > @@ -1271,7 +1277,7 @@ static int qsv_device_create(AVHWDeviceContext *ctx, > const > char *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_devices[ctx->type] = > av_buffer_create((uint8_t*)ctx, sizeof(*ctx), 0, ctx, 0); > + child_device->internal->derived_devices[ctx->type] = > av_buffer_create((uint8_t*)ctx, sizeof(*ctx), qsv_ctx_free, ctx, 0); > if (!child_device->internal->derived_devices[ctx->type]) { > return AVERROR(ENOMEM); > } Hi Haihao, sorry for getting back late and thanks for your suggested fix. I'll apply it and submit a new patch version. Best regards, softworkz
Hi softworkz: Any updates on this patch? I want to submit pathes to qsv, but they depends on this patch Thanks Wenbin > -----Original Message----- > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Soft > Works > Sent: Thursday, August 19, 2021 3:52 PM > To: FFmpeg development discussions and patches <ffmpeg- > devel@ffmpeg.org> > Subject: Re: [FFmpeg-devel] [PATCH v2] avutils/hwcontext: When deriving a > hwdevice, search for existing device in both directions > > > > > -----Original Message----- > > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of > Xiang, > > Haihao > > Sent: Thursday, 19 August 2021 09:37 > > To: ffmpeg-devel@ffmpeg.org > > Subject: Re: [FFmpeg-devel] [PATCH v2] avutils/hwcontext: When deriving a > > hwdevice, search for existing device in both directions > > > > On Fri, 2021-08-13 at 06:29 +0000, Xiang, Haihao wrote: > > > On Tue, 2021-08-10 at 09:52 +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. > > > > > > [..] > > > > > > > video:143kB audio:0kB subtitle:0kB other streams:0kB global headers:0kB > > muxing > > > overhead: unknown > > > corrupted size vs. prev_size in fastbins > > > Aborted > > > > > > > Hi Softworks, > > > > + child_device->internal->derived_devices[ctx->type] = > > av_buffer_create((uint8_t*)ctx, sizeof(*ctx), 0, ctx, 0); > > > > The above change introduces a new AVBufferRef for ctx. The first > AVBufferRef > > for > > ctx is created when function av_hwdevice_ctx_alloc is called. So there are > > two > > different AVBufferRefs referring to the same ctx, then ctx will be double- > > freed > > > > The change below is a bit ugly, but it may fix this double-free issue. > > > > +static void qsv_ctx_free(void *opaque, uint8_t *ctx) > > +{ > > + // Do nothing here > > + // ctx is freed in hwdevice_ctx_free > > +} > > + > > static int qsv_device_create(AVHWDeviceContext *ctx, const char *device, > > AVDictionary *opts, int flags) > > { > > @@ -1271,7 +1277,7 @@ static int qsv_device_create(AVHWDeviceContext > *ctx, > > const > > char *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_devices[ctx->type] = > > av_buffer_create((uint8_t*)ctx, sizeof(*ctx), 0, ctx, 0); > > + child_device->internal->derived_devices[ctx->type] = > > av_buffer_create((uint8_t*)ctx, sizeof(*ctx), qsv_ctx_free, ctx, 0); > > if (!child_device->internal->derived_devices[ctx->type]) { > > return AVERROR(ENOMEM); > > } > > Hi Haihao, > > sorry for getting back late and thanks for your suggested fix. > I'll apply it and submit a new patch version. > > Best regards, > softworkz > > > > > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
> -----Original Message----- > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of > Chen, Wenbin > Sent: Wednesday, 8 September 2021 07:58 > To: FFmpeg development discussions and patches <ffmpeg- > devel@ffmpeg.org> > Subject: Re: [FFmpeg-devel] [PATCH v2] avutils/hwcontext: When > deriving a hwdevice, search for existing device in both directions > > Hi softworkz: > > Any updates on this patch? > I want to submit pathes to qsv, but they depends on this patch > > Thanks > Wenbin Hi Wenbin, Haihao has discovered a regression but I haven't looked into it yet. Will do within the next few days. softworkz
> -----Original Message----- > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Soft > Works > Sent: Wednesday, September 8, 2021 2:25 PM > To: FFmpeg development discussions and patches <ffmpeg- > devel@ffmpeg.org> > Subject: Re: [FFmpeg-devel] [PATCH v2] avutils/hwcontext: When deriving a > hwdevice, search for existing device in both directions > > > > > -----Original Message----- > > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of > > Chen, Wenbin > > Sent: Wednesday, 8 September 2021 07:58 > > To: FFmpeg development discussions and patches <ffmpeg- > > devel@ffmpeg.org> > > Subject: Re: [FFmpeg-devel] [PATCH v2] avutils/hwcontext: When > > deriving a hwdevice, search for existing device in both directions > > > > Hi softworkz: > > > > Any updates on this patch? > > I want to submit pathes to qsv, but they depends on this patch > > > > Thanks > > Wenbin > > Hi Wenbin, > > Haihao has discovered a regression but I haven't looked into it > yet. Will do within the next few days. > > softworkz Got it Thank you > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
> -----Original Message----- > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of > Xiang, Haihao > Sent: Thursday, August 19, 2021 9:37 AM > To: ffmpeg-devel@ffmpeg.org > Subject: Re: [FFmpeg-devel] [PATCH v2] avutils/hwcontext: When > deriving a hwdevice, search for existing device in both directions > > On Fri, 2021-08-13 at 06:29 +0000, Xiang, Haihao wrote: > > On Tue, 2021-08-10 at 09:52 +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) > > > > > > > This change causes a regression when running the command below: > > > > $> ffmpeg -y -hwaccel qsv -c:v h264_qsv -i input.h264 - > filter_complex > > "split[s0][s1]" -map "[s0]" -c:v h264_qsv out0.h264 -map "[s1]" - > c:v h264_qsv > > out1.h264 > > > > ... > > > > video:143kB audio:0kB subtitle:0kB other streams:0kB global > headers:0kB muxing > > overhead: unknown > > corrupted size vs. prev_size in fastbins > > Aborted > > > > Hi Softworks, > > + child_device->internal->derived_devices[ctx->type] = > av_buffer_create((uint8_t*)ctx, sizeof(*ctx), 0, ctx, 0); > > The above change introduces a new AVBufferRef for ctx. The first > AVBufferRef for > ctx is created when function av_hwdevice_ctx_alloc is called. So > there are two > different AVBufferRefs referring to the same ctx, then ctx will be > double-freed > > The change below is a bit ugly, but it may fix this double-free > issue. > > +static void qsv_ctx_free(void *opaque, uint8_t *ctx) > +{ > + // Do nothing here > + // ctx is freed in hwdevice_ctx_free > +} > + > static int qsv_device_create(AVHWDeviceContext *ctx, const char > *device, > AVDictionary *opts, int flags) > { > @@ -1271,7 +1277,7 @@ static int qsv_device_create(AVHWDeviceContext > *ctx, const > char *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_devices[ctx->type] = > av_buffer_create((uint8_t*)ctx, sizeof(*ctx), 0, ctx, 0); > + child_device->internal->derived_devices[ctx->type] = > av_buffer_create((uint8_t*)ctx, sizeof(*ctx), qsv_ctx_free, ctx, 0); > if (!child_device->internal->derived_devices[ctx->type]) { > return AVERROR(ENOMEM); > } > > > Thanks > Haihao Got it done now. Thanks for pointing this out and sorry for pushing it forward for so long. Kind regards, softworkz
diff --git a/libavutil/hwcontext.c b/libavutil/hwcontext.c index d13d0f7c9b..7f4e541553 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 0; +} + 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 08a6e0ee1c..b26910f95d 100644 --- a/libavutil/hwcontext_qsv.c +++ b/libavutil/hwcontext_qsv.c @@ -1268,8 +1268,16 @@ 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_devices[ctx->type] = av_buffer_create((uint8_t*)ctx, sizeof(*ctx), 0, 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 = {
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> --- v2: allow storing multiple derived devices in a device ctx; add checks for oom libavutil/hwcontext.c | 38 ++++++++++++++++++++++++++++++++++ libavutil/hwcontext.h | 1 + libavutil/hwcontext_internal.h | 6 ++++++ libavutil/hwcontext_qsv.c | 10 ++++++++- 4 files changed, 54 insertions(+), 1 deletion(-)