[FFmpeg-devel] [PATCH v2] avutils/hwcontext: When deriving a hwdevice, search for existing device in both directions

[Soft Works]

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of
> Xiang, Haihao
> Sent: Thursday, August 19, 2021 9:37 AM
> To: ffmpeg-devel at 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