[FFmpeg-devel] [PATCH] avfilter: add wpsnr video filter

[Tomas Härdin]

tor 2021-10-28 klockan 21:09 +0200 skrev Paul B Mahol:
> 
> +FRAMESYNC_DEFINE_CLASS(wpsnr, WPSNRContext, fs);
> +
> +#define COMPUTE_HX(type, stype, depth)               \
> +static void compute_hx##depth(const uint8_t *ssrc,   \
> +                              int linesize,          \
> +                              int w, int h,          \
> +                              uint16_t *dstp,        \
> +                              int dst_linesize)      \
> +{                                                    \
> +    const type *src = (const type *)ssrc;            \
> +    stype *dst = (stype *)dstp;                      \
> +                                                     \
> +    linesize /= (depth / 8);                         \

Can linesize ever be odd? Probably not, so this should be fine.

> +static double get_hx(const uint8_t *src, int linesize, int w, int h)
> +{
> +    int64_t sum = 0;
> +
> +    for (int y = 0; y < h; y++) {
> +        for (int x = 0; x < w; x++) {
> +            sum += 12 * src[x] -
> +                    2 * (src[x-1] + src[x+1] +
> +                         src[x + linesize] +
> +                         src[x - linesize]) -
> +                    1 * (src[x - 1 - linesize] +
> +                         src[x + 1 - linesize] +
> +                         src[x - 1 + linesize] +
> +                         src[x + 1 + linesize]);
> +        }
> +
> +        src += linesize;
> +    }
> +
> +    return fabs(sum * 0.25);
> +}
> +
> +static double get_hx16(const uint8_t *ssrc, int linesize, int w, int
> h)
> +{
> +    const uint16_t *src = (const uint16_t *)ssrc;

This is not -fstrict-aliasing safe

> +    int64_t sum = 0;
> +
> +    linesize /= 2;
> +
> +    for (int y = 0; y < h; y++) {
> +        for (int x = 0; x < w; x++) {
> +            sum += 12 * src[x] -
> +                    2 * (src[x-1] + src[x+1] +
> +                         src[x + linesize] +
> +                         src[x - linesize]) -
> +                    1 * (src[x - 1 - linesize] +
> +                         src[x + 1 - linesize] +
> +                         src[x - 1 + linesize] +
> +                         src[x + 1 + linesize]);
> +        }
> +
> +        src += linesize;
> +    }
> +
> +    return fabs(sum * 0.25);
> +}

Why not use the same kind of templatization for these as compute_hx*?

> +
> +static double get_sd(const uint8_t *ref, int ref_linesize,
> +                     const uint8_t *main, int main_linesize,
> +                     int w, int h)
> +{
> +    int64_t sum = 0;
> +
> +    for (int y = 0; y < h; y++) {
> +        for (int x = 0; x < w; x++)
> +            sum += pow_2(ref[x] - main[x]);
> +        ref += ref_linesize;
> +        main += main_linesize;
> +    }
> +
> +    return sum;
> +}
> +
> +static double get_sd16(const uint8_t *rref, int ref_linesize,
> +                       const uint8_t *mmain, int main_linesize,
> +                       int w, int h)
> +{
> +    const uint16_t *ref = (const uint16_t *)rref;
> +    const uint16_t *main = (const uint16_t *)mmain;
> +    int64_t sum = 0;
> +
> +    ref_linesize /= 2;
> +    main_linesize /= 2;
> +
> +    for (int y = 0; y < h; y++) {
> +        for (int x = 0; x < w; x++)
> +            sum += pow_2(ref[x] - main[x]);
> +        ref += ref_linesize;
> +        main += main_linesize;
> +    }
> +
> +    return sum;
> +}

Same here, and for more functions in the patch it seems so I'm not
going to bother repeating myself any more

> +static void set_meta(AVDictionary **metadata, const char *key, char
> comp, float d)
> +{
> +    char value[128];
> +    snprintf(value, sizeof(value), "%f", d);
> +    if (comp) {
> +        char key2[128];
> +        snprintf(key2, sizeof(key2), "%s%c", key, comp);
> +        av_dict_set(metadata, key2, value, 0);
> +    } else {
> +        av_dict_set(metadata, key, value, 0);
> +    }
> +}

We should probably add av_dict_set_* for int, double etc at some point

/Tomas