On Sun, 15 Dec 2019, Michael Niedermayer wrote:
On Sun, Dec 15, 2019 at 02:16:49AM +0100, Marton Balint wrote:
Unfortunately the ld() and st() operations store the variables in the AVExpr
context and not in the parser in order to keep the stored values between
evaluations (which is a rarely(?) used undocumented(?) feature of AVExpr).
This causes data race issues when the same expression is evaluated from
multiple threads. Let's use the Parser as variable store during evaluation and
only sync the AVExpr context variables with the Parser before and after
evaluation. This keeps the feature working for single threaded cases and fixes
using ld() and st() usage for the "normal" use case when the ld()-s only
reference st()-d values from the same evaluation.
Maybe we should just remove this feature instead?
having some variable that is not local to the current point evaluation is
needed for random() as it uses it as state
ld/st are also documented in doc/utils.texi and are the basis for multiple
features so they themselfs cant be removed either
lt() and st() is documented, but it is not documented that they keep
their values between evaluations.
{
Parser p = { 0 };
- p.var= e->var;
+ double ret;
+ memcpy(p.var, e->var, sizeof(p.var));
p.const_values = const_values;
p.opaque = opaque;
- return eval_expr(&p, e);
+ ret = eval_expr(&p, e);
+ memcpy(e->var, p.var, sizeof(p.var));
this does not look thread safe to me
It is thread safe if the expression is deterministic in a sense that
it does not use the state variables without setting them first during a
single evaluation.
also it slows the code down by extra memcopies
I think the main question is, do we need to share a expression evaluator
between threads ?
Is that useful to any feature or usecase ?
If it has no use case then the obvious solution is not to share the expression
evaluator or at least not the things written to per evaluation
The ticket this patch fix is a pretty good example where it matters.
The confusion is caused by mixing the state with the parsed expression.
Maybe we should decouple them, or at least add the possibility to the
user to use a custom state.
Anyway, I am inclined to accept what Gyan proposed, to determine if the
expression uses lt() st() or rand() and if it does fall back to 1
thread. Or implement something like av_expr_is_stateless() to do this in a
helper function.
Regards,
Marton
_______________________________________________
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".
