[FFmpeg-devel] [PATCH] lavfi/scale: accept named option, make parsing more robust

[Michael Niedermayer]

On Sun, Oct 28, 2012 at 05:33:37PM +0100, Stefano Sabatini wrote:
> On date Thursday 2012-10-25 06:05:50 +0200, Michael Niedermayer encoded:
> > On Thu, Oct 25, 2012 at 12:21:20AM +0200, Stefano Sabatini wrote:
> > > On date Saturday 2012-10-20 17:34:43 +0200, Michael Niedermayer encoded:
> > > > On Sat, Oct 20, 2012 at 12:30:31PM +0200, Stefano Sabatini wrote:
> > > > > On date Monday 2012-10-15 09:46:52 +0200, Stefano Sabatini encoded:
> > > > > > On date Saturday 2012-10-06 17:21:39 +0200, Michael Niedermayer encoded:
> > > > > [...]
> > > > > > > missing static and missing null termination
> > > > > > 
> > > > > > Fixed.
> > > > > > 
> > > > > > Patch updated.
> > > > > 
> > > > > Ping.
> > > > 
> > > > it still breaks complex filters
> > > 
> > > Do you have a command that make it fail (and which is not already
> > > broken without the patch)?
> > 
> > i seems i cannot reproduce this issue anymore but fate does not pass
> > with the patch
> 
> Updated.
> -- 
> FFmpeg = Frenzy Funny Meaningful Proud Elitist Guru

>  doc/filters.texi       |   62 ++++++++++++++++++++++++++++++++-------------
>  libavfilter/vf_scale.c |   67 +++++++++++++++++++++++++++++++------------------
>  2 files changed, 87 insertions(+), 42 deletions(-)
> 19343915a53d381a9b23cd143864eba4592d5ab2  0001-lavfi-scale-accept-named-option-make-parsing-more-ro.patch
> From d85c7ea19360cb373e8d81a9923f14559dce47a4 Mon Sep 17 00:00:00 2001
> From: Stefano Sabatini <stefasab at gmail.com>
> Date: Sat, 6 Oct 2012 12:30:26 +0200
> Subject: [PATCH] lavfi/scale: accept named option, make parsing more robust
> 
> Also update documentation accordingly.
> 
> TODO: bump micro
[...]
> @@ -76,7 +77,8 @@ typedef struct {
>       *  -1 = keep original aspect
>       */
>      int w, h;
> +    const char *flags_str;      ///sws flags string


> -    unsigned int flags;         ///sws flags
> +    int         flags;          ///sws flags

this doesnt look correct


>  
>      int hsub, vsub;             ///< chroma subsampling
>      int slice_y;                ///< top of current output slice
> @@ -84,35 +86,48 @@ typedef struct {
>      int output_is_pal;          ///< set to 1 if the output format is paletted
>      int interlaced;
>  
> -    char w_expr[256];           ///< width  expression string
> -    char h_expr[256];           ///< height expression string
> +    char *w_expr;               ///< width  expression string
> +    char *h_expr;               ///< height expression string
>  } ScaleContext;
>  
> +#define OFFSET(x) offsetof(ScaleContext, x)
> +#define FLAGS AV_OPT_FLAG_VIDEO_PARAM|AV_OPT_FLAG_FILTERING_PARAM
> +
> +static const AVOption scale_options[] = {
> +    { "w",      "set width expression",    OFFSET(w_expr), AV_OPT_TYPE_STRING, {.str = "iw"}, 0, 0, FLAGS },
> +    { "width",  "set width expression",    OFFSET(w_expr), AV_OPT_TYPE_STRING, {.str = "iw"}, 0, 0, FLAGS },
> +    { "h",      "set height expression",   OFFSET(h_expr), AV_OPT_TYPE_STRING, {.str = "ih"}, 0, 0, FLAGS },
> +    { "height", "set height expression",   OFFSET(h_expr), AV_OPT_TYPE_STRING, {.str = "ih"}, 0, 0, FLAGS },
> +    { "flags",  "set libswscale flags",    OFFSET(flags_str), AV_OPT_TYPE_STRING, {.str = NULL}, 0, INT_MAX, FLAGS },
> +    { "interl", "set interlacing", OFFSET(interlaced), AV_OPT_TYPE_INT, {.i64 = 0 }, -1, 1, FLAGS },
> +    { NULL },
> +};
> +
> +AVFILTER_DEFINE_CLASS(scale);
> +
>  static av_cold int init(AVFilterContext *ctx, const char *args)
>  {
>      ScaleContext *scale = ctx->priv;
> -    const char *p;
> +    static const char *shorthand[] = { "w", "h", NULL };
> +    int ret;
>  
> -    av_strlcpy(scale->w_expr, "iw", sizeof(scale->w_expr));
> -    av_strlcpy(scale->h_expr, "ih", sizeof(scale->h_expr));
> +    scale->class = &scale_class;
> +    av_opt_set_defaults(scale);
> +
> +    if ((ret = av_opt_set_from_string(scale, args, shorthand, "=", ":")) < 0)
> +        return ret;
> +
> +    av_log(ctx, AV_LOG_VERBOSE, "w:%s h:%s flags:%s interl:%d\n",
> +           scale->w_expr, scale->h_expr, scale->flags_str, scale->interlaced);
>  
>      scale->flags = SWS_BILINEAR;
> -    if (args) {
> -        sscanf(args, "%255[^:]:%255[^:]", scale->w_expr, scale->h_expr);
> -        p = strstr(args,"flags=");
> -        if (p) {
> -            const AVClass *class = sws_get_class();
> -            const AVOption    *o = av_opt_find(&class, "sws_flags", NULL, 0,
> -                                               AV_OPT_SEARCH_FAKE_OBJ);
> -            int ret = av_opt_eval_flags(&class, o, p + 6, &scale->flags);
> -
> -            if (ret < 0)
> -                return ret;
> -        }
> -        if(strstr(args,"interl=1")){
> -            scale->interlaced=1;
> -        }else if(strstr(args,"interl=-1"))
> -            scale->interlaced=-1;
> +    if (scale->flags_str) {
> +        const AVClass *class = sws_get_class();
> +        const AVOption    *o = av_opt_find(&class, "sws_flags", NULL, 0,
> +                                           AV_OPT_SEARCH_FAKE_OBJ);
> +        int ret = av_opt_eval_flags(&class, o, scale->flags_str, &scale->flags);
> +        if (ret < 0)
> +            return ret;
>      }

I think it would be better if all options would be sent straight to
sws instead of being parsed into a local AVClass based context.
later requires every option to be duplicated and be kept in sync.
also this fails with multiple (sws_)flags options that is like
flags=+abc:flags=-abc

if w/h need special handling then they could be intercepted and
handlded special instead of being sent to sws.

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

In a rich man's house there is no place to spit but his face.
-- Diogenes of Sinope
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20121028/b588e425/attachment.asc>