[FFmpeg-devel] [PATCH] [GSOC] libavfilter/vf_colorconstancy.c : Adding weighted greyedge

[Moritz Barsnick]

On Wed, Apr 15, 2020 at 01:32:16 +0530, YATENDRA SINGH wrote:

As Michael noted, please resend without broken like feeds. I can't read
most of the diff the way it is now.

Some notes nevertheless:

> Signed-off-by: Yatendra Singh <yatendras at iitbhilai.ac.in>
> ---
>  libavfilter/Makefile            |   1 +
>  libavfilter/allfilters.c        |   1 +
>  libavfilter/vf_colorconstancy.c | 265 +++++++++++++++++++++++++++-----
>  3 files changed, 232 insertions(+), 35 deletions(-)

Documentation update missing (and eventually changelog).

>  #define SQRT3 1.73205080757
> +#define NORAMAL_WHITE 1/SQRT3

Unusued? (And did you mean "NORMAL"?

> +    for (plane = 0; plane < NUM_PLANES; plane++)
> +    {

Bracket style.

> +        for(int h = slice_start; h < slice_end; h++)
> +        {

Bracket style (also "for(" -> "for (").

> +    for (plane = 0; plane < NUM_PLANES; ++plane) {

ffmpeg prefers '++' appended (behind the variable).

> +    while( num_iters < s->max_iters )

Bracket style.

>      s->filtersize = 2 * floor(break_off_sigma * sigma + 0.5) + 1;
> -    if (ret=set_gauss(ctx)) {
> +    if (ret = set_gauss(ctx)) {

Here, you fix existing style. While the result is correct, it doesn't
belong in the same commit as functional changes. On the other hand, you
introduce dozens of style errors.
Do check this:
https://www.ffmpeg.org/developer.html#Code-formatting-conventions
and other sections of ffmpeg code for reference.

>      if (!direct)
> -        av_frame_free(&in);
> +            av_frame_free(&in);

Also an unrelated style fix.

> +#if CONFIG_WEIGHTED_GREYEDGE_FILTER

Shouldn't sections of your code also be disabled if this is not set,
not only the options? And intermingled with #if CONFIG_GREYEDGE_FILTER?

> +#endif /* CONFIG_WEIGHTED_GREY_EDGE_FILTER */
> \ No newline at end of file

Please add a newline at the end of the file.

Cheers,
Moritz