[FFmpeg-devel] Review request - ra288.{c,h} ra144.{c,h}

Vitor Sessak vitor1001
Wed Aug 6 07:09:18 CEST 2008


Michael Niedermayer wrote:
> 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

done

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

I've tried. From r13560 svn log:
----
Revert r13499, log:
Make lpc coefficients 16 bit wide

Only one of my samples didn't decode bit-exact after this change,
but better be safe than sorry.
----

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

Done.

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

Unfortunately not, because ff_acelp_lp_synthesis_filter read also the 
first 10 bytes of curr_sblock, which I cannot write in block.

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

I have to admit that this is the part I understand the least of the 
whole file. Are you saying that b can never be zero?

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

I agree, done.

> the if with av_log above is redundant it could goto here

They are different. The first one do not happen with valid samples.

> and the return below would do with 0
> and retval could be removed

Yes, done.

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

Yes, but it would be quite messy, since the cb coefficients are read 
from the bit reader. I'd say it is not worth the extra complexity.

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

Done.

-Vitor




More information about the ffmpeg-devel mailing list