[FFmpeg-devel] [PATCH] MPEG-4 Parametric Stereo decoder

[Alex Converse]

On Fri, May 21, 2010 at 8:45 PM, Ronald S. Bultje <rsbultje at gmail.com> wrote:
> Hi Alex,
>
> On Thu, May 20, 2010 at 1:51 PM, Alex Converse <alex.converse at gmail.com> wrote:
>> Yes there are some places where it can be further optimized but I'm
>> losing motivation on this quickly so perhaps some review will respark
>> my interest.
>
> That's unfortunate to hear, but I hope you'll continue. Simple stuff:
>
> +#define MAP_GENERIC_20_TO_34(out, in, full) \
> + ? ?if (full) { ? ? ? ? ? ? ? ? ? ? ? ? ? ?\
> + ? ?out[33] = ?in[19]; ? ? ? ? ? ? ? ? ? ? \
> + ? ?out[32] = ?in[19]; ? ? ? ? ? ? ? ? ? ? \
> + ? ?out[31] = ?in[18]; ? ? ? ? ? ? ? ? ? ? \
> + ? ?out[30] = ?in[18]; ? ? ? ? ? ? ? ? ? ? \
> + ? ?out[29] = ?in[18]; ? ? ? ? ? ? ? ? ? ? \
> + ? ?out[28] = ?in[18]; ? ? ? ? ? ? ? ? ? ? \
> + ? ?out[27] = ?in[17]; ? ? ? ? ? ? ? ? ? ? \
> + ? ?out[26] = ?in[17]; ? ? ? ? ? ? ? ? ? ? \
> + ? ?out[25] = ?in[16]; ? ? ? ? ? ? ? ? ? ? \
> + ? ?out[24] = ?in[16]; ? ? ? ? ? ? ? ? ? ? \
> + ? ?out[23] = ?in[15]; ? ? ? ? ? ? ? ? ? ? \
> + ? ?out[22] = ?in[15]; ? ? ? ? ? ? ? ? ? ? \
> + ? ?out[21] = ?in[14]; ? ? ? ? ? ? ? ? ? ? \
> + ? ?out[20] = ?in[14]; ? ? ? ? ? ? ? ? ? ? \
> + ? ?out[19] = ?in[13]; ? ? ? ? ? ? ? ? ? ? \
> + ? ?out[18] = ?in[12]; ? ? ? ? ? ? ? ? ? ? \
> + ? ?out[17] = ?in[11]; ? ? ? ? ? ? ? ? ? ? \
> + ? ?} ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?\
> + ? ?out[16] = ?in[10]; ? ? ? ? ? ? ? ? ? ? \
> + ? ?out[15] = ?in[ 9]; ? ? ? ? ? ? ? ? ? ? \
> + ? ?out[14] = ?in[ 9]; ? ? ? ? ? ? ? ? ? ? \
> + ? ?out[13] = ?in[ 8]; ? ? ? ? ? ? ? ? ? ? \
> + ? ?out[12] = ?in[ 8]; ? ? ? ? ? ? ? ? ? ? \
> + ? ?out[11] = ?in[ 7]; ? ? ? ? ? ? ? ? ? ? \
> + ? ?out[10] = ?in[ 6]; ? ? ? ? ? ? ? ? ? ? \
> + ? ?out[ 9] = ?in[ 5]; ? ? ? ? ? ? ? ? ? ? \
> + ? ?out[ 8] = ?in[ 5]; ? ? ? ? ? ? ? ? ? ? \
> + ? ?out[ 7] = ?in[ 4]; ? ? ? ? ? ? ? ? ? ? \
> + ? ?out[ 6] = ?in[ 4]; ? ? ? ? ? ? ? ? ? ? \
> + ? ?out[ 5] = ?in[ 3]; ? ? ? ? ? ? ? ? ? ? \
> + ? ?out[ 4] = (in[ 2] + in[ 3]) / 2; ? ? ? \
> + ? ?out[ 3] = ?in[ 2]; ? ? ? ? ? ? ? ? ? ? \
> + ? ?out[ 2] = ?in[ 1]; ? ? ? ? ? ? ? ? ? ? \
> + ? ?out[ 1] = (in[ 0] + in[ 1]) / 2; ? ? ? \
> + ? ?out[ 0] = ?in[ 0];
>
> Can /2 be >>1? Also indenting is off.
>

Those need C division rounding. The most used parameter types are
allowed to go negative. I wouldn't waste a lot of time thinking about
ipd/opd which no one seems to use. Maybe when I generalize this code
to MPEG-D Surround it will be worth revisiting.

> + ? ? ? ? ? ? ? ?if (!PS_BASELINE && ps->enable_ipdopd) {
> + ? ? ? ? ? ? ? ?h11i += h11i_step;
> + ? ? ? ? ? ? ? ?h12i += h12i_step;
> + ? ? ? ? ? ? ? ?h21i += h21i_step;
> + ? ? ? ? ? ? ? ?h22i += h22i_step;
> +#if 0
> + ? ? ? ? ? ?h11r = 0;
> + ? ? ? ? ? ?h12r = 1;
> + ? ? ? ? ? ?h21r = 1;
> + ? ? ? ? ? ?h22r = 0;
> + ? ? ? ? ? ?h11i = h12i = h21i = h22i = 0;
> +#endif
> + ? ? ? ? ? ? ? ?l[k][n][0] = h11r*l_re + h21r*r_re - h11i*l_im - h21i*r_im;
> + ? ? ? ? ? ? ? ?l[k][n][1] = h11r*l_im + h21r*r_im + h11i*l_re + h21i*r_re;
> + ? ? ? ? ? ? ? ?r[k][n][0] = h12r*l_re + h22r*r_re - h12i*l_im - h22i*r_im;
> + ? ? ? ? ? ? ? ?r[k][n][1] = h12r*l_im + h22r*r_im + h12i*l_re + h22i*r_re;
> + ? ? ? ? ? ? ? ?} else {
> + ? ? ? ? ? ? ? ?l[k][n][0] = h11r*l_re + h21r*r_re;
> + ? ? ? ? ? ? ? ?l[k][n][1] = h11r*l_im + h21r*r_im;
> + ? ? ? ? ? ? ? ?r[k][n][0] = h12r*l_re + h22r*r_re;
> + ? ? ? ? ? ? ? ?r[k][n][1] = h12r*l_im + h22r*r_im;
> + ? ? ? ? ? ? ? ?}
>
> Your indent is off here (also above this piece).
>
> + ? ? ? ?for (m = 0; m < PS_AP_LINKS; m++) {
> + ? ? ? ? ? ?Q_fract_allpass[1][k][m][0] = cos(-M_PI *
> fractional_delay_links[m] * f_center);
> + ? ? ? ? ? ?Q_fract_allpass[1][k][m][1] = sin(-M_PI *
> fractional_delay_links[m] * f_center);
> + ? ? ? ?}
>
> Calculate -M_PI * ... before the cosf()/sinf() call so you calulate it
> once instead of twice.

ok

>
> + ? ? ? ? ? ? ? ?HA[iid][icc][0] = c2 * cosf(beta + alpha);
> + ? ? ? ? ? ? ? ?HA[iid][icc][1] = c1 * cosf(beta - alpha);
> + ? ? ? ? ? ? ? ?HA[iid][icc][2] = c2 * sinf(beta + alpha);
> + ? ? ? ? ? ? ? ?HA[iid][icc][3] = c1 * sinf(beta - alpha);
>
> Here, values for cosf(x-y) and cosf(x+y) can be calculated once
> instead of twice. Should save 2 calls to math funcs.
>

There seems to be a sinf and a cosf call once for each. Using trig
identities we'd still need sin and cos for both. Either way that's
still 4 calls? Or do you mean just precompute beta +/- alpha?

> + ? ? ? ? ? ? ? ?HB[iid][icc][0] = ?M_SQRT2 * cosf(alpha) * cosf(gamma);
> + ? ? ? ? ? ? ? ?HB[iid][icc][1] = ?M_SQRT2 * sinf(alpha) * cosf(gamma);
> + ? ? ? ? ? ? ? ?HB[iid][icc][2] = -M_SQRT2 * sinf(alpha) * sinf(gamma);
> + ? ? ? ? ? ? ? ?HB[iid][icc][3] = ?M_SQRT2 * cosf(alpha) * sinf(gamma);
>
> Same here for cosf/sinf(alpha/gamma). Saves 4 calls to expensive float
> funcs. I know this last few is just init, but it's still something.
>

I hope the compiler isn't that stupid but fixed.

> Generally, many of these variables have short 1-2letter names and no
> or very little doxy, maybe you can document a few of them, at least
> those declared in .h files? Same for functions, doxyment what they do?
>

There is doxy for iid/icc/ipd/opd in the header. They are all acronyms.

The H matrices are input/decorrelation rotation/scaling matrices. I
probably can think of a better name.

> Ronald

thanks