[FFmpeg-devel] [PATCH 1/3] lavfi: add haldclutsrc filter.

Clément Bœsch ubitux at gmail.com
Mon May 27 20:48:22 CEST 2013


On Sun, May 26, 2013 at 11:21:03PM +0200, Stefano Sabatini wrote:
> On date Saturday 2013-05-25 17:10:55 +0200, Clément Bœsch encoded:
> > TODO: minor bump
> > ---
> >  doc/filters.texi           |   9 ++-
> >  libavfilter/Makefile       |   1 +
> >  libavfilter/allfilters.c   |   1 +
> >  libavfilter/vsrc_testsrc.c | 136 +++++++++++++++++++++++++++++++++++++++++++++
> >  4 files changed, 146 insertions(+), 1 deletion(-)
> > 
> > diff --git a/doc/filters.texi b/doc/filters.texi
> > index 8bb132e..1dadba4 100644
> > --- a/doc/filters.texi
> > +++ b/doc/filters.texi
> > @@ -7394,10 +7394,12 @@ ffplay -f lavfi life=s=300x200:mold=10:r=60:ratio=0.1:death_color=#C83232:life_c
> >  @end example
> >  @end itemize
> >  
> > - at section color, nullsrc, rgbtestsrc, smptebars, smptehdbars, testsrc
> > + at section color, haldclutsrc, nullsrc, rgbtestsrc, smptebars, smptehdbars, testsrc
> >  
> >  The @code{color} source provides an uniformly colored input.
> >  
> > +The @code{haldclutsrc} source provides an identity Hald CLUT.
> 
> Maybe you could mention Eskil Steenberg's website:
> http://www.quelsolaar.com/technology/clut.html
> 

Added, but in haldclut patch. And added a reference to haldclut filter
from haldclutsrc.

> since that's the only place where a reader can find some information
> about Hald CLUTs.
> 

ImageMagick also has some doc.

> > +
> >  The @code{nullsrc} source returns unprocessed video frames. It is
> >  mainly useful to be employed in analysis / debugging tools, or as the
> >  source for filters which ignore the input data.
> > @@ -7426,6 +7428,11 @@ 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 level
> > +Specify the level of the Hald CLUT, only available in the @code{haldclutsrc}
> 
> > +source. A level of @code{N} generates a picture of @code{N*N*N} by @code{N*N*N}
> 
> pixels?
> 

Yes, added.

> Please also specify default value and range.
> 
> Also assuming N levels I'd assume N*N*N different RGB triplets to
> encode in the image. Where am I wrong?
> 

Added the following sentence in the doc: "Each component is
coded on a @code{1/(N*N)} scale." Does that answer your question?

> > +to be used an identity matrix for 3D lookup tables.
> 
> as identity matrix ?
> 

Fixed.

> > +
> >  @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
> > diff --git a/libavfilter/Makefile b/libavfilter/Makefile
> > index fa601b5..f53afb2 100644
> > --- a/libavfilter/Makefile
> > +++ b/libavfilter/Makefile
> > @@ -196,6 +196,7 @@ OBJS-$(CONFIG_ZMQ_FILTER)                    += f_zmq.o
> >  OBJS-$(CONFIG_CELLAUTO_FILTER)               += vsrc_cellauto.o
> >  OBJS-$(CONFIG_COLOR_FILTER)                  += vsrc_testsrc.o
> >  OBJS-$(CONFIG_FREI0R_SRC_FILTER)             += vf_frei0r.o
> > +OBJS-$(CONFIG_HALDCLUTSRC_FILTER)            += vsrc_testsrc.o
> >  OBJS-$(CONFIG_LIFE_FILTER)                   += vsrc_life.o
> >  OBJS-$(CONFIG_MANDELBROT_FILTER)             += vsrc_mandelbrot.o
> >  OBJS-$(CONFIG_MPTESTSRC_FILTER)              += vsrc_mptestsrc.o
> > diff --git a/libavfilter/allfilters.c b/libavfilter/allfilters.c
> > index 0f2442d..075ecca 100644
> > --- a/libavfilter/allfilters.c
> > +++ b/libavfilter/allfilters.c
> > @@ -193,6 +193,7 @@ void avfilter_register_all(void)
> >      REGISTER_FILTER(CELLAUTO,       cellauto,       vsrc);
> >      REGISTER_FILTER(COLOR,          color,          vsrc);
> >      REGISTER_FILTER(FREI0R,         frei0r_src,     vsrc);
> > +    REGISTER_FILTER(HALDCLUTSRC,    haldclutsrc,    vsrc);
> >      REGISTER_FILTER(LIFE,           life,           vsrc);
> >      REGISTER_FILTER(MANDELBROT,     mandelbrot,     vsrc);
> >      REGISTER_FILTER(MPTESTSRC,      mptestsrc,      vsrc);
> > diff --git a/libavfilter/vsrc_testsrc.c b/libavfilter/vsrc_testsrc.c
> > index 075026d..0083f12 100644
> > --- a/libavfilter/vsrc_testsrc.c
> > +++ b/libavfilter/vsrc_testsrc.c
> > @@ -71,6 +71,9 @@ typedef struct {
> >  
> >      /* only used by rgbtest */
> >      uint8_t rgba_map[4];
> > +
> > +    /* only used by haldclut */
> > +    int level;
> >  } TestSourceContext;
> >  
> >  #define OFFSET(x) offsetof(TestSourceContext, x)
> > @@ -269,6 +272,139 @@ AVFilter avfilter_vsrc_color = {
> >  
> >  #endif /* CONFIG_COLOR_FILTER */
> >  
> > +#if CONFIG_HALDCLUTSRC_FILTER
> > +
> > +static const AVOption haldclutsrc_options[] = {
> > +    { "level", "set level", OFFSET(level), AV_OPT_TYPE_INT, {.i64 = 6}, 2, 8, FLAGS },
> > +    COMMON_OPTIONS
> > +    { NULL }
> > +};
> > +
> > +AVFILTER_DEFINE_CLASS(haldclutsrc);
> > +
> > +static void haldclutsrc_fill_picture(AVFilterContext *ctx, AVFrame *frame)
> > +{
> > +    int i, j, k, x = 0, y = 0, is16bit = 0, step;
> > +    uint32_t alpha = 0;
> 
> > +    const TestSourceContext *s = ctx->priv;
> 
> Please use a more meaningful name ("test" is used in the rest of the
> file).
> 

test is not meaningful in this context, renamed to "hc".

> > +    int level = s->level;
> > +    float scale;
> > +    const int w = frame->width;
> > +    const int h = frame->height;
> > +    const uint8_t *data = frame->data[0];
> > +    const int linesize  = frame->linesize[0];
> > +    const AVPixFmtDescriptor *desc = av_pix_fmt_desc_get(frame->format);
> > +    uint8_t rgba_map[4];
> > +
> > +    av_assert0(w == h && w == level*level*level);
> > +
> > +    ff_fill_rgba_map(rgba_map, frame->format);
> > +
> > +    switch (frame->format) {
> > +    case AV_PIX_FMT_RGB48:
> > +    case AV_PIX_FMT_BGR48:
> > +    case AV_PIX_FMT_RGBA64:
> > +    case AV_PIX_FMT_BGRA64:
> > +        is16bit = 1;
> > +        alpha = 0xfffff;
> 
> shouldn't it be    0xffff ?
> 

Oups, fixed.

> > +        break;
> > +    case AV_PIX_FMT_RGBA:
> > +    case AV_PIX_FMT_BGRA:
> > +    case AV_PIX_FMT_ARGB:
> > +    case AV_PIX_FMT_ABGR:
> > +        alpha = 0xff;
> > +        break;
> > +    }
> > +
> > +    step  = av_get_padded_bits_per_pixel(desc) >> (3 + is16bit);
> > +    scale = ((float)(1 << (8*(is16bit+1))) - 1) / (level*level - 1);
> > +
> > +#define LOAD_CLUT(nbits) do {                                                   \
> > +    uint##nbits##_t *dst = ((uint##nbits##_t *)(data + y*linesize)) + x*step;   \
> > +    dst[rgba_map[0]] = av_clip_uint##nbits(i * scale);                          \
> > +    dst[rgba_map[1]] = av_clip_uint##nbits(j * scale);                          \
> > +    dst[rgba_map[2]] = av_clip_uint##nbits(k * scale);                          \
> > +    if (step == 4)                                                              \
> > +        dst[rgba_map[3]] = alpha;                                               \
> > +} while (0)
> > +
> > +    level *= level;
> 
> nit: a distinct level2 variable may be less confusing
> 

Sounds pointless, I prefer that way.

> > +    for (k = 0; k < level; k++) {
> > +        for (j = 0; j < level; j++) {
> > +            for (i = 0; i < level; i++) {
> > +                if (!is16bit)
> > +                    LOAD_CLUT(8);
> > +                else
> > +                    LOAD_CLUT(16);
> > +                if (++x == w) {
> > +                    x = 0;
> > +                    y++;
> > +                }
> > +            }
> > +        }
> > +    }
> > +}
> > +
> > +static av_cold int haldclutsrc_init(AVFilterContext *ctx)
> > +{
> > +    TestSourceContext *s = ctx->priv;
> > +    s->fill_picture_fn = haldclutsrc_fill_picture;
> > +    s->draw_once = 1;
> > +    return init(ctx);
> > +}
> > +
> > +static int haldclutsrc_query_formats(AVFilterContext *ctx)
> > +{
> > +    static const enum AVPixelFormat pix_fmts[] = {
> > +        AV_PIX_FMT_RGB24,  AV_PIX_FMT_BGR24,
> > +        AV_PIX_FMT_RGBA,   AV_PIX_FMT_BGRA,
> > +        AV_PIX_FMT_ARGB,   AV_PIX_FMT_ABGR,
> > +        AV_PIX_FMT_0RGB,   AV_PIX_FMT_0BGR,
> > +        AV_PIX_FMT_RGB0,   AV_PIX_FMT_BGR0,
> > +        AV_PIX_FMT_RGB48,  AV_PIX_FMT_BGR48,
> > +        AV_PIX_FMT_RGBA64, AV_PIX_FMT_BGRA64,
> > +        AV_PIX_FMT_NONE,
> > +    };
> > +    ff_set_common_formats(ctx, ff_make_format_list(pix_fmts));
> > +    return 0;
> > +}
> > +
> > +static int haldclutsrc_config_props(AVFilterLink *outlink)
> > +{
> > +    AVFilterContext *ctx = outlink->src;
> > +    TestSourceContext *s = ctx->priv;
> > +
> 
> > +    if (outlink->w || outlink->h) {
> > +        av_log(ctx, AV_LOG_ERROR, "output size must be configured with a level option\n");
> > +        return AVERROR(EINVAL);
> > +    }
> > +    s->w = s->h = s->level * s->level * s->level;
> 
> You could disable this option in this source, and set w/h depending on
> the level. Implies that you have to add size to the list of non-common
> options for the other sources (more lines but looks like the right
> thing to do).

I changed that. Using the w/h for the CLUT is not the common way of
defining it. Reference implementation and ImageMagick seems to rely on
that level.

-- 
Clément B.
-------------- 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/20130527/1f20dee1/attachment.asc>


More information about the ffmpeg-devel mailing list