[FFmpeg-devel] [PATCH 2/5] lavfi/color: switch to AV_OPT_TYPE_COLOR

[Stefano Sabatini]

On date Thursday 2013-05-16 22:06:53 +0000, Paul B Mahol encoded:
> On 5/16/13, Stefano Sabatini <stefasab at gmail.com> wrote:
> > On date Monday 2013-05-13 15:11:37 +0000, Paul B Mahol encoded:
> >> Signed-off-by: Paul B Mahol <onemda at gmail.com>
> >> ---
> >>  libavfilter/vsrc_testsrc.c | 26 ++------------------------
> >>  1 file changed, 2 insertions(+), 24 deletions(-)
> >>
> >> diff --git a/libavfilter/vsrc_testsrc.c b/libavfilter/vsrc_testsrc.c
> >> index dc8984a..8c4c51a 100644
> >> --- a/libavfilter/vsrc_testsrc.c
> >> +++ b/libavfilter/vsrc_testsrc.c
> >> @@ -63,7 +63,6 @@ typedef struct {
> >>      void (* fill_picture_fn)(AVFilterContext *ctx, AVFrame *frame);
> >>
> >>      /* only used by color */
> >> -    char *color_str;
> >>      FFDrawContext draw;
> >>      FFDrawColor color;
> >>      uint8_t color_rgba[4];
> >> @@ -87,8 +86,8 @@ typedef struct {
> >>
> >>  static const AVOption color_options[] = {
> >>      /* only used by color */
> >> -    { "color", "set color", OFFSET(color_str), AV_OPT_TYPE_STRING, {.str
> >> = "black"}, CHAR_MIN, CHAR_MAX, FLAGS },
> >> -    { "c",     "set color", OFFSET(color_str), AV_OPT_TYPE_STRING, {.str
> >> = "black"}, CHAR_MIN, CHAR_MAX, FLAGS },
> >> +    { "color", "set color", OFFSET(color_rgba), AV_OPT_TYPE_COLOR, {.str
> >> = "black"}, CHAR_MIN, CHAR_MAX, FLAGS },
> >> +    { "c",     "set color", OFFSET(color_rgba), AV_OPT_TYPE_COLOR, {.str
> >> = "black"}, CHAR_MIN, CHAR_MAX, FLAGS },
> >>
> >>      COMMON_OPTIONS
> >>      { NULL },
> >> @@ -106,7 +105,6 @@ static const AVOption options[] = {
> >>  static av_cold int init(AVFilterContext *ctx)
> >>  {
> >>      TestSourceContext *test = ctx->priv;
> >> -    int ret = 0;
> >>
> >>      if (test->nb_decimals && strcmp(ctx->filter->name, "testsrc")) {
> >>          av_log(ctx, AV_LOG_WARNING,
> >> @@ -114,18 +112,6 @@ static av_cold int init(AVFilterContext *ctx)
> >>                 ctx->filter->name);
> >>      }
> >>
> >
> >> -    if (test->color_str) {
> >> -        if (!strcmp(ctx->filter->name, "color")) {
> >> -            ret = av_parse_color(test->color_rgba, test->color_str, -1,
> >> ctx);
> >> -            if (ret < 0)
> >> -                return ret;
> >> -        } else {
> >> -            av_log(ctx, AV_LOG_WARNING,
> >> -                   "Option 'color' is ignored with source '%s'\n",
> >> -                   ctx->filter->name);
> >> -        }
> >> -    }
> >

> > Uhm not very happy about now there is no warning in case color is set
> > on a source != filter, and duplicating the options or keeping this

ugh it was: source != color

> > code might be overkill...
> 
> Please elaborate.
> 
> Also I do not see how that warning is much helpful.

> Options could be split if you insist on this.

I don't insist, since I'm not sure what's better between code bloat
and a potentially misleading silent setting of an unused option. Feel
free to push the variant you like better.

> >
> >> -
> >>      test->time_base = av_inv_q(test->frame_rate);
> >>      test->nb_frame = 0;
> >>      test->pts = 0;
> >> @@ -241,8 +227,6 @@ static int color_config_props(AVFilterLink *inlink)
> >>      if ((ret = config_props(inlink)) < 0)
> >>          return ret;
> >>
> >> -    av_log(ctx, AV_LOG_VERBOSE, "color:0x%02x%02x%02x%02x\n",
> >> -           test->color_rgba[0], test->color_rgba[1], test->color_rgba[2],
> >> test->color_rgba[3]);
> >>      return 0;
> >>  }
> >>
> >> @@ -253,17 +237,11 @@ static int color_process_command(AVFilterContext
> >> *ctx, const char *cmd, const ch
> >>      int ret;
> >>
> >>      if (!strcmp(cmd, "color") || !strcmp(cmd, "c")) {
> >> -        char *color_str;
> >>          uint8_t color_rgba[4];
> >>
> >>          ret = av_parse_color(color_rgba, args, -1, ctx);
> >>          if (ret < 0)
> >>              return ret;
> >
> >> -        color_str = av_strdup(args);
> >> -        if (!color_str)
> >> -            return AVERROR(ENOMEM);
> >> -        av_free(test->color_str);
> >> -        test->color_str = color_str;
> >
> > What if you set the color in the context with av_opt_set()?
> 
> What?

Uhm no not really needed after more thinking.
-- 
FFmpeg = Fancy Fabulous Meaningless Philosofic Easy Game