[FFmpeg-devel] [PATCH] avfilter/fade: add color option.

[Clément Bœsch]

On Sat, Nov 09, 2013 at 07:47:19PM +0100, Stefano Sabatini wrote:
> Subject nit: lavfi/fade: add color option
> 

Current trending is "avfilter", even though I prefer "lavfi".

> On date Saturday 2013-11-09 00:08:05 +0100, Clément Bœsch encoded:
[...]
> > -    ff_set_common_formats(ctx, ff_make_format_list(pix_fmts));
> > +    if (memcmp(s->color_rgba, "\x00\x00\x00\xff", 4))
> > +        ff_set_common_formats(ctx, ff_make_format_list(pix_fmts_rgb));
> > +    else
> > +        ff_set_common_formats(ctx, ff_make_format_list(pix_fmts));
> 
> Add a comment since this is not obvious.
> 
>    if (!memcmp(s->color_rgba, "\x00\x00\x00\xff", 4))
>        ff_set_common_formats(ctx, ff_make_format_list(pix_fmts)); // black, default
>    else
>        ff_set_common_formats(ctx, ff_make_format_list(pix_fmts_rgb));
> 

Added a local flag which I re-use later; it should clarify.

> >      return 0;
> >  }
> >  
> > @@ -138,6 +149,37 @@ static int config_props(AVFilterLink *inlink)
> >      return 0;
> >  }
> >  
> 
> > +static int filter_slice_rgb(AVFilterContext *ctx, void *arg, int jobnr,
> > +                            int nb_jobs)
> 
> less confusing/more descriptive: filter_slice_rgba_interp?
> 

I'm just following the naming scheme.

> > +{
> > +    FadeContext *s = ctx->priv;
> > +    AVFrame *frame = arg;
> > +    int slice_start = (frame->height *  jobnr   ) / nb_jobs;
> > +    int slice_end   = (frame->height * (jobnr+1)) / nb_jobs;
> > +    int i, j;
> > +
> 
> > +    const uint8_t r  = s->rgba_map[R];
> > +    const uint8_t g  = s->rgba_map[G];
> > +    const uint8_t b  = s->rgba_map[B];
> > +    const uint8_t a  = s->rgba_map[A];
> > +    const uint8_t *c = s->color_rgba;
> 
> nit: r/g/b/a -> ridx, gidx, etc...
> 

Renamed

> > +
> > +    for (i = slice_start; i < slice_end; i++) {
> > +        uint8_t *p = frame->data[0] + i * frame->linesize[0];
> > +        for (j = 0; j < frame->width; j++) {
> 
> > +#define INTERP(layer, value) av_clip_uint8(((c[value]<<16) + ((int)p[layer] - (int)c[value]) * s->factor + (1<<15)) >> 16)
> 
> nit: "value" is confusing, I suggest cidx
> 

Yes, that was a bad naming I forgot to update, changed.

> > +            p[r] = INTERP(r, 0);
> > +            p[g] = INTERP(g, 1);
> > +            p[b] = INTERP(b, 2);
> > +            if (s->alpha)
> > +                p[a] = INTERP(a, 3);
> > +            p += s->bpp;
> > +        }
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> >  static int filter_slice_luma(AVFilterContext *ctx, void *arg, int jobnr,
> >                               int nb_jobs)
> >  {
> > @@ -271,8 +313,10 @@ static int filter_frame(AVFilterLink *inlink, AVFrame *frame)
> >          if (s->alpha) {
> >              ctx->internal->execute(ctx, filter_slice_alpha, frame, NULL,
> >                                  FFMIN(frame->height, ctx->graph->nb_threads));
> 
> > +        } else if (s->is_packed_rgb) {
> > +            ctx->internal->execute(ctx, filter_slice_rgb, frame, NULL,
> > +                                   FFMIN(frame->height, ctx->graph->nb_threads));
> 
> seems partially unrelated, shouldn't you use the other routine
> (possible faster if color=black?)
> 

Good idea, call disabled for when black fade.

> >          } else {
> > -            /* luma or rgb plane */
> >              ctx->internal->execute(ctx, filter_slice_luma, frame, NULL,
> >                                  FFMIN(frame->height, ctx->graph->nb_threads));
> >  
> > @@ -315,6 +359,8 @@ static const AVOption fade_options[] = {
> >                                                      OFFSET(duration),    AV_OPT_TYPE_DURATION, {.i64 = 0. }, 0, INT32_MAX, FLAGS },
> >      { "d",           "Duration of the effect in seconds.",
> >                                                      OFFSET(duration),    AV_OPT_TYPE_DURATION, {.i64 = 0. }, 0, INT32_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 },
> >      { NULL }
> >  };

New patch attached.

-- 
Clément B.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-avfilter-fade-add-color-option.patch
Type: text/x-diff
Size: 6895 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20131109/36272b6a/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 490 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20131109/36272b6a/attachment.asc>