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

[Stefano Sabatini]

On date Wednesday 2010-07-07 15:53:16 -0400, Daniel G. Taylor encoded:
> On 07/01/2010 10:27 AM, Stefano Sabatini wrote:
> >>Basically, I don't know how I can do this easily, and writing a
> >>bunch of code doing it a stupid way seems like a waste of time, so
> >>any advice would be appreciated if we do want to go this way.
> >
> >libavutil/eval.h, check how it is done in the libavfilter-soc overlay
> >and setpts filters. This requires to add eval evalutation to the scale
> >filter, check the recent patch by Baptiste on which I commented
> >yesterday.
> 
> Thanks for the help here Stefano.
> 
> I've run into a bit of a snag I'd say... See attached patch to
> reproduce what I'm running into, but basically:
> 
>  * av_parse_expr and such use a comma as a delimiter for function
>    arguments
>  * avfilter's graph parsing uses a comma as a delimiter for filters
> 
> Because of that it means anytime you put a comma into the filter
> arguments it thinks you are passing in the next filter. If we are
> going to allow functions and such as filter arguments it might be a
> good idea to change the syntax for specifying filters, maybe using a
> pipe character like "|" or something, e.g. -vf
> "scale|unsharp|...|crop".
> 
> Attached patch works for stuff like -vf "scale=320:240" or -vf
> "scale=W/2:H/2" and such but if you try -vf "scale=W:round(H, 16)"
> you'll immediately see the issue.
> 
> Thoughts?

Possible solutions:

1) change the character currently used, which is ",", and which is
very likely to appear in expressions inside the filter args.

'|' is an option, note that we didn't choose '|' in the first place
because it's a special sh character, for example this doesn't work in
sh:
-vf scale|unsharp|crop

you need:
-vf "scale|unsharp|crop"

For me this doesn't look like a big issue, but we should wart the
users in the docs.

The requirement is that the choosen character cannot compare inside
the filter args.

2) make the separator definable by the user, for example we could have
something like:
-vfd "%" -vf "scale % unsharp % ... % crop".

3) Implement an escaping mechanism. Since at the first level of
escaping there is just one special character, we may use '\' to escape
the ',', for example:

scale="W:round(H\, 16), unsharp, crop"

The problem with this approach is that combined with the shell
escaping mechanism this can easily became an escaping hell.

So I tend to prefer the option 2, and use by default the value "|" for
the separator.
 
> 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,9 +25,41 @@
>   */
>  
>  #include "avfilter.h"
> +#include "libavutil/eval.h"
> +#include "libavutil/mem.h"
>  #include "libavutil/pixdesc.h"
>  #include "libswscale/swscale.h"
>  
> +static const char *const_names[]={
> +    "W",
> +    "H",
> +    NULL
> +};

While at it add also some nice constant, PI, E, and convince Michael
that PHI is cool to have in FFmpeg ;-).

> +
> +enum PosOfValue {
> +    POV_W,
> +    POV_H,
> +    POV_NULL
> +};

PosOfValue => poor name, you choose a bad example (setpts rather than
overlay ;-))

> +
> +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[]={
> +    "round",
> +    NULL
> +};
> +
> +static double (* const *funcs2[])(void *, double, double)={
> +    (void *)round_clip,
> +    NULL
> +};
> +
>  typedef struct {
>      struct SwsContext *sws;     ///< software scaler context
>  
> @@ -41,27 +74,44 @@
>      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 *wexpr;                   ///< width expression parser
> +    AVExpr *hexpr;                   ///< height expression parser

is not a parser, I'd say just "expression", also w_expr/h_expr is more
readable IMO

> +    double const_values[POV_NULL+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 *cp, *width, *height;
> +    int ret;
> +    char *save_ptr = NULL;
>  
>      scale->flags = SWS_BILINEAR;

>      if (args){
> -        sscanf(args, "%d:%d", &scale->w, &scale->h);
> +        cp = av_strdup(args);
> +        width = strtok_r(cp, ":", &save_ptr);
> +        height = strtok_r(NULL, ":", &save_ptr);
>          p= strstr(args,"flags=");
>          if(p) scale->flags= strtoul(p+6, NULL, 0);
>      }

why not use sscanf ("%255[^:]:%255[^:]") ? Looks simpler.

Also this is leaking cp, and you're forgetting to assign the default
expression to w and h.

As for strtok_r, since it is not C99 I believe we may need to add a
check for that in the configure (I didn't know that strtok was not
re-entrant, is that really true?).

> -
> -    /* 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->wexpr, args ? width : "W",
> +                        const_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->hexpr, args ? height : "H",
> +                        const_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;
> +    }
>  
>      return 0;
>  }
> @@ -109,6 +159,12 @@
>      AVFilterLink *inlink = outlink->src->inputs[0];
>      ScaleContext *scale = ctx->priv;
>      int64_t w, h;
> +    
> +    scale->const_values[POV_W] = inlink->w;
> +    scale->const_values[POV_H] = inlink->h;
> +    

> +    scale->w = av_eval_expr(scale->wexpr, scale->const_values, scale);
> +    scale->h = av_eval_expr(scale->hexpr, scale->const_values, scale);

Maybe we should add a check here: !is_nan(av_eval_expr(scale->w_expr, ...))

>      if (!(w = scale->w))
>          w = inlink->w;

This can be removed once you assign the default expression.

Thanks, regards,
-- 
FFmpeg = Fierce & Freak Meaningless Powered Embarassing God