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

[Nielkie]

On Fri, Nov 26, 2010 at 6:21 AM, Reimar D?ffinger
<Reimar.Doeffinger at gmx.de>wrote:

> 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 ;-).
>
>
Yeah at the time it seemed like a good idea to reproduce the "quirks" of the
original code :\. Thanks, updated.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: super2xsai.diff
Type: application/octet-stream
Size: 14586 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20101126/1b594a29/attachment.obj>