[FFmpeg-devel] [PATCH] vf_colorspace: add floyd-steinberg dithering option to full conversion.

[Ronald S. Bultje]

Hi,

On Thu, May 5, 2016 at 8:16 AM, Michael Niedermayer <michael at niedermayer.cc>
wrote:

> On Tue, May 03, 2016 at 01:53:42PM -0400, Ronald S. Bultje wrote:
> > ---
> >  doc/filters.texi                     |  13 ++++
> >  libavfilter/colorspacedsp.c          |  12 ++++
> >  libavfilter/colorspacedsp.h          |   6 ++
> >  libavfilter/colorspacedsp_template.c | 128
> +++++++++++++++++++++++++++++++++++
> >  libavfilter/vf_colorspace.c          |  58 +++++++++++++++-
> >  5 files changed, 214 insertions(+), 3 deletions(-)
> >
> > diff --git a/doc/filters.texi b/doc/filters.texi
> > index b17b115..98a002b 100644
> > --- a/doc/filters.texi
> > +++ b/doc/filters.texi
> > @@ -5104,6 +5104,19 @@ YUV 4:4:4 planar 12-bits
> >  Do a fast conversion, which skips gamma/primary correction. This will
> take
> >  significantly less CPU, but will be mathematically incorrect. To get
> output
> >  compatible with that produced by the colormatrix filter, use fast=1.
> > +
> > + at item dither
> > +Specify dithering mode.
> > +
> > +The accepted values are:
> > + at table @samp
> > + at item none
> > +No dithering
> > +
> > + at item fsb
> > +Floyd-Steinberg dithering
> > + at end table
> > +
> >  @end table
> >
> >  The filter converts the transfer characteristics, color space and color
> > diff --git a/libavfilter/colorspacedsp.c b/libavfilter/colorspacedsp.c
> > index d4c43c3..f95805b 100644
> > --- a/libavfilter/colorspacedsp.c
> > +++ b/libavfilter/colorspacedsp.c
> > @@ -20,6 +20,9 @@
> >
> >  #include "colorspacedsp.h"
> >
>
> > +/*
> > + * SS_W/H are synonyms for AVPixFmtDescriptor->log2_chroma_w/h.
>
> doesnt explain what SS stands for, that makes this harder to remember
>

Fixed.

> @@ -114,6 +117,15 @@ void ff_colorspacedsp_init(ColorSpaceDSPContext *dsp)
> >      init_rgb2yuv_fn(1, 10);
> >      init_rgb2yuv_fn(2, 12);
> >
> > +#define init_rgb2yuv_fsb_fn(idx, bit) \
> > +    dsp->rgb2yuv_fsb[idx][0] = rgb2yuv_fsb_444p##bit##_c; \
> > +    dsp->rgb2yuv_fsb[idx][1] = rgb2yuv_fsb_422p##bit##_c; \
> > +    dsp->rgb2yuv_fsb[idx][2] = rgb2yuv_fsb_420p##bit##_c
> > +
> > +    init_rgb2yuv_fsb_fn(0,  8);
> > +    init_rgb2yuv_fsb_fn(1, 10);
> > +    init_rgb2yuv_fsb_fn(2, 12);
>
> is there a plan for bit depth other than these and subsamplings the
> than these ?
>

For bit depth: I currently don't need more, but I see no reason why we
wouldn't eventually want to support up to 14 here (which is the hevc/h264
upper limit, right?).

for chroma subsamplings: not sure. My feeling is that this should address
most real-world needs, but this is obviously hard to say. I think if we
want more, we should simply fix the indexing into this array in
vf_colorspace.c from being hardcoded into working only for 444/422/420 into
becoming a real AV_PIX_FMT_* <-> internal-enum mapping.

0,1,2 is bad, these should be named constants/enums for the subsamplings
> or maybe a 2d array
>

Enum done, but in separate patch.

> diff --git a/libavfilter/colorspacedsp.h b/libavfilter/colorspacedsp.h
> > index 4e70c6c..2ca7b19 100644
> > --- a/libavfilter/colorspacedsp.h
> > +++ b/libavfilter/colorspacedsp.h
> > @@ -32,6 +32,11 @@ typedef void (*rgb2yuv_fn)(uint8_t *yuv[3], ptrdiff_t
> yuv_stride[3],
> >                             int16_t *rgb[3], ptrdiff_t rgb_stride,
> >                             int w, int h, const int16_t
> rgb2yuv_coeffs[3][3][8],
> >                             const int16_t yuv_offset[8]);
>
> > +typedef void (*rgb2yuv_fsb_fn)(uint8_t *yuv[3], ptrdiff_t yuv_stride[3],
> > +                               int16_t *rgb[3], ptrdiff_t rgb_stride,
> > +                               int w, int h, const int16_t
> rgb2yuv_coeffs[3][3][8],
> > +                               const int16_t yuv_offset[8],
> > +                               int *rnd[3][2]);
>
> lacks documentation
> the strides also can be const as well as rgb
>
>
> >  typedef void (*yuv2yuv_fn)(uint8_t *yuv_out[3], ptrdiff_t
> yuv_out_stride[3],
> >                             uint8_t *yuv_in[3], ptrdiff_t
> yuv_in_stride[3],
>
> off topic but, same here


Both done, as a separate patch also.

>                             int w, int h, const int16_t
> yuv2yuv_coeffs[3][3][8],
> > @@ -40,6 +45,7 @@ typedef void (*yuv2yuv_fn)(uint8_t *yuv_out[3],
> ptrdiff_t yuv_out_stride[3],
> >  typedef struct ColorSpaceDSPContext {
> >      yuv2rgb_fn yuv2rgb[3 /* 0: 8bit, 1: 10bit, 2: 12bit */][3 /* 0:
> 444, 1: 422, 2: 420 */];
> >      rgb2yuv_fn rgb2yuv[3 /* 0: 8bit, 1: 10bit, 2: 12bit */][3 /* 0:
> 444, 1: 422, 2: 420 */];
> > +    rgb2yuv_fsb_fn rgb2yuv_fsb[3 /* 0: 8bit, 1: 10bit, 2: 12bit */][3
> /* 0: 444, 1: 422, 2: 420 */];
> >      yuv2yuv_fn yuv2yuv[3 /* in_depth */][3 /* out_depth */][3 /* 0:
> 444, 1: 422, 2: 420 */];
> >
> >      void (*multiply3x3)(int16_t *data[3], ptrdiff_t stride,
> > diff --git a/libavfilter/colorspacedsp_template.c
> b/libavfilter/colorspacedsp_template.c
> > index f225391..db4a8d2 100644
> > --- a/libavfilter/colorspacedsp_template.c
> > +++ b/libavfilter/colorspacedsp_template.c
>
> > @@ -199,6 +199,134 @@ static void fn(rgb2yuv)(uint8_t *_yuv[3],
> ptrdiff_t yuv_stride[3],
> >      }
> >  }
> >
> > +/* floyd-steinberg dithering - for any mid-top pixel A in a 3x2 block
> of pixels:
> > + *    1 A 2
> > + *    3 4 5
> > + * the rounding error is distributed over the neighbouring pixels:
> > + *    2: 7/16th, 3: 3/16th, 4: 5/16th and 5: 1/16th
> > + */
>
> do we want non public functions to be available in doxygen ?
> if no then ok otherwise missing /**
>

I believe doxy is typically for externally visible functions only, so I'll
keep /* instead of /** for now.

    [...]
> > @@ -888,15 +910,39 @@ static int filter_frame(AVFilterLink *link,
> AVFrame *in)
> >      else if (s->iall != CS_UNSPECIFIED)
> >          in->color_trc = default_trc[s->iall];
> >      if (rgb_sz != s->rgb_sz) {
> > +        const AVPixFmtDescriptor *desc =
> av_pix_fmt_desc_get(out->format);
> > +        int uvw = in->width >> desc->log2_chroma_w;
> > +
> >          av_freep(&s->rgb[0]);
> >          av_freep(&s->rgb[1]);
> >          av_freep(&s->rgb[2]);
> >          s->rgb_sz = 0;
> > +        av_freep(&s->dither_scratch_base[0][0]);
> > +        av_freep(&s->dither_scratch_base[0][1]);
> > +        av_freep(&s->dither_scratch_base[1][0]);
> > +        av_freep(&s->dither_scratch_base[1][1]);
> > +        av_freep(&s->dither_scratch_base[2][0]);
> > +        av_freep(&s->dither_scratch_base[2][1]);
> >
> >          s->rgb[0] = av_malloc(rgb_sz);
> >          s->rgb[1] = av_malloc(rgb_sz);
> >          s->rgb[2] = av_malloc(rgb_sz);
> > -        if (!s->rgb[0] || !s->rgb[1] || !s->rgb[2]) {
>
> > +        s->dither_scratch_base[0][0] = av_malloc(sizeof(int) *
> (in->width + 4));
> > +        s->dither_scratch_base[0][1] = av_malloc(sizeof(int) *
> (in->width + 4));
> > +        s->dither_scratch_base[1][0] = av_malloc(sizeof(int) * (uvw +
> 4));
> > +        s->dither_scratch_base[1][1] = av_malloc(sizeof(int) * (uvw +
> 4));
> > +        s->dither_scratch_base[2][0] = av_malloc(sizeof(int) * (uvw +
> 4));
> > +        s->dither_scratch_base[2][1] = av_malloc(sizeof(int) * (uvw +
> 4));
>
> sizeof should be using dither_scratch_base not duplicating its type


Fixed.

Ronald