diff mbox

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

Message ID 20191228144625.28804-2-cus@passwd.hu
State New
Headers show

Commit Message

Marton Balint Dec. 28, 2019, 2:46 p.m. UTC
Fixes ticket #7528.

Signed-off-by: Marton Balint <cus@passwd.hu>
---
 libavfilter/vf_geq.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

Comments

Michael Niedermayer Dec. 29, 2019, 3:23 p.m. UTC | #1
On Sat, Dec 28, 2019 at 03:46:24PM +0100, Marton Balint wrote:
> Fixes ticket #7528.
> 
> Signed-off-by: Marton Balint <cus@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

thx

[...]
Marton Balint Dec. 29, 2019, 4:03 p.m. UTC | #2
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@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?

Thanks,
Marton
Michael Niedermayer Dec. 30, 2019, 8:22 p.m. UTC | #3
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@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

thx

[...]
Marton Balint Dec. 30, 2019, 9:22 p.m. UTC | #4
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@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
diff mbox

Patch

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);
 
     return 0;
 }