> -----Original Message----- > From: ffmpeg-devel <ffmpeg-devel-boun...@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 _______________________________________________ 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".