[FFmpeg-devel] Review request - ra288.{c,h} ra144.{c,h}
Michael Niedermayer
michaelni
Fri Aug 1 17:56:32 CEST 2008
On Tue, Jul 29, 2008 at 08:20:45PM +0200, Vitor Sessak wrote:
> Hi,
>
> Those four files never passed a review. I've just finished cleaning them
> up, so if anyone wants to review them (Michael already said he will),
> now is time.
ra144 below
[...]
> /**
> * Evaluate sqrt(x << 24). x must fit in 20 bits. This value is evaluated in an
> * odd way to make the output identical to the binary decoder.
> */
> static int t_sqrt(unsigned int x)
> {
> int s = 2;
> while (x > 0xfff) {
> s++;
> x = x >> 2;
x >>= 2
> }
>
> return ff_sqrt(x << 20) << s;
> }
>
> /**
> * Evaluate the LPC filter coefficients from the reflection coefficients.
> * Does the inverse of the eval_refl() function.
> */
> static void eval_coefs(int *coefs, const int *refl)
> {
> int buffer[10];
> int *b1 = buffer;
> int *b2 = coefs;
> int i, j;
>
> for (i=0; i < 10; i++) {
> b1[i] = refl[i] << 4;
>
> for (j=0; j < i; j++)
> b1[j] = ((refl[i] * b2[i-j-1]) >> 12) + b2[j];
>
> FFSWAP(int *, b1, b2);
> }
>
> for (i=0; i < 10; i++)
> coefs[i] >>= 4;
> }
does the output need 32bit? if not then maybe it could output 16 and avoid
the convert to 16later
[...]
> static unsigned int rms(const int *data)
> {
> int i;
> unsigned int res = 0x10000;
> int b = 0;
>
> for (i=0; i < 10; i++) {
> res = (((0x1000000 - data[i]*data[i]) >> 12) * res) >> 12;
>
> if (res == 0)
> return 0;
>
> while (res <= 0x3fff) {
> b++;
> res <<= 2;
> }
> }
>
> res = t_sqrt(res);
>
> res >>= (b + 10);
the +10 can be moved up to b=10
> return res;
> }
>
> static void do_output_subblock(RA144Context *ractx, const uint16_t *lpc_coefs,
> int gval, GetBitContext *gb)
> {
> uint16_t buffer_a[40];
> uint16_t *block;
> int cba_idx = get_bits(gb, 7); // index of the adaptive CB, 0 if none
> int gain = get_bits(gb, 8);
> int cb1_idx = get_bits(gb, 7);
> int cb2_idx = get_bits(gb, 7);
> int m[3];
>
> if (cba_idx) {
> cba_idx += BLOCKSIZE/2 - 1;
> copy_and_dup(buffer_a, ractx->adapt_cb, cba_idx);
> m[0] = (irms(buffer_a) * gval) >> 12;
> } else {
> m[0] = 0;
> }
>
> m[1] = (cb1_base[cb1_idx] * gval) >> 8;
> m[2] = (cb2_base[cb2_idx] * gval) >> 8;
>
> memmove(ractx->adapt_cb, ractx->adapt_cb + BLOCKSIZE,
> (BUFFERSIZE - BLOCKSIZE) * sizeof(*ractx->adapt_cb));
>
> block = ractx->adapt_cb + BUFFERSIZE - BLOCKSIZE;
>
> add_wav(block, gain, cba_idx, m, buffer_a,
> cb1_vects[cb1_idx], cb2_vects[cb2_idx]);
>
> memcpy(ractx->curr_sblock, ractx->curr_sblock + 40,
> 10*sizeof(*ractx->curr_sblock));
> memcpy(ractx->curr_sblock + 10, block,
> BLOCKSIZE*sizeof(*ractx->curr_sblock));
>
> if (ff_acelp_lp_synthesis_filter(
> ractx->curr_sblock + 10, lpc_coefs,
> ractx->curr_sblock + 10, BLOCKSIZE,
> 10, 1, 0xfff)
> )
> memset(ractx->curr_sblock, 0, 50*sizeof(*ractx->curr_sblock));
hmm, cant ff_acelp_lp_synthesis_filter read block and write into curr_sblock
to avoid the memcpy?
[...]
> static int eval_refl(int *refl, const int16_t *coefs, RA144Context *ractx)
> {
> int retval = 0;
> int b, c, i;
> unsigned int u;
> int buffer1[10];
> int buffer2[10];
> int *bp1 = buffer1;
> int *bp2 = buffer2;
>
> for (i=0; i < 10; i++)
> buffer2[i] = coefs[i];
>
> u = refl[9] = bp2[9];
>
> if (u + 0x1000 > 0x1fff) {
> av_log(ractx, AV_LOG_ERROR, "Overflow. Broken sample?\n");
> return 1;
> }
>
> for (c=8; c >= 0; c--) {
> if (u == 0x1000)
> u++;
>
> if (u == 0xfffff000)
> u--;
>
> b = 0x1000-((u * u) >> 12);
>
> if (b == 0)
> b++;
These ifs look redundant, as is the third is never true but the first 2
can i think be replaced by the 3rd if it did b-=2
>
> for (u=0; u<=c; u++)
> bp1[u] = ((bp2[u] - ((refl[c+1] * bp2[c-u]) >> 12)) * (0x1000000 / b)) >> 12;
>
> refl[c] = u = bp1[c];
>
> if ((u + 0x1000) > 0x1fff)
> retval = 1;
return 1 i guess
the if with av_log above is redundant it could goto here
and the return below would do with 0
and retval could be removed
[...]
> for (i=0; i<10; i++)
> lpc_refl[i] = lpc_refl_cb[i][get_bits(&gb, sizes[i])];
>
> eval_coefs(ractx->lpc_coef[0], lpc_refl);
> ractx->lpc_refl_rms[0] = rms(lpc_refl);
neither of these functions changes lpc_refl thus lpc_refl_cb could be
used directly and lpc_refl would be unneeded
>
> energy = energy_tab[get_bits(&gb, 5)];
>
> refl_rms[0] = interp(ractx, block_coefs[0], 0, 1, ractx->old_energy);
> refl_rms[1] = interp(ractx, block_coefs[1], 1, energy <= ractx->old_energy,
> t_sqrt(energy*ractx->old_energy) >> 12);
> refl_rms[2] = interp(ractx, block_coefs[2], 2, 0, energy);
> refl_rms[3] = rescale_rms(ractx->lpc_refl_rms[0], energy);
>
> int_to_int16(block_coefs[3], ractx->lpc_coef[0]);
>
> for (i=0; i < 4; i++) {
> do_output_subblock(ractx, block_coefs[i], refl_rms[i], &gb);
>
> for (j=0; j < BLOCKSIZE; j++)
> *data++ = av_clip_int16(ractx->curr_sblock[j + 10] << 2);
> }
>
> ractx->old_energy = energy;
> ractx->lpc_refl_rms[1] = ractx->lpc_refl_rms[0];
>
> FFSWAP(unsigned int *, ractx->lpc_coef[0], ractx->lpc_coef[1]);
>
> *data_size = 2*160;
the available space is not checked before writing
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Let us carefully observe those good qualities wherein our enemies excel us
and endeavor to excel them, by avoiding what is faulty, and imitating what
is excellent in them. -- Plutarch
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080801/58b59c77/attachment.pgp>
More information about the ffmpeg-devel
mailing list