[FFmpeg-devel] [PATCH] avfilter/vf_nlmeans: add >8 bit support

[Clément Bœsch]

On Wed, Nov 20, 2019 at 10:54:42AM +0100, Paul B Mahol wrote:
> Signed-off-by: Paul B Mahol <onemda at gmail.com>
> ---
>  libavfilter/aarch64/vf_nlmeans_init.c |   6 +-
>  libavfilter/nlmeans_template.c        | 370 ++++++++++++++++++
>  libavfilter/vf_nlmeans.c              | 539 ++++++--------------------
>  libavfilter/vf_nlmeans.h              |  59 ++-
>  4 files changed, 548 insertions(+), 426 deletions(-)
>  create mode 100644 libavfilter/nlmeans_template.c

On Aarch64:

In file included from src/libavfilter/aarch64/vf_nlmeans_init.c:20:
src/libavfilter/vf_nlmeans.h:52:11: error: unknown type name 'AVClass'
   52 |     const AVClass *class;
      |           ^~~~~~~
In file included from src/libavfilter/aarch64/vf_nlmeans_init.c:20:
src/libavfilter/vf_nlmeans.h:74:26: error: unknown type name 'AVFilterContext'
   74 |     int (*nlmeans_plane)(AVFilterContext *ctx, int w, int h, int p, int r,
      |                          ^~~~~~~~~~~~~~~
src/libavfilter/vf_nlmeans.h:77:1: warning: no semicolon at end of struct or union
   77 | } NLMeansContext;
      | ^
CC	libavfilter/vf_nlmeans.o

Note: not the same issue as the one reported by Michael.

[...]
> -static av_cold int init(AVFilterContext *ctx)
> +static int config_input(AVFilterLink *inlink)
>  {
> -    int i;
> +    AVFilterContext *ctx = inlink->dst;
>      NLMeansContext *s = ctx->priv;
> -    const double h = s->sigma * 10.;
> +    const AVPixFmtDescriptor *desc = av_pix_fmt_desc_get(inlink->format);
> +    double h;
> +    int e;
>  

> +    s->depth = desc->comp[0].depth;
> +    h = s->sigma * 10. * (1 << (s->depth - 8));
>      s->pdiff_scale = 1. / (h * h);
> -    s->max_meaningful_diff = log(255.) / s->pdiff_scale;
> +    s->max_meaningful_diff = FFMIN(log(255.) / s->pdiff_scale, INT32_MAX / 8);

I'm assuming log((1 << s->depth) - 1) / s->pdiff_scale wasn't doing the expected?

Can you elaborate on this FFMIN?

>      s->weight_lut = av_calloc(s->max_meaningful_diff, sizeof(*s->weight_lut));
>      if (!s->weight_lut)
>          return AVERROR(ENOMEM);
> -    for (i = 0; i < s->max_meaningful_diff; i++)
> +    for (int i = 0; i < s->max_meaningful_diff; i++)

Could be split but...

>          s->weight_lut[i] = exp(-i * s->pdiff_scale);
>  
>      CHECK_ODD_FIELD(research_size,   "Luma research window");
> @@ -542,11 +183,75 @@ static av_cold int init(AVFilterContext *ctx)
>      s->patch_hsize       = s->patch_size       / 2;
>      s->patch_hsize_uv    = s->patch_size_uv    / 2;
>  
> +    e = FFMAX(s->research_hsize, s->research_hsize_uv) +
> +        FFMAX(s->patch_hsize,    s->patch_hsize_uv);
>      av_log(ctx, AV_LOG_INFO, "Research window: %dx%d / %dx%d, patch size: %dx%d / %dx%d\n",
>             s->research_size, s->research_size, s->research_size_uv, s->research_size_uv,
>             s->patch_size,    s->patch_size,    s->patch_size_uv,    s->patch_size_uv);

You're moving a lot of code around, I believe the patch could have been split,
it's hard to follow as is.

[...]
> -void ff_nlmeans_init(NLMeansDSPContext *dsp);
> -void ff_nlmeans_init_aarch64(NLMeansDSPContext *dsp);
> +struct weighted_avg {
> +    float total_weight;
> +    float sum;
> +};

No point in switching to double for 9+ bits?

-- 
Clément B.