[FFmpeg-devel] [RFC] eval API review

[Michael Niedermayer]

On Sun, Apr 04, 2010 at 05:03:03PM +0200, Stefano Sabatini wrote:
> On date Sunday 2010-04-04 14:50:27 +0200, Michael Niedermayer encoded:
> > On Fri, Apr 02, 2010 at 06:32:31PM +0200, Stefano Sabatini wrote:
> > > Hi all,
> > > 
> > > it has been mentioned many times that the eval API should be moved
> > > from lavc to lavu and make it public, where it could be used by
> > > non-lavc dependant applications and libraries (e.g. libswscale and
> > > libavfilter).
> > > 
> > > Michael also mentioned that he wanted to review the API before to make
> > > it public, follows an attempt at it.
> > 
> > a review of the existing API is in the form of
> > pieces of code of the existing api (that is .h file) interleaved with
> > comments about them like what you consider bad and why.
> > Iam completely puzzled about what you did or why
> > we have a existing api that is well tested and well working it just is
> > missing a review to make sure its future proof and fit for being made
> > public. You here post a changes api mixing cosmetic and functional
> > changes and its not even a patch nor do you explain why you made
> > these changes.
> > summary everything you did does not belong to a review
> > everything that does belong to a review is missing
> > a review implicates no changes
> 
> I started by working to some patches for the eval API, but then I
> concluded that was it better to just show how the whole API would
> appear once applied all the changes I'm going to propose.
> 
> Then once it is clear which is the final destination, providing each
> single patch should be simpler and almost mechanics (because the
> design and its rationale has been already discussed).
> 
> Follows my comments to the current API.
> 
> ...
> 
> General considerations.
> 

> I propose to change the interface of the various function, to make it
> always return an int expressing an error condition, rather than
> provide a string with the error.
> 
> This is both more consistent with the rest of the API, and imo easier
> to use. Error condition can be printed by av_log(), this also allows
> to specify in detail the error condition, while we currently return a
> const char* just describing the kind of error encountered (I mean "The
> variable "foo" is unknown." is generally more helpful than "Unknown
> variable.").

iam ok with using av_log() instead of returning a string.


> 
> Then I suggest to use a more intuitive naming for the current
> functions and in some cases to change the order of parameters.
> 
> ...
> 
> |/**
> | * Parses and evaluates an expression.
> | * Note, this is significantly slower than ff_parse_eval()
> | * @param s expression as a zero terminated string for example "1+2^3+5*5+sin(2/3)"
> | * @param func1 NULL terminated array of function pointers for functions which take 1 argument
> | * @param func2 NULL terminated array of function pointers for functions which take 2 arguments
> | * @param const_name NULL terminated array of zero terminated strings of constant identifers for example {"PI", "E", 0}
> | * @param func1_name NULL terminated array of zero terminated strings of func1 identifers
> | * @param func2_name NULL terminated array of zero terminated strings of func2 identifers
> | * @param error pointer to a char* which is set to an error message if something goes wrong
> | * @param const_value a zero terminated array of values for the identifers from const_name
> | * @param opaque a pointer which will be passed to all functions from func1 and func2
> | * @return the value of the expression
> | */
> |double ff_eval2(const char *s, const double *const_value, const char * const *const_name,
> |               double (**func1)(void *, double), const char **func1_name,
> |               double (**func2)(void *, double, double), const char **func2_name,
> |               void *opaque, const char **error);
> 
> I propose to change this to:
> int av_parse_eval_expr(double *res,
>                        const char *expr,
>                        const char * const *var_names  , const double *var_values,
>                        const char * const *func1_names, double (**func1)(void *, double),
>                        const char * const *func2_names, double (**func2)(void *, double, double),
>                        void *opaque);
> 
> In this variant the disposition of identifier names <->
> values/functions referred is thighter than in ff_eval2(), which should
> help usability.
> 

> |typedef struct ff_expr_s AVEvalExpr;

why not AVExpr ?

and if so all the names might need to be adapted to that


[...]
> |[...]
> |double av_strtod(const char *numstr, char **tail);
> 
> I propose to replace this with a function:
> int av_strtod2(double *res, const char *numstr, char **tail);
> 
> for consistency with the rest of the API.

libc strtod() has a widespread API, iam against us using a different one
that is unless the widespread one has a shortcomming.


> 
> At the end the API should consist of the following functions:
> av_parse_expr()      // parse an expression and create an AVEvalExpr
> av_eval_expr()       // evaluate an already parsed expression

> av_parse_eval_expr() // parse and evaluate at once an expression

if you want to use long names then this is missing an "and"

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

I have never wished to cater to the crowd; for what I know they do not
approve, and what they approve I do not know. -- Epicurus
-------------- 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/20100405/8c184cf3/attachment.pgp>