[FFmpeg-devel] [PATCH 1/3] avutil/eval: separate AVExpr state to a new AVExprState struct

[Marton Balint]

On Sun, 29 Dec 2019, Michael Niedermayer wrote:

> On Sat, Dec 28, 2019 at 03:46:23PM +0100, Marton Balint wrote:
>> Also add helper functions to allocate and free such a struct, and make it
>> usable by providing a new av_eval_expr2 function for which you can specify a
>> custom AVExprState.
>>
>> Signed-off-by: Marton Balint <cus at passwd.hu>
>> ---
>>  doc/APIchanges      |  3 +++
>>  libavutil/eval.c    | 31 +++++++++++++++++++++++++------
>>  libavutil/eval.h    | 27 +++++++++++++++++++++++++++
>>  libavutil/version.h |  2 +-
>>  4 files changed, 56 insertions(+), 7 deletions(-)
>>
>> diff --git a/doc/APIchanges b/doc/APIchanges
>> index 3c24dc6fbc..e4ad364ed9 100644
>> --- a/doc/APIchanges
>> +++ b/doc/APIchanges
>> @@ -15,6 +15,9 @@ libavutil:     2017-10-21
>>
>>  API changes, most recent first:
>>
>> +2020-01-xx - xxxxxxxxxx - lavu 56.39.100 - eval.h
>> +  Add av_expr_eval2, av_expr_state_alloc, av_expr_state_free
>> +
>>  2019-12-27 - xxxxxxxxxx - lavu 56.38.100 - eval.h
>>    Add av_expr_count_func().
>>
>> diff --git a/libavutil/eval.c b/libavutil/eval.c
>> index d527f6a9d0..86fb634fd8 100644
>> --- a/libavutil/eval.c
>> +++ b/libavutil/eval.c
>> @@ -173,7 +173,11 @@ struct AVExpr {
>>          double (*func2)(void *, double, double);
>>      } a;
>>      struct AVExpr *param[3];
>> -    double *var;
>> +    AVExprState *state;
>> +};
>> +
>> +struct AVExprState {
>> +    double vars[VARS];
>>  };
>>
>>  static double etime(double v)
>> @@ -333,13 +337,23 @@ static double eval_expr(Parser *p, AVExpr *e)
>>
>>  static int parse_expr(AVExpr **e, Parser *p);
>>
>> +AVExprState *av_expr_state_alloc(void)
>> +{
>> +    return av_mallocz(sizeof(AVExprState));
>> +}
>> +
>> +void av_expr_state_free(AVExprState **ps)
>> +{
>> +    av_freep(ps);
>> +}
>> +
>>  void av_expr_free(AVExpr *e)
>>  {
>>      if (!e) return;
>>      av_expr_free(e->param[0]);
>>      av_expr_free(e->param[1]);
>>      av_expr_free(e->param[2]);
>> -    av_freep(&e->var);
>> +    av_expr_state_free(&e->state);
>>      av_freep(&e);
>>  }
>>
>> @@ -724,8 +738,8 @@ int av_expr_parse(AVExpr **expr, const char *s,
>>          ret = AVERROR(EINVAL);
>>          goto end;
>>      }
>> -    e->var= av_mallocz(sizeof(double) *VARS);
>> -    if (!e->var) {
>> +    e->state = av_expr_state_alloc();
>> +    if (!e->state) {
>>          ret = AVERROR(ENOMEM);
>>          goto end;
>>      }
>> @@ -763,16 +777,21 @@ int av_expr_count_func(AVExpr *e, unsigned *counter, int size, int arg)
>>      return expr_count(e, counter, size, ((int[]){e_const, e_func1, e_func2})[arg]);
>>  }
>>
>> -double av_expr_eval(AVExpr *e, const double *const_values, void *opaque)
>> +double av_expr_eval2(AVExpr *e, AVExprState *s, const double *const_values, void *opaque)
>>  {
>>      Parser p = { 0 };
>> -    p.var= e->var;
>> +    p.var = s ? s->vars : e->state->vars;
>>
>>      p.const_values = const_values;
>>      p.opaque     = opaque;
>>      return eval_expr(&p, e);
>>  }
>>
>> +double av_expr_eval(AVExpr *e, const double *const_values, void *opaque)
>> +{
>> +    return av_expr_eval2(e, NULL, const_values, opaque);
>> +}
>> +
>>  int av_expr_parse_and_eval(double *d, const char *s,
>>                             const char * const *const_names, const double *const_values,
>>                             const char * const *func1_names, double (* const *funcs1)(void *, double),
>> diff --git a/libavutil/eval.h b/libavutil/eval.h
>> index 068c62cdab..8bd1592f6c 100644
>> --- a/libavutil/eval.h
>> +++ b/libavutil/eval.h
>> @@ -29,6 +29,7 @@
>>  #include "avutil.h"
>>
>>  typedef struct AVExpr AVExpr;
>> +typedef struct AVExprState AVExprState;
>>
>>  /**
>>   * Parse and evaluate an expression.
>> @@ -86,6 +87,22 @@ int av_expr_parse(AVExpr **expr, const char *s,
>>   */
>>  double av_expr_eval(AVExpr *e, const double *const_values, void *opaque);
>>
>> +/**
>> + * Evaluate a previously parsed expression using a custom state.
>> + *
>> + * Some expressions can use stateful functions, like random(), st() and ld().
>> + * With this function you can provide your own state to the evaluator instead
>> + * of using the internal state of the AVExpr. This makes it possible to use the
>> + * same AVExpr in multiple threads each with their own AVExprState avoiding
>> + * unprotected concurrent access of the internal AVExpr state.
>> + *
>> + * @param s the state of the expression, if NULL, the internal state of AVExpr will be used
>> + * @param const_values a zero terminated array of values for the identifiers from av_expr_parse() const_names
>> + * @param opaque a pointer which will be passed to all functions from funcs1 and funcs2
>> + * @return the value of the expression
>> + */
>> +double av_expr_eval2(AVExpr *e, AVExprState *s, const double *const_values, void *opaque);
>> +
>>  /**
>>   * Track the presence of variables and their number of occurrences in a parsed expression
>>   *
>> @@ -134,4 +151,14 @@ void av_expr_free(AVExpr *e);
>>   */
>>  double av_strtod(const char *numstr, char **tail);
>>
>> +/**
>> + * Allocate a new AVExprState struct
>> + */
>> +AVExprState *av_expr_state_alloc(void);
>
> maybe this should have a double [] argument so the caller can set the
> initial variables (or pass NULL if not)

Maybe it is better to simply make the AVExprState struct public 
if setting/getting state manually is something we want to support. What do 
you think?

Thanks,
Marton