[MPlayer-dev-eng] [PATCH] new video filter : vf_deflicker

[Reimar Döffinger]

On Sun, May 16, 2010 at 11:27:20AM +0200, alexandre wrote:
> +    if (vf->priv->state == -1) {        // buffer allocation
> +        vf->priv->buffer =
> +            malloc(sizeof(unsigned char) * mpi->h * mpi->w *
> +                   vf->priv->wSz);
> +        vf->priv->means = malloc(sizeof(int) * mpi->h * mpi->w);
> +        vf->priv->state = 0;

IMO just use
if (!vf->priv->buffer) instead of requiring a state.

> +    if (vf->priv->nFrames == vf->priv->wSz)
> +        vf->priv->state = 1;
> +
> +    if (vf->priv->state == 0) { // buffer fill-in
> +        memcpy(vf->priv->buffer + vf->priv->nFrames * mpi->h * mpi->w,
> +               mpi->planes[0], mpi->h * mpi->w);
> +    }
> +
> +    if (vf->priv->state == 1) {

A
if (vf->priv->nFrames < vf->priv->wSz) { // buffer fill-in
} else {
...
}

Seems more obvious to me.

> +        // computation of the moving averages
> +        for (y = 0; y < mpi->h; y++) {
> +            for (x = 0; x < mpi->w; x++) {
> +                vf->priv->means[x + y * mpi->w] = 0;
> +                for (t = 1; t < vf->priv->wSz; t++) {   // todo: be more cache-friendly ?
> +                    vf->priv->means[x + y * mpi->w] +=
> +                        vf->priv->buffer[x + y * mpi->w +
> +                                         t * mpi->w * mpi->h];
> +                    vf->priv->buffer[x + y * mpi->w +
> +                                     (t - 1) * mpi->w * mpi->h] =
> +                        vf->priv->buffer[x + y * mpi->w +
> +                                         t * mpi->w * mpi->h];
> +                }

This scales horrible with wSz (which btw. is not a good variable name).
First, you should be using a ring-buffer so you don't have to move the
data around on each new frame, and secondly if you keep the means buffer
unscaled by wSz you can just subtract the values of each frame when
it leaves the buffer and add the values of the new frame when it comes in.
Since in a ring-buffer the "incoming" frame will be at the same location
as the "outgoing" one that also should be quite cache-friendly and cut
down on address calculations.

> +            // don't change Y plane
> +            for (y = 0; y < mpi->h; y++) {
> +                for (x = 0; x < mpi->w; x++) {
> +                    dmpi->planes[0][x + dmpi->stride[0] * y] =
> +                        mpi->planes[0][x + mpi->stride[0] * y];
> +                }
> +            }
> +        }
> +    } else {                    // buffer still not filled-in
> +        memcpy(dmpi->planes[0], mpi->planes[0], mpi->h * mpi->w);
> +    }
> +
> +    // copy UV plane
> +    for (y = 0; y < mpi->h; y++) {
> +        for (x = 0; x < mpi->w; x++) {
> +            dmpi->planes[2][x + dmpi->stride[2] * y] =
> +                mpi->planes[2][x + mpi->stride[2] * y];
> +        }
> +    }

There's a memcpy_pic copy function that does the stride handling that
you should probably use.
Also I think the whole desing would be much nicer if this filter just
calculated some adjustment values and then passed this on to another
filter or even directly to the vo.
Of course this only works well if we have a filter that does the right
adjustment, haven't checked what exactly you do, but basically you'd
have to settle with only doing a simple brightness+contrast adjustment
(where supported gamma would be possible, too).