[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

[Alexander Kravchenko]

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

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




> >  #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.



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

Thanks,
Alexander