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

Hi, could you please review updated patch?

Fixes according on Mark's review:
* Macroses changed to functions
* error level of AMF_RETURN_IF_FALSE changed to fatal (all cases it returns are fatal according on fatal error level description)
* used AMF_RETURN_IF_FALSE for case if a frame reference has been completely lost (was just warning before)

Hopefully this patch is ok enough to be applied :).
FYI, near time I am going to send the following patches 
* cosmetic fixes
* hwcontext_amf (to be reused in encoder and color space converter)
* AMF colors space converter (input memory types: opencl, dx9, dx11, host; output memory types: opencl, dx9, dx11)


---
 libavcodec/amfenc.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
 libavcodec/amfenc.h |  5 ++-
 2 files changed, 97 insertions(+), 2 deletions(-)

On 09/04/18 17:48, Alexander Kravchenko wrote:
> Hi, could you please review updated patch?
> 
> Fixes according on Mark's review:
> * Macroses changed to functions
> * error level of AMF_RETURN_IF_FALSE changed to fatal (all cases it returns are fatal according on fatal error level description)
> * used AMF_RETURN_IF_FALSE for case if a frame reference has been completely lost (was just warning before)
> 
> Hopefully this patch is ok enough to be applied :).

Two minor points below, not enough to merit another cycle so I fixed them and pushed.

> FYI, near time I am going to send the following patches 
> * cosmetic fixes
> * hwcontext_amf (to be reused in encoder and color space converter)
> * AMF colors space converter (input memory types: opencl, dx9, dx11, host; output memory types: opencl, dx9, dx11)
> 
> 
> ---
   ^
(When adding commentary which isn't part of the commit message to an email please place it after this line so that it doesn't end up in the commit message.)

>  libavcodec/amfenc.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  libavcodec/amfenc.h |  5 ++-
>  2 files changed, 97 insertions(+), 2 deletions(-)
> 
> diff --git a/libavcodec/amfenc.c b/libavcodec/amfenc.c
> index 89a10ff253..ea2c5acbbf 100644
> --- a/libavcodec/amfenc.c
> +++ b/libavcodec/amfenc.c
> @@ -157,6 +157,9 @@ static int amf_init_context(AVCodecContext *avctx)
>      AmfContext         *ctx = avctx->priv_data;
>      AMF_RESULT          res = AMF_OK;
>  
> +    ctx->hwsurfaces_in_queue = 0;
> +    ctx->hwsurfaces_in_queue_max = 16;
> +
>      // configure AMF logger
>      // the return of these functions indicates old state and do not affect behaviour
>      ctx->trace->pVtbl->EnableWriter(ctx->trace, AMF_TRACE_WRITER_DEBUG_OUTPUT, ctx->log_to_dbg != 0 );
> @@ -187,6 +190,7 @@ static int amf_init_context(AVCodecContext *avctx)
>                          if (!ctx->hw_frames_ctx) {
>                              return AVERROR(ENOMEM);
>                          }
> +                        ctx->hwsurfaces_in_queue_max = device_ctx->initial_pool_size - 1;

initial_pool_size need not be set to a positive value.  It will be set for all internally-created frames contexts (since the setup code there is intended for use with the decoder), but it need not be for externally-created one which might not use array textures and therefore need not have fixed size.

Changed to only update the queue size if initial_pool_size is positive (i.e. use the 16 length if the pool isn't of fixed size).

>                      } else {
>                          if(res == AMF_NOT_SUPPORTED)
>                              av_log(avctx, AV_LOG_INFO, "avctx->hw_frames_ctx has D3D11 device which doesn't have D3D11VA interface, switching to default\n");
> @@ -443,6 +447,75 @@ int ff_amf_encode_init(AVCodecContext *avctx)
>      return ret;
>  }
>  
> +static AMF_RESULT amf_set_property_buffer(AMFSurface *object, const wchar_t *name, AMFBuffer *val)
> +{
> +    AMF_RESULT res;
> +    AMFVariantStruct var;
> +    res = AMFVariantInit(&var);
> +    if (res == AMF_OK) {
> +        AMFGuid guid_AMFInterface = IID_AMFInterface();
> +        AMFInterface *amf_interface;
> +        res = val->pVtbl->QueryInterface(val, &guid_AMFInterface, (void**)&amf_interface);
> +
> +        if (res == AMF_OK) {
> +            res = AMFVariantAssignInterface(&var, amf_interface);
> +            amf_interface->pVtbl->Release(amf_interface);
> +        }
> +        if (res == AMF_OK) {
> +            res = object->pVtbl->SetProperty(object, name, var);
> +        }
> +        AMFVariantClear(&var);
> +    }
> +    return res;
> +}
> +
> +static AMF_RESULT amf_get_property_buffer(AMFData *object, const wchar_t *name, AMFBuffer **val)
> +{
> +    AMF_RESULT res;
> +    AMFVariantStruct var;
> +    res = AMFVariantInit(&var);
> +    if (res == AMF_OK) {
> +        res = object->pVtbl->GetProperty(object, name, &var);
> +        if (res == AMF_OK) {
> +            if (var.type == AMF_VARIANT_INTERFACE) {
> +                AMFGuid guid_AMFBuffer = IID_AMFBuffer();
> +                AMFInterface *amf_interface = AMFVariantInterface(&var);
> +                res = amf_interface->pVtbl->QueryInterface(amf_interface, &guid_AMFBuffer, (void**)val);
> +            } else {
> +                res = AMF_INVALID_DATA_TYPE;
> +            }
> +        }
> +        AMFVariantClear(&var);
> +    }
> +    return res;
> +}
> +
> +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 +557,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)
>          )) {
> +            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 +571,14 @@ 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");
> +
> +            res = amf_set_property_buffer(surface, L"av_frame_ref", frame_ref_storage_buffer);
> +            AMF_RETURN_IF_FALSE(ctx, res == AMF_OK, AVERROR_UNKNOWN, "SetProperty failed for \"av_frame_ref\" with error %d\n", res);
> +            ctx->hwsurfaces_in_queue++;
> +            frame_ref_storage_buffer->pVtbl->Release(frame_ref_storage_buffer);
>          } 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 +643,15 @@ 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;
> +                res = amf_get_property_buffer(data, L"av_frame_ref", &frame_ref_storage_buffer);
> +                AMF_RETURN_IF_FALSE(ctx, res == AMF_OK, AVERROR_UNKNOWN, "GetProperty failed for \"av_frame_ref\" with error %d\n", res);
> +                amf_release_buffer_with_frame_ref(frame_ref_storage_buffer);
> +                ctx->hwsurfaces_in_queue--;
> +            }
> +
>              data->pVtbl->Release(data);
>  
>              AMF_RETURN_IF_FALSE(ctx, ret >= 0, ret, "amf_copy_buffer() failed with error %d\n", ret);
> @@ -589,7 +681,7 @@ int ff_amf_receive_packet(AVCodecContext *avctx, AVPacket *avpkt)
>                      av_log(avctx, AV_LOG_WARNING, "Data acquired but delayed drain submission got AMF_INPUT_FULL- should not happen\n");
>                  }
>              }
> -        } else if (ctx->delayed_surface != NULL || ctx->delayed_drain || (ctx->eof && res_query != AMF_EOF)) {
> +        } else if (ctx->delayed_surface != NULL || ctx->delayed_drain || (ctx->eof && res_query != AMF_EOF) || (ctx->hwsurfaces_in_queue >= ctx->hwsurfaces_in_queue_max)) {
>              block_and_wait = 1;
>              av_usleep(1000); // wait and poll again
>          }
> diff --git a/libavcodec/amfenc.h b/libavcodec/amfenc.h
> index 84f0aad2fa..0e4c6e4b89 100644
> --- a/libavcodec/amfenc.h
> +++ b/libavcodec/amfenc.h
> @@ -62,6 +62,9 @@ typedef struct AmfContext {
>      AVBufferRef        *hw_device_ctx; ///< pointer to HW accelerator (decoder)
>      AVBufferRef        *hw_frames_ctx; ///< pointer to HW accelerator (frame allocator)
>  
> +    int                 hwsurfaces_in_queue;
> +    int                 hwsurfaces_in_queue_max;
> +
>      // helpers to handle async calls
>      int                 delayed_drain;
>      AMFSurface         *delayed_surface;
> @@ -140,7 +143,7 @@ extern const enum AVPixelFormat ff_amf_pix_fmts[];
>  */
>  #define AMF_RETURN_IF_FALSE(avctx, exp, ret_value, /*message,*/ ...) \
>      if (!(exp)) { \
> -        av_log(avctx, AV_LOG_ERROR, __VA_ARGS__); \
> +        av_log(avctx, AV_LOG_FATAL, __VA_ARGS__); \

This isn't related, I removed it.

>          return ret_value; \
>      }
>  
> 

Thanks,

- Mark

> -----Original Message-----
> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf Of Mark Thompson
> Sent: Wednesday, April 11, 2018 2:13 AM
> To: ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH] lavc/amfenc: Retain a reference to D3D frames used as input during the encoding process
> 

> 
> Two minor points below, not enough to merit another cycle so I fixed them and pushed.
> 

Hi Mark,
Thanks for your help reviewing, fixing, and pushing patch

I have send one more patch with subject: [PATCH] lavc/amfenc: DXVA2 textures support implementation by AMF encoder
http://ffmpeg.org/pipermail/ffmpeg-devel/2018-April/228186.html

Could you please review it?

FYI, my next steps are:
* hwcontext_amf + apply using one in amfenc.
* vf_scale_amf
* code cleanup

Thanks,
Alexander