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

[Anton Khirnov]

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.

-- 
Anton Khirnov