[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

Mark Thompson sw at jkqxz.net
Sun May 13 01:20:44 EEST 2018


On 11/05/18 19:37, Alexander Kravchenko wrote:
> 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?

Is there any later benefit to reusing the pointer, or is it just saving on the load/initialisation code?  If there isn't a strong reason to do so I would avoid storing it.  (Note that doing so would require more machinery to guard against data races as well.)

>>> +
>>> +#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?

Since this is ending up as immutable public API, we should be avoiding adding anything which isn't absolutely needed.  If they are both needed then that's fine - I don't think including the CreateComponent pointer instead would be any simpler.

> 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?

Maybe use the opts dict passed to av_hwdevice_ctx_create()?  (They're accessible in the ffmpeg utility via -init_hw_device.)

- Mark


More information about the ffmpeg-devel mailing list