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