[FFmpeg-devel] [Patch] Scale filter should use multiples of 2

[Stefano Sabatini]

On date Thursday 2010-07-22 11:33:10 -0400, Daniel G. Taylor encoded:
> On 07/21/2010 08:04 PM, Stefano Sabatini wrote:
>> Ehm, after further inspection of the code I discovered that the graph
>> parser *already* supports escaping (through the av_get_token()
>> function).
>>
>> So for the particular case above:
>> "scale=W:round(H\, 16)"
>>
>> or even:
>> 'scale=W:round(H, 16)'
>
> Okay, awesome. I've confirmed that works, along with stuff like:
>
> ffmpeg -i in.mp4 -vf "scale=PI*100:round(PI*100/A\, 16)" out.mp4
>
> I've also fixed all the comments from the other mail, and as you can see  
> above added PI and E. Updated patch is attached. There are now two  
> issues I'm a little stuck with and would like your thoughts on:
>
>   * With zero arguments it crashes. It appears 'flags=0x...' is
>     magically appended to args and I have no idea where and what
>     is going on. Can one of you C string parsing gurus take a look
>     at the best way to parse the args here?

This is automatically added by ffmpeg or the graphparser, that's an
hack we use to propagate the scaling flags to use.

I suppose we may fix that *requiring* a syntax of the kind:
W:H:FLAGS

rather than try to intercept the flags= string.

Anyway I'd rather expect the filter to issue a configuration error,
further debugging information is required.

>   * The special case of -1 can't work within the expression parser, so
>     if you want to round e.g. scale=320:-1 you now would have to
>     do "scale=320:round(320/H\, 16)". Any way to fix this or is
>     it okay this way?

We may keep the interpretation of -1 as a special value, but also
using W2/H1 looks fine to me (and imho less surprising for a new
user).

> Take care,
> -- 
> Daniel G. Taylor
> http://programmer-art.org/

> Index: libavfilter/vf_scale.c
> ===================================================================
> --- libavfilter/vf_scale.c	(revision 24077)
> +++ libavfilter/vf_scale.c	(working copy)
> @@ -1,5 +1,6 @@
>  /*
>   * copyright (c) 2007 Bobby Bingham
> + * copyright (c) 2010 Daniel G. Taylor <dan at programmer-art.org>
>   *
>   * This file is part of FFmpeg.
>   *
> @@ -24,44 +25,91 @@
>   */
>  
>  #include "avfilter.h"
> +#include "libavutil/eval.h"
>  #include "libavutil/pixdesc.h"
>  #include "libswscale/swscale.h"

> +static const char *var_names[]={
> +    "PI",
> +    "E",
> +    "W",
> +    "H",
> +    "A",
> +    NULL
> +};
> +
> +enum var_name {
> +    PI,
> +    E,
> +    W,
> +    H,
> +    A,
> +    VARS_NB
> +};

alphabetical order, avoid potential bugs.

> +
> +static inline double round_clip(void *ctx, double arg1, double arg2)
> +{
> +    int orig = (int)arg1;
> +    int clip = (int)arg2;
> +    
> +    return orig -= (orig % clip < clip / 2) ? orig % clip : -(clip - (orig % clip));
> +}
> +

> +static const char *func2_names[]={

nit: _=_{

> +    "round",
> +    NULL
> +};
> +
>
> +static double (* const *funcs2[])(void *, double, double)={
> +    (void *)round_clip,
> +    NULL
> +};
> +
>  typedef struct {
>      struct SwsContext *sws;     ///< software scaler context
>  
> -    /**
> -     * New dimensions. Special values are:
> -     *   0 = original width/height
> -     *  -1 = keep original aspect
> -     */
> -    int w, h;
> -    unsigned int flags;         ///sws flags
> +    int w, h;                   ///< new dimensions
> +    unsigned int flags;         ///< sws flags
>  
>      int hsub, vsub;             ///< chroma subsampling
>      int slice_y;                ///< top of current output slice
>      int input_is_pal;           ///< set to 1 if the input format is paletted
> +    
> +    AVExpr *w_expr;                   ///< width expression
> +    AVExpr *h_expr;                   ///< height expression
> +    double var_values[VARS_NB+1];     ///< expression parsing constants
>  } ScaleContext;
>  
>  static av_cold int init(AVFilterContext *ctx, const char *args, void *opaque)
>  {
>      ScaleContext *scale = ctx->priv;
>      const char *p;
> +    char width[255] = "W", height[255] = "H";
> +    int ret;
> +    char *save_ptr = NULL;
>  
>      scale->flags = SWS_BILINEAR;

>      if (args){
> -        sscanf(args, "%d:%d", &scale->w, &scale->h);
> +        sscanf(args, "%255[^:]:%255[^:]", &width, &height);
>          p= strstr(args,"flags=");
>          if(p) scale->flags= strtoul(p+6, NULL, 0);
>      }

this fails if you have the string: flags=.. appended to args.

I believe the best solution would be to change the scale syntax to
W:H:FLAGS, thoughts?

> -
> -    /* sanity check params */
> -    if (scale->w <  -1 || scale->h <  -1) {
> -        av_log(ctx, AV_LOG_ERROR, "Size values less than -1 are not acceptable.\n");
> -        return -1;
> +    

> +    ret = av_parse_expr(&scale->w_expr, width, var_names, NULL, NULL,
> +                        func2_names, funcs2, 0, ctx);
> +    if (ret < 0) {
> +        av_log(ctx, AV_LOG_ERROR,
> +               "Error while parsing width expression '%s'\n", width);
> +        return ret;
>      }
> -    if (scale->w == -1 && scale->h == -1)
> -        scale->w = scale->h = 0;
> +    
> +    ret = av_parse_expr(&scale->h_expr, height, var_names, NULL, NULL,
> +                        func2_names, funcs2, 0, ctx);
> +    if (ret < 0) {
> +        av_log(ctx, AV_LOG_ERROR,
> +               "Error while parsing height expression '%s'\n", height);
> +        return ret;
> +    }

code duplication, look at how it's done in the libavfilter soc overlay
filter to avoid it.

>  
>      return 0;
>  }
> @@ -109,11 +157,19 @@
>      AVFilterLink *inlink = outlink->src->inputs[0];
>      ScaleContext *scale = ctx->priv;
>      int64_t w, h;

> +    
> +    scale->var_values[PI] = M_PI;
> +    scale->var_values[E] = M_E;
> +    scale->var_values[W] = inlink->w;
> +    scale->var_values[H] = inlink->h;
> +    scale->var_values[A] = (float) inlink->w / inlink->h;

vertical align.

> +    
> +    scale->w = av_eval_expr(scale->w_expr, scale->var_values, scale);
> +    scale->h = av_eval_expr(scale->h_expr, scale->var_values, scale);

BTW in order to avoid to parse and eval as two separate operations as
you need to evaluate the expression just once, you may just use here
av_parse_and_eval_expr(), that may simplify the code a bit.

>  
> -    if (!(w = scale->w))
> -        w = inlink->w;
> -    if (!(h = scale->h))
> -        h = inlink->h;
> +    w = scale->w;
> +    h = scale->h;
> +    
>      if (w == -1)
>          w = av_rescale(h, inlink->w, inlink->h);
>      if (h == -1)

Regards.
-- 
FFmpeg = Furious and Fundamental Multipurpose Programmable Elaborated Gigant