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

[Ashish Pratap Singh]

Hi,

On Sat, Jul 29, 2017 at 3:15 PM, Moritz Barsnick <barsnick at gmx.net> wrote:

> On Sat, Jul 29, 2017 at 14:16:29 +0530, Ashish Pratap Singh wrote:
>
> > +Both input videos must have the same resolution and pixel format for
> > +this filter to work correctly. Also it assumes that both inputs
> > +have the same number of frames, which are compared one by one.
>
> Nit, for all these filters: If the inputs are checked, you should
> probably just write "must have the same resolution and pixel format",
> or "must have the same resolution and pixel format to work". Since it's
> checked, there's no chance of working incorrectly.
>
> > +const int vif_filter1d_width1[4] = { 17, 9, 5, 3 };
> > +const float vif_filter1d_table[4][17] = {
> > +    { 0x1.e8a77p-8,  0x1.d373b2p-7, 0x1.9a1cf6p-6, 0x1.49fd9ep-5,
> 0x1.e7092ep-5,
>
> My brain has a hard time interpreting scientific hex notation, but I
> guess it's fine. ;-)
>
> Later it will be converted to fixed point.

> > +                num_val = 1.0 - sigma2_sq*sigma_max_inv;
> > +                den_val = 1.0;
>
> 1.0f to avoid double promotion.
>
> > +    if (*score_den == 0.0) {
> > +        *score = 1.0f;
>
> Nit: I know the compiler fixes the right hand side, but since you're
> assigning to a double, you shouldn't force the const to a float. "0.0"
> or "0" as you chose elsewhere is more consistent.
>
> Ok, point noted.

> > +    int i,j; \
> [...]
> > +    for(i = 0; i < h; i++) { \
> > +        for(j = 0; j < w; j++) { \
>
> Minor nit: Whitespace. "for (", "i, j"
>
> > +        av_log(ctx, AV_LOG_ERROR, "Width and height of input videos
> must be same.\n");
>
> Either "must be the same" or "must be identical".
>
> > +        av_log(ctx, AV_LOG_ERROR, "error: av_malloc failed for
> data_buf.\n");
>
> Are these messages of any help to anyone?
>
What would be an appropriate error message or should I don't write anything
at all?

>
> Moritz
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>