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

Stefano Sabatini stefasab at gmail.com
Tue Jun 19 18:42:58 CEST 2012


On date Tuesday 2012-06-19 16:07:45 +0000, Paul B Mahol encoded:
> Signed-off-by: Paul B Mahol <onemda at gmail.com>
> ---
>  doc/filters.texi         |   12 +++---
>  libavfilter/vsrc_color.c |   83 +++++++++++++++++++++++++++++++++++++---------
>  2 files changed, 73 insertions(+), 22 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 ":".

Please mention the old deprecated syntax:

|This source accepts a list of options in the form of
|@var{key}=@var{value} pairs separated by ":".
|
|Alternatively, it accepts a string in the form
|@var{color}:@var{size}:@var{rate}, but this syntax is
|deprecated.

>  
>  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]"

still borken?

>  @end example
>  
>  @section movie
> diff --git a/libavfilter/vsrc_color.c b/libavfilter/vsrc_color.c
> index 112c27c..51c959b 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_str;
> +    char *color_str;
>      uint8_t color_rgba[4];
>      AVRational time_base;
>      uint64_t pts;
> @@ -43,35 +47,82 @@ 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_str), AV_OPT_TYPE_STRING, {.str = "25"}, 0, 0 },
> +    { "r",     "set frame rate", OFFSET(rate_str), 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 },
> +    { 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,
> +};
> +
>  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;
> +    char color_string[128] = "black";
> +    char frame_rate[128] = "25";
> +    char frame_size[128] = "320x240";
> +    char *colon = 0, *equal = 0;
>      int ret;
>  
> -    if (args)
> -        sscanf(args, "%127[^:]:%127[^:]:%127s", color_string, frame_size, frame_rate);
> +    color->class = &color_class;
>  
> -    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 (args) {
> +        colon = strrchr(args, ':');
> +        equal = strrchr(args, '=');

>      }
>  
> -    if (av_parse_video_rate(&frame_rate_q, frame_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);
> +    if (!args || (equal && (!colon || equal < colon))) {
> +        av_opt_set_defaults(color);
> +        if ((ret = av_set_options_string(color, args, "=", ":")) < 0) {
> +            av_log(ctx, AV_LOG_ERROR, "Error parsing options string: '%s'\n", args);
> +            goto fail;
> +        }

> +        if (av_parse_video_rate(&frame_rate_q, color->rate_str) < 0 ||
> +                frame_rate_q.den <= 0 || frame_rate_q.num <= 0) {
> +            av_log(ctx, AV_LOG_ERROR, "Invalid frame rate: %s\n", color->rate_str);
> +            ret = AVERROR(EINVAL);
> +            goto fail;
> +        }
> +        if (av_parse_color(color->color_rgba, color->color_str, -1, ctx) < 0) {
> +            ret = AVERROR(EINVAL);
> +            goto fail;
> +        }
> +    } else {
> +        av_log(ctx, AV_LOG_WARNING, "Flat options syntax is deprecated, use key=value pairs.\n");
> +        sscanf(args, "%127[^:]:%127[^:]:%127s", color_string, frame_size, frame_rate);
> +        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 (av_parse_video_rate(&frame_rate_q, frame_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);
> +        }
> +        if (av_parse_color(color->color_rgba, color_string, -1, ctx) < 0)
> +            return AVERROR(EINVAL);
>      }

I see code duplication but removing it seems not worth the effort,
hopefully we'll get rid of it when we'll drop the old syntax (next
bump?).

> +
>      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)
> -        return ret;
> -
>      return 0;

> +fail:

This is reached even in case of non-fail, I believe "end" or "exit" is
more appropriate/less confusing.

[...]

Looks fine otherwise if tested, thanks.
-- 
FFmpeg = Foolish Fundamental Mysterious Patchable Ecumenical Geisha


More information about the ffmpeg-devel mailing list