[FFmpeg-devel] [PATCH] Port mp=eq/eq2 to lavfi

[Christophe Gisquet]

Hi,

so my comments are not aimed at that specific patch at all, but when looking at:

2015-01-24 22:06 GMT+01:00 Paul B Mahol <onemda at gmail.com>:
> +static void process_c(EQParameters *param, uint8_t *dst, int dst_stride,
> +                      uint8_t *src, int src_stride, int w, int h)
> +{
> +    int x, y, pel;
> +    int contrast, brightness;
> +
> +    contrast = (int) (param->contrast * 256 * 16);
> +    brightness = ((int) (100.0 * param->brightness + 100.0) * 511) / 200 - 128 - contrast / 32;
> +
> +    for (y = 0; y < h; y++) {
> +        for (x = 0; x < w; x++) {
> +            pel = ((src[y * src_stride + x] * contrast) >> 12) + brightness;
> +
> +            if (pel & 768)
> +                pel = (-pel) >> 31;
> +
> +            dst[y * dst_stride + x] = pel;
> +        }
> +    }
> +}

versus

> +static void process_MMX(EQParameters *param, uint8_t *dst, int dst_stride,
> +                        uint8_t *src, int src_stride, int w, int h)
> +{
[...]
> +
> +        while (h--) {
[...]
> +                for (i = w&7; i; i--) {
> +                        pel = ((*src++ * contrast) >> 12) + brightness;
> +                        if (pel & 768)
> +                            pel = (-pel) >> 31;
> +                        *dst++ = pel;
> +                }
> +        }

... what's the sense in this?

The MMX function looks broken, as if the "overflow" check would only
apply to the end of the lines. And iirc that doesn't pass fate, as C
and MMX produces different output. (see my reply in some thread where
I also display the CRC of the output sequence)

Overall, why 768? And not eg ~255? or "<0" ? Sorry for being dense and
not understanding what's done here. To me, it looks like just a normal
clipping is needed.

So maybe that thing should be checked by people that have a clue on
what that filter attempts to do, or that have a purpose in using it.
Then make that valid for MMX, and have the C match it.

-- 
Christophe