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

[wm4]

On Mon, 26 Jan 2015 11:31:49 +0100
Christophe Gisquet <christophe.gisquet at gmail.com> wrote:

> 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)

But it's faster!!!!!!!!!111111111111

> 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.
>