[FFmpeg-devel] lavc/amfenc: Retain a reference to D3D frames used as input during the encoding process

This fixes frame corruption issue when decoder started reusing frames while they are still in use of encoding process
Issue with frame corruption  was reproduced using:
ffmpeg.exe -y -hwaccel d3d11va -hwaccel_output_format d3d11 -i input.h264  -an -c:v h264_amf output.mkv

Previous questions and answers
> How many frames can end up queued inside the encoder here?
16

> Is there always an exact 1->1 correspondence between input frames and output packets?  That is, is it guaranteed that no frames
are
> ever dropped, even in the low-latency modes?
yes

> These look even more like they should be functions rather than macros now?  In particular, the returns in them interact badly with
the code below.
Macroses were used because they works with different types. I have removed returns from them (sorry, I missed this comment).

---
 libavcodec/amfenc.c | 89 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 89 insertions(+)

             ID3D11Texture2D *texture = (ID3D11Texture2D*)frame->data[0]; // actual texture
@@ -496,6 +564,16 @@ int ff_amf_send_frame(AVCodecContext *avctx, const AVFrame *frame)
             // input HW surfaces can be vertically aligned by 16; tell AMF the real size
             surface->pVtbl->SetCrop(surface, 0, 0, frame->width, frame->height);
 #endif
+
+            frame_ref_storage_buffer = amf_create_buffer_with_frame_ref(frame, ctx->context);
+            AMF_RETURN_IF_FALSE(ctx, frame_ref_storage_buffer != NULL, AVERROR(ENOMEM), "create_buffer_with_frame_ref() returned
NULL\n");
+
+            AMF_AV_ASSIGN_PROPERTY_INTERFACE(res, surface, L"av_frame_ref", frame_ref_storage_buffer);
+            if (res != AMF_OK) {
+                surface->pVtbl->Release(surface);
+            }
+            frame_ref_storage_buffer->pVtbl->Release(frame_ref_storage_buffer);
+            AMF_RETURN_IF_FALSE(ctx, res == AMF_OK, AVERROR(ENOMEM), "SetProperty failed for \"av_frame_ref\" with error %d\n",
res);
         } else {
             res = ctx->context->pVtbl->AllocSurface(ctx->context, AMF_MEMORY_HOST, ctx->format, avctx->width, avctx->height,
&surface);
             AMF_RETURN_IF_FALSE(ctx, res == AMF_OK, AVERROR(ENOMEM), "AllocSurface() failed  with error %d\n", res);
@@ -560,6 +638,17 @@ int ff_amf_receive_packet(AVCodecContext *avctx, AVPacket *avpkt)
             ret = amf_copy_buffer(avctx, avpkt, buffer);
 
             buffer->pVtbl->Release(buffer);
+
+            if (data->pVtbl->HasProperty(data, L"av_frame_ref")) {
+                AMFBuffer *frame_ref_storage_buffer;
+                AMF_AV_GET_PROPERTY_INTERFACE(res, data, L"av_frame_ref", AMFBuffer, frame_ref_storage_buffer);
+                if (res == AMF_OK) {
+                    amf_release_buffer_with_frame_ref(frame_ref_storage_buffer);
+                } else {
+                    av_log(avctx, AV_LOG_WARNING, "GetProperty failed for \"av_frame_ref\" with error %d\n", res);
+                }
+            }
+
             data->pVtbl->Release(data);
 
             AMF_RETURN_IF_FALSE(ctx, ret >= 0, ret, "amf_copy_buffer() failed with error %d\n", ret);

On 4/5/2018 12:23 PM, Alexander Kravchenko wrote:
> 
> This fixes frame corruption issue when decoder started reusing frames while they are still in use of encoding process
> Issue with frame corruption  was reproduced using:
> ffmpeg.exe -y -hwaccel d3d11va -hwaccel_output_format d3d11 -i input.h264  -an -c:v h264_amf output.mkv
> 
> Previous questions and answers
>> How many frames can end up queued inside the encoder here?
> 16
> 
>> Is there always an exact 1->1 correspondence between input frames and output packets?  That is, is it guaranteed that no frames
> are
>> ever dropped, even in the low-latency modes?
> yes
> 
>> These look even more like they should be functions rather than macros now?  In particular, the returns in them interact badly with
> the code below.
> Macroses were used because they works with different types. I have removed returns from them (sorry, I missed this comment).
> 
> ---
>  libavcodec/amfenc.c | 89 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 89 insertions(+)
> 
> diff --git a/libavcodec/amfenc.c b/libavcodec/amfenc.c
> index 89a10ff253..084b06e56d 100644
> --- a/libavcodec/amfenc.c
> +++ b/libavcodec/amfenc.c
> @@ -443,6 +443,73 @@ int ff_amf_encode_init(AVCodecContext *avctx)
>      return ret;
>  }
>  
> +#define AMF_AV_QUERY_INTERFACE(res, from, InterfaceTypeTo, to) \
> +    { \
> +        AMFGuid guid_##InterfaceTypeTo = IID_##InterfaceTypeTo(); \
> +        res = from->pVtbl->QueryInterface(from, &guid_##InterfaceTypeTo, (void**)&to); \
> +    }
> +
> +#define AMF_AV_ASSIGN_PROPERTY_INTERFACE(res, pThis, name, val) \
> +    { \
> +        AMFInterface *amf_interface; \
> +        AMFVariantStruct var; \
> +        res = AMFVariantInit(&var); \
> +        if (res == AMF_OK) { \
> +            AMF_AV_QUERY_INTERFACE(res, val, AMFInterface, amf_interface)\
> +            if (res == AMF_OK) { \
> +                res = AMFVariantAssignInterface(&var, amf_interface); \
> +                amf_interface->pVtbl->Release(amf_interface); \
> +            } \
> +            if (res == AMF_OK) { \
> +                res = pThis->pVtbl->SetProperty(pThis, name, var); \
> +            } \
> +            AMFVariantClear(&var); \
> +        }\
> +    }
> +
> +#define AMF_AV_GET_PROPERTY_INTERFACE(res, pThis, name, TargetType, val) \
> +    { \
> +        AMFVariantStruct var; \
> +        res = AMFVariantInit(&var); \
> +        if (res == AMF_OK) { \
> +            res = pThis->pVtbl->GetProperty(pThis, name, &var); \
> +            if (res == AMF_OK) { \
> +                if (var.type == AMF_VARIANT_INTERFACE && AMFVariantInterface(&var)) { \
> +                    AMF_AV_QUERY_INTERFACE(res, AMFVariantInterface(&var), TargetType, val); \
> +                } else { \
> +                    res = AMF_INVALID_DATA_TYPE; \
> +                } \
> +            } \
> +            AMFVariantClear(&var); \
> +        }\
> +    }
> +
> +static AMFBuffer* amf_create_buffer_with_frame_ref(const AVFrame* frame, AMFContext *context)
> +{
> +    AVFrame *frame_ref;
> +    AMFBuffer *frame_ref_storage_buffer = NULL;
> +    AMF_RESULT res;
> +
> +    res = context->pVtbl->AllocBuffer(context, AMF_MEMORY_HOST, sizeof(frame_ref), &frame_ref_storage_buffer);
> +    if (res == AMF_OK) {
> +        frame_ref = av_frame_clone(frame);
> +        if (frame_ref) {
> +            memcpy(frame_ref_storage_buffer->pVtbl->GetNative(frame_ref_storage_buffer), &frame_ref, sizeof(frame_ref));
> +        } else {
> +            frame_ref_storage_buffer->pVtbl->Release(frame_ref_storage_buffer);
> +            frame_ref_storage_buffer = NULL;
> +        }
> +    }
> +    return frame_ref_storage_buffer;
> +}
> +
> +static void amf_release_buffer_with_frame_ref(AMFBuffer *frame_ref_storage_buffer)
> +{
> +    AVFrame *av_frame_ref;
> +    memcpy(&av_frame_ref, frame_ref_storage_buffer->pVtbl->GetNative(frame_ref_storage_buffer), sizeof(av_frame_ref));
> +    av_frame_free(&av_frame_ref);
> +    frame_ref_storage_buffer->pVtbl->Release(frame_ref_storage_buffer);
> +} 
>  
>  int ff_amf_send_frame(AVCodecContext *avctx, const AVFrame *frame)
>  {
> @@ -484,6 +551,7 @@ int ff_amf_send_frame(AVCodecContext *avctx, const AVFrame *frame)
>              (ctx->hw_device_ctx && ((AVHWFramesContext*)frame->hw_frames_ctx->data)->device_ctx ==
>              (AVHWDeviceContext*)ctx->hw_device_ctx->data)
>          )) {
> +            AMFBuffer *frame_ref_storage_buffer;
>  #if CONFIG_D3D11VA
>              static const GUID AMFTextureArrayIndexGUID = { 0x28115527, 0xe7c3, 0x4b66, { 0x99, 0xd3, 0x4f, 0x2a, 0xe6, 0xb4, 0x7f,
> 0xaf } };
>              ID3D11Texture2D *texture = (ID3D11Texture2D*)frame->data[0]; // actual texture
> @@ -496,6 +564,16 @@ int ff_amf_send_frame(AVCodecContext *avctx, const AVFrame *frame)
>              // input HW surfaces can be vertically aligned by 16; tell AMF the real size
>              surface->pVtbl->SetCrop(surface, 0, 0, frame->width, frame->height);
>  #endif
> +
> +            frame_ref_storage_buffer = amf_create_buffer_with_frame_ref(frame, ctx->context);
> +            AMF_RETURN_IF_FALSE(ctx, frame_ref_storage_buffer != NULL, AVERROR(ENOMEM), "create_buffer_with_frame_ref() returned
> NULL\n");
> +
> +            AMF_AV_ASSIGN_PROPERTY_INTERFACE(res, surface, L"av_frame_ref", frame_ref_storage_buffer);
> +            if (res != AMF_OK) {
> +                surface->pVtbl->Release(surface);
> +            }
> +            frame_ref_storage_buffer->pVtbl->Release(frame_ref_storage_buffer);
> +            AMF_RETURN_IF_FALSE(ctx, res == AMF_OK, AVERROR(ENOMEM), "SetProperty failed for \"av_frame_ref\" with error %d\n",
> res);
>          } else {
>              res = ctx->context->pVtbl->AllocSurface(ctx->context, AMF_MEMORY_HOST, ctx->format, avctx->width, avctx->height,
> &surface);
>              AMF_RETURN_IF_FALSE(ctx, res == AMF_OK, AVERROR(ENOMEM), "AllocSurface() failed  with error %d\n", res);
> @@ -560,6 +638,17 @@ int ff_amf_receive_packet(AVCodecContext *avctx, AVPacket *avpkt)
>              ret = amf_copy_buffer(avctx, avpkt, buffer);
>  
>              buffer->pVtbl->Release(buffer);
> +
> +            if (data->pVtbl->HasProperty(data, L"av_frame_ref")) {
> +                AMFBuffer *frame_ref_storage_buffer;
> +                AMF_AV_GET_PROPERTY_INTERFACE(res, data, L"av_frame_ref", AMFBuffer, frame_ref_storage_buffer);
> +                if (res == AMF_OK) {
> +                    amf_release_buffer_with_frame_ref(frame_ref_storage_buffer);
> +                } else {
> +                    av_log(avctx, AV_LOG_WARNING, "GetProperty failed for \"av_frame_ref\" with error %d\n", res);
> +                }
> +            }
> +
>              data->pVtbl->Release(data);
>  
>              AMF_RETURN_IF_FALSE(ctx, ret >= 0, ret, "amf_copy_buffer() failed with error %d\n", ret);
> 

This breaks the testcase described in
https://trac.ffmpeg.org/ticket/6990 which is basically the same as the
one you described in this patch.

I get the following spammed repeatedly:

[AVHWFramesContext @ 000000000502d340] Static surface pool size exceeded.
[mpeg2video @ 0000000002f8d400] get_buffer() failed
[mpeg2video @ 0000000002f8d400] thread_get_buffer() failed
[mpeg2video @ 0000000002f8d400] get_buffer() failed (-12 0000000000000000)
Error while decoding stream #0:1: Operation not permitted

> 
> This breaks the testcase described in
> https://trac.ffmpeg.org/ticket/6990 which is basically the same as the
> one you described in this patch.
> 
> I get the following spammed repeatedly:
> 
> [AVHWFramesContext @ 000000000502d340] Static surface pool size exceeded.
> [mpeg2video @ 0000000002f8d400] get_buffer() failed
> [mpeg2video @ 0000000002f8d400] thread_get_buffer() failed
> [mpeg2video @ 0000000002f8d400] get_buffer() failed (-12 0000000000000000)
> Error while decoding stream #0:1: Operation not permitted

Hi,
I have checked the test, it causes such error because dxva decoder allocates small pool size for input of AV_CODEC_ID_MPEG2VIDEO

Option  "-extra_hw_frames 16" solves this problem.

I have checked test using the following command line. Result video looks ok comparing to original video.
./ffmpeg -hwaccel d3d11va -hwaccel_output_format d3d11 -extra_hw_frames 12 -i matrixbench_mpeg2.mpg -an -c:v h264_amf out.mkv

I have found the logic in dxva.c which sets initial pool size
    /* add surfaces based on number of possible refs */
    if (avctx->codec_id == AV_CODEC_ID_H264 || avctx->codec_id == AV_CODEC_ID_HEVC)
        num_surfaces += 16;
    else if (avctx->codec_id == AV_CODEC_ID_VP9)
        num_surfaces += 8;
    else
        num_surfaces += 2;
...
    frames_ctx->initial_pool_size = num_surfaces;

There is alternative way to solve this problem, such as using copy surface before submitting to encoder, but this scenario will spend additional resources.