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 _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".