Hi Mark, Thank you for your comments. Could you see my comments bellow > -----Original Message----- > From: ffmpeg-devel [mailto:ffmpeg-devel-boun...@ffmpeg.org] On Behalf Of Mark > Thompson > Sent: Sunday, May 13, 2018 1:41 AM > To: ffmpeg-devel@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 _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel