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

Vitor Sessak vitor1001
Wed Aug 6 23:09:30 CEST 2008


Michael Niedermayer wrote:
> On Wed, Aug 06, 2008 at 07:09:18AM +0200, Vitor Sessak wrote:
> [...]
>>>>     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.
> 
> ff_acelp_lp_synthesis_filter() does not read the 10 previous input values,
> it reads the 10 previous output values.

Good catch, yet another memcpy removed.

>>> [...]
>>>> 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?
> 
> if (u == 0x1000)
>     u++;
> changes 4096 to 4097
> if (u == 0xfffff000)
>     u--;
> chnages -4096 to -4097
> 
> b = 0x1000-((u * u) >> 12);
> 
> turns +-4095 into  2
> turns +-4096 into  0
> turns +-4097 into -2
> 
> and as its monotone and its input can never be 4096 its output can never be 0
> (assuming it doesnt overflow)
> and as is this code turns +-4096 into +-4097 and then -2 thus if(!b) b=-2 

Indeed. It allowed for quite some simplification (including removing the 
var u).

>>>>         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.
> 
> actually they are asymetric, are you sure this is correct?
> they trigger for +4096 but not -4096

Probably if I change it it won't be bit-identical to realmedia decoder 
anymore.

> and if they triggered for both the checks above might be unneeded
> 
> I do not think the one who designed this understood what he was
> doing.

Nor the one who reverse-engineered it...

-Vitor




More information about the ffmpeg-devel mailing list