Hi,

On Sun, Apr 1, 2012 at 4:55 AM, Diego Biurrun <[email protected]> wrote:
>  DECLARE_ALIGNED(8, const uint8_t, dither_8x8_128)[8][8] = {
> -{  36, 68, 60, 92, 34, 66, 58, 90,},
> -{ 100,  4,124, 28, 98,  2,122, 26,},
> -{  52, 84, 44, 76, 50, 82, 42, 74,},
> -{ 116, 20,108, 12,114, 18,106, 10,},
> -{  32, 64, 56, 88, 38, 70, 62, 94,},
> -{  96,  0,120, 24,102,  6,126, 30,},
> -{  48, 80, 40, 72, 54, 86, 46, 78,},
> -{ 112, 16,104,  8,118, 22,110, 14,},
> +    { 36,  68, 60,  92, 34,  66, 58,  90, },
> +    { 100, 4,  124, 28, 98,  2,  122, 26, },
> +    { 52,  84, 44,  76, 50,  82, 42,  74, },
> +    { 116, 20, 108, 12, 114, 18, 106, 10, },
> +    { 32,  64, 56,  88, 38,  70, 62,  94, },
> +    { 96,  0,  120, 24, 102, 6,  126, 30, },
> +    { 48,  80, 40,  72, 54,  86, 46,  78, },
> +    { 112, 16, 104, 8,  118, 22, 110, 14, },
>  };

Please right-align integers:

  0, x
 64, x
128, x

instead of

0,   x
64,  x
128, x

> -        for (j = 0; j < filterSize; j++) {
> +        for (j = 0; j < filterSize; j++)
>             val += src[srcPos + j] * filter[filterSize * i + j];
> -        }

I've seen you do this in a few places now, I really don't know if it's
a good idea to remove these brackets unconditionally... This isn't
K&R, this is third/first person bikesheddery. I want blue.

> -        for (j = 0; j < filterSize; j++) {
> +        for (j = 0; j < filterSize; j++)
>             val += src[srcPos + j] * filter[filterSize * i + j];
> -        }

Blue.

> -        for (j=0; j<filterSize; j++) {
> -            val += ((int)src[srcPos + j])*filter[filterSize*i + j];
> -        }
> +        for (j = 0; j < filterSize; j++)
> +            val += ((int)src[srcPos + j]) * filter[filterSize * i + j];

Blue.

> -        //filter += hFilterSize;
> -        dst[i] = FFMIN(val>>7, (1<<15)-1); // the cubic equation does 
> overflow ...
> -        //dst[i] = val>>7;
> +        // filter += hFilterSize;
> +        dst[i] = FFMIN(val >> 7, (1 << 15) - 1); // the cubic equation does 
> overflow ...
> +        // dst[i] = val>>7;

Just remove these two comment lines (filter += ... and dst[i] = ...).

> -        for (j=0; j<filterSize; j++) {
> -            val += ((int)src[srcPos + j])*filter[filterSize*i + j];
> -        }
> +        for (j = 0; j < filterSize; j++)
> +            val += ((int)src[srcPos + j]) * filter[filterSize * i + j];

Blue.

> -        //filter += hFilterSize;
> -        dst[i] = FFMIN(val>>3, (1<<19)-1); // the cubic equation does 
> overflow ...
> -        //dst[i] = val>>7;
> +        // filter += hFilterSize;
> +        dst[i] = FFMIN(val >> 3, (1 << 19) - 1); // the cubic equation does 
> overflow ...
> +        // dst[i] = val>>7;

Remove two comment lines.

> +    const int vChrBufSize            = c->vChrBufSize;
> +    uint8_t *formatConvBuffer        = c->formatConvBuffer;
> +    const int chrSrcSliceY =     srcSliceY  >> c->chrSrcVSubSample;
> +    const int chrSrcSliceH = -((-srcSliceH) >> c->chrSrcVSubSample);

?

> +    yuv2packed2_fn yuv2packed2     = c->yuv2packed2;
> +    yuv2packedX_fn yuv2packedX     = c->yuv2packedX;
>     int should_dither = is9_OR_10BPS(c->srcFormat) || is16BPS(c->srcFormat);

?

> +    for (; dstY < dstH; dstY++) {
> +        const int chrDstY = dstY >> c->chrDstVSubSample;
> +        uint8_t *dest[4]  = {
>             dst[0] + dstStride[0] * dstY,
>             dst[1] + dstStride[1] * chrDstY,
>             dst[2] + dstStride[2] * chrDstY,
> -            (CONFIG_SWSCALE_ALPHA && alpPixBuf) ? dst[3] + dstStride[3] * 
> dstY : NULL,
> +            (CONFIG_SWSCALE_ALPHA && alpPixBuf) ? dst[3] + dstStride[3] * 
> dstY
> +            : NULL,

Probably don't break this line.

>         // Do we have enough lines in this slice to output the dstY line
> -        enough_lines = lastLumSrcY2 < srcSliceY + srcSliceH && lastChrSrcY < 
> -((-srcSliceY - srcSliceH)>>c->chrSrcVSubSample);
> +        enough_lines = lastLumSrcY2 < srcSliceY + srcSliceH &&
> +                       lastChrSrcY < -((-srcSliceY - srcSliceH) >>
> +                                       c->chrSrcVSubSample);

Don't break this line.

>                     int lumAlpha = vLumFilter[2 * dstY + 1];
>                     int chrAlpha = vChrFilter[2 * dstY + 1];
> -                    lumMmxFilter[2] =
> -                    lumMmxFilter[3] = vLumFilter[2 * dstY   ] * 0x10001;
> -                    chrMmxFilter[2] =
> -                    chrMmxFilter[3] = vChrFilter[2 * chrDstY] * 0x10001;
> +                    lumMmxFilter[2]     =
> +                        lumMmxFilter[3] = vLumFilter[2 * dstY] * 0x10001;
> +                    chrMmxFilter[2]     =
> +                        chrMmxFilter[3] = vChrFilter[2 * chrDstY] * 0x10001;

This looks wrong.

> diff --git a/libswscale/utils.c b/libswscale/utils.c
[..]

This not reviewed. Let's do one file at a time.

Ronald
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel

Reply via email to