[FFmpeg-devel] [PATCHv2 1/3] avutil/eval: separate AVExpr state to a new AVExprState struct
Marton Balint
cus at passwd.hu
Sat Jan 11 18:36:03 EET 2020
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 at 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
More information about the ffmpeg-devel
mailing list