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

[Reimar Döffinger]

On Fri, Nov 26, 2010 at 12:32:26AM +1000, Nielkie wrote:
> On Thu, Nov 25, 2010 at 10:27 PM, Tomas H?rdin <tomas.hardin at codemill.se>wrote:
> > > +static void Super2xSaI_ex(Super2xSaIContext *c,
> > > +                          uint8_t *src, uint32_t src_pitch,
> > > +                          uint8_t *dst, uint32_t dst_pitch,
> > > +                          uint32_t width, uint32_t height)
> > > +{
> > > ...
> > > +    for (y = 0; y < height; y++) {
> > > +        uint8_t *dst_line[2];
> > > +
> > > +        dst_line[0] = dst + dst_pitch*2*y;
> > > +        dst_line[1] = dst + dst_pitch*(2*y+1);
> > > +
> > > +        /* Todo: x = width - 2, x = width - 1 */
> >
> > Fix it?
> >
> 
> I honestly don't know what it is referring to. I just removed the comment as
> it looks like has been done in other implementations. (If anyone has any
> other ideas...)

Well, I guess there is at least some uncertainty concerning how to fill
up the boundary values, e.g. most of the code extends the image at the
borders the way MPEG (maybe even all modern?) video codecs do it,
but the very first initialization is slightly different:

> +    color[0] =  GET_COLOR(0, 0); color[1] =  GET_COLOR(0, 0); color[2] =  GET_COLOR(0, 0); color[3] =  GET_COLOR(0, 0);
> +    color[4] =  GET_COLOR(0, 0); color[5] =  GET_COLOR(0, 0); color[6] =  GET_COLOR(1, 0); color[7] =  GET_COLOR(2, 0);
> +    color[8] =  GET_COLOR(0, 2); color[9] =  GET_COLOR(0, 2); color[10] = GET_COLOR(1, 2); color[11] = GET_COLOR(2, 2);
> +    color[12] = GET_COLOR(0, 3); color[13] = GET_COLOR(0, 3); color[14] = GET_COLOR(1, 3); color[15] = GET_COLOR(2, 3);

2 and 6 and 3 and 7 should have the same value.
And since you already remove the "optimization" from the original code, at least use
consistently GET_COLOR(?, 1); in the second line, then it at least gains something in
readability.
Also the code is missing a check for width or height <= 3, the code will read
outside the allocated memory in that case.

> +            if (x < width - 3) {
> +                color[3] =  GET_COLOR(3, 0);
> +                color[7] =  GET_COLOR(3, 1);
> +                color[11] = GET_COLOR(3, 2);
> +                color[15] = GET_COLOR(3, 3);
> +            }
> +        }
> +
> +        /* We're done with one line, so we shift the source lines up */
> +        src_line[0] = src_line[1];
> +        src_line[1] = src_line[2];
> +        src_line[2] = src_line[3];
> +
> +        /* Read next line */
> +        if (y + 3 >= height)
> +            src_line[3] = src_line[2];
> +        else
> +            src_line[3] = src_line[2] + src_pitch;

This is stupid and inconsistent with the x case.
if (y < height - 3)
    src_line[3] += src_pitch;

> +        /* Then shift the color matrix up */
> +        x = 0;
> +        color[0] =  GET_COLOR(0, 0); color[1] =  GET_COLOR(0, 0); color[2] =  GET_COLOR(1, 0); color[3] =  GET_COLOR(2, 0);
> +        color[4] =  GET_COLOR(0, 1); color[5] =  GET_COLOR(0, 1); color[6] =  GET_COLOR(1, 1); color[7] =  GET_COLOR(2, 1);
> +        color[8] =  GET_COLOR(0, 2); color[9] =  color[9];        color[10] = GET_COLOR(1, 2); color[11] = GET_COLOR(2, 2);
> +        color[12] = GET_COLOR(0, 3); color[13] = GET_COLOR(0, 3); color[14] = GET_COLOR(1, 3); color[15] = GET_COLOR(2, 3);

I can't see how the [9] case could be correct.
And if you go with this comment and the one in the very first initialization
you'll notice that it's actually the same code that is pointlessly duplicated,
so they don't have to share bugs but can have some of their own each ;-).