[FFmpeg-devel] [PATCH 1/5] Change eval internal functions, ff_parse_expr() and ff_parse_and_eval_expr() interface.

[Stefano Sabatini]

On date Wednesday 2010-04-21 23:36:05 +0200, Michael Niedermayer encoded:
> On Wed, Apr 21, 2010 at 02:22:58AM +0200, Stefano Sabatini wrote:
> > On date Tuesday 2010-04-20 02:37:55 +0200, Michael Niedermayer encoded:
> > > On Tue, Apr 20, 2010 at 01:18:01AM +0200, Stefano Sabatini wrote:
> > [...]
> > > > > > A possible solution would be to add a simple flag log_error to
> > > > > > ff_parse_expr()/ff_parse_and_eval_expr().
> > > > > 
> > > > > iam against this, its unreasonable unflexible, even a offset to the
> > > > > log level would be better
> > > > 
> > > > Problem with the map is that we don't have just 8 values for log, it
> > > > is an int ranging from -8 to 48, that's why I can't grasp the 8 entry
> > > > array map idea, please elaborate.
> > > 
> > > right, sorry, i meant something like:
> > > level += map[av_clip(level>>3,...)]
> > 
> > If I understood it correctly this would mean to set an ad-hoc log
> > level for each log context, right?
> 
> it would allow to compensate the log level per context.
> without it or a equialent system av_log() can not be used in generic utility
> code ever. This also would mean all uses in libavutil would have to be
> removed
> the global log level, that is for example "print errors" breaks down once
> you go down into a function of some generic util, its error might not be
> an error for the main application at all or it might be it all depends on
> what the failure of such util means in each specific instance to the main
> application.

My idea is to implement something of the kind:

static inline int av_log_get_level_offset(void *log_ctx)
{
    int *log_level = log_ctx ? log_ctx+sizeof(AVClass *) : NULL;
    
    return log_level ? *log_level : 0;
}

static inline int av_log_set_level_offset(void *log_ctx) { ... };

But this would require to put ant int log_level_offset in the contexts
just after the class pointers:

typedef struct TestContext
{
    const AVClass *class;
    int log_level_offset;
    ...
} TestContext;

...

TestContext test_ctx;

test_ctx.class            = &test_class;
test_ctx.log_level_offset = 2;

void av_vlog(void *log_ctx, int level, const char *fmt, va_list vl)
{
    int *log_lvl = log_ctx ? (int *)(*log_ctx+sizeof(AVClass *)) : NULL;
    if (avcl)
        level += av_clip(log_lvl >> 3, AV_LOG_QUIET, AV_LOG_DEBUG);
    av_log_callback(log_ctx, level, fmt, vl);
}

This would require to add a field in all the contexts, thus breaking
backward compatility.

Maybe we could generalize and have something of the kind:
test_ctx.class        = &test_class;
test_ctx.priv_log_ctx = ...

with the priv_log_ctx member being a pointer to a struct containing
the *non const* data used by the log context. This may contain for
example the log level offset/map, and eventually a specific callback
for the context. This would allow for example to log different
contexts in different files / using different formats.

Let me know if you like the idea, or if you was thinking about
something else.

> > > > Other ideas:
> > > > // write the error message to the user-provided error buffer, allows
> > > > // ad-hoc context-sensitive messages
> > > > int av_parse_expr(..., const char *error, size_t error_size);
> > > > 
> > > > or maybe we could just provide more than one interface:
> > > > av_parse_expr (..., void *log_ctx);
> > > > av_parse_expr2(..., const char *error, size_t error_size);
> > > > 
> > > > Please tell me what you consider acceptable.
> > > 
> > > i dont like multiple interfaces, things are complex enough already
> > 
> > So what about the other solution:
> > int av_parse_expr(..., const char *error, size_t error_size);
> > ?
> 
> this would not allow debug and warnings to be printed
> 
> av_parse_expr(..., int log_level_offset)
> would still be the clearly better choice

I'll go for this solution then.

Regards.
-- 
FFmpeg = Fantastic and Fascinating Mean Puritan Efficient Gadget