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

Stefano Sabatini stefasab at gmail.com
Sat Dec 15 20:45:09 CET 2012


On date Saturday 2012-12-15 20:03:12 +0100, Clément Bœsch encoded:
> On Sat, Dec 15, 2012 at 05:03:38PM +0100, Stefano Sabatini wrote:
> > Also fix documentation accordingly.
> > 
> > TODO: bump micro
> > ---
> >  doc/filters.texi      |   48 +++++++++++++++++++++++-----------
> >  libavfilter/vf_crop.c |   69 +++++++++++++++++++++++++++++--------------------
> >  2 files changed, 74 insertions(+), 43 deletions(-)
> > 
> > diff --git a/doc/filters.texi b/doc/filters.texi
> > index 44654fe..a8b6221 100644
> > --- a/doc/filters.texi
> > +++ b/doc/filters.texi
> > @@ -1518,11 +1518,40 @@ testing purposes.
> >  
> >  @section crop
> >  
> > -Crop the input video to @var{out_w}:@var{out_h}:@var{x}:@var{y}:@var{keep_aspect}
> > +Crop the input video.
> >  
> > -The @var{keep_aspect} parameter is optional, if specified and set to a
> > -non-zero value will force the output display aspect ratio to be the
> > -same of the input, by changing the output sample aspect ratio.
> > +This filter accepts a list of @var{key}=@var{value} pairs as argument,
> > +separated by ":". If the key of the first options is omitted, the
> 
> NitNote: I remember using ':' instead of ":", we may want to make this
> consistent at some point.

Changed to ':', more C-friendly.

> 
> > +arguments are interpreted according to the syntax
> > + at var{out_w}:@var{out_h}:@var{x}:@var{y}:@var{keep_aspect}.
> > +
> 
> Yet another wording for introducing the shorthand syntax? :)

Yes variety is beatiful. A possible plan which I support is having all
filters supporting options, at that point we may factorize code and
docs.

> 
> > +A description of the accepted options follows:
> > + at table @option
> > + at item w, out_w
> > +Set the crop area width. It defaults to @code{iw}.
> > +This expression is evaluated only once during the filter
> > +configuration.
> > +
> > + at item h, out_h
> > +Set the crop area width. It defaults to @code{ih}.
> > +This expression is evaluated only once during the filter
> > +configuration.
> > +
> > + at item x
> > +Set the expression for the x top-left coordinate of the cropped area.
> > +It defaults to @code{(in_w-out_w)/2}.
> > +This expression is evaluated per-frame.
> > +
> > + at item y
> > +Set the expression for the y top-left coordinate of the cropped area.
> > +It defaults to @code{(in_h-out_h)/2}.
> > +This expression is evaluated per-frame.
> > +
> > + at item keep_aspect
> > +If set to 1 will force the output display aspect ratio
> > +to be the same of the input, by changing the output sample aspect
> > +ratio. It defaults to 0.
> > + at end table
> >  
> 
> I wonder if a good example could be a crop=10:20:30:40:keep_aspect=1, but
> the shorthand syntax might need to be introduced differently to avoid any
> confusion.
> 
> >  The @var{out_w}, @var{out_h}, @var{x}, @var{y} parameters are
> >  expressions containing the following constants:
> > @@ -1568,13 +1597,6 @@ timestamp expressed in seconds, NAN if the input timestamp is unknown
> >  
> >  @end table
> >  
> > -The @var{out_w} and @var{out_h} parameters specify the expressions for
> > -the width and height of the output (cropped) video. They are
> > -evaluated just at the configuration of the filter.
> > -
> > -The default value of @var{out_w} is "in_w", and the default value of
> > - at var{out_h} is "in_h".
> > -
> >  The expression for @var{out_w} may depend on the value of @var{out_h},
> >  and the expression for @var{out_h} may depend on @var{out_w}, but they
> >  cannot depend on @var{x} and @var{y}, as @var{x} and @var{y} are
> > @@ -1585,10 +1607,6 @@ position of the top-left corner of the output (non-cropped) area. They
> >  are evaluated for each frame. If the evaluated value is not valid, it
> >  is approximated to the nearest valid value.
> >  
> > -The default value of @var{x} is "(in_w-out_w)/2", and the default
> > -value for @var{y} is "(in_h-out_h)/2", which set the cropped area at
> > -the center of the input image.
> > -
> >  The expression for @var{x} may depend on @var{y}, and the expression
> >  for @var{y} may depend on @var{x}.
> >  
> > diff --git a/libavfilter/vf_crop.c b/libavfilter/vf_crop.c
> > index d0f464f..d4cb710 100644
> > --- a/libavfilter/vf_crop.c
> > +++ b/libavfilter/vf_crop.c
> > @@ -37,6 +37,7 @@
> >  #include "libavutil/libm.h"
> >  #include "libavutil/imgutils.h"
> >  #include "libavutil/mathematics.h"
> > +#include "libavutil/opt.h"
> >  
> >  static const char *const var_names[] = {
> >      "in_w", "iw",   ///< width  of the input video
> > @@ -75,6 +76,7 @@ enum var_name {
> >  };
> >  
> >  typedef struct {
> > +    const AVClass *class;
> >      int  x;             ///< x offset of the non-cropped area with respect to the input area
> >      int  y;             ///< y offset of the non-cropped area with respect to the input area
> >      int  w;             ///< width of the cropped area
> > @@ -85,11 +87,44 @@ typedef struct {
> >  
> >      int max_step[4];    ///< max pixel step for each plane, expressed as a number of bytes
> >      int hsub, vsub;     ///< chroma subsampling
> > -    char x_expr[256], y_expr[256], ow_expr[256], oh_expr[256];
> > +    char *x_expr, *y_expr, *w_expr, *h_expr;
> >      AVExpr *x_pexpr, *y_pexpr;  /* parsed expressions for x and y */
> >      double var_values[VAR_VARS_NB];
> >  } CropContext;
> >  
> > +#define OFFSET(x) offsetof(CropContext, x)
> > +#define FLAGS AV_OPT_FLAG_FILTERING_PARAM|AV_OPT_FLAG_VIDEO_PARAM
> > +
> > +static const AVOption crop_options[] = {
> > +    { "x", "set the x crop area expression", OFFSET(x_expr), AV_OPT_TYPE_STRING, {.str = "(in_w-out_w)/2"}, CHAR_MIN, CHAR_MAX, FLAGS },
> > +    { "y", "set the y crop area expression", OFFSET(y_expr), AV_OPT_TYPE_STRING, {.str = "(in_h-out_h)/2"}, CHAR_MIN, CHAR_MAX, FLAGS },
> > +    { "out_w", "set the width crop area expression", OFFSET(w_expr), AV_OPT_TYPE_STRING, {.str = "iw"}, CHAR_MIN, CHAR_MAX, FLAGS },
> > +    { "w", "set the width crop area expression", OFFSET(w_expr), AV_OPT_TYPE_STRING, {.str = "iw"}, CHAR_MIN, CHAR_MAX, FLAGS },
> > +    { "out_h", "set the height crop area expression", OFFSET(h_expr), AV_OPT_TYPE_STRING, {.str = "ih"}, CHAR_MIN, CHAR_MAX, FLAGS },
> > +    { "h", "set the height crop area expression", OFFSET(h_expr), AV_OPT_TYPE_STRING, {.str = "ih"}, CHAR_MIN, CHAR_MAX, FLAGS },
> > +    { "keep_aspect", "force packed RGB in input and output", OFFSET(keep_aspect), AV_OPT_TYPE_INT, {.i64=0}, 0, 1, FLAGS },
> > +    {NULL}
> > +};
> 
> Please either align or break lines.

Partially aligned.

> > +
> > +AVFILTER_DEFINE_CLASS(crop);
> > +
> > +static av_cold int init(AVFilterContext *ctx, const char *args)
> > +{
> > +    CropContext *crop = ctx->priv;
> > +    static const char *shorthand[] = { "w", "h", "x", "y", "keep_aspect", NULL };
> > +
> > +    crop->class = &crop_class;
> > +    av_opt_set_defaults(crop);
> > +
> > +    return av_opt_set_from_string(crop, args, shorthand, "=", ":");
> > +}
> > +
> > +static av_cold void uninit(AVFilterContext *ctx)
> > +{
> > +    CropContext *crop = ctx->priv;
> > +    av_opt_free(crop);
> > +}
> > +
> >  static int query_formats(AVFilterContext *ctx)
> >  {
> >      static const enum AVPixelFormat pix_fmts[] = {
> > @@ -123,29 +158,6 @@ static int query_formats(AVFilterContext *ctx)
> >      return 0;
> >  }
> >  
> > -static av_cold int init(AVFilterContext *ctx, const char *args)
> > -{
> > -    CropContext *crop = ctx->priv;
> > -
> > -    av_strlcpy(crop->ow_expr, "iw", sizeof(crop->ow_expr));
> > -    av_strlcpy(crop->oh_expr, "ih", sizeof(crop->oh_expr));
> > -    av_strlcpy(crop->x_expr, "(in_w-out_w)/2", sizeof(crop->x_expr));
> > -    av_strlcpy(crop->y_expr, "(in_h-out_h)/2", sizeof(crop->y_expr));
> > -
> > -    if (args)
> > -        sscanf(args, "%255[^:]:%255[^:]:%255[^:]:%255[^:]:%d", crop->ow_expr, crop->oh_expr, crop->x_expr, crop->y_expr, &crop->keep_aspect);
> > -
> > -    return 0;
> > -}
> > -
> > -static av_cold void uninit(AVFilterContext *ctx)
> > -{
> > -    CropContext *crop = ctx->priv;
> > -
> > -    av_expr_free(crop->x_pexpr); crop->x_pexpr = NULL;
> > -    av_expr_free(crop->y_pexpr); crop->y_pexpr = NULL;
> > -}
> > -
> >  static inline int normalize_double(int *n, double d)
> >  {
> >      int ret = 0;
> > @@ -189,16 +201,16 @@ static int config_input(AVFilterLink *link)
> >      crop->hsub = pix_desc->log2_chroma_w;
> >      crop->vsub = pix_desc->log2_chroma_h;
> >  
> > -    if ((ret = av_expr_parse_and_eval(&res, (expr = crop->ow_expr),
> > +    if ((ret = av_expr_parse_and_eval(&res, (expr = crop->w_expr),
> >                                        var_names, crop->var_values,
> >                                        NULL, NULL, NULL, NULL, NULL, 0, ctx)) < 0) goto fail_expr;
> >      crop->var_values[VAR_OUT_W] = crop->var_values[VAR_OW] = res;
> > -    if ((ret = av_expr_parse_and_eval(&res, (expr = crop->oh_expr),
> > +    if ((ret = av_expr_parse_and_eval(&res, (expr = crop->h_expr),
> >                                        var_names, crop->var_values,
> >                                        NULL, NULL, NULL, NULL, NULL, 0, ctx)) < 0) goto fail_expr;
> >      crop->var_values[VAR_OUT_H] = crop->var_values[VAR_OH] = res;
> >      /* evaluate again ow as it may depend on oh */
> > -    if ((ret = av_expr_parse_and_eval(&res, (expr = crop->ow_expr),
> > +    if ((ret = av_expr_parse_and_eval(&res, (expr = crop->w_expr),
> >                                        var_names, crop->var_values,
> >                                        NULL, NULL, NULL, NULL, NULL, 0, ctx)) < 0) goto fail_expr;
> >      crop->var_values[VAR_OUT_W] = crop->var_values[VAR_OW] = res;
> > @@ -207,7 +219,7 @@ static int config_input(AVFilterLink *link)
> >          av_log(ctx, AV_LOG_ERROR,
> >                 "Too big value or invalid expression for out_w/ow or out_h/oh. "
> >                 "Maybe the expression for out_w:'%s' or for out_h:'%s' is self-referencing.\n",
> > -               crop->ow_expr, crop->oh_expr);
> > +               crop->w_expr, crop->h_expr);
> >          return AVERROR(EINVAL);
> >      }
> >      crop->w &= ~((1 << crop->hsub) - 1);
> > @@ -348,4 +360,5 @@ AVFilter avfilter_vf_crop = {
> >  
> >      .inputs    = avfilter_vf_crop_inputs,
> >      .outputs   = avfilter_vf_crop_outputs,
> > +    .priv_class = &crop_class,
> >  };
> 
> Rest LGTM, nice improvement, thanks.

Will push it soon, thanks.
-- 
FFmpeg = Free Foolish Monstrous Pitiless Explosive Gangster


More information about the ffmpeg-devel mailing list