[libav-devel] [RFC] Getting rid of the global log callback

Luca Barbato lu_zero at gentoo.org
Sun Dec 17 14:35:13 CET 2017


On 11/12/2017 14:48, wm4 wrote:
> The log callback, set with av_log_set_callback(), is global mutable
> state, and as such not something we want in Libav, at all. but getting
> rid of it is very complicated, because in most cases, av_log() does not
> have enough context available to find per-context log callbacks.
> 
> av_log() has a context pointer as first argument. This is just a void*.
> If it's non-NULL, then it's assumed to be a struct, with a AVClass*
> field as first member (e.g. AVCodecContext.av_class). This points to
> const memory, so it's not mutable state, and there's no way to put a
> private log callback there.
> 
> So this whole problem boils down to how to change AVClass to store a
> per-context log callback in structs using AVClass.
> 
> You can pass a NULL context to av_log() too. This would inherently
> require a global log callback, so we have to deprecate this usage
> entirely.
> 
> A log callback would include a pointer, and a user pointer:
> 
> struct AVLogCallback {
>    void *user_context;
>    // context is the caller (a struct with AVClass* as first field)
>    // user_context as the same value as the field above.
>    void (*callback)(void *context, void *user_context, int level,
>                     const char *fmt, va_list vl);
> }

user_context may need a user_free(),

> Personally, I'd prefer if we had mandatory state per struct. Let's call
> it AVObject:
> 
> struct AVObject {
>    const AVClass *av_class;
>    AVLogCallback log_callback:
> }
> 
> (Sorry for the bad taste in names, but this whole AVClass nonsense
> wasn't my idea anyway.)
> 
> We'd have a new AVClass field to signal presence of this:
> 
> struct AVClass {
>    const char* class_name;
>    ...
>    int is_new_class;
> }
> 
> The following function would work on my Libav context that works with
> av_log() (provided all conversions have been done):
> 
> int av_class_set_log_callback(void *ctx, AVLogCallback cb);
> 
> // Example usage:
> struct AVCodecContext {
>    AVObject object;
>    // ... other fields
> }
> 
> AVCodecContext *avctx = ...;
> av_class_set_log_callback(avctx, cb);
> 
> 
> This probably won't work, because:
> 
> 1. AVCodecContext.av_class probably can't go away
> 2. Would require an ABI bump too, which might be too late now, or not
>     good for incremental conversion
> 
> So instead of AVObject/is_new_class we'd just have another field
> offset, which I think is a bit clunky and possibly braindamaged, but
> it would work. We would have:

If it is fine to copy it around you can simply alloc+copy and store the 
pointer I guess.

I doubt we'll have to do that in a hotpath anyway.

> I'm arguing for such an extensible AVClassState, so we can stuff other
> things like AVClass.log_level_offset_offset in there. Possibly settings
> like CPU flags can go there too.

Might not hurt. As long it is something that you use at setup time it is 
fine to fit it there.

> I'm also against something like a AVLibraryState struct, which would
> imply to be globally shared across all contexts. I think it's better if
> the state gets "inherited" by copying all the parameters as contexts
> create sub-contexts. This is simpler and more flexible, and instead of
> having the user pass around such an AVLibraryState, the user would just
> configure the parameters with calls like av_class_set_log_callback() on
> contexts.

We can make that an implementation detail by passing the opaque to the 
_alloc() function. It is either copied over or ref-counted-shared.

If we copy the struct around we should add a user_copy() next to the 
user_free() callback thought.

> Anyway, once these mechanisms are in place, we'd deprecate the global
> log callback, and the possibility to pass NULL as first argument to
> av_log().
> 
> Thoughts? I'd like to implement this stuff once we agree on how to
> proceed.
Beside the remarks above I'm fine with this plan.

lu


More information about the libav-devel mailing list