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 <one...@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@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
signature.asc
Description: Digital signature
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
