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

[Mark Thompson]

On 29/10/18 22:16, Alexander Kravchenko wrote:
> Hi Mark,
> see my comments bellow.
> 
> вт, 30 окт. 2018 г. в 0:23, Mark Thompson <sw at jkqxz.net>:
> 
>> On 29/10/18 11:45, Alexander Kravchenko wrote:
>>> Hi, Mark.
>>> Thanks for review.
>>> Could you please check the following comments/questions?
>>>
>>>> +
>>>>> +static const AVClass amflib_class = {
>>>>> +    .class_name = "amf",
>>>>> +    .item_name = av_default_item_name,
>>>>> +    .version = LIBAVUTIL_VERSION_INT,
>>>>> +};
>>>>
>>>> This class shouldn't be needed - the right class to use is the one in
>> the
>>>> AVHWDeviceContext, you should be able to pass it to the right place via
>>>> your AMFDeviceContextPrivate structure.
>>>>
>>>>> +
>>>>> +typedef struct AMFLibraryContext {
>>>>> +    const AVClass      *avclass;
>>>>> +} AMFLibraryContext;
>>>>> +
>>>>> +static AMFLibraryContext amflib_context =
>>>>> +{
>>>>> +    .avclass = &amflib_class,
>>>>> +};
>>>>
>>>> This structure is just a dummy for the class?  Use the
>> AVHWDeviceContext.
>>>>
>>>>> +
>>>>> +typedef struct AmfTraceWriter {
>>>>> +    const AMFTraceWriterVtbl    *vtbl;
>>>>> +    void                        *avcl;
>>>>> +} AmfTraceWriter;
>>>>> +
>>>>> +static void AMF_CDECL_CALL AMFTraceWriter_Write(AMFTraceWriter *pThis,
>>>>
>>>> It would be sensible to take the opportunity to fix the function name to
>>>> conform to ffmpeg style.
>>>>
>>>>> +    const wchar_t *scope, const wchar_t *message)
>>>>> +{
>>>>> +    AmfTraceWriter *tracer = (AmfTraceWriter*)pThis;
>>>>> +    av_log(tracer->avcl, AV_LOG_DEBUG, "%ls: %ls", scope, message);
>>>>> +}
>>>>> +
>>>>> +static void AMF_CDECL_CALL AMFTraceWriter_Flush(AMFTraceWriter *pThis)
>>>>> +{
>>>>> +}
>>>>> +
>>>>> +static const AMFTraceWriterVtbl tracer_vtbl =
>>>>> +{
>>>>> +    .Write = AMFTraceWriter_Write,
>>>>> +    .Flush = AMFTraceWriter_Flush,
>>>>
>>>> Is this function really required to exist, given that it doesn't do
>>>> anything?
>>>>
>>>>> +};
>>>>> +
>>>>> +static const AmfTraceWriter amf_trace_writer =
>>>>> +{
>>>>> +    .vtbl = &tracer_vtbl,
>>>>> +    .avcl = &amflib_context,
>>>>> +};
>>>>
>>>> This should probably be inside the AMFDeviceContextPrivate, so that it
>> can
>>>> point to the right context structure.
>>>>
>>>> This is the question.
>>> AMF Library has global Trace settings, not per AMFContext object.
>>
>> So that global state is inside the AMF library?
>>
>> How does that interact with the fact that you reload it and reconfigure it
>> every time it gets loaded - if one thread calls amf_device_create() while
>> another one is inside the encoder and generating log output (say), what
>> happens?
>>
> 
> One of my first proposed patch had singleton amf_library, which was
> refcounted. main goal of it was init library (set Trace options) when the
> first reference is requested and clean when it was finally released (last
> hwcontext_amf is destroyed).
> I was told that global state is not good (global amf_lib object keeper) and
> I removed it and now Tracer options are updated each time amf_device_create
> is called.
> 
> Now there is no global state in avutils/hwcontext_amf : all globals here
> are const static objects. The only global state in AMF Library itself
> (configuring tracer is thread-safe in AMF).

Oh well.  I guess this ends up being the least objectionable way of dealing with the API, so ok.

>> (I also note that it presumably means the current AMF encoder code will
>> crash if you have two encoders with nested lifetimes - the second encoder
>> will overwrite the global state, and once it finishes and gets freed the
>> global trace output from the first encoder will have an invalid class
>> pointer.)
>>
> 
> Yes, I aware the issue since I started to support amfenc and this is the
> first reason to move amf lib code to hwcontext_amf.

Right, makes sense.

>>> My intension was to create global AMF lib class and Tracer object which
>>> refers to it as class parameter in av_log call.
>>> It is required in scenario when multiple hwcontext_amf are created during
>>> application lifecycle.
>>> if this way is ok, should I add comments to code describes this? or is
>>> there another way to have global object to handle this?
>> Global state in libraries really isn't ok.  I think in response to that I
>> would force it to be completely off by default, maybe switch it on if the
>> user passes some special named option to amf_device_create()?
>>
> 
> Probably this is the option, but current implementation should not cause
> any issues except multiple work of tracer configuration.

Given the above, fair enough.

Thanks,

- Mark