| Message ID | 20191230222340.5832-1-cus@passwd.hu |
|---|---|
| State | New |
| Headers | show |
On Mon, Dec 30, 2019 at 11:23:40PM +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@passwd.hu> > --- > doc/APIchanges | 4 ++++ > libavutil/eval.c | 36 +++++++++++++++++++++++++----------- > libavutil/eval.h | 41 +++++++++++++++++++++++++++++++++++++++++ > libavutil/version.h | 2 +- > 4 files changed, 71 insertions(+), 12 deletions(-) > > diff --git a/doc/APIchanges b/doc/APIchanges > index 3c24dc6fbc..d0b33bda02 100644 > --- a/doc/APIchanges > +++ b/doc/APIchanges > @@ -15,6 +15,10 @@ libavutil: 2017-10-21 > > API changes, most recent first: > > +2020-01-xx - xxxxxxxxxx - lavu 56.39.100 - eval.h > + Add AVExprState struct and av_expr_eval2, av_expr_state_alloc, > + av_expr_state_free functions > + > 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..4619d0fba0 100644 > --- a/libavutil/eval.c > +++ b/libavutil/eval.c > @@ -53,7 +53,6 @@ typedef struct Parser { > void *opaque; > int log_offset; > void *log_ctx; > -#define VARS 10 > double *var; > } Parser; > > @@ -173,7 +172,7 @@ struct AVExpr { > double (*func2)(void *, double, double); > } a; > struct AVExpr *param[3]; > - double *var; > + AVExprState *state; > }; > > static double etime(double v) > @@ -191,7 +190,7 @@ static double eval_expr(Parser *p, AVExpr *e) > case e_func2: return e->value * e->a.func2(p->opaque, eval_expr(p, e->param[0]), eval_expr(p, e->param[1])); > case e_squish: return 1/(1+exp(4*eval_expr(p, e->param[0]))); > case e_gauss: { double d = eval_expr(p, e->param[0]); return exp(-d*d/2)/sqrt(2*M_PI); } > - case e_ld: return e->value * p->var[av_clip(eval_expr(p, e->param[0]), 0, VARS-1)]; > + case e_ld: return e->value * p->var[av_clip(eval_expr(p, e->param[0]), 0, AV_EXPR_STATE_NB_VARS-1)]; > case e_isnan: return e->value * !!isnan(eval_expr(p, e->param[0])); > case e_isinf: return e->value * !!isinf(eval_expr(p, e->param[0])); > case e_floor: return e->value * floor(eval_expr(p, e->param[0])); > @@ -230,7 +229,7 @@ static double eval_expr(Parser *p, AVExpr *e) > return x; > } > case e_random:{ > - int idx= av_clip(eval_expr(p, e->param[0]), 0, VARS-1); > + int idx= av_clip(eval_expr(p, e->param[0]), 0, AV_EXPR_STATE_NB_VARS-1); > uint64_t r= isnan(p->var[idx]) ? 0 : p->var[idx]; > r= r*1664525+1013904223; > p->var[idx]= r; > @@ -245,7 +244,7 @@ static double eval_expr(Parser *p, AVExpr *e) > case e_taylor: { > double t = 1, d = 0, v; > double x = eval_expr(p, e->param[1]); > - int id = e->param[2] ? av_clip(eval_expr(p, e->param[2]), 0, VARS-1) : 0; > + int id = e->param[2] ? av_clip(eval_expr(p, e->param[2]), 0, AV_EXPR_STATE_NB_VARS-1) : 0; > int i; > double var0 = p->var[id]; > for(i=0; i<1000; i++) { > @@ -320,7 +319,7 @@ static double eval_expr(Parser *p, AVExpr *e) > case e_div: return e->value * ((!CONFIG_FTRAPV || d2 ) ? (d / d2) : d * INFINITY); > case e_add: return e->value * (d + d2); > case e_last:return e->value * d2; > - case e_st : return e->value * (p->var[av_clip(d, 0, VARS-1)]= d2); > + case e_st : return e->value * (p->var[av_clip(d, 0, AV_EXPR_STATE_NB_VARS-1)]= d2); > case e_hypot:return e->value * hypot(d, d2); > case e_atan2:return e->value * atan2(d, d2); > case e_bitand: return isnan(d) || isnan(d2) ? NAN : e->value * ((long int)d & (long int)d2); > @@ -333,13 +332,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 +733,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 +772,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..d5b34e54e8 100644 > --- a/libavutil/eval.h > +++ b/libavutil/eval.h > @@ -30,6 +30,21 @@ > > typedef struct AVExpr AVExpr; > > +/** > + * This structure stores a state used by the expression evaluator. > + * It must be allocated and freed with its proper allocation functions. > + * > + * @see av_expr_state_alloc() > + * @see av_expr_state_free() > + */ > +typedef struct AVExprState { > +#define AV_EXPR_STATE_NB_VARS 10 This would make the NB_VARS part of the public API and it would not be changeable easily, theres nothing here that says it cannot be used by the user [...]
On Wed, 1 Jan 2020, Michael Niedermayer wrote: > On Mon, Dec 30, 2019 at 11:23:40PM +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@passwd.hu> >> --- >> doc/APIchanges | 4 ++++ >> libavutil/eval.c | 36 +++++++++++++++++++++++++----------- >> libavutil/eval.h | 41 +++++++++++++++++++++++++++++++++++++++++ >> libavutil/version.h | 2 +- >> 4 files changed, 71 insertions(+), 12 deletions(-) >> >> diff --git a/doc/APIchanges b/doc/APIchanges >> index 3c24dc6fbc..d0b33bda02 100644 >> --- a/doc/APIchanges >> +++ b/doc/APIchanges >> @@ -15,6 +15,10 @@ libavutil: 2017-10-21 >> >> API changes, most recent first: >> >> +2020-01-xx - xxxxxxxxxx - lavu 56.39.100 - eval.h >> + Add AVExprState struct and av_expr_eval2, av_expr_state_alloc, >> + av_expr_state_free functions >> + >> 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..4619d0fba0 100644 >> --- a/libavutil/eval.c >> +++ b/libavutil/eval.c >> @@ -53,7 +53,6 @@ typedef struct Parser { >> void *opaque; >> int log_offset; >> void *log_ctx; >> -#define VARS 10 >> double *var; >> } Parser; >> >> @@ -173,7 +172,7 @@ struct AVExpr { >> double (*func2)(void *, double, double); >> } a; >> struct AVExpr *param[3]; >> - double *var; >> + AVExprState *state; >> }; >> >> static double etime(double v) >> @@ -191,7 +190,7 @@ static double eval_expr(Parser *p, AVExpr *e) >> case e_func2: return e->value * e->a.func2(p->opaque, eval_expr(p, e->param[0]), eval_expr(p, e->param[1])); >> case e_squish: return 1/(1+exp(4*eval_expr(p, e->param[0]))); >> case e_gauss: { double d = eval_expr(p, e->param[0]); return exp(-d*d/2)/sqrt(2*M_PI); } >> - case e_ld: return e->value * p->var[av_clip(eval_expr(p, e->param[0]), 0, VARS-1)]; >> + case e_ld: return e->value * p->var[av_clip(eval_expr(p, e->param[0]), 0, AV_EXPR_STATE_NB_VARS-1)]; >> case e_isnan: return e->value * !!isnan(eval_expr(p, e->param[0])); >> case e_isinf: return e->value * !!isinf(eval_expr(p, e->param[0])); >> case e_floor: return e->value * floor(eval_expr(p, e->param[0])); >> @@ -230,7 +229,7 @@ static double eval_expr(Parser *p, AVExpr *e) >> return x; >> } >> case e_random:{ >> - int idx= av_clip(eval_expr(p, e->param[0]), 0, VARS-1); >> + int idx= av_clip(eval_expr(p, e->param[0]), 0, AV_EXPR_STATE_NB_VARS-1); >> uint64_t r= isnan(p->var[idx]) ? 0 : p->var[idx]; >> r= r*1664525+1013904223; >> p->var[idx]= r; >> @@ -245,7 +244,7 @@ static double eval_expr(Parser *p, AVExpr *e) >> case e_taylor: { >> double t = 1, d = 0, v; >> double x = eval_expr(p, e->param[1]); >> - int id = e->param[2] ? av_clip(eval_expr(p, e->param[2]), 0, VARS-1) : 0; >> + int id = e->param[2] ? av_clip(eval_expr(p, e->param[2]), 0, AV_EXPR_STATE_NB_VARS-1) : 0; >> int i; >> double var0 = p->var[id]; >> for(i=0; i<1000; i++) { >> @@ -320,7 +319,7 @@ static double eval_expr(Parser *p, AVExpr *e) >> case e_div: return e->value * ((!CONFIG_FTRAPV || d2 ) ? (d / d2) : d * INFINITY); >> case e_add: return e->value * (d + d2); >> case e_last:return e->value * d2; >> - case e_st : return e->value * (p->var[av_clip(d, 0, VARS-1)]= d2); >> + case e_st : return e->value * (p->var[av_clip(d, 0, AV_EXPR_STATE_NB_VARS-1)]= d2); >> case e_hypot:return e->value * hypot(d, d2); >> case e_atan2:return e->value * atan2(d, d2); >> case e_bitand: return isnan(d) || isnan(d2) ? NAN : e->value * ((long int)d & (long int)d2); >> @@ -333,13 +332,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 +733,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 +772,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..d5b34e54e8 100644 >> --- a/libavutil/eval.h >> +++ b/libavutil/eval.h >> @@ -30,6 +30,21 @@ >> >> typedef struct AVExpr AVExpr; >> >> +/** >> + * This structure stores a state used by the expression evaluator. >> + * It must be allocated and freed with its proper allocation functions. >> + * >> + * @see av_expr_state_alloc() >> + * @see av_expr_state_free() >> + */ >> +typedef struct AVExprState { > >> +#define AV_EXPR_STATE_NB_VARS 10 > > This would make the NB_VARS part of the public API and it would > not be changeable easily, theres nothing here that says it cannot be used > by the user I assumed it is OK as long as you only increase this. If you are adding other fields, then you are of course locked to 10 vars. What do you propose instead? Thanks, Marton
On Wed, 1 Jan 2020, Marton Balint wrote: > > > On Wed, 1 Jan 2020, Michael Niedermayer wrote: > >> On Mon, Dec 30, 2019 at 11:23:40PM +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@passwd.hu> >>> --- >>> doc/APIchanges | 4 ++++ >>> libavutil/eval.c | 36 +++++++++++++++++++++++++----------- >>> libavutil/eval.h | 41 +++++++++++++++++++++++++++++++++++++++++ >>> libavutil/version.h | 2 +- >>> 4 files changed, 71 insertions(+), 12 deletions(-) >>> >>> diff --git a/doc/APIchanges b/doc/APIchanges >>> index 3c24dc6fbc..d0b33bda02 100644 >>> --- a/doc/APIchanges >>> +++ b/doc/APIchanges >>> @@ -15,6 +15,10 @@ libavutil: 2017-10-21 >>> >>> API changes, most recent first: >>> >>> +2020-01-xx - xxxxxxxxxx - lavu 56.39.100 - eval.h >>> + Add AVExprState struct and av_expr_eval2, av_expr_state_alloc, >>> + av_expr_state_free functions >>> + >>> 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..4619d0fba0 100644 >>> --- a/libavutil/eval.c >>> +++ b/libavutil/eval.c >>> @@ -53,7 +53,6 @@ typedef struct Parser { >>> void *opaque; >>> int log_offset; >>> void *log_ctx; >>> -#define VARS 10 >>> double *var; >>> } Parser; >>> >>> @@ -173,7 +172,7 @@ struct AVExpr { >>> double (*func2)(void *, double, double); >>> } a; >>> struct AVExpr *param[3]; >>> - double *var; >>> + AVExprState *state; >>> }; >>> >>> static double etime(double v) >>> @@ -191,7 +190,7 @@ static double eval_expr(Parser *p, AVExpr *e) >>> case e_func2: return e->value * e->a.func2(p->opaque, > eval_expr(p, e->param[0]), eval_expr(p, e->param[1])); >>> case e_squish: return 1/(1+exp(4*eval_expr(p, e->param[0]))); >>> case e_gauss: { double d = eval_expr(p, e->param[0]); return > exp(-d*d/2)/sqrt(2*M_PI); } >>> - case e_ld: return e->value * p->var[av_clip(eval_expr(p, > e->param[0]), 0, VARS-1)]; >>> + case e_ld: return e->value * p->var[av_clip(eval_expr(p, > e->param[0]), 0, AV_EXPR_STATE_NB_VARS-1)]; >>> case e_isnan: return e->value * !!isnan(eval_expr(p, > e->param[0])); >>> case e_isinf: return e->value * !!isinf(eval_expr(p, > e->param[0])); >>> case e_floor: return e->value * floor(eval_expr(p, > e->param[0])); >>> @@ -230,7 +229,7 @@ static double eval_expr(Parser *p, AVExpr *e) >>> return x; >>> } >>> case e_random:{ >>> - int idx= av_clip(eval_expr(p, e->param[0]), 0, VARS-1); >>> + int idx= av_clip(eval_expr(p, e->param[0]), 0, > AV_EXPR_STATE_NB_VARS-1); >>> uint64_t r= isnan(p->var[idx]) ? 0 : p->var[idx]; >>> r= r*1664525+1013904223; >>> p->var[idx]= r; >>> @@ -245,7 +244,7 @@ static double eval_expr(Parser *p, AVExpr *e) >>> case e_taylor: { >>> double t = 1, d = 0, v; >>> double x = eval_expr(p, e->param[1]); >>> - int id = e->param[2] ? av_clip(eval_expr(p, e->param[2]), 0, > VARS-1) : 0; >>> + int id = e->param[2] ? av_clip(eval_expr(p, e->param[2]), 0, > AV_EXPR_STATE_NB_VARS-1) : 0; >>> int i; >>> double var0 = p->var[id]; >>> for(i=0; i<1000; i++) { >>> @@ -320,7 +319,7 @@ static double eval_expr(Parser *p, AVExpr *e) >>> case e_div: return e->value * ((!CONFIG_FTRAPV || d2 ) ? > (d / d2) : d * INFINITY); >>> case e_add: return e->value * (d + d2); >>> case e_last:return e->value * d2; >>> - case e_st : return e->value * (p->var[av_clip(d, 0, > VARS-1)]= d2); >>> + case e_st : return e->value * (p->var[av_clip(d, 0, > AV_EXPR_STATE_NB_VARS-1)]= d2); >>> case e_hypot:return e->value * hypot(d, d2); >>> case e_atan2:return e->value * atan2(d, d2); >>> case e_bitand: return isnan(d) || isnan(d2) ? NAN : > e->value * ((long int)d & (long int)d2); >>> @@ -333,13 +332,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 +733,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 +772,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..d5b34e54e8 100644 >>> --- a/libavutil/eval.h >>> +++ b/libavutil/eval.h >>> @@ -30,6 +30,21 @@ >>> >>> typedef struct AVExpr AVExpr; >>> >>> +/** >>> + * This structure stores a state used by the expression evaluator. >>> + * It must be allocated and freed with its proper allocation functions. >>> + * >>> + * @see av_expr_state_alloc() >>> + * @see av_expr_state_free() >>> + */ >>> +typedef struct AVExprState { >> >>> +#define AV_EXPR_STATE_NB_VARS 10 >> >> This would make the NB_VARS part of the public API and it would >> not be changeable easily, theres nothing here that says it cannot be used >> by the user > > I assumed it is OK as long as you only increase this. If you are adding > other fields, then you are of course locked to 10 vars. What do you > propose instead? Ping for this. Thanks, Marton
On Wed, Jan 01, 2020 at 05:40:33PM +0100, Marton Balint wrote: > > > On Wed, 1 Jan 2020, Michael Niedermayer wrote: > > >On Mon, Dec 30, 2019 at 11:23:40PM +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@passwd.hu> > >>--- > >> doc/APIchanges | 4 ++++ > >> libavutil/eval.c | 36 +++++++++++++++++++++++++----------- > >> libavutil/eval.h | 41 +++++++++++++++++++++++++++++++++++++++++ > >> libavutil/version.h | 2 +- > >> 4 files changed, 71 insertions(+), 12 deletions(-) > >> > >>diff --git a/doc/APIchanges b/doc/APIchanges > >>index 3c24dc6fbc..d0b33bda02 100644 > >>--- a/doc/APIchanges > >>+++ b/doc/APIchanges > >>@@ -15,6 +15,10 @@ libavutil: 2017-10-21 > >> > >> API changes, most recent first: > >> > >>+2020-01-xx - xxxxxxxxxx - lavu 56.39.100 - eval.h > >>+ Add AVExprState struct and av_expr_eval2, av_expr_state_alloc, > >>+ av_expr_state_free functions > >>+ > >> 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..4619d0fba0 100644 > >>--- a/libavutil/eval.c > >>+++ b/libavutil/eval.c > >>@@ -53,7 +53,6 @@ typedef struct Parser { > >> void *opaque; > >> int log_offset; > >> void *log_ctx; > >>-#define VARS 10 > >> double *var; > >> } Parser; > >> > >>@@ -173,7 +172,7 @@ struct AVExpr { > >> double (*func2)(void *, double, double); > >> } a; > >> struct AVExpr *param[3]; > >>- double *var; > >>+ AVExprState *state; > >> }; > >> > >> static double etime(double v) > >>@@ -191,7 +190,7 @@ static double eval_expr(Parser *p, AVExpr *e) > >> case e_func2: return e->value * e->a.func2(p->opaque, eval_expr(p, e->param[0]), eval_expr(p, e->param[1])); > >> case e_squish: return 1/(1+exp(4*eval_expr(p, e->param[0]))); > >> case e_gauss: { double d = eval_expr(p, e->param[0]); return exp(-d*d/2)/sqrt(2*M_PI); } > >>- case e_ld: return e->value * p->var[av_clip(eval_expr(p, e->param[0]), 0, VARS-1)]; > >>+ case e_ld: return e->value * p->var[av_clip(eval_expr(p, e->param[0]), 0, AV_EXPR_STATE_NB_VARS-1)]; > >> case e_isnan: return e->value * !!isnan(eval_expr(p, e->param[0])); > >> case e_isinf: return e->value * !!isinf(eval_expr(p, e->param[0])); > >> case e_floor: return e->value * floor(eval_expr(p, e->param[0])); > >>@@ -230,7 +229,7 @@ static double eval_expr(Parser *p, AVExpr *e) > >> return x; > >> } > >> case e_random:{ > >>- int idx= av_clip(eval_expr(p, e->param[0]), 0, VARS-1); > >>+ int idx= av_clip(eval_expr(p, e->param[0]), 0, AV_EXPR_STATE_NB_VARS-1); > >> uint64_t r= isnan(p->var[idx]) ? 0 : p->var[idx]; > >> r= r*1664525+1013904223; > >> p->var[idx]= r; > >>@@ -245,7 +244,7 @@ static double eval_expr(Parser *p, AVExpr *e) > >> case e_taylor: { > >> double t = 1, d = 0, v; > >> double x = eval_expr(p, e->param[1]); > >>- int id = e->param[2] ? av_clip(eval_expr(p, e->param[2]), 0, VARS-1) : 0; > >>+ int id = e->param[2] ? av_clip(eval_expr(p, e->param[2]), 0, AV_EXPR_STATE_NB_VARS-1) : 0; > >> int i; > >> double var0 = p->var[id]; > >> for(i=0; i<1000; i++) { > >>@@ -320,7 +319,7 @@ static double eval_expr(Parser *p, AVExpr *e) > >> case e_div: return e->value * ((!CONFIG_FTRAPV || d2 ) ? (d / d2) : d * INFINITY); > >> case e_add: return e->value * (d + d2); > >> case e_last:return e->value * d2; > >>- case e_st : return e->value * (p->var[av_clip(d, 0, VARS-1)]= d2); > >>+ case e_st : return e->value * (p->var[av_clip(d, 0, AV_EXPR_STATE_NB_VARS-1)]= d2); > >> case e_hypot:return e->value * hypot(d, d2); > >> case e_atan2:return e->value * atan2(d, d2); > >> case e_bitand: return isnan(d) || isnan(d2) ? NAN : e->value * ((long int)d & (long int)d2); > >>@@ -333,13 +332,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 +733,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 +772,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..d5b34e54e8 100644 > >>--- a/libavutil/eval.h > >>+++ b/libavutil/eval.h > >>@@ -30,6 +30,21 @@ > >> > >> typedef struct AVExpr AVExpr; > >> > >>+/** > >>+ * This structure stores a state used by the expression evaluator. > >>+ * It must be allocated and freed with its proper allocation functions. > >>+ * > >>+ * @see av_expr_state_alloc() > >>+ * @see av_expr_state_free() > >>+ */ > >>+typedef struct AVExprState { > > > >>+#define AV_EXPR_STATE_NB_VARS 10 > > > >This would make the NB_VARS part of the public API and it would > >not be changeable easily, theres nothing here that says it cannot be used > >by the user > > I assumed it is OK as long as you only increase this. If you are adding > other fields, then you are of course locked to 10 vars. What do you propose > instead? Well, AV_EXPR_STATE_NB_VARS should not be a value in a public header which does not contain a comment saying that it must not be used outside libavutil also the question is, can this struct be made opaque for the user ? if so we can change it in any way in the future, including making it non opaque if needed Thanks [...]
On Thu, 9 Jan 2020, Michael Niedermayer wrote: > On Wed, Jan 01, 2020 at 05:40:33PM +0100, Marton Balint wrote: >> >> >> On Wed, 1 Jan 2020, Michael Niedermayer wrote: >> >>> On Mon, Dec 30, 2019 at 11:23:40PM +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@passwd.hu> >>>> --- >>>> doc/APIchanges | 4 ++++ >>>> libavutil/eval.c | 36 +++++++++++++++++++++++++----------- >>>> libavutil/eval.h | 41 +++++++++++++++++++++++++++++++++++++++++ >>>> libavutil/version.h | 2 +- >>>> 4 files changed, 71 insertions(+), 12 deletions(-) >>>> >>>> diff --git a/doc/APIchanges b/doc/APIchanges >>>> index 3c24dc6fbc..d0b33bda02 100644 >>>> --- a/doc/APIchanges >>>> +++ b/doc/APIchanges >>>> @@ -15,6 +15,10 @@ libavutil: 2017-10-21 >>>> >>>> API changes, most recent first: >>>> >>>> +2020-01-xx - xxxxxxxxxx - lavu 56.39.100 - eval.h >>>> + Add AVExprState struct and av_expr_eval2, av_expr_state_alloc, >>>> + av_expr_state_free functions >>>> + >>>> 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..4619d0fba0 100644 >>>> --- a/libavutil/eval.c >>>> +++ b/libavutil/eval.c >>>> @@ -53,7 +53,6 @@ typedef struct Parser { >>>> void *opaque; >>>> int log_offset; >>>> void *log_ctx; >>>> -#define VARS 10 >>>> double *var; >>>> } Parser; >>>> >>>> @@ -173,7 +172,7 @@ struct AVExpr { >>>> double (*func2)(void *, double, double); >>>> } a; >>>> struct AVExpr *param[3]; >>>> - double *var; >>>> + AVExprState *state; >>>> }; >>>> >>>> static double etime(double v) >>>> @@ -191,7 +190,7 @@ static double eval_expr(Parser *p, AVExpr *e) >>>> case e_func2: return e->value * e->a.func2(p->opaque, eval_expr(p, e->param[0]), eval_expr(p, e->param[1])); >>>> case e_squish: return 1/(1+exp(4*eval_expr(p, e->param[0]))); >>>> case e_gauss: { double d = eval_expr(p, e->param[0]); return exp(-d*d/2)/sqrt(2*M_PI); } >>>> - case e_ld: return e->value * p->var[av_clip(eval_expr(p, e->param[0]), 0, VARS-1)]; >>>> + case e_ld: return e->value * p->var[av_clip(eval_expr(p, e->param[0]), 0, AV_EXPR_STATE_NB_VARS-1)]; >>>> case e_isnan: return e->value * !!isnan(eval_expr(p, e->param[0])); >>>> case e_isinf: return e->value * !!isinf(eval_expr(p, e->param[0])); >>>> case e_floor: return e->value * floor(eval_expr(p, e->param[0])); >>>> @@ -230,7 +229,7 @@ static double eval_expr(Parser *p, AVExpr *e) >>>> return x; >>>> } >>>> case e_random:{ >>>> - int idx= av_clip(eval_expr(p, e->param[0]), 0, VARS-1); >>>> + int idx= av_clip(eval_expr(p, e->param[0]), 0, AV_EXPR_STATE_NB_VARS-1); >>>> uint64_t r= isnan(p->var[idx]) ? 0 : p->var[idx]; >>>> r= r*1664525+1013904223; >>>> p->var[idx]= r; >>>> @@ -245,7 +244,7 @@ static double eval_expr(Parser *p, AVExpr *e) >>>> case e_taylor: { >>>> double t = 1, d = 0, v; >>>> double x = eval_expr(p, e->param[1]); >>>> - int id = e->param[2] ? av_clip(eval_expr(p, e->param[2]), 0, VARS-1) : 0; >>>> + int id = e->param[2] ? av_clip(eval_expr(p, e->param[2]), 0, AV_EXPR_STATE_NB_VARS-1) : 0; >>>> int i; >>>> double var0 = p->var[id]; >>>> for(i=0; i<1000; i++) { >>>> @@ -320,7 +319,7 @@ static double eval_expr(Parser *p, AVExpr *e) >>>> case e_div: return e->value * ((!CONFIG_FTRAPV || d2 ) ? (d / d2) : d * INFINITY); >>>> case e_add: return e->value * (d + d2); >>>> case e_last:return e->value * d2; >>>> - case e_st : return e->value * (p->var[av_clip(d, 0, VARS-1)]= d2); >>>> + case e_st : return e->value * (p->var[av_clip(d, 0, AV_EXPR_STATE_NB_VARS-1)]= d2); >>>> case e_hypot:return e->value * hypot(d, d2); >>>> case e_atan2:return e->value * atan2(d, d2); >>>> case e_bitand: return isnan(d) || isnan(d2) ? NAN : e->value * ((long int)d & (long int)d2); >>>> @@ -333,13 +332,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 +733,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 +772,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..d5b34e54e8 100644 >>>> --- a/libavutil/eval.h >>>> +++ b/libavutil/eval.h >>>> @@ -30,6 +30,21 @@ >>>> >>>> typedef struct AVExpr AVExpr; >>>> >>>> +/** >>>> + * This structure stores a state used by the expression evaluator. >>>> + * It must be allocated and freed with its proper allocation functions. >>>> + * >>>> + * @see av_expr_state_alloc() >>>> + * @see av_expr_state_free() >>>> + */ >>>> +typedef struct AVExprState { >>> >>>> +#define AV_EXPR_STATE_NB_VARS 10 >>> >>> This would make the NB_VARS part of the public API and it would >>> not be changeable easily, theres nothing here that says it cannot be used >>> by the user >> >> I assumed it is OK as long as you only increase this. If you are adding >> other fields, then you are of course locked to 10 vars. What do you propose >> instead? > > Well, AV_EXPR_STATE_NB_VARS should not be a value in a public header > which does not contain a comment saying that it must not be used outside > libavutil My intention was that it _is_ public, because it lets the user know the length (or rather the minimum length) of the variable array. The user could just as easily use FF_ARRAY_ELEMS(AVExprState->vars). > > also the question is, can this struct be made opaque for the user ? > if so we can change it in any way in the future, including making it > non opaque if needed Well, in v1 the struct was opaque but you wanted to be able to initialize the state and I preferred a non-opaque struct to do that instead of a parameter list in _alloc(). I can revert to v1, but I still think using _alloc() is not the way to initialize state. How do you want me to proceed? Thanks, Marton
Quoting Marton Balint (2019-12-30 23:23:40) > 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. Why not just parse the expression multiple times? Performance? Beyond that, it seems to me that the user now has to juggle multiple contexts manually, which is complex and can be avoided. How about introducing a function for duplicating AVExpr instead? Would that not solve this as well?
On Fri, 10 Jan 2020, Anton Khirnov wrote: > Quoting Marton Balint (2019-12-30 23:23:40) >> 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. > > Why not just parse the expression multiple times? Performance? For fixing the vf_geq issue in the second patch, yes parsing the expression multiple times should work, parsing MAX_THREADS * 4 expressions is totally OK. However API-wise, it is cleaner to separate state from the expression, I find it totally unintuitive that an expression has a state or that evaluating an expression is not thread safe. > Beyond that, it seems to me that the user now has to juggle multiple > contexts manually, which is complex and can be avoided. It is only a possibility, most users can use av_eval_expr as before and use the internal state of the expression. > How about introducing a function for duplicating AVExpr instead? Would > that not solve this as well? I don't really like this idea, you want to duplicate the state, not the expression. Regards, Marton
On Fri, Jan 10, 2020 at 12:49:08AM +0100, Marton Balint wrote: > > > On Thu, 9 Jan 2020, Michael Niedermayer wrote: > > >On Wed, Jan 01, 2020 at 05:40:33PM +0100, Marton Balint wrote: > >> > >> > >>On Wed, 1 Jan 2020, Michael Niedermayer wrote: > >> > >>>On Mon, Dec 30, 2019 at 11:23:40PM +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@passwd.hu> > >>>>--- > >>>>doc/APIchanges | 4 ++++ > >>>>libavutil/eval.c | 36 +++++++++++++++++++++++++----------- > >>>>libavutil/eval.h | 41 +++++++++++++++++++++++++++++++++++++++++ > >>>>libavutil/version.h | 2 +- > >>>>4 files changed, 71 insertions(+), 12 deletions(-) > >>>> > >>>>diff --git a/doc/APIchanges b/doc/APIchanges > >>>>index 3c24dc6fbc..d0b33bda02 100644 > >>>>--- a/doc/APIchanges > >>>>+++ b/doc/APIchanges > >>>>@@ -15,6 +15,10 @@ libavutil: 2017-10-21 > >>>> > >>>>API changes, most recent first: > >>>> > >>>>+2020-01-xx - xxxxxxxxxx - lavu 56.39.100 - eval.h > >>>>+ Add AVExprState struct and av_expr_eval2, av_expr_state_alloc, > >>>>+ av_expr_state_free functions > >>>>+ > >>>>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..4619d0fba0 100644 > >>>>--- a/libavutil/eval.c > >>>>+++ b/libavutil/eval.c > >>>>@@ -53,7 +53,6 @@ typedef struct Parser { > >>>> void *opaque; > >>>> int log_offset; > >>>> void *log_ctx; > >>>>-#define VARS 10 > >>>> double *var; > >>>>} Parser; > >>>> > >>>>@@ -173,7 +172,7 @@ struct AVExpr { > >>>> double (*func2)(void *, double, double); > >>>> } a; > >>>> struct AVExpr *param[3]; > >>>>- double *var; > >>>>+ AVExprState *state; > >>>>}; > >>>> > >>>>static double etime(double v) > >>>>@@ -191,7 +190,7 @@ static double eval_expr(Parser *p, AVExpr *e) > >>>> case e_func2: return e->value * e->a.func2(p->opaque, eval_expr(p, e->param[0]), eval_expr(p, e->param[1])); > >>>> case e_squish: return 1/(1+exp(4*eval_expr(p, e->param[0]))); > >>>> case e_gauss: { double d = eval_expr(p, e->param[0]); return exp(-d*d/2)/sqrt(2*M_PI); } > >>>>- case e_ld: return e->value * p->var[av_clip(eval_expr(p, e->param[0]), 0, VARS-1)]; > >>>>+ case e_ld: return e->value * p->var[av_clip(eval_expr(p, e->param[0]), 0, AV_EXPR_STATE_NB_VARS-1)]; > >>>> case e_isnan: return e->value * !!isnan(eval_expr(p, e->param[0])); > >>>> case e_isinf: return e->value * !!isinf(eval_expr(p, e->param[0])); > >>>> case e_floor: return e->value * floor(eval_expr(p, e->param[0])); > >>>>@@ -230,7 +229,7 @@ static double eval_expr(Parser *p, AVExpr *e) > >>>> return x; > >>>> } > >>>> case e_random:{ > >>>>- int idx= av_clip(eval_expr(p, e->param[0]), 0, VARS-1); > >>>>+ int idx= av_clip(eval_expr(p, e->param[0]), 0, AV_EXPR_STATE_NB_VARS-1); > >>>> uint64_t r= isnan(p->var[idx]) ? 0 : p->var[idx]; > >>>> r= r*1664525+1013904223; > >>>> p->var[idx]= r; > >>>>@@ -245,7 +244,7 @@ static double eval_expr(Parser *p, AVExpr *e) > >>>> case e_taylor: { > >>>> double t = 1, d = 0, v; > >>>> double x = eval_expr(p, e->param[1]); > >>>>- int id = e->param[2] ? av_clip(eval_expr(p, e->param[2]), 0, VARS-1) : 0; > >>>>+ int id = e->param[2] ? av_clip(eval_expr(p, e->param[2]), 0, AV_EXPR_STATE_NB_VARS-1) : 0; > >>>> int i; > >>>> double var0 = p->var[id]; > >>>> for(i=0; i<1000; i++) { > >>>>@@ -320,7 +319,7 @@ static double eval_expr(Parser *p, AVExpr *e) > >>>> case e_div: return e->value * ((!CONFIG_FTRAPV || d2 ) ? (d / d2) : d * INFINITY); > >>>> case e_add: return e->value * (d + d2); > >>>> case e_last:return e->value * d2; > >>>>- case e_st : return e->value * (p->var[av_clip(d, 0, VARS-1)]= d2); > >>>>+ case e_st : return e->value * (p->var[av_clip(d, 0, AV_EXPR_STATE_NB_VARS-1)]= d2); > >>>> case e_hypot:return e->value * hypot(d, d2); > >>>> case e_atan2:return e->value * atan2(d, d2); > >>>> case e_bitand: return isnan(d) || isnan(d2) ? NAN : e->value * ((long int)d & (long int)d2); > >>>>@@ -333,13 +332,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 +733,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 +772,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..d5b34e54e8 100644 > >>>>--- a/libavutil/eval.h > >>>>+++ b/libavutil/eval.h > >>>>@@ -30,6 +30,21 @@ > >>>> > >>>>typedef struct AVExpr AVExpr; > >>>> > >>>>+/** > >>>>+ * This structure stores a state used by the expression evaluator. > >>>>+ * It must be allocated and freed with its proper allocation functions. > >>>>+ * > >>>>+ * @see av_expr_state_alloc() > >>>>+ * @see av_expr_state_free() > >>>>+ */ > >>>>+typedef struct AVExprState { > >>> > >>>>+#define AV_EXPR_STATE_NB_VARS 10 > >>> > >>>This would make the NB_VARS part of the public API and it would > >>>not be changeable easily, theres nothing here that says it cannot be used > >>>by the user > >> > >>I assumed it is OK as long as you only increase this. If you are adding > >>other fields, then you are of course locked to 10 vars. What do you propose > >>instead? > > > >Well, AV_EXPR_STATE_NB_VARS should not be a value in a public header > >which does not contain a comment saying that it must not be used outside > >libavutil > > My intention was that it _is_ public, because it lets the user know the > length (or rather the minimum length) of the variable array. The user could > just as easily use FF_ARRAY_ELEMS(AVExprState->vars). > > > > >also the question is, can this struct be made opaque for the user ? > >if so we can change it in any way in the future, including making it > >non opaque if needed > > Well, in v1 the struct was opaque but you wanted to be able to initialize > the state and I preferred a non-opaque struct to do that instead of a > parameter list in _alloc(). > > I can revert to v1, but I still think using _alloc() is not the way to > initialize state. How do you want me to proceed? Well First question is what is the state? * always an array of doubles ? then id say let the user decide the length for each instance of the state * something arbitrary extendible (int, byte, stacks, fifos, string, filehandles, sockets, arrays, UI elements, ...) that needs something better designed, an opaque struct and alloc/free/set/get function could be a minimlistic solution [...] Thanks
On Sat, 11 Jan 2020, Michael Niedermayer wrote: > On Fri, Jan 10, 2020 at 12:49:08AM +0100, Marton Balint wrote: >> >> >> On Thu, 9 Jan 2020, Michael Niedermayer wrote: >> >>> On Wed, Jan 01, 2020 at 05:40:33PM +0100, Marton Balint wrote: >>>> >>>> >>>> On Wed, 1 Jan 2020, Michael Niedermayer wrote: >>>> >>>>> On Mon, Dec 30, 2019 at 11:23:40PM +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@passwd.hu> >>>>>> --- >>>>>> doc/APIchanges | 4 ++++ >>>>>> libavutil/eval.c | 36 +++++++++++++++++++++++++----------- >>>>>> libavutil/eval.h | 41 +++++++++++++++++++++++++++++++++++++++++ >>>>>> libavutil/version.h | 2 +- >>>>>> 4 files changed, 71 insertions(+), 12 deletions(-) >>>>>> >>>>>> diff --git a/doc/APIchanges b/doc/APIchanges >>>>>> index 3c24dc6fbc..d0b33bda02 100644 >>>>>> --- a/doc/APIchanges >>>>>> +++ b/doc/APIchanges >>>>>> @@ -15,6 +15,10 @@ libavutil: 2017-10-21 >>>>>> >>>>>> API changes, most recent first: >>>>>> >>>>>> +2020-01-xx - xxxxxxxxxx - lavu 56.39.100 - eval.h >>>>>> + Add AVExprState struct and av_expr_eval2, av_expr_state_alloc, >>>>>> + av_expr_state_free functions >>>>>> + >>>>>> 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..4619d0fba0 100644 >>>>>> --- a/libavutil/eval.c >>>>>> +++ b/libavutil/eval.c >>>>>> @@ -53,7 +53,6 @@ typedef struct Parser { >>>>>> void *opaque; >>>>>> int log_offset; >>>>>> void *log_ctx; >>>>>> -#define VARS 10 >>>>>> double *var; >>>>>> } Parser; >>>>>> >>>>>> @@ -173,7 +172,7 @@ struct AVExpr { >>>>>> double (*func2)(void *, double, double); >>>>>> } a; >>>>>> struct AVExpr *param[3]; >>>>>> - double *var; >>>>>> + AVExprState *state; >>>>>> }; >>>>>> >>>>>> static double etime(double v) >>>>>> @@ -191,7 +190,7 @@ static double eval_expr(Parser *p, AVExpr *e) >>>>>> case e_func2: return e->value * e->a.func2(p->opaque, eval_expr(p, e->param[0]), eval_expr(p, e->param[1])); >>>>>> case e_squish: return 1/(1+exp(4*eval_expr(p, e->param[0]))); >>>>>> case e_gauss: { double d = eval_expr(p, e->param[0]); return exp(-d*d/2)/sqrt(2*M_PI); } >>>>>> - case e_ld: return e->value * p->var[av_clip(eval_expr(p, e->param[0]), 0, VARS-1)]; >>>>>> + case e_ld: return e->value * p->var[av_clip(eval_expr(p, e->param[0]), 0, AV_EXPR_STATE_NB_VARS-1)]; >>>>>> case e_isnan: return e->value * !!isnan(eval_expr(p, e->param[0])); >>>>>> case e_isinf: return e->value * !!isinf(eval_expr(p, e->param[0])); >>>>>> case e_floor: return e->value * floor(eval_expr(p, e->param[0])); >>>>>> @@ -230,7 +229,7 @@ static double eval_expr(Parser *p, AVExpr *e) >>>>>> return x; >>>>>> } >>>>>> case e_random:{ >>>>>> - int idx= av_clip(eval_expr(p, e->param[0]), 0, VARS-1); >>>>>> + int idx= av_clip(eval_expr(p, e->param[0]), 0, AV_EXPR_STATE_NB_VARS-1); >>>>>> uint64_t r= isnan(p->var[idx]) ? 0 : p->var[idx]; >>>>>> r= r*1664525+1013904223; >>>>>> p->var[idx]= r; >>>>>> @@ -245,7 +244,7 @@ static double eval_expr(Parser *p, AVExpr *e) >>>>>> case e_taylor: { >>>>>> double t = 1, d = 0, v; >>>>>> double x = eval_expr(p, e->param[1]); >>>>>> - int id = e->param[2] ? av_clip(eval_expr(p, e->param[2]), 0, VARS-1) : 0; >>>>>> + int id = e->param[2] ? av_clip(eval_expr(p, e->param[2]), 0, AV_EXPR_STATE_NB_VARS-1) : 0; >>>>>> int i; >>>>>> double var0 = p->var[id]; >>>>>> for(i=0; i<1000; i++) { >>>>>> @@ -320,7 +319,7 @@ static double eval_expr(Parser *p, AVExpr *e) >>>>>> case e_div: return e->value * ((!CONFIG_FTRAPV || d2 ) ? (d / d2) : d * INFINITY); >>>>>> case e_add: return e->value * (d + d2); >>>>>> case e_last:return e->value * d2; >>>>>> - case e_st : return e->value * (p->var[av_clip(d, 0, VARS-1)]= d2); >>>>>> + case e_st : return e->value * (p->var[av_clip(d, 0, AV_EXPR_STATE_NB_VARS-1)]= d2); >>>>>> case e_hypot:return e->value * hypot(d, d2); >>>>>> case e_atan2:return e->value * atan2(d, d2); >>>>>> case e_bitand: return isnan(d) || isnan(d2) ? NAN : e->value * ((long int)d & (long int)d2); >>>>>> @@ -333,13 +332,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 +733,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 +772,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..d5b34e54e8 100644 >>>>>> --- a/libavutil/eval.h >>>>>> +++ b/libavutil/eval.h >>>>>> @@ -30,6 +30,21 @@ >>>>>> >>>>>> typedef struct AVExpr AVExpr; >>>>>> >>>>>> +/** >>>>>> + * This structure stores a state used by the expression evaluator. >>>>>> + * It must be allocated and freed with its proper allocation functions. >>>>>> + * >>>>>> + * @see av_expr_state_alloc() >>>>>> + * @see av_expr_state_free() >>>>>> + */ >>>>>> +typedef struct AVExprState { >>>>> >>>>>> +#define AV_EXPR_STATE_NB_VARS 10 >>>>> >>>>> This would make the NB_VARS part of the public API and it would >>>>> not be changeable easily, theres nothing here that says it cannot be used >>>>> by the user >>>> >>>> I assumed it is OK as long as you only increase this. If you are adding >>>> other fields, then you are of course locked to 10 vars. What do you propose >>>> instead? >>> >>> Well, AV_EXPR_STATE_NB_VARS should not be a value in a public header >>> which does not contain a comment saying that it must not be used outside >>> libavutil >> >> My intention was that it _is_ public, because it lets the user know the >> length (or rather the minimum length) of the variable array. The user could >> just as easily use FF_ARRAY_ELEMS(AVExprState->vars). >> >>> >>> also the question is, can this struct be made opaque for the user ? >>> if so we can change it in any way in the future, including making it >>> non opaque if needed >> >> Well, in v1 the struct was opaque but you wanted to be able to initialize >> the state and I preferred a non-opaque struct to do that instead of a >> parameter list in _alloc(). >> >> I can revert to v1, but I still think using _alloc() is not the way to >> initialize state. How do you want me to proceed? > > Well > First question is what is the state? > * always an array of doubles ? > then id say let the user decide the length for each instance of the state > > * something arbitrary extendible (int, byte, stacks, fifos, string, filehandles, sockets, arrays, UI elements, ...) > that needs something better designed, an opaque struct and alloc/free/set/get > function could be a minimlistic solution To me the second option seems more future proof, but I have no specific use case in mind. Regards, Marton
Quoting Marton Balint (2020-01-10 18:42:03) > > > On Fri, 10 Jan 2020, Anton Khirnov wrote: > > > Quoting Marton Balint (2019-12-30 23:23:40) > >> 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. > > > > Why not just parse the expression multiple times? Performance? > > For fixing the vf_geq issue in the second patch, yes parsing the > expression multiple times should work, parsing MAX_THREADS * 4 expressions > is totally OK. > > However API-wise, it is cleaner to separate state from the expression, I > find it totally unintuitive that an expression has a state or that > evaluating an expression is not thread safe. I agree that it is unintuitive and that it would be good to address that. My concern is that you are adding new ways to use the API, but do not remove or deprecate the previous unintuitive ones. > > > Beyond that, it seems to me that the user now has to juggle multiple > > contexts manually, which is complex and can be avoided. > > It is only a possibility, most users can use av_eval_expr as before and > use the internal state of the expression. And get burned by the exact same issue you are fixing here. Our APIs generally have a bad reputation for being obscure and hard to use. I think we should put more effort into making it hard to use them incorrectly. > > > How about introducing a function for duplicating AVExpr instead? Would > > that not solve this as well? > > I don't really like this idea, you want to duplicate the state, > not the expression. The question is: what is (or should be) AVExpr? Is it an immutable parsed expression? Then why should it be visible to the API users at all? the only meaningful operation on it is instantiating the expression evaluation state. We could also just rename AVExpr to AVExprState - or maybe AVExpr(Eval)Context to be consistent with other APIs (keeping the old name as a deprecated alias until next bump) to make it clear that it has internal state.
diff --git a/doc/APIchanges b/doc/APIchanges index 3c24dc6fbc..d0b33bda02 100644 --- a/doc/APIchanges +++ b/doc/APIchanges @@ -15,6 +15,10 @@ libavutil: 2017-10-21 API changes, most recent first: +2020-01-xx - xxxxxxxxxx - lavu 56.39.100 - eval.h + Add AVExprState struct and av_expr_eval2, av_expr_state_alloc, + av_expr_state_free functions + 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..4619d0fba0 100644 --- a/libavutil/eval.c +++ b/libavutil/eval.c @@ -53,7 +53,6 @@ typedef struct Parser { void *opaque; int log_offset; void *log_ctx; -#define VARS 10 double *var; } Parser; @@ -173,7 +172,7 @@ struct AVExpr { double (*func2)(void *, double, double); } a; struct AVExpr *param[3]; - double *var; + AVExprState *state; }; static double etime(double v) @@ -191,7 +190,7 @@ static double eval_expr(Parser *p, AVExpr *e) case e_func2: return e->value * e->a.func2(p->opaque, eval_expr(p, e->param[0]), eval_expr(p, e->param[1])); case e_squish: return 1/(1+exp(4*eval_expr(p, e->param[0]))); case e_gauss: { double d = eval_expr(p, e->param[0]); return exp(-d*d/2)/sqrt(2*M_PI); } - case e_ld: return e->value * p->var[av_clip(eval_expr(p, e->param[0]), 0, VARS-1)]; + case e_ld: return e->value * p->var[av_clip(eval_expr(p, e->param[0]), 0, AV_EXPR_STATE_NB_VARS-1)]; case e_isnan: return e->value * !!isnan(eval_expr(p, e->param[0])); case e_isinf: return e->value * !!isinf(eval_expr(p, e->param[0])); case e_floor: return e->value * floor(eval_expr(p, e->param[0])); @@ -230,7 +229,7 @@ static double eval_expr(Parser *p, AVExpr *e) return x; } case e_random:{ - int idx= av_clip(eval_expr(p, e->param[0]), 0, VARS-1); + int idx= av_clip(eval_expr(p, e->param[0]), 0, AV_EXPR_STATE_NB_VARS-1); uint64_t r= isnan(p->var[idx]) ? 0 : p->var[idx]; r= r*1664525+1013904223; p->var[idx]= r; @@ -245,7 +244,7 @@ static double eval_expr(Parser *p, AVExpr *e) case e_taylor: { double t = 1, d = 0, v; double x = eval_expr(p, e->param[1]); - int id = e->param[2] ? av_clip(eval_expr(p, e->param[2]), 0, VARS-1) : 0; + int id = e->param[2] ? av_clip(eval_expr(p, e->param[2]), 0, AV_EXPR_STATE_NB_VARS-1) : 0; int i; double var0 = p->var[id]; for(i=0; i<1000; i++) { @@ -320,7 +319,7 @@ static double eval_expr(Parser *p, AVExpr *e) case e_div: return e->value * ((!CONFIG_FTRAPV || d2 ) ? (d / d2) : d * INFINITY); case e_add: return e->value * (d + d2); case e_last:return e->value * d2; - case e_st : return e->value * (p->var[av_clip(d, 0, VARS-1)]= d2); + case e_st : return e->value * (p->var[av_clip(d, 0, AV_EXPR_STATE_NB_VARS-1)]= d2); case e_hypot:return e->value * hypot(d, d2); case e_atan2:return e->value * atan2(d, d2); case e_bitand: return isnan(d) || isnan(d2) ? NAN : e->value * ((long int)d & (long int)d2); @@ -333,13 +332,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 +733,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 +772,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..d5b34e54e8 100644 --- a/libavutil/eval.h +++ b/libavutil/eval.h @@ -30,6 +30,21 @@ typedef struct AVExpr AVExpr; +/** + * This structure stores a state used by the expression evaluator. + * It must be allocated and freed with its proper allocation functions. + * + * @see av_expr_state_alloc() + * @see av_expr_state_free() + */ +typedef struct AVExprState { +#define AV_EXPR_STATE_NB_VARS 10 + /* + * Internal variables used by stateful functions like lt(), st() and rand(). + */ + double vars[AV_EXPR_STATE_NB_VARS]; +} AVExprState; + /** * Parse and evaluate an expression. * Note, this is significantly slower than av_expr_eval(). @@ -86,6 +101,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 +165,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); + +/** + * Free an allocated AVExprState struct and set its pointer to NULL + */ +void av_expr_state_free(AVExprState **ps); + #endif /* AVUTIL_EVAL_H */ diff --git a/libavutil/version.h b/libavutil/version.h index af8f614aff..2bc1b98615 100644 --- a/libavutil/version.h +++ b/libavutil/version.h @@ -79,7 +79,7 @@ */ #define LIBAVUTIL_VERSION_MAJOR 56 -#define LIBAVUTIL_VERSION_MINOR 38 +#define LIBAVUTIL_VERSION_MINOR 39 #define LIBAVUTIL_VERSION_MICRO 100 #define LIBAVUTIL_VERSION_INT AV_VERSION_INT(LIBAVUTIL_VERSION_MAJOR, \
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@passwd.hu> --- doc/APIchanges | 4 ++++ libavutil/eval.c | 36 +++++++++++++++++++++++++----------- libavutil/eval.h | 41 +++++++++++++++++++++++++++++++++++++++++ libavutil/version.h | 2 +- 4 files changed, 71 insertions(+), 12 deletions(-)