[FFmpeg-devel] [PATCH 1/4] avfilter/all: check and propagate the return code of av_expr_parse_and_eval

[Ganesh Ajjanagadde]

On Sun, Nov 1, 2015 at 2:14 PM, Michael Niedermayer
<michael at niedermayer.cc> wrote:
> On Sat, Oct 31, 2015 at 10:47:54AM -0400, Ganesh Ajjanagadde wrote:
>> This function can return ENOMEM that needs to be propagated.
>>
>> Signed-off-by: Ganesh Ajjanagadde <gajjanagadde at gmail.com>
>> ---
>>  libavfilter/vf_pad.c    | 10 ++++++----
>>  libavfilter/vf_rotate.c |  3 +--
>>  libavfilter/vf_scale.c  |  5 +++--
>>  libavfilter/vf_zscale.c |  5 +++--
>>  4 files changed, 13 insertions(+), 10 deletions(-)
>>
>> diff --git a/libavfilter/vf_pad.c b/libavfilter/vf_pad.c
>> index 63dc6a8..e10d41b 100644
>> --- a/libavfilter/vf_pad.c
>> +++ b/libavfilter/vf_pad.c
>> @@ -114,9 +114,10 @@ static int config_input(AVFilterLink *inlink)
>>      var_values[VAR_VSUB]  = 1 << s->draw.vsub_max;
>>
>>      /* evaluate width and height */
>> -    av_expr_parse_and_eval(&res, (expr = s->w_expr),
>> +    if ((ret = av_expr_parse_and_eval(&res, (expr = s->w_expr),
>>                             var_names, var_values,
>> -                           NULL, NULL, NULL, NULL, NULL, 0, ctx);
>> +                           NULL, NULL, NULL, NULL, NULL, 0, ctx)) < 0)
>> +        goto eval_fail;
>>      s->w = var_values[VAR_OUT_W] = var_values[VAR_OW] = res;
>>      if ((ret = av_expr_parse_and_eval(&res, (expr = s->h_expr),
>>                                        var_names, var_values,
>> @@ -131,9 +132,10 @@ static int config_input(AVFilterLink *inlink)
>>      s->w = var_values[VAR_OUT_W] = var_values[VAR_OW] = res;
>>
>>      /* evaluate x and y */
>> -    av_expr_parse_and_eval(&res, (expr = s->x_expr),
>> +    if ((ret = av_expr_parse_and_eval(&res, (expr = s->x_expr),
>>                             var_names, var_values,
>> -                           NULL, NULL, NULL, NULL, NULL, 0, ctx);
>> +                           NULL, NULL, NULL, NULL, NULL, 0, ctx)) < 0)
>> +        goto eval_fail;
>>      s->x = var_values[VAR_X] = res;
>>      if ((ret = av_expr_parse_and_eval(&res, (expr = s->y_expr),
>>                                        var_names, var_values,
>> diff --git a/libavfilter/vf_rotate.c b/libavfilter/vf_rotate.c
>> index f12a103..d5e01e2 100644
>> --- a/libavfilter/vf_rotate.c
>> +++ b/libavfilter/vf_rotate.c
>> @@ -235,8 +235,7 @@ static int config_props(AVFilterLink *outlink)
>>  } while (0)
>>
>>      /* evaluate width and height */
>> -    av_expr_parse_and_eval(&res, expr = rot->outw_expr_str, var_names, rot->var_values,
>> -                           func1_names, func1, NULL, NULL, rot, 0, ctx);
>> +    SET_SIZE_EXPR(outw, "out_w");
>>      rot->var_values[VAR_OUT_W] = rot->var_values[VAR_OW] = res;
>>      rot->outw = res + 0.5;
>>      SET_SIZE_EXPR(outh, "out_w");
>
>
>> diff --git a/libavfilter/vf_scale.c b/libavfilter/vf_scale.c
>> index 1032688..2cf14e0 100644
>> --- a/libavfilter/vf_scale.c
>> +++ b/libavfilter/vf_scale.c
>> @@ -265,9 +265,10 @@ static int config_props(AVFilterLink *outlink)
>>      var_values[VAR_OVSUB] = 1 << out_desc->log2_chroma_h;
>>
>>      /* evaluate width and height */
>> -    av_expr_parse_and_eval(&res, (expr = scale->w_expr),
>> +    if ((ret = av_expr_parse_and_eval(&res, (expr = scale->w_expr),
>>                             var_names, var_values,
>> -                           NULL, NULL, NULL, NULL, NULL, 0, ctx);
>> +                           NULL, NULL, NULL, NULL, NULL, 0, ctx)) < 0)
>> +        goto fail;
>>      scale->w = var_values[VAR_OUT_W] = var_values[VAR_OW] = res;
>>      if ((ret = av_expr_parse_and_eval(&res, (expr = scale->h_expr),
>>                                        var_names, var_values,
>
> the first evaluation can fail and such failure does not represent an
> error for the calling code
>
> this is needed to support referencing the output parameters,
> for example
> scale=iw/ih*oh*sar:240
>
> here the first evalution of width fails but the 3rd succeeds as
> the 2nd made "oh" available
>
> please make sure the other hunks dont introduce similar issues
> in other filters (theres similar such double eval in other filters)
> also it would make sense to add comments to each such occurance of
> double evaluation so noone else adds a check there by mistake

So if I understand correctly there is both ENOMEM and EINVAL. I think
ENOMEM should be a "hard" failure that exits immediately - I doubt
that a second evaluation makes sense in such a case.

For EINVAL, your point makes sense. This will need a little more
careful rework. Thanks. Will repost patches once other two are
reviewed.

>
> [...]
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> The worst form of inequality is to try to make unequal things equal.
> -- Aristotle
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>