[FFmpeg-devel] [PATCH] lavfi/tinterlace: add support to option parsing

[Stefano Sabatini]

On date Thursday 2012-12-06 22:10:59 +0100, Clément Bœsch encoded:
> On Thu, Dec 06, 2012 at 10:04:54PM +0100, Stefano Sabatini wrote:
> > Simplify code, and provide introspection through the AVOption system.
> > 
> > TODO: bump minor
> > ---
> >  doc/filters.texi            |    5 +--
> >  libavfilter/vf_tinterlace.c |   71 ++++++++++++++++++-------------------------
> >  2 files changed, 32 insertions(+), 44 deletions(-)
> > 
> > diff --git a/doc/filters.texi b/doc/filters.texi
> > index f745517..100f979 100644
> > --- a/doc/filters.texi
> > +++ b/doc/filters.texi
> > @@ -3865,8 +3865,9 @@ Perform various types of temporal field interlacing.
> >  Frames are counted starting from 1, so the first input frame is
> >  considered odd.
> >  
> > -This filter accepts a single parameter specifying the mode. Available
> > -modes are:
> > +This filter accepts a single option @option{mode} specifying the mode,
> > +which can be specified either by specyfing @code{mode=VALUE} either
> > +specifying the value alone. Available values are:
> >  
> >  @table @samp
> >  @item merge, 0
> > diff --git a/libavfilter/vf_tinterlace.c b/libavfilter/vf_tinterlace.c
> > index 53c955b..1606f06 100644
> > --- a/libavfilter/vf_tinterlace.c
> > +++ b/libavfilter/vf_tinterlace.c
> > @@ -25,6 +25,7 @@
> >   * temporal field interlace filter, ported from MPlayer/libmpcodecs
> >   */
> >  
> > +#include "libavutil/opt.h"
> >  #include "libavutil/imgutils.h"
> >  #include "libavutil/avassert.h"
> >  #include "avfilter.h"
> > @@ -38,20 +39,11 @@ enum TInterlaceMode {
> >      MODE_INTERLEAVE_TOP,
> >      MODE_INTERLEAVE_BOTTOM,
> >      MODE_INTERLACEX2,
> > -};
> > -
> > -static const char *tinterlace_mode_str[] = {
> > -    "merge",
> > -    "drop_even",
> > -    "drop_odd",
> > -    "pad",
> > -    "interleave_top",
> > -    "interleave_bottom",
> > -    "interlacex2",
> > -    NULL
> > +    MODE_NB,
> >  };
> >  
> >  typedef struct {
> > +    const AVClass *class;
> >      enum TInterlaceMode mode;   ///< interlace mode selected
> >      int frame;                  ///< number of the output frame
> >      int vsub;                   ///< chroma vertical subsampling
> > @@ -61,6 +53,24 @@ typedef struct {
> >      int black_linesize[4];
> >  } TInterlaceContext;
> >  
> > +#define OFFSET(x) offsetof(TInterlaceContext, x)
> > +#define FLAGS AV_OPT_FLAG_FILTERING_PARAM|AV_OPT_FLAG_VIDEO_PARAM
> > +
> > +static const AVOption tinterlace_options[] = {
> > +    {"mode",              "select interlace mode", OFFSET(mode), AV_OPT_TYPE_INT, {.i64=MODE_MERGE}, 0, MODE_NB-1, FLAGS, "mode"},
> > +    {"merge",             "merge fields",                                 0, AV_OPT_TYPE_CONST, {.i64=MODE_MERGE}, INT_MIN, INT_MAX, FLAGS, "mode"},
> > +    {"drop_even",         "drop even fields",                             0, AV_OPT_TYPE_CONST, {.i64=MODE_DROP_EVEN}, INT_MIN, INT_MAX, FLAGS, "mode"},
> > +    {"drop_odd",          "drop odd fields",                              0, AV_OPT_TYPE_CONST, {.i64=MODE_DROP_ODD}, INT_MIN, INT_MAX, FLAGS, "mode"},
> > +    {"pad",               "pad alternate lines with black",               0, AV_OPT_TYPE_CONST, {.i64=MODE_PAD}, INT_MIN, INT_MAX, FLAGS, "mode"},
> > +    {"interleave_top",    "interleave top and bottom fields",             0, AV_OPT_TYPE_CONST, {.i64=MODE_INTERLEAVE_TOP}, INT_MIN, INT_MAX, FLAGS, "mode"},
> > +    {"interleave_bottom", "interleave bottom and top fields",             0, AV_OPT_TYPE_CONST, {.i64=MODE_INTERLEAVE_BOTTOM}, INT_MIN, INT_MAX, FLAGS, "mode"},
> > +    {"interlacex2",       "interlace fields from two consecutive frames", 0, AV_OPT_TYPE_CONST, {.i64=MODE_INTERLACEX2}, INT_MIN, INT_MAX, FLAGS, "mode"},
> > +
> 

> nit:  we often like to indent the flags below the the option.
> nit+: did you get bored by the vertical align at the end?

Aligned (a macro would be the best solution for fixing the verbosity).

> 
> > +    {NULL}
> > +};
> > +
> > +AVFILTER_DEFINE_CLASS(tinterlace);
> > +
> >  #define FULL_SCALE_YUVJ_FORMATS \
> >      AV_PIX_FMT_YUVJ420P, AV_PIX_FMT_YUVJ422P, AV_PIX_FMT_YUVJ444P, AV_PIX_FMT_YUVJ440P
> >  
> > @@ -84,36 +94,12 @@ static int query_formats(AVFilterContext *ctx)
> >  static av_cold int init(AVFilterContext *ctx, const char *args)
> >  {
> >      TInterlaceContext *tinterlace = ctx->priv;
> > -    int i;
> > -    char c;
> > -
> > -    tinterlace->mode = MODE_MERGE;
> > -
> > -    if (args) {
> > -        if (sscanf(args, "%d%c", (int *)&tinterlace->mode, &c) == 1) {
> > -            if (tinterlace->mode > 6) {
> > -                av_log(ctx, AV_LOG_ERROR,
> > -                       "Invalid mode '%s', use an integer between 0 and 6\n", args);
> > -                return AVERROR(EINVAL);
> > -            }
> > -
> > -            av_log(ctx, AV_LOG_WARNING,
> > -                   "Using numeric constant is deprecated, use symbolic values\n");
> > -        } else {
> > -            for (i = 0; tinterlace_mode_str[i]; i++) {
> > -                if (!strcmp(tinterlace_mode_str[i], args)) {
> > -                    tinterlace->mode = i;
> > -                    break;
> > -                }
> > -            }
> > -            if (!tinterlace_mode_str[i]) {
> > -                av_log(ctx, AV_LOG_ERROR, "Invalid argument '%s'\n", args);
> > -                return AVERROR(EINVAL);
> > -            }
> > -        }
> > -    }
> > +    static const char *shorthand[] = { "mode", NULL };
> >  
> > -    return 0;
> > +    tinterlace->class = &tinterlace_class;
> > +    av_opt_set_defaults(tinterlace);
> > +
> > +    return av_opt_set_from_string(tinterlace, args, shorthand, "=", ":");
> 

> Nice simplification. Maybe add a av_opt_free in case a string is added at
> some point?

Done, applied, thanks.
-- 
FFmpeg = Fancy & Fiendish Murdering Proud Emblematic Glue