[FFmpeg-soc] [PATCH] Add fade filter to libavfilter

[Reimar Döffinger]

On Wed, Mar 31, 2010 at 05:25:38PM -0400, Brandon Mintern wrote:
> On Wed, Mar 31, 2010 at 5:19 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
> > On Wed, Mar 31, 2010 at 01:23:29AM -0400, Brandon Mintern wrote:
> >> On Wed, Mar 31, 2010 at 12:43 AM, Brandon Mintern <bmintern at gmail.com> wrote:
> >> > I am happy to present my first-ever open source code contribution, a
> >> > "fade" filter for libavfilter!
> >> [snip]
> > [...]
> >
> >> +static void draw_slice(AVFilterLink *link, int y, int h, int slice_dir)
> >> +{
> >> +    FadeContext *fade = link->dst->priv;
> >> +    AVFilterPicRef *in  = link->cur_pic;
> >> +    AVFilterPicRef *out = link->dst->outputs[0]->outpic;
> >> +    uint8_t *inrow, *outrow;
> >> +    int i, j, plane;
> >> +
> >> +    /* luma plane */
> >> +    inrow  = in-> data[0] + y * in-> linesize[0];
> >> +    outrow = out->data[0] + y * out->linesize[0];
> >> +    for(i = 0; i < h; i ++) {
> >> +        for(j = 0; j < link->w; j ++)
> >> +            outrow[j] = (uint8_t) ((float) inrow[j] * fade->fade_factor + 0.5);
> >
> > please avoid floats, it makes regression testing unneccesarily hard
> > Its probably ok to use floats/doubles in init, but it feels wrong
> > at the pixel level
> >
> > anyway, real review and approval left to vitor or bobby
> >
> > [...]
> > --
> > Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
> 
> Thanks for the feedback. The line you indicated is actually just an
> explicit cast that I can remove if you'd like. However, the
> floating-point arithmetic is being used to calculate the new pixel
> values and round them to the nearest uint8_t. I can instead stick to
> integer arithmetic, but then the numbers will be truncated instead of
> rounded. Do you have another suggestion? Is AVRational (I think that's
> what it called) something I should be using instead?

Fixed-point arithmetic, i.e. the least significant bit does not mean
1 but e.g. 1/16.
In this case something like
int_fade_factor = (int)(fade_factor * (1 << 16));
outrow = (inrow * int_fade_factor + (1 << 15)) >> 16;

Of course that's just to give you an idea, you have to make sure
there won't be any integer overflows etc.