[FFmpeg-devel] [PATCH] avfilter: add nlmeans filter

[Clément Bœsch]

On Wed, Sep 21, 2016 at 02:39:55PM +0200, Benoit Fouet wrote:
[...]
> > diff --git a/Changelog b/Changelog
> > index 2d0a449..a5282b4 100644
> > --- a/Changelog
> > +++ b/Changelog
> > @@ -31,6 +31,7 @@ version <next>:
> >   - MediaCodec HEVC decoding
> >   - TrueHD encoder
> >   - Meridian Lossless Packing (MLP) encoder
> > +- nlmeans filter (denoiser)
> 
> The full name could be used here: Non-Local Means (nlmeans) denoising filter
> 

changed.

> >   version 3.1:
> > diff --git a/doc/filters.texi b/doc/filters.texi
> > index 070e57d..7e9ab60 100644
> > --- a/doc/filters.texi
> > +++ b/doc/filters.texi
> > @@ -9695,6 +9695,41 @@ Negate input video.
> >   It accepts an integer in input; if non-zero it negates the
> >   alpha component (if available). The default value in input is 0.
> > + at section nlmeans
> > +
> > +Denoise frames using Non-Local Means algorithm.
> > +
> > +Each pixel is adjusted by looking for other pixels with similar contexts. This
> > +context similarity is defined by their surrounding patch of size
> 
> "is defined by comparing their surrounding patches" ?
> 

OK

> > + at option{p}x at option{p}. Patches are researched in an area of
> > + at option{r}x at option{r} surrouding the pixel.
> > +
> 
> surrounding, or even simply "around"
> Also "research" sounds weird (I'd use "search"), but maybe wait for someone
> native to comment
> 

Sounds good, changed.

[...]
> > +    int patch_size,   patch_hsize;          // patch size and half size
> > +    int patch_size_c, patch_hsize_c;        // patch size and half size for chroma planes
> 
> nit: could you use _uv instead of _c (the latter has a taste of C vs ASM)?
> 

Yeah, renamed.

[...]
> > +static const AVOption nlmeans_options[] = {
> > +    { "s",  "denoising strength", OFFSET(sigma), AV_OPT_TYPE_DOUBLE, { .dbl = 1.0 }, 1.0, 30.0, FLAGS },
> > +    { "p",  "patch size",                   OFFSET(patch_size),   AV_OPT_TYPE_INT, { .i64 = 3*2+1 }, 0, 99, FLAGS },
> > +    { "pc", "patch size for chroma planes", OFFSET(patch_size_c), AV_OPT_TYPE_INT, { .i64 = 0 },     0, 99, FLAGS },
> > +    { "r",  "research window",                   OFFSET(research_size),   AV_OPT_TYPE_INT, { .i64 = 7*2+1 }, 0, 99, FLAGS },
> 
> Nit: if there is a correlation between the default patch size and research
> window size, maybe add defines? Like:
> #define NLMEANS_DEFAULT_PATCH_SIZE (3*2+1)
> #define NLMEANS_DEFAULT_RESEARCH_WINDOW_SIZE (NLMEANS_DEFAULT_PATCH_SIZE +
> 1)

No real relationship here actually.

[...]
> > +static inline int get_ssd_patch(const uint32_t *ii, int ii_lz_32, int x, int y, int p)
> 
> Actually, this is not really about SSD value here. This function does not
> really care about what has been integrated, it just compute the patch value
> only by knowing the buffer is an integral image.
> get_patch_value (maybe too much generic)?
> Anyway... this is nitpicking, feel free to just ignore, I just felt I needed
> to explain a bit more why I wanted another name :-)
> 

Renamed to get_integral_patch_value()

[...]
> > + * The above line and left column of dst are always readable.
> > + *
> 
> The line above dst and the column to its left are always readable.
> 

changed

[...]
> > + * On the other hand, the above line and left column of dst are still always
> > + * readable.
> > + *
> 
> same
> 

also changed

[...]
> > +        for (x = startx; x < startx + w; x++) {
> > +            const int s1x = av_clip(x -  r,         0, sw - 1);
> > +            const int s2x = av_clip(x - (r + offx), 0, sw - 1);
> > +            const int s1y = av_clip(y -  r,         0, sh - 1);
> > +            const int s2y = av_clip(y - (r + offy), 0, sh - 1);
> > +            const uint8_t v1 = src[s1y*linesize + s1x];
> > +            const uint8_t v2 = src[s2y*linesize + s2x];
> > +            const int d = v1 - v2;
> > +            acc += d * d;
> > +            dst[y*dst_linesize_32 + x] = dst[(y-1)*dst_linesize_32 + x] + acc;
> > +        }
> 
> I can understand this is done on smaller portions, but it would still be
> good to (at least) move the y-only parts out of the x loop.
> 

Yeah sure, moved s1y and s2y out of the x loop

[...]
> > +    // allocate weighted average for every pixel
> > +    s->wa_linesize = inlink->w;
> > +    s->wa = av_malloc_array(s->wa_linesize, inlink->h * sizeof(*s->wa));
> > +    if (!s->wa)
> > +        return AVERROR(ENOMEM);
> 
> this leaks s->ii_orig
> 

No that's fine, uninit() is called when init() fails

> > +static av_cold void uninit(AVFilterContext *ctx)
> > +{
> > +    NLMeansContext *s = ctx->priv;
> > +    av_freep(&s->ii_orig);
> 
> s->wa also needs to be freed
> 

Ah I fixed that in another branch but forgot to backport. Fixed.

Patch applied, thanks for the review

-- 
Clément B.