[FFmpeg-devel] [PATCH] Channels correlation

[Reimar Döffinger]

On Thu, Oct 29, 2009 at 01:57:51PM +0100, Nicolas George wrote:
> +#include <math.h>

Better use libavutil/mathematics.h, there is a good chance that
whatever you use from math.h is not available on all supported systems.

> +static void append_buffer(struct correl_state *state, double *d, unsigned n)

All functions lack doxygen-compatible documentation.

> +{
> +    unsigned size = state->buf_pos + n;

Possible integer overflow?

> +    if (state->buf_size < size) {
> +        if (state->buf_size == 0)
> +            state->buf_size = 4096;
> +        while (state->buf_size < size)
> +            state->buf_size *= 2;
> +        state->buf = av_realloc(state->buf, state->buf_size * sizeof(double));
> +    }

Just use av_fast_realloc.

> +    memmove(state->buf + state->buf_pos, d, n * sizeof(double));

d is a bad name, it usually stands for "destination", which it is not.
Also it should be const and you should use sizeof(*d) instead of sizeof(double).

> +static void feed_correl(struct correl_state *state, double *d1, unsigned n1,
> +    double *d2, unsigned n2)
> +{
> +    unsigned nc1 = state->nch[0];
> +    unsigned nc2 = state->nch[1];
> +    unsigned i, j;
> +    double *m;
> +
> +    while (n1 > 0 && n2 > 0) {
> +        m = state->matr_io;

Declaration and initialization of m could be merged.

> +        for (i = 0; i < nc1; i++)
> +            for (j = 0; j < nc1; j++)
> +                *(m++) += d1[i] * d1[j];
> +        for (i = 0; i < nc2; i++)
> +            for (j = 0; j < nc1; j++)
> +                *(m++) += d2[i] * d1[j];
> +        d1 += nc1;
> +        d2 += nc2;
> +        n1--;
> +        n2--;

I'd expect that it would be faster to swap those loops around, with the
n1/n2 stuff innermost since that means that m[...] could be kept in a register
in the innermost loop and also since nc1 and nc2 are probably much smaller values
this would have less branch mis-predictions.

> +    if (n1 > 0) {
> +        append_buffer(state, d1, n1 * nc1);
> +        state->buf_id = 0;
> +    } else {
> +        append_buffer(state, d2, n2 * nc2);
> +        state->buf_id = 1;
> +    }

If d1/d2, n1/n2, nc1/nc2 were arrays this would just be
int buf_id = state->buf_id = n1 > 0;
append_buffer(state, d[buf_id], n[buf_id] * nc[buf_id]);

> +static void feed_buffer(struct correl_state *state, unsigned id,
> +    double *data, unsigned n)
> +{
> +    unsigned nch = state->nch[id];
> +
> +    if (id == state->buf_id) {
> +        append_buffer(state, data, n * nch);

do a return and get rid of the else.

> +            for (i = 0; i < nch; i++)
> +                gt[i] = 0;

memset

> +    double t;

Not a good name.

> +            for (j = 0; j < l; j++) {
> +                t = m[j * c + i];
> +                m[j * c + i] = m[j * c + p];
> +                m[j * c + p] = t;

FFSWAP