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
_______________________________________________
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel
