[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