[FFmpeg-devel] [RFC] eval API review

Stefano Sabatini stefano.sabatini-lala
Sun Apr 4 17:03:03 CEST 2010


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.").

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;
|
|/**
| * Parses a expression.
| * @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
| * @return AVEvalExpr which must be freed with ff_eval_free by the user when it is not needed anymore
| *         NULL if anything went wrong
| */
|AVEvalExpr * ff_parse(const char *s, const char * const *const_name,
|               double (**func1)(void *, double), const char **func1_name,
|               double (**func2)(void *, double, double), const char **func2_name,
|               const char **error);

I propose to change this to:
int av_parse_expr(AVEvalExpr **eval_expr,
                  const char *expr,
                  const char * const *var_names,
                  const char * const *func1_names, double (**func1)(void *, double),
                  const char * const *func2_names, double (**func2)(void *, double, double));

Same considerations for the order of parameters as above apply.

|/**
| * Evaluates a previously parsed expression.
| * @param const_value a zero terminated array of values for the identifers from ff_parse 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_parse_eval(AVEvalExpr * e, const double *const_value, void *opaque);

I propose to change this to:

int av_eval_expr(double *res, AVEvalExpr *expr, const double *var_values, void *opaque);

I found the current name "parse_eval" rather misleading, since the
function doesn't parse at all.


|void ff_eval_free(AVEvalExpr * e);

I propose to change this to:
av_free_eval_expr(AVEvalExpr * e);

|[...]
|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.

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
av_free_eval_expr()  // free an AVEvalExpr created by av_parse_expr()

Regards.
-- 
FFmpeg = Fabulous and Forgiving Minimal Philosofic EnGraver



More information about the ffmpeg-devel mailing list