[FFmpeg-devel] [PATCH] lavfi/aspect: ripristinate ratio parsing

[Clément Bœsch]

On Tue, Apr 16, 2013 at 10:39:19PM +0200, Stefano Sabatini wrote:
> Allow to set a ratio as "a:b" (with proper escaping), and correctly
> honour the max parameter.
> ---
>  libavfilter/vf_aspect.c |   29 +++++++++++++++++++----------
>  1 file changed, 19 insertions(+), 10 deletions(-)
> 
> diff --git a/libavfilter/vf_aspect.c b/libavfilter/vf_aspect.c
> index 2b9ba15..e1f616b 100644
> --- a/libavfilter/vf_aspect.c
> +++ b/libavfilter/vf_aspect.c
> @@ -42,22 +42,31 @@ typedef struct {
>  #if FF_API_OLD_FILTER_OPTS
>      float aspect_num, aspect_den;
>  #endif
> +    char *ratio_str;
>  } AspectContext;
>  
> -#if FF_API_OLD_FILTER_OPTS
>  static av_cold int init(AVFilterContext *ctx)
>  {
>      AspectContext *s = ctx->priv;
> +    int ret;
>  
> +#if FF_API_OLD_FILTER_OPTS

If you do that, you need to remove it around the .init in avfilter_vf_*,
otherwise you'll get a surprise at next bump.

>      if (s->aspect_num > 0 && s->aspect_den > 0) {
>          av_log(ctx, AV_LOG_WARNING,
>                 "num:den syntax is deprecated, please use num/den or named options instead\n");
>          s->aspect = av_d2q(s->aspect_num / s->aspect_den, s->max);
>      }
> -
> +#endif
> +    if (s->ratio_str) {
> +        ret = av_parse_ratio(&s->aspect, s->ratio_str, s->max, 0, ctx);
> +        if (ret < 0 || s->aspect.num < 0 || s->aspect.den <= 0) {
> +            av_log(ctx, AV_LOG_ERROR,
> +                   "Invalid string '%s' for aspect ratio\n", s->ratio_str);
> +            return AVERROR(EINVAL);
> +        }
> +    }
>      return 0;
>  }
> -#endif
>  
>  static int filter_frame(AVFilterLink *link, AVFrame *frame)
>  {
> @@ -80,7 +89,7 @@ static int setdar_config_props(AVFilterLink *inlink)
>      if (aspect->aspect.num && aspect->aspect.den) {
>          av_reduce(&aspect->aspect.num, &aspect->aspect.den,
>                     aspect->aspect.num * inlink->h,

> -                   aspect->aspect.den * inlink->w, 100);
> +                   aspect->aspect.den * inlink->w, aspect->max);

This sounds like a bug I introduced and partially fixed in 7bd014ea. If
you could commit this separately it would be nice.

>          inlink->sample_aspect_ratio = aspect->aspect;
>      } else {
>          inlink->sample_aspect_ratio = (AVRational){ 1, 1 };
> @@ -99,9 +108,9 @@ static const AVOption setdar_options[] = {
>      { "dar_num", NULL, OFFSET(aspect_num), AV_OPT_TYPE_FLOAT, { .dbl = 0 }, 0, FLT_MAX, FLAGS },
>      { "dar_den", NULL, OFFSET(aspect_den), AV_OPT_TYPE_FLOAT, { .dbl = 0 }, 0, FLT_MAX, FLAGS },
>  #endif
> -    { "dar",   "set display aspect ratio", OFFSET(aspect), AV_OPT_TYPE_RATIONAL, {.dbl=0}, 0, INT_MAX, FLAGS },
> -    { "ratio", "set display aspect ratio", OFFSET(aspect), AV_OPT_TYPE_RATIONAL, {.dbl=0}, 0, INT_MAX, FLAGS },
> -    { "r",     "set display aspect ratio", OFFSET(aspect), AV_OPT_TYPE_RATIONAL, {.dbl=0}, 0, INT_MAX, FLAGS },
> +    { "dar",   "set display aspect ratio", OFFSET(ratio_str), AV_OPT_TYPE_STRING, {.str="0"}, .flags=FLAGS },
> +    { "ratio", "set display aspect ratio", OFFSET(ratio_str), AV_OPT_TYPE_STRING, {.str="0"}, .flags=FLAGS },
> +    { "r",     "set display aspect ratio", OFFSET(ratio_str), AV_OPT_TYPE_STRING, {.str="0"}, .flags=FLAGS },
>      { "max",   "set max value for nominator or denominator in the ratio", OFFSET(max), AV_OPT_TYPE_INT, {.i64=100}, 1, INT_MAX, FLAGS },
>      { NULL }
>  };
> @@ -162,9 +171,9 @@ static const AVOption setsar_options[] = {
>      { "sar_num", NULL, OFFSET(aspect_num), AV_OPT_TYPE_FLOAT, { .dbl = 0 }, 0, FLT_MAX, FLAGS },
>      { "sar_den", NULL, OFFSET(aspect_den), AV_OPT_TYPE_FLOAT, { .dbl = 0 }, 0, FLT_MAX, FLAGS },
>  #endif
> -    { "sar",   "set sample (pixel) aspect ratio", OFFSET(aspect), AV_OPT_TYPE_RATIONAL, {.dbl=0}, 0, INT_MAX, FLAGS },
> -    { "ratio", "set sample (pixel) aspect ratio", OFFSET(aspect), AV_OPT_TYPE_RATIONAL, {.dbl=0}, 0, INT_MAX, FLAGS },
> -    { "r",     "set sample (pixel) aspect ratio", OFFSET(aspect), AV_OPT_TYPE_RATIONAL, {.dbl=0}, 0, INT_MAX, FLAGS },
> +    { "sar",   "set sample (pixel) aspect ratio", OFFSET(ratio_str), AV_OPT_TYPE_STRING, {.str="0"}, .flags=FLAGS },
> +    { "ratio", "set sample (pixel) aspect ratio", OFFSET(ratio_str), AV_OPT_TYPE_STRING, {.str="0"}, .flags=FLAGS },
> +    { "r",     "set sample (pixel) aspect ratio", OFFSET(ratio_str), AV_OPT_TYPE_STRING, {.str="0"}, .flags=FLAGS },
>      { "max",   "set max value for nominator or denominator in the ratio", OFFSET(max), AV_OPT_TYPE_INT, {.i64=100}, 1, INT_MAX, FLAGS },
>      { NULL }
>  };

Could you make one of the FATE test use the ':' form?

Rest LGTM, thanks.

-- 
Clément B.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 490 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20130416/5ec5f563/attachment.asc>