[FFmpeg-devel] [PATCH] lavfi/color: use AVOptions

[Stefano Sabatini]

On date Tuesday 2012-06-19 13:32:01 +0000, Paul B Mahol encoded:
> Signed-off-by: Paul B Mahol <onemda at gmail.com>
> ---
>  doc/filters.texi         |   12 +++++-----
>  libavfilter/vsrc_color.c |   49 +++++++++++++++++++++++++++++++++------------
>  2 files changed, 42 insertions(+), 19 deletions(-)
> 
> diff --git a/doc/filters.texi b/doc/filters.texi
> index 50cd72e..fa953ab 100644
> --- a/doc/filters.texi
> +++ b/doc/filters.texi
> @@ -3419,24 +3419,24 @@ cellauto=p='@@@@ @@ @@@@':s=100x400:full=0:rule=18
>  
>  Provide an uniformly colored input.
>  
> -It accepts the following parameters:
> - at var{color}:@var{frame_size}:@var{frame_rate}
> +This source accepts list of options in the form of
> + at var{key}=@var{value} pairs separated by ":".
>  
>  Follows the description of the accepted parameters.
>  
>  @table @option
>  
> - at item color
> + at item color, c
>  Specify the color of the source. It can be the name of a color (case
>  insensitive match) or a 0xRRGGBB[AA] sequence, possibly followed by an
>  alpha specifier. The default value is "black".
>  
> - at item frame_size
> + at item size, s
>  Specify the size of the sourced video, it may be a string of the form
>  @var{width}x at var{height}, or the name of a size abbreviation. The
>  default value is "320x240".
>  
> - at item frame_rate
> + at item rate, r
>  Specify the frame rate of the sourced video, as the number of frames
>  generated per second. It has to be a string in the format
>  @var{frame_rate_num}/@var{frame_rate_den}, an integer number, a float
> @@ -3451,7 +3451,7 @@ frames per second, which will be overlayed over the source connected
>  to the pad with identifier "in".
>  
>  @example
> -"color=red@@0.2:qcif:10 [color]; [in][color] overlay [out]"
> +"color=c=red@@0.2:qcif:10 [color]; [in][color] overlay [out]"

...s=qcif:r=10

>  @end example
>  
>  @section movie
> diff --git a/libavfilter/vsrc_color.c b/libavfilter/vsrc_color.c
> index 112c27c..3d16c69 100644
> --- a/libavfilter/vsrc_color.c
> +++ b/libavfilter/vsrc_color.c
> @@ -31,11 +31,15 @@
>  #include "libavutil/colorspace.h"
>  #include "libavutil/imgutils.h"
>  #include "libavutil/mathematics.h"
> +#include "libavutil/opt.h"
>  #include "libavutil/parseutils.h"
>  #include "drawutils.h"
>  
>  typedef struct {
> +    const AVClass *class;
>      int w, h;

> +    char *rate;
> +    char *color_str;

nit: either call them *_str or with no _str

>      uint8_t color_rgba[4];
>      AVRational time_base;
>      uint64_t pts;
> @@ -43,32 +47,51 @@ typedef struct {
>      FFDrawColor color;
>  } ColorContext;
>  
> +#define OFFSET(x) offsetof(ColorContext, x)
> +
> +static const AVOption color_options[]= {
> +    { "size",     "set frame size",     OFFSET(w),        AV_OPT_TYPE_IMAGE_SIZE, {.str = "320x240"}, 0, 0 },
> +    { "s",        "set frame size",     OFFSET(w),        AV_OPT_TYPE_IMAGE_SIZE, {.str = "320x240"}, 0, 0 },
> +    { "rate",     "set frame rate",     OFFSET(rate),     AV_OPT_TYPE_STRING, {.str = "25"},          0, 0 },
> +    { "r",        "set frame rate",     OFFSET(rate),     AV_OPT_TYPE_STRING, {.str = "25"},          0, 0 },

> +    { "color",    "set color",          OFFSET(color_str),AV_OPT_TYPE_STRING, {.str = "black"},       CHAR_MIN, CHAR_MAX },
> +    { "c",        "set color",          OFFSET(color_str),AV_OPT_TYPE_STRING, {.str = "black"},       CHAR_MIN, CHAR_MAX },

whitespaces are cheap these days ;-)

> +    { NULL },
> +};
> +
> +static const AVClass color_class = {
> +    .class_name = "color",
> +    .item_name  = av_default_item_name,
> +    .option     = color_options,
> +    .version    = LIBAVUTIL_VERSION_INT,
> +    .category   = AV_CLASS_CATEGORY_FILTER,
> +};

Note: maybe we could create a macro for this (AVFILTER_DEFINE_CLASS)...

> +
>  static av_cold int color_init(AVFilterContext *ctx, const char *args, void *opaque)
>  {
>      ColorContext *color = ctx->priv;
> -    char color_string[128] = "black";
> -    char frame_size  [128] = "320x240";
> -    char frame_rate  [128] = "25";
>      AVRational frame_rate_q;
>      int ret;
>  
> -    if (args)
> -        sscanf(args, "%127[^:]:%127[^:]:%127s", color_string, frame_size, frame_rate);
> +    color->class = &color_class;
> +    av_opt_set_defaults(color);
>  
> -    if (av_parse_video_size(&color->w, &color->h, frame_size) < 0) {
> -        av_log(ctx, AV_LOG_ERROR, "Invalid frame size: %s\n", frame_size);
> -        return AVERROR(EINVAL);
> +    if ((ret = (av_set_options_string(color, args, "=", ":"))) < 0) {
> +        av_log(ctx, AV_LOG_ERROR, "Error parsing options string: '%s'\n", args);
> +        return ret;
>      }

We should really keep backward compatibility here, and deprecate the
old syntax. Check how this is done in
libavfilter/buffersrc.c:init_video().

>  
> -    if (av_parse_video_rate(&frame_rate_q, frame_rate) < 0 ||
> +    if (ret = av_parse_video_rate(&frame_rate_q, color->rate) < 0 ||
>          frame_rate_q.den <= 0 || frame_rate_q.num <= 0) {
> -        av_log(ctx, AV_LOG_ERROR, "Invalid frame rate: %s\n", frame_rate);
> -        return AVERROR(EINVAL);
> +        av_log(ctx, AV_LOG_ERROR, "Invalid frame rate: %s\n", color->rate);
> +        return ret;

Return AVERROR(EINVAL) -> return ret is unrelated, stash it in a
separate patch if you care.

>      }
> +    av_freep(&color->rate);
> +
>      color->time_base.num = frame_rate_q.den;
>      color->time_base.den = frame_rate_q.num;
>  
> -    if ((ret = av_parse_color(color->color_rgba, color_string, -1, ctx)) < 0)
> +    if ((ret = av_parse_color(color->color_rgba, color->color_str, -1, ctx)) < 0)
>          return ret;

Leak on color->color_str.

Free it here or keep it for all the life of the filter, and destroy
both strings on uninit.

[...]

Thanks (I wanted to do this myself since some time).
-- 
FFmpeg = Fast and Fostering Martial Pitiless Ephemeral Gigant