[FFmpeg-devel] [PATCH 3/5] avfilter/scale: separate exprs parse and eval

[Gyan]

Ping for this and remaining patches in set.

On 17-12-2019 02:55 pm, Gyan wrote:
>
>
> On 17-12-2019 01:44 am, Michael Niedermayer wrote:
>> On Sun, Dec 15, 2019 at 10:36:58PM +0530, Gyan wrote:
>>> 3rd of 5 factorized patches; supersedes
>>> https://patchwork.ffmpeg.org/patch/16272/
>>>   scale_eval.c |   69 --------------
>>>   vf_scale.c   |  286 
>>> +++++++++++++++++++++++++++++++++++++++++++++++++++++++----
>>>   2 files changed, 271 insertions(+), 84 deletions(-)
>>> 2a3ae4ce4e91893fddb020485e6936c82894eb81 
>>> 0003-avfilter-scale-separate-exprs-parse-and-eval.patch
>>>  From 00b54948b88ae60aa3ab7c158b98e55cb8b967d3 Mon Sep 17 00:00:00 2001
>>> From: Gyan Doshi <ffmpeg at gyani.pro>
>>> Date: Thu, 12 Dec 2019 22:54:31 +0530
>>> Subject: [PATCH 3/5] avfilter/scale: separate exprs parse and eval
>>>
>>> Will allow adding animation support.
>> [...]
>>> @@ -566,19 +746,87 @@ static int process_command(AVFilterContext 
>>> *ctx, const char *cmd, const char *ar
>>>                              char *res, int res_len, int flags)
>>>   {
>>>       ScaleContext *scale = ctx->priv;
>>> -    int ret;
>>> +    AVFilterLink *outlink = ctx->outputs[0];
>>> +    char *old_w_str, *old_h_str;
>>> +    AVExpr *old_w_pexpr, *old_h_pexpr;
>>> +    int ret, w = 0, h = 0;
>>> +    const char scale2ref = ctx->filter == &ff_vf_scale2ref;
>>> +    const char *const *names = scale2ref ? var_names_scale2ref : 
>>> var_names;
>>> +
>>> +    w = !strcmp(cmd, "width")  || !strcmp(cmd, "w");
>>> +    h = !strcmp(cmd, "height")  || !strcmp(cmd, "h");
>>> +
>>> +    if (w || h) {
>>>   -    if (   !strcmp(cmd, "width")  || !strcmp(cmd, "w")
>>> -        || !strcmp(cmd, "height") || !strcmp(cmd, "h")) {
>>> +        if (w) {
>>> +            old_w_str = av_strdup(scale->w_expr);
>>> +            if (!old_w_str)
>>> +                return AVERROR(ENOMEM);
>>> +            old_w_pexpr = scale->w_pexpr;
>>> +            scale->w_pexpr = NULL;
>>> +        }
>>>   -        int old_w = scale->w;
>>> -        int old_h = scale->h;
>>> -        AVFilterLink *outlink = ctx->outputs[0];
>>> +        if (h) {
>>> +            old_h_str = av_strdup(scale->h_expr);
>>> +            if (!old_h_str)
>>> +                return AVERROR(ENOMEM);
>>> +            old_h_pexpr = scale->h_pexpr;
>>> +            scale->h_pexpr = NULL;
>>> +        }
>>>             av_opt_set(scale, cmd, args, 0);
>>> +
>>> +        if (w) {
>>> +            ret = av_expr_parse(&scale->w_pexpr, scale->w_expr,
>>> +                            names,
>>> +                            NULL, NULL, NULL, NULL, 0, ctx);
>>> +            if (ret < 0) {
>>> +                av_log(ctx, AV_LOG_ERROR, "Cannot parse width 
>>> expression: '%s'\n", scale->w_expr);
>>> +                av_opt_set(scale, "w", old_w_str, 0);
>>> +                av_free(old_w_str);
>>> +                scale->w_pexpr = old_w_pexpr;
>>> +                return ret;
>>> +            }
>>> +        }
>>> +
>>> +        if (h) {
>>> +            ret = av_expr_parse(&scale->h_pexpr, scale->h_expr,
>>> +                            names,
>>> +                            NULL, NULL, NULL, NULL, 0, ctx);
>>> +            if (ret < 0) {
>>> +                av_log(ctx, AV_LOG_ERROR, "Cannot parse height 
>>> expression: '%s'\n", scale->h_expr);
>>> +                av_opt_set(scale, "h", old_h_str, 0);
>>> +                av_free(old_h_str);
>>> +                scale->h_pexpr = old_h_pexpr;
>>> +                return ret;
>>> +            }
>>> +        }
>>> +
>>>           if ((ret = config_props(outlink)) < 0) {
>>> -            scale->w = old_w;
>>> -            scale->h = old_h;
>>> +
>>> +            if (w) {
>>> +                av_opt_set(scale, "w", old_w_str, 0);
>>> +                av_free(old_w_str);
>>> +                av_expr_free(scale->w_pexpr);
>>> +                scale->w_pexpr = old_w_pexpr;
>>> +            }
>>> +            if (h) {
>>> +                av_opt_set(scale, "h", old_h_str, 0);
>>> +                av_free(old_h_str);
>>> +                av_expr_free(scale->h_pexpr);
>>> +                scale->h_pexpr = old_h_pexpr;
>>> +            }
>>> +            av_log(ctx, AV_LOG_ERROR, "Command failed. Continuing 
>>> with existing parameters.\n");
>>> +            return ret;
>>> +        }
>> the cleanup code is duplicated
>>
>> also if you can make the overall change this patch is making
>> cleaner/clearer that would be welcome too. Its just a feeling
>> but this seems more messy than what i would expect from spliting
>> parse out
>
> More compact v2 patch attached.
>
> Which changes do you find messy? Once parse and eval are separated, 
> the AVExpr and expr strings are backed up for restoration should the 
> command fail.
>
> Gyan
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".