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

[Michael Niedermayer]

On Tue, Apr 20, 2010 at 01:18:01AM +0200, Stefano Sabatini wrote:
> 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.

right, sorry, i meant something like:
level += map[av_clip(level>>3,...)]


> 
> 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

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

No human being will ever know the Truth, for even if they happen to say it
by chance, they would not even known they had done so. -- Xenophanes
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100420/bfec8a25/attachment.pgp>