[FFmpeg-devel] [PATCH] Port MPlayer 2xSaI filter to libavfilter

[Reimar Döffinger]

I am only nit-picking, if someone wants to apply do not let
me stop you...

On Sat, Nov 27, 2010 at 07:52:52PM +1000, Nielkie wrote:
> +static inline uint32_t readPixel(int bytesPerPixel, uint8_t *src, int offset)
> +{
> +    switch(bytesPerPixel) {
> +    case 4: return AV_RN32A(src + 4*offset);
> +    case 3: return AV_RL24 (src + 3*offset);
> +    case 2: return AV_RN16 (src + 2*offset);
> +    }
> +    return 0;

What I meant is that e.g.
> +    switch(bytesPerPixel) {
> +    case 4: return AV_RN32A(src + 4*offset);
> +    case 3: return AV_RL24 (src + 3*offset);
> +    default: /* hack to avoid an extra comparison */
> +    case 2: return AV_RN16 (src + 2*offset);
> +    }

Is one compare less in the worst case (and at least one conditional
jump will become uncoditional or the compiler will avoid it completely,
in either case one thing less in the branch prediction).
If the compiler still generates a warning,
then one case could just be completely moved outside the switch.

> +    unsigned int x = 0, y = 0, my = 0;

There's no point in initializing them.
Personally I don't like using unsigned types if there's no
good reason but that seems to be just me.
Also x is only used inside the y loop so moving its
declaration there would make sense.

> +    uint32_t color[4][4];
> +    uint8_t *src_line[4], *dst_line[2];

Actually the color array and dst_line can be moved there as well.

> +        /* Initialise the color matrix for this row.
> +            ( (1,1) being the source pixel being generated)
> +        */
> +        x = 0;

Pointless now.

> +            } else if (color[1][1] == color[2][2] && color[2][1] == color[1][2]) {
> +                int r = 0;
> +
> +                r += GET_RESULT(color[1][2], color[1][1], color[2][0], color[3][1]);

Hopefully the compiler get it (though I am not convinced of it), but adding 0 is rather pointless.

Also looking at this:
> +                r += GET_RESULT(color[1][2], color[1][1], color[2][0], color[3][1]);
> +                    product1b = INTERPOLATE(color[1][1], color[1][2]);
> +                    product2b = INTERPOLATE_3_1(color[2][2], color[2][1]);

The order of the macro arguments is quite a mess, sometimes the left pixel is
first, sometimes it's the right one.
Personally I'd say it makes sense to swap the (first) two arguments of GET_RESULT
and INTERPOLATE_3_1.

> +            /* Move color matrix forward */
> +            for (my = 0; my < 4; my++) {
> +                color[my][0] = color[my][1];
> +                color[my][1] = color[my][2];
> +                color[my][2] = color[my][3];
> +            }

Using a loop here seems likely to have a major negative impact on performance.