On 29/10/18 22:16, Alexander Kravchenko wrote: > Hi Mark, > see my comments bellow. > > вт, 30 окт. 2018 г. в 0:23, Mark Thompson <s...@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 _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel