[FFmpeg-devel] [PATCH 2/3] avfilter/vf_geq: use per-thread state for expression evaluation

[Marton Balint]

On Mon, 30 Dec 2019, Michael Niedermayer wrote:

> On Sun, Dec 29, 2019 at 05:03:55PM +0100, Marton Balint wrote:
>>
>>
>> On Sun, 29 Dec 2019, Michael Niedermayer wrote:
>>
>>> On Sat, Dec 28, 2019 at 03:46:24PM +0100, Marton Balint wrote:
>>>> Fixes ticket #7528.
>>>>
>>>> Signed-off-by: Marton Balint <cus at passwd.hu>
>>>> ---
>>>> libavfilter/vf_geq.c | 9 +++++++--
>>>> 1 file changed, 7 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/libavfilter/vf_geq.c b/libavfilter/vf_geq.c
>>>> index 2905efae24..417b92222d 100644
>>>> --- a/libavfilter/vf_geq.c
>>>> +++ b/libavfilter/vf_geq.c
>>>> @@ -377,6 +377,7 @@ static int slice_geq_filter(AVFilterContext *ctx, void *arg, int jobnr, int nb_j
>>>>     int x, y;
>>>>     uint8_t *ptr;
>>>>     uint16_t *ptr16;
>>>> +    AVExprState *state = av_expr_state_alloc();
>>>>
>>>>     double values[VAR_VARS_NB];
>>>>     values[VAR_W] = geq->values[VAR_W];
>>>> @@ -386,6 +387,9 @@ static int slice_geq_filter(AVFilterContext *ctx, void *arg, int jobnr, int nb_j
>>>>     values[VAR_SH] = geq->values[VAR_SH];
>>>>     values[VAR_T] = geq->values[VAR_T];
>>>>
>>>> +    if (!state)
>>>> +        return AVERROR(ENOMEM);
>>>> +
>>>>     if (geq->bps == 8) {
>>>>         for (y = slice_start; y < slice_end; y++) {
>>>>             ptr = geq->dst + linesize * y;
>>>> @@ -393,7 +397,7 @@ static int slice_geq_filter(AVFilterContext *ctx, void *arg, int jobnr, int nb_j
>>>>
>>>>             for (x = 0; x < width; x++) {
>>>>                 values[VAR_X] = x;
>>>> -                ptr[x] = av_expr_eval(geq->e[plane], values, geq);
>>>> +                ptr[x] = av_expr_eval2(geq->e[plane], state, values, geq);
>>>>             }
>>>>             ptr += linesize;
>>>>         }
>>>> @@ -404,10 +408,11 @@ static int slice_geq_filter(AVFilterContext *ctx, void *arg, int jobnr, int nb_j
>>>>             values[VAR_Y] = y;
>>>>             for (x = 0; x < width; x++) {
>>>>                 values[VAR_X] = x;
>>>> -                ptr16[x] = av_expr_eval(geq->e[plane], values, geq);
>>>> +                ptr16[x] = av_expr_eval2(geq->e[plane], state, values, geq);
>>>>             }
>>>>         }
>>>>     }
>>>> +    av_expr_state_free(&state);
>>>
>>> a filter which adds random noise would get its seed reset on each frame and
>>> slice with this, unless iam missing something.
>>>
>>> The random values produced by random should be uncorrelatec between frame n and m when n!=m
>>
>> I guess I can make one AVExprState for each thread and keep them during the
>> lifetime of the filter. That would resolve the frame correlation, but not
>> the slice correlation.
>>
>> I don't see too many simple ways to deal with this:
>>
>> 1) accept slice correlation and keep the AVExprStates across executions
>>
>> 2) document that the number of filter threads has to be 1 if uncorrelated
>> random is required
>>
>> 3) initialize the state with the line number at the beginning of each line,
>> which is kind of ugly but would make the filter invariant of the thread
>> count used.
>>
>> Do you have something else in mind? Do you prefer any of these?
>
> for the purpose of random numbers we do not need dependancies accross
> slices or frames and we should have none
>
> It probably makes sense though that in the future each pixels evaluation
> can access the state of the previously evaluated touching pixels both
> temporal and spatial. Threading could then be done depending on where
> the depandancies are.
> This may introduce different slice evaluation orders (like in columns
> instead of rows or diagonally if both left and top state is accessed)
>
> But back to the subject,
>
> We could add a expression to initialize the state at the slice or line start,
> this could be used to set the state based on the line number, iam a bit
> undecided on this though, its not impossible to do ATM without changes

Yeah, it is doable right now by prepending the expression with this:

ifnot(X,st(0,Y));

So I don't think adding an extra expression for this is worth it.

Regards,
Marton