[FFmpeg-devel] [PATCH] Channels correlation

[Nicolas George]

Hi. Thanks for your fast reply.

L'octidi 8 brumaire, an CCXVIII, Reimar D?ffinger a ?crit?:
> Better use libavutil/mathematics.h, there is a good chance that
> whatever you use from math.h is not available on all supported systems.

I use it for the declaration of sqrt and fabs, which are very old and really
really common. I seriously doubt they can cause any problem, and they are
not redeclared in mathematics.h anyway.

> All functions lack doxygen-compatible documentation.

Added.

> > +    unsigned size = state->buf_pos + n;
> Possible integer overflow?

True, although other parts of the code using av_fast_realloc seem to be
happy with a similar problem.

What is the policy in this case?

if (will overflow) {
    av_log(..., "Streams to much out of sync.\n");
    return -1;
}

?

> Just use av_fast_realloc.

Cool, I did not know it. Changed.

> 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).

Changed.

> Declaration and initialization of m could be merged.

Done.

> 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.

I think you slightly misread the code, since each cell of m is accessed
exactly once in these loops. What these loops do exactly is:
	for all 0 <= i < n1 and 0 <= j < n1
		M1[i, j] += d1[i] * d1[j];
	for all 0 <= i < n2 and 0 <= j < n1
		M2[i, j] += d2[i] * d1[j];

Anyway, I tried various ways to inverse the order of the loops but only got
slightly slower code. The current version walks through m in memory order,
which is supposed to be nicer to the cache.

> 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]);

That is true, but on the other hand that adds a lot of brackets in the code
just before that, and that makes it harder to read. Furthermore, gcc (at
least 4.3.4) seems to produce slightly slower code with arrays.

For both these reasons, I think I'd rather keep the current version, if this
is fine with you.

> do a return and get rid of the else.

Done.

> memset

Done.

> Not a good name.

Done, and another too laconic variable too.

> FFSWAP

Done.

Also added a documentation paragraph about packet_size, and the forgotten
av_free for the allocated memory blocks.

Regards,

-- 
  Nicolas George
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ffmpeg-correl-20091029b.diff
Type: text/x-diff
Size: 11997 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20091029/d9f43fa2/attachment.diff>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20091029/d9f43fa2/attachment.pgp>