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

[Stefano Sabatini]

On date Sunday 2010-04-18 00:07:25 +0200, Michael Niedermayer encoded:
> On Mon, Apr 12, 2010 at 11:37:01PM +0200, Stefano Sabatini wrote:
> > Make them print error messages on the log using a log_ctx rather than
> > set a constant error string in the Parser.
> > 
> > Simplify the interface, and allow the error message to be more
> > expressive, as it is not anymore a generic const char * string.
> > ---
> >  libavcodec/eval.c        |   18 +++++++++---------
> >  libavcodec/eval.h        |    8 ++++----
> >  libavcodec/opt.c         |    6 ++----
> >  libavcodec/ratecontrol.c |    5 ++---
> >  4 files changed, 17 insertions(+), 20 deletions(-)
> > 
> > diff --git a/libavcodec/eval.c b/libavcodec/eval.c
> > index 0b2aed9..0149a55 100644
> > --- a/libavcodec/eval.c
> > +++ b/libavcodec/eval.c
> > @@ -39,7 +39,7 @@ typedef struct Parser{
> >      double (* const *func2)(void *, double a, double b); // NULL terminated
> >      const char * const *func2_name;          // NULL terminated
> >      void *opaque;
> > -    const char **error;
> > +    void *log_ctx;
> >  #define VARS 10
> >      double var[VARS];
> >  } Parser;
> > @@ -201,7 +201,7 @@ static AVExpr * parse_primary(Parser *p) {
> >  
> >      p->s= strchr(p->s, '(');
> >      if(p->s==NULL){
> > -        *p->error = "undefined constant or missing (";
> > +        av_log(p->log_ctx, AV_LOG_ERROR, "undefined constant or missing (\n");
> 
> this patch (not this change specifically) causes problems
> one no longer can evaluate an expression without spamming the user
> thats all fine if what is evaluated is a user supplied string, for other
> cases though it is not appropriate.
> If av_log() is used then we need a way to put a limit on the error level
> an error for evaluation may or may not be an error for the calling code.
> maybe we can just add an 8 entry array to AVClass and use that to
> remap the error levels. With that eval would in this case need to
> be run on a custom object with an AVClass that silences it.
> Thats a quite generic solution (that likely will come in handy
> for other code outside evel) but maybe its not the simplest solution
> 
> the next obvious alternative is to leave the returning of error messages
> as it is.

I see the problem but maybe the remap solution is overkill, for
example that would require a mapping and a reverse mapping in the
current code in lavc which uses it.

A possible solution would be to add a simple flag log_error to
ff_parse_expr()/ff_parse_and_eval_expr().

As for keeping the current interface I don't particularly like it as
it leads to too generic/unhelpful error messages.

Resuming:

1. Current solution:
AVExpr *ff_parse_expr(..., const char **error);

2. Use log_ctx and a log_error flag which enables logging:
AVExpr *ff_parse_expr(..., void *log_ctx, int enable_log);

3. Print to the error buffer provided by the caller the error message if any:
AVExpr *ff_parse_expr(..., const char *error, size_t error_size);

I have a preference for 2.

Regards.
-- 
FFmpeg = Faithless Fanciful Mastering Puritan Evanescent Gigant