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

[Moritz Barsnick]

On Thu, Mar 30, 2017 at 23:06:36 +0200, Betty Wu wrote:
> A new filter ANSNR is added. libavfilter/Makefile is changed.

That line isn't really needed in a commit message (too obvious).

> +/*
> + * Copyright 2016-2017 Netflix, Inc.
> + * Copyright (c) 2017 Betty Wu
> + *
> + * This file is part of FFmpeg.
> + *
> + * FFmpeg is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.

Are you the copyright holder of this code? It looks very similar to
https://github.com/Netflix/vmaf/blob/master/feature/src/ansnr.c and its
dependencies, which are licensed Apache 2.0. IINAL, but you can't
"just" change the license, unless given permission to do so.

> +/* Whether to use border replication instead of zero extension. */
> +/* #define ANSNR_OPT_BORDER_REPLICATE */
> +
> +/* Whether to save intermediate results to files. */
> +/* #define ANSNR_OPT_DEBUG_DUMP */
> +
> +/* Whether to use a 1-D approximation of filters. */
> +/* #define ANSNR_OPT_FILTER_1D */
> +
> +/* Whether to normalize result by dividing against maximum ANSNR. */
> +/* #define ANSNR_OPT_NORMALIZE */
> +
> +/* Whether to use single precision for computation. */
> +#define ANSNR_OPT_SINGLE_PRECISION
> +//#define ANSNR_OPT_DOUBLE_PRECISION

This produces a lot of dead code. Should these not be run-time options,
so that the user can make use of all the code? (Single vs. double
precision could possibly be maintained as two separate filters in one
source file, instead of a run-time option, if there's any reasoning for
keeping both.)

> +#endif /* ANSNR_OPTIONS_H_ */

This shows that this is a verbatim copy of ansnr_options.h from the
named link, and that you don't know what these #define guards are good
for. ;-)


Just some random comments to the source code, I didn't try to
understand the functionality:

> +                    if (ii < 0) ii = -ii;

FFABS()

> +                    imgcoeff = (float)src[ii * src_px_stride + jj]+OPT_RANGE_PIXEL_OFFSET;

Please leave spaces around operators for readability.

> +    if (SIZE_MAX / buf_sz_one < 3)
> +    {
> +        goto fail;
> +    }

ffmpeg's bracket style is different (and some say they could be omitted
here, some say not).

> +    ref_filtr = (number_t *)data_top; data_top += buf_sz_one;

I believe it is preferred not to combine two statements onto one line.


> +    if( !strcmp(frame_format,"yuv420p") || !strcmp(frame_format,"yuv422p") || !strcmp(frame_format,"yuv444p"))
> +        psnr_max=60;
> +    if( !strcmp(frame_format,"yuv420p10le") || !strcmp(frame_format,"yuv422p10le") || !strcmp(frame_format,"yuv444p10le"))
> +        psnr_max=72;

Bracket style.

> +    s->score=score;

Whitespace.

> +    //if (ARCH_X86)
> +    //    ff_ansnr_init_x86(&s->dsp, desc->comp[0].depth);

Unused code shouldn't go in.

Another observation:
Is stats_version initialized anywhere?

Moritz