[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