Hi,
On Thu, May 5, 2016 at 8:16 AM, Michael Niedermayer <mich...@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.
> > +
> > +@item dither
> > +Specify dithering mode.
> > +
> > +The accepted values are:
> > +@table @samp
> > +@item none
> > +No dithering
> > +
> > +@item fsb
> > +Floyd-Steinberg dithering
> > +@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
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
