[Ffmpeg-devel] [PATCH] optimize ff_eval

[Michael Niedermayer]

Hi

On Fri, Oct 27, 2006 at 12:25:06PM +0200, Oded Shimon wrote:
> On Thu, Oct 26, 2006 at 10:25:23PM +0200, Michael Niedermayer wrote:
> > Hi
> > 
> > On Thu, Oct 26, 2006 at 08:45:02PM +0200, Oded Shimon wrote:
> > > I took the liberty of optimizing ff_eval for multiple runs by seperating 
> > > the parsing/compiling and runtime steps.
> > > 
> > > before:
> > > 97730 dezicycles in ff_eval, 1023 runs, 1 skips
> > > after:
> > > 13643 dezicycles in ff_parse, 1023 runs, 1 skips
> > 
> > great :)
> > what about the eval.o size?
> 
> old: -rw-------  1 ods15 ods15 70604 Oct 27 12:13 eval.o
> new: -rw-------  1 ods15 ods15 73900 Oct 27 12:13 eval.o
> both:-rw-------  1 ods15 ods15 82088 Oct 27 12:13 eval.o

perfect


> 
> 
> > > What should I do with this? Deperecate or keep old ff_eval? update current 
> > > calls for ff_eval to this new thing?
> > 
> > hmm, id say deprecate "now" and remove with the next major version increase
> > and update all in a seperate patch as soon as this is cleaned up reviewed and
> > commited
> 
> Thing is, current version is ~3 times faster for one-shot evals, not to 
> mention simpler API and no mallocs. I think AVOption uses eval in one-shot 
> cases? Might be a good idea to keep both, it would involve some inevitable 
> duplicate code though...

iam against that, a ff_eval() which calles the new parse & eval is enough
the small number of cycles during parsing of command line params doesnt
matter, all? other cases use the same expression many times


[...]
> +struct ff_expr_s {
> +    int type;
> +    double value; // is sign in other types
> +    union {
> +        int const_index;
> +        double (*func0)(double);
> +        double (*func1)(void *, double);
> +        double (*func2)(void *, double, double);
> +    } a;
> +    AVEvalExpr * param[2];
> +};
> +
> +static double eval_expr(Parser * p, AVEvalExpr * e) {
> +    switch (e->type) {
> +    	case 0: return e->value;
> +    	case 1: return e->value * p->const_value[e->a.const_index];
> +        case 2: return e->value * e->a.func0(eval_expr(p, e->param[0]));
> +        case 3: return e->value * e->a.func1(p->opaque, eval_expr(p, e->param[0]));
> +        case 4: return e->value * e->a.func2(p->opaque, eval_expr(p, e->param[0]), eval_expr(p, e->param[1]));
> +        case 5: return 1/(1+exp(4*eval_expr(p, e->param[0])));
> +        case 6: { double d = eval_expr(p, e->param[0]); return exp(-d*d/2)/sqrt(2*M_PI); }
> +        default: {
> +            double d = eval_expr(p, e->param[0]);
> +            double d2 = eval_expr(p, e->param[1]);
> +            switch (e->type) {
> +                case  7: return e->value * (d - floor(d/d2)*d2);
> +                case  8: return e->value * (d >  d2 ?   d : d2);
> +                case  9: return e->value * (d <  d2 ?   d : d2);
> +                case 10: return e->value * (d == d2 ? 1.0 : 0.0);
> +                case 11: return e->value * (d >  d2 ? 1.0 : 0.0);
> +                case 12: return e->value * (d >= d2 ? 1.0 : 0.0);
> +                case 13: return e->value * pow(d, d2);
> +                case 14: return e->value * (d * d2);
> +                case 15: return e->value * (d / d2);
> +                case 16: return e->value * (d + d2);

please use a enum for e->type


[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

In the past you could go to a library and read, borrow or copy any book
Today you'd get arrested for mere telling someone where the library is