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

[Reimar Döffinger]

On Sun, May 16, 2010 at 04:04:51PM +0200, alexandre wrote:
> My luma correction is neither linear nor power, and may not fit in
> the simple adjustement framework you are talking about, if I
> understood correctly.

Fair enough, have you done any research that indicates it works
any better than a more simple brghtness/contrast adjustment?
If so, it would be good to document it.

> +        bzero(vf->priv->means, mpi->h * mpi->w);

bzero is deprecated and even removed from the newest POSIX versions.

> +        for (t = 0; t < vf->priv->windowSize; t++) {
> +            for (y = 0; y < mpi->h; y++) {
> +                for (x = 0; x < mpi->w; x++) {
> +                    vf->priv->means[x + y * mpi->w] +=
> +                        vf->priv->buffer[x + y * mpi->w +
> +                                         t * mpi->w * mpi->h] /
> +                        vf->priv->windowSize;

Divisions in an inner loop are bad, if you stick with floating point
they should be multiplications by the inverse instead.
However, you should not be using floating point after the alogrithmic
change, you risk accumulating rounding errors.
It makes more sense to make "means" an int array and do the division
on use.
This also allows optimizing special-cases (power of two frame count)
by using shift if desired later on.

> +            // current frame histogram
> +            for (n = 0; n < 256; n++)
> +                h1[n] = 0;
> +
> +            for (y = 0; y < mpi->h; y++) {
> +                for (x = 0; x < mpi->w; x++) {
> +                    h1[mpi->planes[0][x + mpi->stride[0] * y]] +=
> +                        1. / (mpi->h * mpi->w);
> +                }
> +            }
> +
> +            val = 0;
> +            for (n = 0; n < 256; n++) {
> +                val += h1[n];
> +                F1[n] = val;
> +            }

Since you don't need h1 anymore there is no need for an extra F1,
it only causes needeless cache pressure.
In addition since you only compare the two histograms, the specific
values are completely irrelevant, thus you should use ints and
do a simple
h1[mpi->planes[0][x + mpi->stride[0] * y]]++;
in the innermost loop.
I admit it will need changing the histogram matching code a bit.

> +            for (y = 0; y < mpi->h; y++) {
> +                for (x = 0; x < mpi->w; x++) {
> +                    h2[vf->priv->means[x + y * mpi->w]] +=
> +                        1. / (mpi->h * mpi->w);
> +                }

This looks dangerous. If e.g. due to rounding errors means[i] is
outside the range, you have a possible exploit.
Using integers and doing the division (or shift if applicable) here should
avoid that.

> +            // histogram matching
> +            for (n = 0; n < 256; n++) {
> +                for (k = (n == 0 ? 0 : F1[n - 1]);
> +                     k < 256 && F1[n] > F2[k]; k++);
> +                L[n] = (k < 255 ? k : 255);
> +            }

Simpler as
k = 0;
for (n = 0; n < 256; n++) {
    while (k < 255 && F1[n] > F2[k]) k++;
    L[n] = k;
}