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

[Xiang, Haihao]

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