[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