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

(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.)

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

> AMFTraceWriterVtbl.Flush - I am not sure that it can be set as null
> pointer, I could double check with AMD developers.

It doesn't really matter; just the empty function didn't seem useful.

>>>  #include <AMF/components/VideoEncoderVCE.h>
>>>  #include <AMF/components/VideoEncoderHEVC.h>
>>
>> Kindof unrelated, but is there any reason why both of these are in the
>> header rather than in the per-codec files?
>>
>>
> Component management code is the same for all encoder components.
> The only encoder id defines is used for component creation here.

Yep, missed that.  Sure.

>>
>> The log_to_dbg option is orphaned by this change.  Is it worth keeping?
>> (If you want to keep it then maybe it could be a named option to
>> av_hwdevice_ctx_create() -> amf_device_create().)
>>
>> log_to_dbg is removed from here, because this setting is global for AMF
> library, not per component(encoder) or per AMFContext(hwcontext_amf object)
> 
> Probably I could implement some global AMF lib options which configures
> tracer more precisely in general

Comment above applies, I think.

- Mark