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

[Michael Niedermayer]

On Mon, Jan 26, 2015 at 11:31:49AM +0100, Christophe Gisquet 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.

packuswb should do the cliping in process_MMX


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

yes

also on a differnt topic, i think "gamma*" should be removed from
eq, the implementation is simply wrong and it cannot be done
conveniently in a yuv based filter.
we already have lut based filters for rgb space, they could easily
do correct gamma, a "gamma" filter could be added based on that


> 
> 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
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

It is dangerous to be right in matters on which the established authorities
are wrong. -- Voltaire
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20150126/2e2ecb07/attachment.asc>