[FFmpeg-devel] [PATCH] avfilter/scale: allow dynamic output via expressions

Gyan ffmpeg at gyani.pro
Fri Nov 15 07:40:26 EET 2019



On 15-11-2019 04:01 am, Michael Niedermayer wrote:
> On Thu, Nov 14, 2019 at 11:12:23PM +0530, Gyan wrote:
>>
>> On 14-11-2019 01:12 am, Michael Niedermayer wrote:
>>> Moving and changing code at the same time makes it hard to see th difference.
>>> Idealy all code moves should be seperate from changes to the code.
>>>
>>> also more generally, spliting this patch up would simpify review
>> Split into two. First patch mostly moves code and keeps existing
>> functionality. 2nd patch introduces new features and requires the new eval
>> function.
>>
>> Thanks,
>> Gyan
>>   Makefile   |    4 -
>>   scale.c    |   72 +---------------------
>>   vf_scale.c |  192 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>>   3 files changed, 196 insertions(+), 72 deletions(-)
>> 77579fdbd7add3be08bada5ee401df41f60ea236  v2-0001-avfilter-scale-shift-ff_scale_eval_dimensions-inl.patch
>>  From 359f538703865c8ebeda16b5d1846d2cf1cf9c4d Mon Sep 17 00:00:00 2001
>> From: Gyan Doshi <ffmpeg at gyani.pro>
>> Date: Thu, 14 Nov 2019 21:08:32 +0530
>> Subject: [PATCH v2 1/2] avfilter/scale: shift ff_scale_eval_dimensions inline
>>
>> This is a perfunctory change in preparation of adding
>> direct animation support to scale and scale2ref filters
>> ---
>>   libavfilter/Makefile   |   4 +-
>>   libavfilter/scale.c    |  72 +---------------
>>   libavfilter/vf_scale.c | 192 ++++++++++++++++++++++++++++++++++++++++-
>>   3 files changed, 196 insertions(+), 72 deletions(-)
>>
>> diff --git a/libavfilter/Makefile b/libavfilter/Makefile
>> index fce930360d..f1f6994574 100644
>> --- a/libavfilter/Makefile
>> +++ b/libavfilter/Makefile
>> @@ -358,12 +358,12 @@ OBJS-$(CONFIG_ROBERTS_OPENCL_FILTER)         += vf_convolution_opencl.o opencl.o
>>                                                   opencl/convolution.o
>>   OBJS-$(CONFIG_ROTATE_FILTER)                 += vf_rotate.o
>>   OBJS-$(CONFIG_SAB_FILTER)                    += vf_sab.o
>> -OBJS-$(CONFIG_SCALE_FILTER)                  += vf_scale.o scale.o
>> +OBJS-$(CONFIG_SCALE_FILTER)                  += vf_scale.o
>>   OBJS-$(CONFIG_SCALE_CUDA_FILTER)             += vf_scale_cuda.o vf_scale_cuda.ptx.o
>>   OBJS-$(CONFIG_SCALE_NPP_FILTER)              += vf_scale_npp.o scale.o
>>   OBJS-$(CONFIG_SCALE_QSV_FILTER)              += vf_scale_qsv.o
>>   OBJS-$(CONFIG_SCALE_VAAPI_FILTER)            += vf_scale_vaapi.o scale.o vaapi_vpp.o
>> -OBJS-$(CONFIG_SCALE2REF_FILTER)              += vf_scale.o scale.o
>> +OBJS-$(CONFIG_SCALE2REF_FILTER)              += vf_scale.o
>>   OBJS-$(CONFIG_SCROLL_FILTER)                 += vf_scroll.o
>>   OBJS-$(CONFIG_SELECT_FILTER)                 += f_select.o
>>   OBJS-$(CONFIG_SELECTIVECOLOR_FILTER)         += vf_selectivecolor.o
>> diff --git a/libavfilter/scale.c b/libavfilter/scale.c
>> index eaee95fac6..668aa04622 100644
>> --- a/libavfilter/scale.c
>> +++ b/libavfilter/scale.c
>> @@ -60,49 +60,6 @@ enum var_name {
>>       VARS_NB
>>   };
>>   
>> -/**
>> - * This must be kept in sync with var_names so that it is always a
>> - * complete list of var_names with the scale2ref specific names
>> - * appended. scale2ref values must appear in the order they appear
>> - * in the var_name_scale2ref enum but also be below all of the
>> - * non-scale2ref specific values.
>> - */
>> -static const char *const var_names_scale2ref[] = {
>> -    "PI",
>> -    "PHI",
>> -    "E",
>> -    "in_w",   "iw",
>> -    "in_h",   "ih",
>> -    "out_w",  "ow",
>> -    "out_h",  "oh",
>> -    "a",
>> -    "sar",
>> -    "dar",
>> -    "hsub",
>> -    "vsub",
>> -    "ohsub",
>> -    "ovsub",
>> -    "main_w",
>> -    "main_h",
>> -    "main_a",
>> -    "main_sar",
>> -    "main_dar", "mdar",
>> -    "main_hsub",
>> -    "main_vsub",
>> -    NULL
>> -};
>> -
>> -enum var_name_scale2ref {
>> -    VAR_S2R_MAIN_W,
>> -    VAR_S2R_MAIN_H,
>> -    VAR_S2R_MAIN_A,
>> -    VAR_S2R_MAIN_SAR,
>> -    VAR_S2R_MAIN_DAR, VAR_S2R_MDAR,
>> -    VAR_S2R_MAIN_HSUB,
>> -    VAR_S2R_MAIN_VSUB,
>> -    VARS_S2R_NB
>> -};
>> -
>>   int ff_scale_eval_dimensions(void *log_ctx,
>>       const char *w_expr, const char *h_expr,
>>       AVFilterLink *inlink, AVFilterLink *outlink,
>> @@ -115,16 +72,7 @@ int ff_scale_eval_dimensions(void *log_ctx,
>>       int factor_w, factor_h;
>>       int eval_w, eval_h;
>>       int ret;
>> -    const char scale2ref = outlink->src->nb_inputs == 2 && outlink->src->inputs[1] == inlink;
>> -    double var_values[VARS_NB + VARS_S2R_NB], res;
>> -    const AVPixFmtDescriptor *main_desc;
>> -    const AVFilterLink *main_link;
>> -    const char *const *names = scale2ref ? var_names_scale2ref : var_names;
>> -
>> -    if (scale2ref) {
>> -        main_link = outlink->src->inputs[0];
>> -        main_desc = av_pix_fmt_desc_get(main_link->format);
>> -    }
>> +    double var_values[VARS_NB], res;
>>   
>>       var_values[VAR_PI]    = M_PI;
>>       var_values[VAR_PHI]   = M_PHI;
>> @@ -142,32 +90,20 @@ int ff_scale_eval_dimensions(void *log_ctx,
>>       var_values[VAR_OHSUB] = 1 << out_desc->log2_chroma_w;
>>       var_values[VAR_OVSUB] = 1 << out_desc->log2_chroma_h;
>>   
>> -    if (scale2ref) {
>> -        var_values[VARS_NB + VAR_S2R_MAIN_W] = main_link->w;
>> -        var_values[VARS_NB + VAR_S2R_MAIN_H] = main_link->h;
>> -        var_values[VARS_NB + VAR_S2R_MAIN_A] = (double) main_link->w / main_link->h;
>> -        var_values[VARS_NB + VAR_S2R_MAIN_SAR] = main_link->sample_aspect_ratio.num ?
>> -            (double) main_link->sample_aspect_ratio.num / main_link->sample_aspect_ratio.den : 1;
>> -        var_values[VARS_NB + VAR_S2R_MAIN_DAR] = var_values[VARS_NB + VAR_S2R_MDAR] =
>> -            var_values[VARS_NB + VAR_S2R_MAIN_A] * var_values[VARS_NB + VAR_S2R_MAIN_SAR];
>> -        var_values[VARS_NB + VAR_S2R_MAIN_HSUB] = 1 << main_desc->log2_chroma_w;
>> -        var_values[VARS_NB + VAR_S2R_MAIN_VSUB] = 1 << main_desc->log2_chroma_h;
>> -    }
>> -
>>       /* evaluate width and height */
>>       av_expr_parse_and_eval(&res, (expr = w_expr),
>> -                           names, var_values,
>> +                           var_names, var_values,
>>                              NULL, NULL, NULL, NULL, NULL, 0, log_ctx);
>>       eval_w = var_values[VAR_OUT_W] = var_values[VAR_OW] = (int) res == 0 ? inlink->w : (int) res;
>>   
>>       if ((ret = av_expr_parse_and_eval(&res, (expr = h_expr),
>> -                                      names, var_values,
>> +                                      var_names, var_values,
>>                                         NULL, NULL, NULL, NULL, NULL, 0, log_ctx)) < 0)
>>           goto fail;
>>       eval_h = var_values[VAR_OUT_H] = var_values[VAR_OH] = (int) res == 0 ? inlink->h : (int) res;
>>       /* evaluate again the width, as it may depend on the output height */
>>       if ((ret = av_expr_parse_and_eval(&res, (expr = w_expr),
>> -                                      names, var_values,
>> +                                      var_names, var_values,
>>                                         NULL, NULL, NULL, NULL, NULL, 0, log_ctx)) < 0)
>>           goto fail;
>>       eval_w = (int) res == 0 ? inlink->w : (int) res;
>> diff --git a/libavfilter/vf_scale.c b/libavfilter/vf_scale.c
>> index 41ddec7661..e7d52faccc 100644
>> --- a/libavfilter/vf_scale.c
>> +++ b/libavfilter/vf_scale.c
>> @@ -29,9 +29,9 @@
>>   #include "avfilter.h"
>>   #include "formats.h"
>>   #include "internal.h"
>> -#include "scale.h"
>>   #include "video.h"
>>   #include "libavutil/avstring.h"
>> +#include "libavutil/eval.h"
>>   #include "libavutil/internal.h"
>>   #include "libavutil/mathematics.h"
>>   #include "libavutil/opt.h"
>> @@ -41,6 +41,85 @@
>>   #include "libavutil/avassert.h"
>>   #include "libswscale/swscale.h"
>>   
>> +static const char *const var_names[] = {
>> +    "PI",
>> +    "PHI",
>> +    "E",
>> +    "in_w",   "iw",
>> +    "in_h",   "ih",
>> +    "out_w",  "ow",
>> +    "out_h",  "oh",
>> +    "a",
>> +    "sar",
>> +    "dar",
>> +    "hsub",
>> +    "vsub",
>> +    "ohsub",
>> +    "ovsub",
>> +    NULL
>> +};
>> +
>> +enum var_name {
>> +    VAR_PI,
>> +    VAR_PHI,
>> +    VAR_E,
>> +    VAR_IN_W,   VAR_IW,
>> +    VAR_IN_H,   VAR_IH,
>> +    VAR_OUT_W,  VAR_OW,
>> +    VAR_OUT_H,  VAR_OH,
>> +    VAR_A,
>> +    VAR_SAR,
>> +    VAR_DAR,
>> +    VAR_HSUB,
>> +    VAR_VSUB,
>> +    VAR_OHSUB,
>> +    VAR_OVSUB,
>> +    VARS_NB
>
>
> [...]
>
>> +static int scale_eval_dimensions(void *log_ctx,
>> +    const char *w_expr, const char *h_expr,
>> +    AVFilterLink *inlink, AVFilterLink *outlink,
>> +    int *ret_w, int *ret_h)
>> +{
>> +    const AVPixFmtDescriptor *desc = av_pix_fmt_desc_get(inlink->format);
>> +    const AVPixFmtDescriptor *out_desc = av_pix_fmt_desc_get(outlink->format);
>> +    const char *expr;
>> +    int w, h;
>> +    int factor_w, factor_h;
>> +    int eval_w, eval_h;
>> +    int ret;
>> +    const char scale2ref = outlink->src->nb_inputs == 2 && outlink->src->inputs[1] == inlink;
>> +    double var_values[VARS_NB + VARS_S2R_NB], res;
>> +    const AVPixFmtDescriptor *main_desc;
>> +    const AVFilterLink *main_link;
>> +    const char *const *names = scale2ref ? var_names_scale2ref : var_names;
>> +
>> +    if (scale2ref) {
>> +        main_link = outlink->src->inputs[0];
>> +        main_desc = av_pix_fmt_desc_get(main_link->format);
>> +    }
>> +
>> +    var_values[VAR_PI]    = M_PI;
>> +    var_values[VAR_PHI]   = M_PHI;
>> +    var_values[VAR_E]     = M_E;
>> +    var_values[VAR_IN_W]  = var_values[VAR_IW] = inlink->w;
>> +    var_values[VAR_IN_H]  = var_values[VAR_IH] = inlink->h;
>> +    var_values[VAR_OUT_W] = var_values[VAR_OW] = NAN;
>> +    var_values[VAR_OUT_H] = var_values[VAR_OH] = NAN;
>> +    var_values[VAR_A]     = (double) inlink->w / inlink->h;
>> +    var_values[VAR_SAR]   = inlink->sample_aspect_ratio.num ?
>> +        (double) inlink->sample_aspect_ratio.num / inlink->sample_aspect_ratio.den : 1;
>> +    var_values[VAR_DAR]   = var_values[VAR_A] * var_values[VAR_SAR];
>> +    var_values[VAR_HSUB]  = 1 << desc->log2_chroma_w;
>> +    var_values[VAR_VSUB]  = 1 << desc->log2_chroma_h;
>> +    var_values[VAR_OHSUB] = 1 << out_desc->log2_chroma_w;
>> +    var_values[VAR_OVSUB] = 1 << out_desc->log2_chroma_h;
>> +
>> +    if (scale2ref) {
>> +        var_values[VARS_NB + VAR_S2R_MAIN_W] = main_link->w;
>> +        var_values[VARS_NB + VAR_S2R_MAIN_H] = main_link->h;
>> +        var_values[VARS_NB + VAR_S2R_MAIN_A] = (double) main_link->w / main_link->h;
>> +        var_values[VARS_NB + VAR_S2R_MAIN_SAR] = main_link->sample_aspect_ratio.num ?
>> +            (double) main_link->sample_aspect_ratio.num / main_link->sample_aspect_ratio.den : 1;
>> +        var_values[VARS_NB + VAR_S2R_MAIN_DAR] = var_values[VARS_NB + VAR_S2R_MDAR] =
>> +            var_values[VARS_NB + VAR_S2R_MAIN_A] * var_values[VARS_NB + VAR_S2R_MAIN_SAR];
>> +        var_values[VARS_NB + VAR_S2R_MAIN_HSUB] = 1 << main_desc->log2_chroma_w;
>> +        var_values[VARS_NB + VAR_S2R_MAIN_VSUB] = 1 << main_desc->log2_chroma_h;
>> +    }
>> +
>> +    /* evaluate width and height */
>> +    av_expr_parse_and_eval(&res, (expr = w_expr),
>> +                           names, var_values,
>> +                           NULL, NULL, NULL, NULL, NULL, 0, log_ctx);
>> +    eval_w = var_values[VAR_OUT_W] = var_values[VAR_OW] = (int) res == 0 ? inlink->w : (int) res;
>> +
>> +    if ((ret = av_expr_parse_and_eval(&res, (expr = h_expr),
>> +                                      names, var_values,
>> +                                      NULL, NULL, NULL, NULL, NULL, 0, log_ctx)) < 0)
>> +        goto fail;
>> +    eval_h = var_values[VAR_OUT_H] = var_values[VAR_OH] = (int) res == 0 ? inlink->h : (int) res;
>> +    /* evaluate again the width, as it may depend on the output height */
>> +    if ((ret = av_expr_parse_and_eval(&res, (expr = w_expr),
>> +                                      names, var_values,
>> +                                      NULL, NULL, NULL, NULL, NULL, 0, log_ctx)) < 0)
>> +        goto fail;
>> +    eval_w = (int) res == 0 ? inlink->w : (int) res;
>> +
>> +    w = eval_w;
>> +    h = eval_h;
>> +
>> +    /* Check if it is requested that the result has to be divisible by a some
>> +     * factor (w or h = -n with n being the factor). */
>> +    factor_w = 1;
>> +    factor_h = 1;
>> +    if (w < -1) {
>> +        factor_w = -w;
>> +    }
>> +    if (h < -1) {
>> +        factor_h = -h;
>> +    }
>> +
>> +    if (w < 0 && h < 0) {
>> +        w = inlink->w;
>> +        h = inlink->h;
>> +    }
>> +
>> +    /* Make sure that the result is divisible by the factor we determined
>> +     * earlier. If no factor was set, it is nothing will happen as the default
>> +     * factor is 1 */
>> +    if (w < 0)
>> +        w = av_rescale(h, inlink->w, inlink->h * factor_w) * factor_w;
>> +    if (h < 0)
>> +        h = av_rescale(w, inlink->h, inlink->w * factor_h) * factor_h;
>> +
>> +    *ret_w = w;
>> +    *ret_h = h;
>> +
>> +    return 0;
>> +
>> +fail:
>> +    av_log(log_ctx, AV_LOG_ERROR,
>> +           "Error when evaluating the expression '%s'.\n"
>> +           "Maybe the expression for out_w:'%s' or for out_h:'%s' is self-referencing.\n",
>> +           expr, w_expr, h_expr);
>> +    return ret;
>> +}
> duplicated code
> it would be more ideal if no code duplication is required

This function is modified in next patch. This patch is only for 
convenient review, as you requested.

The existing scale.c function is still used by three hw filters., but it 
can't be used for the animation feature in the next patch since scale.c 
uses av_expr_parse_and_eval.
For animation, I need to retain the parsed expression, which means 
storing it the filter's private context..etc.

>> +
>>   static int config_props(AVFilterLink *outlink)
>>   {
>>       AVFilterContext *ctx = outlink->src;
>> @@ -231,7 +419,7 @@ static int config_props(AVFilterLink *outlink)
>>       int w, h;
>>       int ret;
>>   
>> -    if ((ret = ff_scale_eval_dimensions(ctx,
>> +    if ((ret =    scale_eval_dimensions(ctx,
>>                                           scale->w_expr, scale->h_expr,
>>                                           inlink, outlink,
>>                                           &w, &h)) < 0)
>> -- 
>> 2.24.0
>>   vf_scale.c |  466 ++++++++++++++++++++++++++++++++++++++++++-------------------
>>   1 file changed, 323 insertions(+), 143 deletions(-)
>> 942c421f637c3bde95f12b7c32b9c674e310d9c0  v2-0002-avfilter-scale-allow-dynamic-output-via-expressio.patch
>>  From 001a16725336b657a9b88c82ebca046bfa16341d Mon Sep 17 00:00:00 2001
>> From: Gyan Doshi <ffmpeg at gyani.pro>
>> Date: Thu, 14 Nov 2019 23:03:39 +0530
>> Subject: [PATCH v2 2/2] avfilter/scale: allow dynamic output via expressions
>>
>> At present, dynamic output can be had from scale and scale2ref filters
>> by sending commands or varying the resolution of the input (in scale) or
>> reference stream (in scale2ref). This is unwieldy when an interpolated
>> or otherwise animated output is desired.
>>
>> This commit introduces standard temporal variables that can be used in
>> width and height expressions, so dynamic output can be configured at
>> initialization or via commands.
>>
>> Such varying output ought to be sent to filters which can accommodate
>> them, such as the overlay filter.
>> ---
>>   libavfilter/vf_scale.c | 466 ++++++++++++++++++++++++++++-------------
>>   1 file changed, 323 insertions(+), 143 deletions(-)
>>
>> diff --git a/libavfilter/vf_scale.c b/libavfilter/vf_scale.c
>> index e7d52faccc..cbacd34827 100644
>> --- a/libavfilter/vf_scale.c
>> +++ b/libavfilter/vf_scale.c
>> @@ -42,9 +42,6 @@
>>   #include "libswscale/swscale.h"
>>   
>>   static const char *const var_names[] = {
>> -    "PI",
>> -    "PHI",
>> -    "E",
>>       "in_w",   "iw",
>>       "in_h",   "ih",
>>       "out_w",  "ow",
>> @@ -56,13 +53,23 @@ static const char *const var_names[] = {
>>       "vsub",
>>       "ohsub",
>>       "ovsub",
>> +    "n",
>> +    "t",
>> +    "pos",
>> +    "main_w",
>> +    "main_h",
>> +    "main_a",
>> +    "main_sar",
>> +    "main_dar", "mdar",
>> +    "main_hsub",
>> +    "main_vsub",
>> +    "main_n",
>> +    "main_t",
>> +    "main_pos",
>>       NULL
>>   };
>>   
>>   enum var_name {
>> -    VAR_PI,
>> -    VAR_PHI,
>> -    VAR_E,
>>       VAR_IN_W,   VAR_IW,
>>       VAR_IN_H,   VAR_IH,
>>       VAR_OUT_W,  VAR_OW,
>> @@ -74,42 +81,9 @@ enum var_name {
>>       VAR_VSUB,
>>       VAR_OHSUB,
>>       VAR_OVSUB,
>> -    VARS_NB
>> -};
>> -
>> -/**
>> - * This must be kept in sync with var_names so that it is always a
>> - * complete list of var_names with the scale2ref specific names
>> - * appended. scale2ref values must appear in the order they appear
>> - * in the var_name_scale2ref enum but also be below all of the
>> - * non-scale2ref specific values.
>> - */
>> -static const char *const var_names_scale2ref[] = {
>> -    "PI",
>> -    "PHI",
>> -    "E",
>> -    "in_w",   "iw",
>> -    "in_h",   "ih",
>> -    "out_w",  "ow",
>> -    "out_h",  "oh",
>> -    "a",
>> -    "sar",
>> -    "dar",
>> -    "hsub",
>> -    "vsub",
>> -    "ohsub",
>> -    "ovsub",
>> -    "main_w",
>> -    "main_h",
>> -    "main_a",
>> -    "main_sar",
>> -    "main_dar", "mdar",
>> -    "main_hsub",
>> -    "main_vsub",
>> -    NULL
>> -};
>> -
>> -enum var_name_scale2ref {
>> +    VAR_N,
>> +    VAR_T,
>> +    VAR_POS,
>>       VAR_S2R_MAIN_W,
>>       VAR_S2R_MAIN_H,
>>       VAR_S2R_MAIN_A,
>> @@ -117,7 +91,10 @@ enum var_name_scale2ref {
>>       VAR_S2R_MAIN_DAR, VAR_S2R_MDAR,
>>       VAR_S2R_MAIN_HSUB,
>>       VAR_S2R_MAIN_VSUB,
>> -    VARS_S2R_NB
>> +    VAR_S2R_MAIN_N,
>> +    VAR_S2R_MAIN_T,
>> +    VAR_S2R_MAIN_POS,
>> +    VARS_NB
>>   };
>>   
>>   enum EvalMode {
>> @@ -151,6 +128,10 @@ typedef struct ScaleContext {
>>   
>>       char *w_expr;               ///< width  expression string
>>       char *h_expr;               ///< height expression string
>> +    AVExpr *w_pexpr;
>> +    AVExpr *h_pexpr;
>> +    double var_values[VARS_NB];
>> +
>>       char *flags_str;
>>   
>>       char *in_color_matrix;
>> @@ -175,6 +156,8 @@ typedef struct ScaleContext {
>>   
>>   AVFilter ff_vf_scale2ref;
>>   
>> +static int check_exprs(AVFilterContext *ctx);
> The function could be put here avoiding the need for the forward declaration

Will do.

>> +
>>   static av_cold int init_dict(AVFilterContext *ctx, AVDictionary **opts)
>>   {
>>       ScaleContext *scale = ctx->priv;
>> @@ -186,8 +169,10 @@ static av_cold int init_dict(AVFilterContext *ctx, AVDictionary **opts)
>>               return AVERROR(EINVAL);
>>       }
>>   
>> -    if (scale->w_expr && !scale->h_expr)
>> +    if (scale->w_expr && !scale->h_expr) {
>> +        av_log(ctx, AV_LOG_INFO, "No height expression found. Treating width expression as video size.\n");
>>           FFSWAP(char *, scale->w_expr, scale->size_str);
>> +    }
>>   
>>       if (scale->size_str) {
>>           char buf[32];
>> @@ -206,6 +191,26 @@ static av_cold int init_dict(AVFilterContext *ctx, AVDictionary **opts)
>>       if (!scale->h_expr)
>>           av_opt_set(scale, "h", "ih", 0);
>>   
>> +    ret = av_expr_parse(&scale->w_pexpr, scale->w_expr,
>> +                            var_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);
>> +        return ret;
>> +    }
>> +
>> +    ret = av_expr_parse(&scale->h_pexpr, scale->h_expr,
>> +                            var_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);
>> +        return ret;
>> +    }
>> +
>> +    ret = check_exprs(ctx);
>> +    if (ret < 0)
>> +        return ret;
>> +
>>       av_log(ctx, AV_LOG_VERBOSE, "w:%s h:%s flags:'%s' interl:%d\n",
>>              scale->w_expr, scale->h_expr, (char *)av_x_if_null(scale->flags_str, ""), scale->interlaced);
>>   
>> @@ -228,6 +233,9 @@ static av_cold int init_dict(AVFilterContext *ctx, AVDictionary **opts)
>>   static av_cold void uninit(AVFilterContext *ctx)
>>   {
>>       ScaleContext *scale = ctx->priv;
>> +    av_expr_free(scale->w_pexpr);
>> +    av_expr_free(scale->h_pexpr);
>> +    scale->w_pexpr = scale->h_pexpr = NULL;
>>       sws_freeContext(scale->sws);
>>       sws_freeContext(scale->isws[0]);
>>       sws_freeContext(scale->isws[1]);
>> @@ -273,6 +281,45 @@ static int query_formats(AVFilterContext *ctx)
>>       return 0;
>>   }
>>   
>> +static int check_exprs(AVFilterContext *ctx)
>> +{
>> +    ScaleContext *scale = ctx->priv;
>> +
>> +    if (av_expr_count_var(scale->w_pexpr, VAR_OUT_W, VAR_OW+1)) {
>> +        av_log(ctx, AV_LOG_ERROR, "Width expression cannot be self-referencing: '%s'.\n", scale->w_expr);
>> +        return AVERROR(EINVAL);
>> +    }
> This is not very readable because it does not list the variables it counts
> instead it lists the first counted and the first not counted relying on
> the ordering in the enum.
Yes, it counts all variables in a  _range_. These calls could be 
replaced by a series of calls each counting one var only. Which is more 
readable and maintainable, indeed.
Will do that when switching to the counting method you propose below.

>> +
>> +    if (av_expr_count_var(scale->h_pexpr, VAR_OUT_H, VAR_OH+1)) {
> looking at how this is used here
> a simpler and cleaner API might be to pass an empty array to
> av_expr_count_var()
> and on return have a count for each variable in it.
>
> var_use_count[VAR_OUT_W] is much clearer what it refers to. The reader doesnt need
> to know the ordering of elements in the enum
Ok, will do it that way.

>> +        av_log(ctx, AV_LOG_ERROR, "Height expression cannot be self-referencing: '%s'.\n", scale->h_expr);
>> +        return AVERROR(EINVAL);
>> +    }
>> +
>> +    if (av_expr_count_var(scale->w_pexpr, VAR_OUT_H, VAR_OH+1) &&
>> +        av_expr_count_var(scale->h_pexpr, VAR_OUT_W, VAR_OW+1)) {
>> +        av_log(ctx, AV_LOG_ERROR, "Circular expressions invalid for width '%s' and height '%s'.\n", scale->w_expr, scale->h_expr);
>> +        return AVERROR(EINVAL);
>> +    }
>> +
>> +    if (ctx->filter != &ff_vf_scale2ref &&
>> +        (av_expr_count_var(scale->w_pexpr, VAR_S2R_MAIN_W, VARS_NB) ||
>> +         av_expr_count_var(scale->h_pexpr, VAR_S2R_MAIN_W, VARS_NB))) {
>> +        av_log(ctx, AV_LOG_ERROR, "Expressions with scale2ref variables are not valid in scale filter.\n");
>> +        return AVERROR(EINVAL);
>> +    }
>> +
>> +    if (scale->eval_mode == EVAL_MODE_INIT &&
>> +        (av_expr_count_var(scale->w_pexpr, VAR_N, VAR_POS+1) ||
>> +         av_expr_count_var(scale->w_pexpr, VAR_S2R_MAIN_N, VAR_S2R_MAIN_POS+1) ||
>> +         av_expr_count_var(scale->h_pexpr, VAR_N, VAR_POS+1) ||
>> +         av_expr_count_var(scale->h_pexpr, VAR_S2R_MAIN_N, VAR_S2R_MAIN_POS+1))) {
>> +        av_log(ctx, AV_LOG_ERROR, "Expressions with frame variables 'n', 't', 'pos' are not valid in init eval_mode.\n");
>> +        return AVERROR(EINVAL);
>> +    }
>> +
>> +    return 0;
>> +}
> Addition of expression checking should be independant of other features
> and so could be in its own patch
Ok.

>> +
>>   static const int *parse_yuv_type(const char *s, enum AVColorSpace colorspace)
>>   {
>>       if (!s)
>> @@ -297,74 +344,78 @@ static const int *parse_yuv_type(const char *s, enum AVColorSpace colorspace)
>>       return sws_getCoefficients(colorspace);
>>   }
>>   
>> -static int scale_eval_dimensions(void *log_ctx,
>> -    const char *w_expr, const char *h_expr,
>> -    AVFilterLink *inlink, AVFilterLink *outlink,
>> -    int *ret_w, int *ret_h)
>> +static int scale_eval_dimensions(AVFilterContext *ctx)
>>   {
>> +    ScaleContext *scale = ctx->priv;
>> +    const char scale2ref = ctx->filter == &ff_vf_scale2ref;
>> +    const AVFilterLink *inlink = scale2ref ? ctx->inputs[1] : ctx->inputs[0];
>> +    const AVFilterLink *outlink = ctx->outputs[0];
>>       const AVPixFmtDescriptor *desc = av_pix_fmt_desc_get(inlink->format);
>>       const AVPixFmtDescriptor *out_desc = av_pix_fmt_desc_get(outlink->format);
>> -    const char *expr;
>> +    char *expr;
>>       int w, h;
>>       int factor_w, factor_h;
>>       int eval_w, eval_h;
>>       int ret;
>> -    const char scale2ref = outlink->src->nb_inputs == 2 && outlink->src->inputs[1] == inlink;
>> -    double var_values[VARS_NB + VARS_S2R_NB], res;
>> +
>> +    double res;
>>       const AVPixFmtDescriptor *main_desc;
>>       const AVFilterLink *main_link;
>> -    const char *const *names = scale2ref ? var_names_scale2ref : var_names;
>>   
>>       if (scale2ref) {
>> -        main_link = outlink->src->inputs[0];
>> +        main_link = ctx->inputs[0];
>>           main_desc = av_pix_fmt_desc_get(main_link->format);
>>       }
>>   
>> -    var_values[VAR_PI]    = M_PI;
>> -    var_values[VAR_PHI]   = M_PHI;
>> -    var_values[VAR_E]     = M_E;
>> -    var_values[VAR_IN_W]  = var_values[VAR_IW] = inlink->w;
>> -    var_values[VAR_IN_H]  = var_values[VAR_IH] = inlink->h;
>> -    var_values[VAR_OUT_W] = var_values[VAR_OW] = NAN;
>> -    var_values[VAR_OUT_H] = var_values[VAR_OH] = NAN;
>> -    var_values[VAR_A]     = (double) inlink->w / inlink->h;
>> -    var_values[VAR_SAR]   = inlink->sample_aspect_ratio.num ?
>> +    scale->var_values[VAR_IN_W]  = scale->var_values[VAR_IW] = inlink->w;
>> +    scale->var_values[VAR_IN_H]  = scale->var_values[VAR_IH] = inlink->h;
>> +    scale->var_values[VAR_OUT_W] = scale->var_values[VAR_OW] = NAN;
>> +    scale->var_values[VAR_OUT_H] = scale->var_values[VAR_OH] = NAN;
>> +    scale->var_values[VAR_A]     = (double) inlink->w / inlink->h;
>> +    scale->var_values[VAR_SAR]   = inlink->sample_aspect_ratio.num ?
>>           (double) inlink->sample_aspect_ratio.num / inlink->sample_aspect_ratio.den : 1;
>> -    var_values[VAR_DAR]   = var_values[VAR_A] * var_values[VAR_SAR];
>> -    var_values[VAR_HSUB]  = 1 << desc->log2_chroma_w;
>> -    var_values[VAR_VSUB]  = 1 << desc->log2_chroma_h;
>> -    var_values[VAR_OHSUB] = 1 << out_desc->log2_chroma_w;
>> -    var_values[VAR_OVSUB] = 1 << out_desc->log2_chroma_h;
>> +    scale->var_values[VAR_DAR]   = scale->var_values[VAR_A] * scale->var_values[VAR_SAR];
>> +    scale->var_values[VAR_HSUB]  = 1 << desc->log2_chroma_w;
>> +    scale->var_values[VAR_VSUB]  = 1 << desc->log2_chroma_h;
>> +    scale->var_values[VAR_OHSUB] = 1 << out_desc->log2_chroma_w;
>> +    scale->var_values[VAR_OVSUB] = 1 << out_desc->log2_chroma_h;
>>   
>>       if (scale2ref) {
>> -        var_values[VARS_NB + VAR_S2R_MAIN_W] = main_link->w;
>> -        var_values[VARS_NB + VAR_S2R_MAIN_H] = main_link->h;
>> -        var_values[VARS_NB + VAR_S2R_MAIN_A] = (double) main_link->w / main_link->h;
>> -        var_values[VARS_NB + VAR_S2R_MAIN_SAR] = main_link->sample_aspect_ratio.num ?
>> +        scale->var_values[VAR_S2R_MAIN_W] = main_link->w;
>> +        scale->var_values[VAR_S2R_MAIN_H] = main_link->h;
>> +        scale->var_values[VAR_S2R_MAIN_A] = (double) main_link->w / main_link->h;
>> +        scale->var_values[VAR_S2R_MAIN_SAR] = main_link->sample_aspect_ratio.num ?
>>               (double) main_link->sample_aspect_ratio.num / main_link->sample_aspect_ratio.den : 1;
>> -        var_values[VARS_NB + VAR_S2R_MAIN_DAR] = var_values[VARS_NB + VAR_S2R_MDAR] =
>> -            var_values[VARS_NB + VAR_S2R_MAIN_A] * var_values[VARS_NB + VAR_S2R_MAIN_SAR];
>> -        var_values[VARS_NB + VAR_S2R_MAIN_HSUB] = 1 << main_desc->log2_chroma_w;
>> -        var_values[VARS_NB + VAR_S2R_MAIN_VSUB] = 1 << main_desc->log2_chroma_h;
>> +        scale->var_values[VAR_S2R_MAIN_DAR] = scale->var_values[VAR_S2R_MDAR] =
>> +            scale->var_values[VAR_S2R_MAIN_A] * scale->var_values[VAR_S2R_MAIN_SAR];
>> +        scale->var_values[VAR_S2R_MAIN_HSUB] = 1 << main_desc->log2_chroma_w;
>> +        scale->var_values[VAR_S2R_MAIN_VSUB] = 1 << main_desc->log2_chroma_h;
>>       }
>>   
>> -    /* evaluate width and height */
>> -    av_expr_parse_and_eval(&res, (expr = w_expr),
>> -                           names, var_values,
>> -                           NULL, NULL, NULL, NULL, NULL, 0, log_ctx);
>> -    eval_w = var_values[VAR_OUT_W] = var_values[VAR_OW] = (int) res == 0 ? inlink->w : (int) res;
>> +    if (!av_expr_count_var(scale->w_pexpr, VAR_OUT_H, VAR_OH+1)) {
>> +        res = av_expr_eval(scale->w_pexpr, scale->var_values, NULL);
>> +        if (isnan(res)) {
>> +        expr = scale->w_expr;
>> +            goto fail;
>> +        }
>> +        eval_w = scale->var_values[VAR_OUT_W] = scale->var_values[VAR_OW] = (int) res == 0 ? inlink->w : (int) res;
>> +    }
>>   
>> -    if ((ret = av_expr_parse_and_eval(&res, (expr = h_expr),
>> -                                      names, var_values,
>> -                                      NULL, NULL, NULL, NULL, NULL, 0, log_ctx)) < 0)
>> +    res = av_expr_eval(scale->h_pexpr, scale->var_values, NULL);
>> +    if (isnan(res)) {
>> +        expr = scale->h_expr;
>>           goto fail;
>> -    eval_h = var_values[VAR_OUT_H] = var_values[VAR_OH] = (int) res == 0 ? inlink->h : (int) res;
>> -    /* evaluate again the width, as it may depend on the output height */
>> -    if ((ret = av_expr_parse_and_eval(&res, (expr = w_expr),
>> -                                      names, var_values,
>> -                                      NULL, NULL, NULL, NULL, NULL, 0, log_ctx)) < 0)
>> -        goto fail;
>> -    eval_w = (int) res == 0 ? inlink->w : (int) res;
>> +    }
>> +    eval_h = scale->var_values[VAR_OUT_H] = scale->var_values[VAR_OH] = (int) res == 0 ? inlink->h : (int) res;
>> +
>> +    if (av_expr_count_var(scale->w_pexpr, VAR_OUT_H, VAR_OH+1)) {
>> +        res = av_expr_eval(scale->w_pexpr, scale->var_values, NULL);
>> +        if (isnan(res)) {
>> +        expr = scale->w_expr;
>> +            goto fail;
>> +        }
>> +        eval_w = scale->var_values[VAR_OUT_W] = scale->var_values[VAR_OW] = (int) res == 0 ? inlink->w : (int) res;
>> +    }
>>   
>>       w = eval_w;
>>       h = eval_h;
> I assume its intended to do this with more filters
> If we now consider a filter which has more than 2 output variables
> maybe width, height, xpos, ypos as in crop.
> thats 4 outputs and thats 24 orderings to evaluate them. The method
> used above would not work in that case
I know.  With more factors, some form of round robin evaluation order 
will have to determined beforehand, but I don't think it is required in 
this case.

> [...]
>> @@ -548,9 +598,6 @@ static int config_props(AVFilterLink *outlink)
>>              outlink->sample_aspect_ratio.num, outlink->sample_aspect_ratio.den,
>>              scale->flags);
>>       return 0;
>> -
>> -fail:
>> -    return ret;
> this is unrelated to the patchset

Will factorize.

>>   }
>>   
>>   static int config_props_ref(AVFilterLink *outlink)
>> @@ -600,30 +647,68 @@ static int scale_slice(AVFilterLink *link, AVFrame *out_buf, AVFrame *cur_pic, s
>>                            out,out_stride);
>>   }
>>   
>> +#define TS2T(ts, tb) ((ts) == AV_NOPTS_VALUE ? NAN : (double)(ts) * av_q2d(tb))
>> +
>>   static int scale_frame(AVFilterLink *link, AVFrame *in, AVFrame **frame_out)
>>   {
>> -    ScaleContext *scale = link->dst->priv;
>> +    AVFilterContext *ctx = link->dst;
>> +    ScaleContext *scale = ctx->priv;
>>       AVFilterLink *outlink = link->dst->outputs[0];
>>       AVFrame *out;
>>       const AVPixFmtDescriptor *desc = av_pix_fmt_desc_get(link->format);
>>       char buf[32];
>>       int in_range;
>> +    int frame_changed;
>>   
>>       *frame_out = NULL;
>>       if (in->colorspace == AVCOL_SPC_YCGCO)
>>           av_log(link->dst, AV_LOG_WARNING, "Detected unsupported YCgCo colorspace.\n");
>>   
>> -    if (  in->width  != link->w
>> -       || in->height != link->h
>> -       || in->format != link->format
>> -       || in->sample_aspect_ratio.den != link->sample_aspect_ratio.den || in->sample_aspect_ratio.num != link->sample_aspect_ratio.num) {
>> +
>> +    frame_changed = in->width  != link->w ||
>> +                    in->height != link->h ||
>> +                    in->format != link->format ||
>> +                    in->sample_aspect_ratio.den != link->sample_aspect_ratio.den ||
>> +                    in->sample_aspect_ratio.num != link->sample_aspect_ratio.num;
>> +
>> +    if (scale->eval_mode == EVAL_MODE_FRAME || frame_changed) {
>>           int ret;
> This change can be factored out in a seperate factorization patch

Will do.

Thanks,
Gyan


More information about the ffmpeg-devel mailing list