Message ID | BYAPR04MB597605B1168D1F04AD6D36A7BAF49@BYAPR04MB5976.namprd04.prod.outlook.com |
---|---|
State | Superseded, archived |
Headers | show |
Series | [FFmpeg-devel] 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 |
On Sat, Aug 7, 2021 at 3:46 AM Soft Works <softworkz@hotmail.com> 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). > I already see one problem here, if you have so many derived cases happening, its also feasible to assume one source was used to derive more then one device, which this cannot track, and in fact looks like the code would just override and leak a reference. How common would it be that such complex derived chains happen, when you say even this one is untested? - Hendrik
> -----Original Message----- > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of > Hendrik Leppkes > Sent: Saturday, 7 August 2021 08:52 > To: FFmpeg development discussions and patches <ffmpeg- > devel@ffmpeg.org> > Subject: Re: [FFmpeg-devel] [PATCH] avutils/hwcontext: When deriving a > hwdevice, search for existing device in both directions > > On Sat, Aug 7, 2021 at 3:46 AM Soft Works <softworkz@hotmail.com> 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). > > > > I already see one problem here, if you have so many derived cases > happening, its also feasible to assume one source was used to derive more > then one device, which this cannot track, and in fact looks like the code > would just override and leak a reference. > > How common would it be that such complex derived chains happen, when > you say even this one is untested? I'm aware of what you are pointing at, but I couldn't easily construct a case where this would have an impact. What we need to consider here is that this is about derivation of HW device contexts - not frame contexts. In case of frame contexts, we can have pretty long chains of derivation between decoder, filters [0-n] and encoder, where we always have a separate HW frames context in-between. For device contexts, it's instead about creating and maintaining just a single one for each device. From a viewpoint of functionality, it's probably only a theoretical issue, but it might rarely happen that it gets overwritten. Even though it might appear to be overkill, but for a perfect solution, I'd need to maintain an array of derived_devices. Shall I do it? softworkz
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 <softworkz@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 = {
> -----Original Message----- > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of > Xiang, Haihao > Sent: Monday, 9 August 2021 09:27 > To: ffmpeg-devel@ffmpeg.org > Subject: Re: [FFmpeg-devel] [PATCH] avutils/hwcontext: When deriving a > hwdevice, search for existing device in both directions > > On Sat, 2021-08-07 at 01:46 +0000, Soft Works wrote: [...] > > 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. Added checks in the next update. Thanks a lot for reviewing. softworkz
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) { 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); + } - 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> --- libavutil/hwcontext.c | 16 ++++++++++++++++ libavutil/hwcontext_internal.h | 6 ++++++ libavutil/hwcontext_qsv.c | 7 ++++++- 3 files changed, 28 insertions(+), 1 deletion(-)