[FFmpeg-devel] [PATCH] lavc/amfenc: moving amf common code (library and context) to lavu/hwcontext_amf from amfenc to be reused in other amf components

[Alexander Kravchenko]

Hi Mark, 
Thank you for your comments.
Could you see my comments bellow

> -----Original Message-----
> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces at ffmpeg.org] On Behalf Of Mark Thompson
> Sent: Sunday, May 13, 2018 1:41 AM
> To: ffmpeg-devel at ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH] lavc/amfenc: moving amf common code (library and context) to lavu/hwcontext_amf from
> amfenc to be reused in other amf components
> 
> On 12/05/18 09:48, Alexander Kravchenko wrote:
> > This patch moves AMF common parts from amfenc to hwcontext_amf.
> > Now av_hwdevice_ctx API is used for AMF context creation/destroying.
> > This patch does not change component behaviour.
> > it contains only restructurization for further patches with new amf
> > components
> >
> > ---
> > Sending updated patch based on Mark's review
> > 1) simplificated library loading/unloading logic
> > 2) minor fixes
> >
> >
> >  libavcodec/amfenc.c            | 247 +++++-----------------------------------
> >  libavcodec/amfenc.h            |  27 +----
> >  libavutil/Makefile             |   2 +
> >  libavutil/hwcontext.c          |   4 +
> >  libavutil/hwcontext.h          |   1 +
> >  libavutil/hwcontext_amf.c      | 252 +++++++++++++++++++++++++++++++++++++++++
> >  libavutil/hwcontext_amf.h      |  43 +++++++
> >  libavutil/hwcontext_internal.h |   1 +
> >  8 files changed, 338 insertions(+), 239 deletions(-)  create mode
> > 100644 libavutil/hwcontext_amf.c  create mode 100644
> > libavutil/hwcontext_amf.h
> >
> > -
> > -static int amf_load_library(AVCodecContext *avctx)
> > +static int amf_init_context(AVCodecContext *avctx)
> >  {
> > -    AmfContext        *ctx = avctx->priv_data;
> > -    AMFInit_Fn         init_fun;
> > -    AMFQueryVersion_Fn version_fun;
> > -    AMF_RESULT         res;
> > +    AmfContext *ctx = avctx->priv_data;
> > +    AVAMFDeviceContext *amf_ctx;
> > +    int ret;
> >
> >      ctx->delayed_frame = av_frame_alloc();
> >      if (!ctx->delayed_frame) {
> >          return AVERROR(ENOMEM);
> >      }
> > +
> 
> Stray change?
> 

amf_load_library was moved to hwcontext; code which is unrelated to library moved to amf_init_context (delayed_frame object allocation)


> > +
> > +static void amf_device_uninit(AVHWDeviceContext *ctx) {
> > +    AVAMFDeviceContext      *amf_ctx = ctx->hwctx;
> > +    AMFDeviceContextPrivate *priv = ctx->internal->priv;
> > +    if (amf_ctx->context) {
> > +        amf_ctx->context->pVtbl->Terminate(amf_ctx->context);
> > +        amf_ctx->context->pVtbl->Release(amf_ctx->context);
> > +        amf_ctx->context = NULL;
> > +    }
> > +    if(priv->library) {
> > +        dlclose(priv->library);
> > +    }
> 
> This stuff shouldn't be in the uninit function, since this runs on all devices rather than just those created internally.  You want to make
> a function to set as AVHWDeviceContext.free.
> 
OK, will be fixed in coming patch


> > +#include "frame.h"
> > +#include "AMF/core/Context.h"
> > +#include "AMF/core/Factory.h"
> > +
> > +
> > +/**
> > + * This struct is allocated as AVHWDeviceContext.hwctx  */ typedef
> > +struct AVAMFDeviceContext {
> > +    AMFContext *context;
> > +    AMFFactory *factory;
> 
> Might be nice to have a bit more documentation than that.
> 

I will add comments to header in coming patch


> > +} AVAMFDeviceContext;
> > +
> > +

Thanks,
Alexander