[FFmpeg-devel] [PATCH] lavfi/setfield: add support to named options and introspection

[Stefano Sabatini]

On date Saturday 2012-12-08 17:59:36 +0100, Clément Bœsch encoded:
> On Sat, Dec 08, 2012 at 05:36:05PM +0100, Stefano Sabatini wrote:
> > TODO: bump micro
> > ---
> >  doc/filters.texi          |    5 ++++-
> >  libavfilter/vf_setfield.c |   46 +++++++++++++++++++++------------------------
> >  2 files changed, 25 insertions(+), 26 deletions(-)
> > 
> > diff --git a/doc/filters.texi b/doc/filters.texi
> > index 1a2efcd..271fe10 100644
> > --- a/doc/filters.texi
> > +++ b/doc/filters.texi
> > @@ -3632,7 +3632,10 @@ output frames. It does not change the input frame, but only sets the
> >  corresponding property, which affects how the frame is treated by
> >  following filters (e.g. @code{fieldorder} or @code{yadif}).
> >  
> > -It accepts a string parameter, which can assume the following values:
> > +This filter accepts a single option @option{mode}, which can be
> > +specified either by setting @code{mode=VALUE} either setting the
> > +value alone. Available values are:
> > +
> >  @table @samp
> >  @item auto
> >  Keep the same field property.
> > diff --git a/libavfilter/vf_setfield.c b/libavfilter/vf_setfield.c
> > index 026b965..8b5546f 100644
> > --- a/libavfilter/vf_setfield.c
> > +++ b/libavfilter/vf_setfield.c
> > @@ -23,6 +23,7 @@
> >   * set field order
> >   */
> >  
> > +#include "libavutil/opt.h"
> >  #include "avfilter.h"
> >  #include "internal.h"
> >  #include "video.h"
> > @@ -35,39 +36,33 @@ enum SetFieldMode {
> >  };
> >  
> >  typedef struct {
> > +    const AVClass *class;
> >      enum SetFieldMode mode;
> >  } SetFieldContext;
> >  
> > +#define OFFSET(x) offsetof(SetFieldContext, x)
> > +#define FLAGS AV_OPT_FLAG_FILTERING_PARAM|AV_OPT_FLAG_VIDEO_PARAM
> > +
> > +static const AVOption setfield_options[] = {
> > +    {"mode", "select interlace mode", OFFSET(mode), AV_OPT_TYPE_INT, {.i64=MODE_AUTO}, -1, MODE_PROG, FLAGS, "mode"},
> > +    {"auto", "keep the same input field",  0, AV_OPT_TYPE_CONST, {.i64=MODE_AUTO}, INT_MIN, INT_MAX, FLAGS, "mode"},
> > +    {"bff",  "mark as bottom-field-first", 0, AV_OPT_TYPE_CONST, {.i64=MODE_BFF},  INT_MIN, INT_MAX, FLAGS, "mode"},
> > +    {"tff",  "mark as top-field-first",    0, AV_OPT_TYPE_CONST, {.i64=MODE_TFF},  INT_MIN, INT_MAX, FLAGS, "mode"},
> > +    {"prog", "mark as progressive",        0, AV_OPT_TYPE_CONST, {.i64=MODE_PROG}, INT_MIN, INT_MAX, FLAGS, "mode"},
> > +    {NULL}
> > +};
> > +
> > +AVFILTER_DEFINE_CLASS(setfield);
> > +
> >  static av_cold int init(AVFilterContext *ctx, const char *args)
> >  {
> >      SetFieldContext *setfield = ctx->priv;
> > +    static const char *shorthand[] = { "mode", NULL };
> >  
> > -    setfield->mode = MODE_AUTO;
> > -
> > -    if (args) {
> > -        char c;
> > -        if (sscanf(args, "%d%c", &setfield->mode, &c) != 1) {
> > -            if      (!strcmp("tff",  args)) setfield->mode = MODE_TFF;
> > -            else if (!strcmp("bff",  args)) setfield->mode = MODE_BFF;
> > -            else if (!strcmp("prog", args)) setfield->mode = MODE_PROG;
> > -            else if (!strcmp("auto", args)) setfield->mode = MODE_AUTO;
> > -            else {
> > -                av_log(ctx, AV_LOG_ERROR, "Invalid argument '%s'\n", args);
> > -                return AVERROR(EINVAL);
> > -            }
> > -        } else {
> > -            if (setfield->mode < -1 || setfield->mode > 1) {
> > -                av_log(ctx, AV_LOG_ERROR,
> > -                       "Provided integer value %d must be included between -1 and +1\n",
> > -                       setfield->mode);
> > -                return AVERROR(EINVAL);
> > -            }
> > -            av_log(ctx, AV_LOG_WARNING,
> > -                   "Using -1/0/1 is deprecated, use auto/tff/bff/prog\n");
> > -        }
> > -    }
> > +    setfield->class = &setfield_class;
> > +    av_opt_set_defaults(setfield);
> >  
> > -    return 0;
> > +    return av_opt_set_from_string(setfield, args, shorthand, "=", ":");
> >  }
> >  
> >  static int filter_frame(AVFilterLink *inlink, AVFilterBufferRef *picref)
> > @@ -109,4 +104,5 @@ AVFilter avfilter_vf_setfield = {
> >      .priv_size = sizeof(SetFieldContext),
> >      .inputs    = setfield_inputs,
> >      .outputs   = setfield_outputs,
> > +    .priv_class = &setfield_class,
> >  };
> 
> LGTM but we should decide what to do about av_opt_free(): call it all the
> time (to prevent any future leaks), or just when necessary (only needed
> with strings in option, right?)

Added av_opt_free(), pushed.

One possibility would be to require all filters to support named
options, and move init/free to the framework.
-- 
FFmpeg = Formidable and Fanciful Meaningful Perfectionist Evil Guru