[FFmpeg-devel] [PATCH] rotate filter

[Michael Niedermayer]

On Wed, May 04, 2011 at 01:32:46AM +0200, Stefano Sabatini wrote:
> On date Tuesday 2011-05-03 23:40:10 +0200, Stefano Sabatini encoded:
> > On date Wednesday 2010-10-06 16:21:16 +0200, Michael Niedermayer wrote:
> > > On Tue, Oct 05, 2010 at 09:38:06PM +0200, Stefano Sabatini wrote:
> > > > On date Tuesday 2010-10-05 14:36:19 +0200, Michael Niedermayer encoded:
> > > > > On Mon, Oct 04, 2010 at 10:27:29PM +0200, Stefano Sabatini wrote:
> > > > [...]
> > > > > > +/**
> > > > > > + * Interpolate the color in src at position x and y using bilinear
> > > > > > + * interpolation.
> > > > > > + *
> > > > > > + * @param dst_color put here the destination color
> > > > > > + */
> > > > > > +static uint8_t *ipol(uint8_t *dst_color,
> > > > > > +                     const uint8_t *src, const int src_linesize, int x, int y,
> > > > > > +                     int max_x, int max_y)
> > > > > > +{
> > > > > > +    int int_x = x>>16;
> > > > > > +    int int_y = y>>16;
> > > > > > +    int frac_x = x&0xFFFF;
> > > > > > +    int frac_y = y&0xFFFF;
> > > > > > +    int i;
> > > > > > +
> > > > > > +    for (i = 0; i < 3; i++) {
> > > > > > +        int s00 = src[3 * int_x                + i + src_linesize * int_y];
> > > > > > +        int s01 = src[3 * FFMIN(int_x+1,max_x) + i + src_linesize * int_y];
> > > > > > +        int s10 = src[3 * int_x                + i + src_linesize * FFMIN(int_y+1, max_y)];
> > > > > > +        int s11 = src[3 * FFMIN(int_x+1,max_x) + i + src_linesize * FFMIN(int_y+1, max_y)];
> > > > > > +        int s0 = (((1<<16) - frac_x)*s00 + frac_x*s01)>>8;
> > > > > > +        int s1 = (((1<<16) - frac_x)*s10 + frac_x*s11)>>8;
> > > > > > +
> > > > > > +        dst_color[i] = (((1<<16) - frac_y)*s0 + frac_y*s1)>>24;
> > > > > > +    }
> > > > > 
> > > > > the >>8 can be avoided by adjusting perecission sanely
> > > > > the FFMIN doesnt belong in the loop
> > > > 
> > > > I can do:
> > > >       int s0 = ((1<<16) - frac_x)*s00 + frac_x*s01;
> > > >       int s1 = ((1<<16) - frac_x)*s10 + frac_x*s11;
> > > > 
> > > >       dst_color[i] = (((int64_t)(1<<16) - frac_y)*s0 + (int64_t)frac_y*s1)>>32;
> > > > 
> > > > Is this what you suggest?
> > > 
> > > frac_x >>=6
> > > frac_y >>=6
> > > outside the loop
> > 
> > Updated work in progress, with *lots* interface improvements, I have
> > yet to fix the problems spotted by Michael, also IIRC I seem to
> > remeber there was a problem with formats for which hsub != vsub (which
> > are currently disabled).
> > 

> > I'd like to committ the filter, even if we develop a more advanced
> > affine/prospective filter, since this will be possible more efficient
> > (no float arithmetic) and more suited to this particular task (and the
> > filter is laying on ML since too much time IMO).

yes, rotting patches arent good


> 
> Update with some minor fixes, and with regression tests added.
> 
> Bikeshed: I'm not sure if it is better to make the angle mandatory,
> and require thus:
> rotate=123
> 
> rather than make it an option, and thus settable via:
> rotate=a=123

pick whatever you prefer


> 
> the first is slightly simpler (and compatible with the old rotate),
> but doesn't allow rotate with no arguments.
> 
> Michael please comment on the ipol >> issue, I can't find a way as you
> suggest.
[...]
> +/**
> + * Interpolate the color in src at position x and y using bilinear
> + * interpolation.
> + *
> + * @param dst_color put here the destination color
> + */
> +static uint8_t *ipol(uint8_t *dst_color,
> +                     const uint8_t *src, int src_linesize, int src_linestep,
> +                     int x, int y, int max_x, int max_y)
> +{
> +    int int_x = x>>16;
> +    int int_y = y>>16;
> +    int frac_x = x&0xFFFF;
> +    int frac_y = y&0xFFFF;
> +    int i;
> +    int int_x1 = FFMIN(int_x+1,max_x);
> +    int int_y1 = FFMIN(int_y+1,max_y);
> +

> +    for (i = 0; i < src_linestep; i++) {
> +        int s00 = src[src_linestep * int_x  + i + src_linesize * int_y ];
> +        int s01 = src[src_linestep * int_x1 + i + src_linesize * int_y ];
> +        int s10 = src[src_linestep * int_x  + i + src_linesize * int_y1];
> +        int s11 = src[src_linestep * int_x1 + i + src_linesize * int_y1];
> +        int s0 = (((1<<16) - frac_x)*s00 + frac_x*s01);
> +        int s1 = (((1<<16) - frac_x)*s10 + frac_x*s11);
> +
> +        dst_color[i] = ((int64_t)((1<<16) - frac_y)*s0 + (int64_t)frac_y*s1)>>32;
> +    }

frac_x >>= 6;
frac_y >>= 6;
for (i = 0; i < src_linestep; i++) {
    int s00 = src[src_linestep * int_x  + i + src_linesize * int_y ];
    int s01 = src[src_linestep * int_x1 + i + src_linesize * int_y ];
    int s10 = src[src_linestep * int_x  + i + src_linesize * int_y1];
    int s11 = src[src_linestep * int_x1 + i + src_linesize * int_y1];
    int s0 = (((1<<10) - frac_x)*s00 + frac_x*s01);
    int s1 = (((1<<10) - frac_x)*s10 + frac_x*s11);

    dst_color[i] = (((1<<10) - frac_y)*s0 + frac_y*s1)>>20;
}

you can also shift by just 4 instead of 6 and use unsigned values


[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The worst form of inequality is to try to make unequal things equal.
-- Aristotle
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20110504/5dda727b/attachment.asc>