[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 and questions bellow

> -----Original Message-----
> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces at ffmpeg.org] On Behalf Of Mark Thompson
> Sent: Thursday, May 10, 2018 11:43 PM
> 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
> 

> > +static AVBufferRef *amf_library_ctx = NULL;
> 
> Global mutable state in libraries is strongly discouraged.  Just loading it again should be fine?
> 

Yes, loading it again could be fine.
I wanted to prevent additional call of library loading and trace initializations every time amf_device context is created.
User can create many instances of encoder in one process. 
Storing amf_library_ctx allows to detect when cleanup functions can be called (UnregisterWriter...). Of course we can never call dclose and keep library loaded all time.
What is your opinion?

> > +
> > +#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;
> 
> Do you actually need both of these?  It feels like you should be able to derive the creating factory (/ library instance) from the context.
> 

AMFContext does not have pointer to AMFFactory. Now AMFFactory -> CreateComponent is used in encoder initialization.
May be AMFFactory is too wide interface on this level. Pointer to function like CreateComponent can be published in AVAMFDeviceContext instead of factory pointer.
or did you mean something different?

--
I have another question about publish amf library level options (Trace level, trace writers...). This could be in another patch if we decide this option is possible.
What is the best way to add such option? Can command line options be used to configure library? Or somehow publish this API?

Thanks,
Alexander