[FFmpeg-devel] lavc/amfenc: Reference to input AVFrame (hwaccel) is retained during the encoding process

An additional reference to input AVFrame (hwaccel) is retained during the encoding process.
This postpone reusing frames by decoder while they are used by encoder and prevents frame corruption
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


From: Alexander Kravchenko <aakravchenko@luxoft.com>
---
 libavcodec/amfenc.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 61 insertions(+), 3 deletions(-)

--
2.16.2.windows.1

On 23/03/18 11:19, Kravchenko, Alexander wrote:
> Re: [FFmpeg-devel] [PATCH] lavc/amfenc: Reference to input AVFrame (hwaccel) is retained during the encoding process

Say what the change is in the title.  Something like "amfenc: Retain a reference to D3D11 frames used as input during the encoding process", maybe?


> An additional reference to input AVFrame (hwaccel) is retained during the encoding process.
> This postpone reusing frames by decoder while they are used by encoder and prevents frame corruption
> 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

How many frames can end up queued inside the encoder here?

> 
> From: Alexander Kravchenko <aakravchenko@luxoft.com>
> ---
>  libavcodec/amfenc.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 61 insertions(+), 3 deletions(-)
> 
> diff --git a/libavcodec/amfenc.c b/libavcodec/amfenc.c
> index 89a10ff253..9ffc52b0b0 100644
> --- a/libavcodec/amfenc.c
> +++ b/libavcodec/amfenc.c
> @@ -443,6 +443,41 @@ int ff_amf_encode_init(AVCodecContext *avctx)
>      return ret;
>  }
> 
> +#define AV_AMF_QUERY_INTERFACE(res, from, InterfaceType, to ) \
> +    { \
> +        AMFGuid guid_##InterfaceType = IID_##InterfaceType(); \
> +        res = from->pVtbl->QueryInterface(from, &guid_##InterfaceType, (void**)&to); \
> +    }
> +
> +#define AV_AMF_ASSIGN_PROPERTY_INTERFACE(res, pThis, name, val ) \
> +    { \
> +        AMFInterface *amf_interface; \
> +        AV_AMF_QUERY_INTERFACE(res, val, AMFInterface, amf_interface)\
> +        if(res == AMF_OK) { \

"if (", and in all places below too.

> +            AMFVariantStruct var; \
> +            AMFVariantInit(&var); \
> +            AMFVariantAssignInterface(&var, amf_interface); \
> +            amf_interface->pVtbl->Release(amf_interface); \
> +            res = pThis->pVtbl->SetProperty(pThis, name, var); \
> +            AMFVariantClear(&var); \

These AMFVariant functions all return an AMF_RESULT implying that they can fail (memory allocations?) - those should be checked.

> +        } \
> +    }
> +
> +#define AV_AMF_GET_PROPERTY_INTERFACE(res, pThis, name, target_type, val) \
> +    { \
> +        AMFVariantStruct var; \
> +        AMFVariantInit(&var); \
> +        res = pThis->pVtbl->GetProperty(pThis, name, &var); \
> +        if(res == AMF_OK) { \
> +            if(var.type == AMF_VARIANT_INTERFACE && var.pInterface) { \
> +                AMFGuid guid = IID_##target_type(); \
> +                res = var.pInterface->pVtbl->QueryInterface(var.pInterface, &guid, (void**)&val); \
> +            } else { \
> +                res = AMF_INVALID_DATA_TYPE; \
> +            } \
> +            AMFVariantClear(&var); \
> +        } \
> +    }

These should probably be functions rather than macros?  In any case, they shouldn't have the "AV_" prefix which is reserved for external symbols.

> 
>  int ff_amf_send_frame(AVCodecContext *avctx, const AVFrame *frame)
>  {
> @@ -484,6 +519,8 @@ 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)
>          )) {
> +            AVFrame* frame_ref = av_frame_clone(frame);

av_frame_clone() can fail.

> +            AMFBuffer* frame_ref_storage_buffer;

Put the * in the right place - it's part of the declarator, not the declaration-specifiers.

>  #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 +533,12 @@ 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
> +            res = ctx->context->pVtbl->AllocBuffer(ctx->context, AMF_MEMORY_HOST, sizeof(frame_ref), &frame_ref_storage_buffer);
> +            AMF_RETURN_IF_FALSE(ctx, res == AMF_OK, AVERROR(ENOMEM), "AllocBuffer() failed  with error %d\n", res);
> +            memcpy(frame_ref_storage_buffer->pVtbl->GetNative(frame_ref_storage_buffer), &frame_ref, sizeof(frame_ref));
> +
> +            AV_AMF_ASSIGN_PROPERTY_INTERFACE(res, surface, L"av_frame_ref", frame_ref_storage_buffer);
> +            frame_ref_storage_buffer->pVtbl->Release(frame_ref_storage_buffer);

frame_ref is leaked if any of this buffer/property stuff fails.

>          } 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);
> @@ -555,11 +598,26 @@ int ff_amf_receive_packet(AVCodecContext *avctx, AVPacket *avpkt)
>          if (data) {
>              // copy data to packet
>              AMFBuffer* buffer;
> -            AMFGuid guid = IID_AMFBuffer();
> -            data->pVtbl->QueryInterface(data, &guid, (void**)&buffer); // query for buffer interface
> +            AV_AMF_QUERY_INTERFACE(res, data, AMFBuffer, buffer);
> +            AMF_RETURN_IF_FALSE(ctx, res == AMF_OK, AVERROR_UNKNOWN, "Invalid data type from encoder->QueryOutput, should be AMFBuffer, error %d\n", res);
>              ret = amf_copy_buffer(avctx, avpkt, buffer);
> -
>              buffer->pVtbl->Release(buffer);
> +
> +            //try to get attached av_frame_ref and unref
> +            if(data->pVtbl->HasProperty(data, L"av_frame_ref")) {
> +                AMFBuffer *frame_ref_storage_buffer = NULL;
> +                AVFrame *av_frame_ref;
> +
> +                AV_AMF_GET_PROPERTY_INTERFACE(res, data, L"av_frame_ref", AMFBuffer, frame_ref_storage_buffer);
> +                if(res == AMF_OK) {
> +                    memcpy(&av_frame_ref, frame_ref_storage_buffer->pVtbl->GetNative(frame_ref_storage_buffer), sizeof(av_frame_ref));
> +                    av_frame_unref(av_frame_ref);

Leaks the AVFrame structure itself - you want av_frame_free().

> +                    frame_ref_storage_buffer->pVtbl->Release(frame_ref_storage_buffer);
> +                } else {
> +                    av_log(avctx, AV_LOG_WARNING, "av_frame_ref data attached to frame is corrupted\n");
> +                }
> +            }

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?

> +
>              data->pVtbl->Release(data);
> 
>              AMF_RETURN_IF_FALSE(ctx, ret >= 0, ret, "amf_copy_buffer() failed with error %d\n", ret);
> --
> 2.16.2.windows.1
> 
> 
> ________________________________
> 
> This e-mail and any attachment(s) are intended only for the recipient(s) named above and others who have been specifically authorized to receive them. They may contain confidential information. If you are not the intended recipient, please do not read this email or its attachment(s). Furthermore, you are hereby notified that any dissemination, distribution or copying of this e-mail and any attachment(s) is strictly prohibited. If you have received this e-mail in error, please immediately notify the sender by replying to this e-mail and then delete this e-mail and any attachment(s) or copies thereof from your system. Thank you.

Please do not tag emails sent to a public list with this sort of disclaimer.  If you are constrained by some kind of hostile coporate email system then it may be easiest to send from somewhere outside it with a suitable "From:" git tag inside the email.

Thanks,

- Mark