[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,
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).


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



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


> > 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
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>