[FFmpeg-devel] [PATCH] avutil/eval: make av_expr_eval more thread safe

[Michael Niedermayer]

On Sun, Dec 15, 2019 at 04:24:11PM +0100, Marton Balint wrote:
> 
> 
> On Sun, 15 Dec 2019, Michael Niedermayer wrote:
> 
> >On Sun, Dec 15, 2019 at 01:52:52PM +0100, Marton Balint wrote:
> >>
> >>
> >>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.
> >>>>
> >>>
> 
> [...]
> 
> >>
> >>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.
> >
> >yes decoupling them / giving the user more control over the state makes sense
> >
> >
> >>
> >>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.
> >
> >This solution kind of sucks because it forces a heap of cases to run
> >single threaded which would work find with multiple threads
> >OTOH with one AVExpr or AVExprState per thread it would work multi threaded
> 
> That would resolve the data race issue in the ticket. However, if the
> expression depends on previous state then the result will be undeterministic
> because it is random which slices get assigned to which threads, so the
> initial AVExprState they see will be random as well.

well, something needs to be done to make it deterministic at least

we can also make the ld/st pixel local and later add ways to access the left
and top pixels state which could both be done with threads

Either way question is really what is most usefull for expressions


> 
> If you force single thread, the result will be deterministic.
> 
> >
> >The one case the state per thread will need to take a bit of care of is the
> >random seed, which we would want to be different per thread
> 
> We can just say that it is up to the user to initialize the random seed if
> needed, that is the case for single thread as well.

the user of the API yes
but it should not be the command line user as that would be messy to do with
an expression

thanks

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

I have never wished to cater to the crowd; for what I know they do not
approve, and what they approve I do not know. -- Epicurus
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20191215/61face4c/attachment.sig>