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

[Nielkie]

On Sat, Nov 27, 2010 at 8:29 PM, Reimar D?ffinger
<Reimar.Doeffinger at gmx.de>wrote:

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


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

Ok.


> 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.
>
>
I figured it'd be cleaner declaring related variables together, but ok.


> > +        /* 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.
>
>
The compiler would catch that easily. I'ma leave it that way for the purpose
of vertical alignment.


> 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.
>
>
INTERPOLATE_3_1 gives different weights to its arguments, so of course it's
going to be called with arguments in different orders...


> > +            /* 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.
>
>
I'm not sure, I would think the compiler would unroll it.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: super2xsai.diff
Type: application/octet-stream
Size: 14381 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20101128/cd92a760/attachment.obj>