[FFmpeg-devel] avutil/eval: add function to track variable use

Submitted by Gyan on Nov. 3, 2019, 5:44 a.m.

Details

Message ID 72be1962-95c2-e216-ecb1-5414508e47f9@gyani.pro
State New
Headers show

Commit Message

Gyan Nov. 3, 2019, 5:44 a.m.
Helps better identification of expr eval failures.

Gyan
From 19bce329464676f071707b99575f80e5abe1cd4c Mon Sep 17 00:00:00 2001
From: Gyan Doshi <ffmpeg@gyani.pro>
Date: Sat, 2 Nov 2019 20:16:42 +0530
Subject: [PATCH] avutil/eval: add function to track variable use

Helps avoid multiple evals of cross-referenced expressions
and catch the use of non-applicable variables with respect
to eval or special mode in filters
---
 libavutil/eval.c    | 21 +++++++++++++++++++++
 libavutil/eval.h    | 10 ++++++++++
 libavutil/version.h |  4 ++--
 3 files changed, 33 insertions(+), 2 deletions(-)

Comments

Michael Niedermayer Nov. 4, 2019, 11:05 p.m.
On Sun, Nov 03, 2019 at 11:14:25AM +0530, Gyan wrote:
> 
> Helps better identification of expr eval failures.
> 
> Gyan

>  eval.c    |   21 +++++++++++++++++++++
>  eval.h    |   10 ++++++++++
>  version.h |    4 ++--
>  3 files changed, 33 insertions(+), 2 deletions(-)
> 3dd142baa0144fd324eb9da8a9932cfd7ab2cd98  0001-avutil-eval-add-function-to-track-variable-use.patch
> From 19bce329464676f071707b99575f80e5abe1cd4c Mon Sep 17 00:00:00 2001
> From: Gyan Doshi <ffmpeg@gyani.pro>
> Date: Sat, 2 Nov 2019 20:16:42 +0530
> Subject: [PATCH] avutil/eval: add function to track variable use
> 
> Helps avoid multiple evals of cross-referenced expressions
> and catch the use of non-applicable variables with respect
> to eval or special mode in filters

Maybe you should provide more details of the use case of this, maybe
with an example. Because it seems not completely obvious

thx

[...]
Gyan Nov. 5, 2019, 4:43 a.m.
On 05-11-2019 04:35 am, Michael Niedermayer wrote:
> On Sun, Nov 03, 2019 at 11:14:25AM +0530, Gyan wrote:
>> Helps better identification of expr eval failures.
>>
>> Gyan
>>   eval.c    |   21 +++++++++++++++++++++
>>   eval.h    |   10 ++++++++++
>>   version.h |    4 ++--
>>   3 files changed, 33 insertions(+), 2 deletions(-)
>> 3dd142baa0144fd324eb9da8a9932cfd7ab2cd98  0001-avutil-eval-add-function-to-track-variable-use.patch
>>  From 19bce329464676f071707b99575f80e5abe1cd4c Mon Sep 17 00:00:00 2001
>> From: Gyan Doshi <ffmpeg@gyani.pro>
>> Date: Sat, 2 Nov 2019 20:16:42 +0530
>> Subject: [PATCH] avutil/eval: add function to track variable use
>>
>> Helps avoid multiple evals of cross-referenced expressions
>> and catch the use of non-applicable variables with respect
>> to eval or special mode in filters
> Maybe you should provide more details of the use case of this, maybe
> with an example. Because it seems not completely obvious

An example of a cross-referenced expression would be 'y=x+rand(10)'.  
Normally in filters, X would be evaluated first, then Y, then X again. 
The 2nd eval of X will overwrite the first and can generate a different 
value. With this func, we can avoid the 2nd eval.

I'm in the process of modifying the scale filter to allow 'n', 't', 
'pos' variables in frame eval mode (it already supports dynamic output 
based on incoming frame changes) and I would like to catch their 
presence in init mode, since av_expr_eval will fail. Now, it can also 
fail due to circularly referenced expressions e.g. 'x=y+10' and 'y=x+10' 
and there's no way to detect whether it's this case or if inapplicable 
variables have been used.

Regards,
Gyan
Michael Niedermayer Nov. 5, 2019, 10:25 a.m.
On Tue, Nov 05, 2019 at 10:13:52AM +0530, Gyan wrote:
> 
> 
> On 05-11-2019 04:35 am, Michael Niedermayer wrote:
> >On Sun, Nov 03, 2019 at 11:14:25AM +0530, Gyan wrote:
> >>Helps better identification of expr eval failures.
> >>
> >>Gyan
> >>  eval.c    |   21 +++++++++++++++++++++
> >>  eval.h    |   10 ++++++++++
> >>  version.h |    4 ++--
> >>  3 files changed, 33 insertions(+), 2 deletions(-)
> >>3dd142baa0144fd324eb9da8a9932cfd7ab2cd98  0001-avutil-eval-add-function-to-track-variable-use.patch
> >> From 19bce329464676f071707b99575f80e5abe1cd4c Mon Sep 17 00:00:00 2001
> >>From: Gyan Doshi <ffmpeg@gyani.pro>
> >>Date: Sat, 2 Nov 2019 20:16:42 +0530
> >>Subject: [PATCH] avutil/eval: add function to track variable use
> >>
> >>Helps avoid multiple evals of cross-referenced expressions
> >>and catch the use of non-applicable variables with respect
> >>to eval or special mode in filters
> >Maybe you should provide more details of the use case of this, maybe
> >with an example. Because it seems not completely obvious
> 
> An example of a cross-referenced expression would be 'y=x+rand(10)'. 
> Normally in filters, X would be evaluated first, then Y, then X again. The
> 2nd eval of X will overwrite the first and can generate a different value.
> With this func, we can avoid the 2nd eval.
> 
> I'm in the process of modifying the scale filter to allow 'n', 't', 'pos'
> variables in frame eval mode (it already supports dynamic output based on
> incoming frame changes) and I would like to catch their presence in init
> mode, since av_expr_eval will fail. Now, it can also fail due to circularly
> referenced expressions e.g. 'x=y+10' and 'y=x+10' and there's no way to
> detect whether it's this case or if inapplicable variables have been used.

please add this (or similar information) to the commit message

thx

[...]
Nicolas George Nov. 5, 2019, 2:10 p.m.
Gyan (12019-11-05):
> An example of a cross-referenced expression would be 'y=x+rand(10)'. 
> Normally in filters, X would be evaluated first, then Y, then X again. The
> 2nd eval of X will overwrite the first and can generate a different value.
> With this func, we can avoid the 2nd eval.
> 
> I'm in the process of modifying the scale filter to allow 'n', 't', 'pos'
> variables in frame eval mode (it already supports dynamic output based on
> incoming frame changes) and I would like to catch their presence in init
> mode, since av_expr_eval will fail. Now, it can also fail due to circularly
> referenced expressions e.g. 'x=y+10' and 'y=x+10' and there's no way to
> detect whether it's this case or if inapplicable variables have been used.

Then I would prefer that we wait for that to be done and working to
commit this patch. After all, it is entirely possible that you realize
this is not exactly what you need to achieve your goal.

Also, you may consider if it would be worthwhile to globally improve the
expression system: maybe have a context with dedicated structure to
define the variables and custom functions instead of just arrays that
needs to have the same size.

The scale filter is not the only one with multiple expression
evaluation, it would be a waste if the simplification was not applicable
to all cases easily.

(Also, the possibility to define user variables would be nice. But that
would require re-writing most of the parser. It needs doing anyway, but
it is a huge work.)

Regards,
Gyan Nov. 5, 2019, 2:54 p.m.
On 05-11-2019 07:40 pm, Nicolas George wrote:
> Gyan (12019-11-05):
>> An example of a cross-referenced expression would be 'y=x+rand(10)'.
>> Normally in filters, X would be evaluated first, then Y, then X again. The
>> 2nd eval of X will overwrite the first and can generate a different value.
>> With this func, we can avoid the 2nd eval.
>>
>> I'm in the process of modifying the scale filter to allow 'n', 't', 'pos'
>> variables in frame eval mode (it already supports dynamic output based on
>> incoming frame changes) and I would like to catch their presence in init
>> mode, since av_expr_eval will fail. Now, it can also fail due to circularly
>> referenced expressions e.g. 'x=y+10' and 'y=x+10' and there's no way to
>> detect whether it's this case or if inapplicable variables have been used.
> Then I would prefer that we wait for that to be done and working to
> commit this patch. After all, it is entirely possible that you realize
> this is not exactly what you need to achieve your goal.

I've looked for workarounds using existing API and I haven't found one. 
In my scale patch, I've implemented this function locally and it 
fulfills the role. Thought I should insert it generically.
> Also, you may consider if it would be worthwhile to globally improve the
> expression system: maybe have a context with dedicated structure to
> define the variables and custom functions instead of just arrays that
> needs to have the same size
>
> The scale filter is not the only one with multiple expression
> evaluation, it would be a waste if the simplification was not applicable
> to all cases easily.
I have come across the need for this helper in other filters like the 
various draw* filters but it was never a priority. But the changes to 
scale/scale2ref filter need it or its equivalent. This one does the 
trick and I can see how it would be helpful in animation support for 
drawbox where all vars can refer to each other, so each expression is 
evaluated 5 times. That work is half-way done and is my next focus.

> (Also, the possibility to define user variables would be nice. But that
> would require re-writing most of the parser. It needs doing anyway, but
> it is a huge work.)
That sounds desirable but that needs careful design in terms of 
sanitation of user input. That looks to be a job for another day. 
Possibly, early next year.

Thanks,
Gyan

Patch hide | download patch | download mbox

diff --git a/libavutil/eval.c b/libavutil/eval.c
index 48832979e2..ed0fe636f7 100644
--- a/libavutil/eval.c
+++ b/libavutil/eval.c
@@ -735,6 +735,27 @@  end:
     return ret;
 }
 
+int av_expr_count_var(AVExpr *e, int var_start, int var_end)
+{
+    int i, count = 0;
+
+    if (var_start >= var_end)
+        return AVERROR(EINVAL);
+
+    if (!e)
+        return AVERROR(EINVAL);
+
+    for (i = 0; e->type != e_const && i < 3 && e->param[i]; i++)
+        count += av_expr_count_var(e->param[i], var_start, var_end);
+
+    if (e->type == e_const &&
+        e->a.const_index >= var_start &&
+        e->a.const_index < var_end)
+        count++;
+
+    return count;
+}
+
 double av_expr_eval(AVExpr *e, const double *const_values, void *opaque)
 {
     Parser p = { 0 };
diff --git a/libavutil/eval.h b/libavutil/eval.h
index dacd22b96e..030e5617cc 100644
--- a/libavutil/eval.h
+++ b/libavutil/eval.h
@@ -86,6 +86,16 @@  int av_expr_parse(AVExpr **expr, const char *s,
  */
 double av_expr_eval(AVExpr *e, const double *const_values, void *opaque);
 
+/**
+ * Return the number of occurrences of variables within a range in a parsed expression
+ *
+ * @param var_start index of the start of the range of variables being seached for in the const_names identifiers
+ * @param var_end index of the first variable after the range of variables being seached for in the const_names identifiers
+ * @return the number of occurrences of the variables in the range, a negative value indicates that no expression was passed
+ * or the range wasn't strictly monotonic
+ */
+int av_expr_count_var(AVExpr *e, int var_start, int var_end);
+
 /**
  * Free a parsed expression previously created with av_expr_parse().
  */
diff --git a/libavutil/version.h b/libavutil/version.h
index 27d663baf1..af3abf7265 100644
--- a/libavutil/version.h
+++ b/libavutil/version.h
@@ -79,8 +79,8 @@ 
  */
 
 #define LIBAVUTIL_VERSION_MAJOR  56
-#define LIBAVUTIL_VERSION_MINOR  35
-#define LIBAVUTIL_VERSION_MICRO 101
+#define LIBAVUTIL_VERSION_MINOR  36
+#define LIBAVUTIL_VERSION_MICRO 100
 
 #define LIBAVUTIL_VERSION_INT   AV_VERSION_INT(LIBAVUTIL_VERSION_MAJOR, \
                                                LIBAVUTIL_VERSION_MINOR, \