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

[Stefano Sabatini]

On date Monday 2010-04-19 01:01:34 +0200, Michael Niedermayer encoded:
> On Mon, Apr 19, 2010 at 12:52:58AM +0200, Stefano Sabatini wrote:
> > 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.
> 
> where do you see a need to reverse map?
> 
> 
> > 
> > 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.

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.

Regards.
-- 
FFmpeg = Fancy and Fancy Most Power Enlightening Geek