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.
> > +@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
> > +@option{p}x@option{p}. Patches are researched in an area of
> > +@option{r}x@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.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
