[FFmpeg-devel] Review request - ra288.{c,h} ra144.{c,h}
Michael Niedermayer
michaelni
Wed Aug 6 22:24:43 CEST 2008
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.
>
> > [...]
> >> 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
...
>
> >> 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
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.
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Good people do not need laws to tell them to act responsibly, while bad
people will find a way around the laws. -- Plato
-------------- 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/20080806/7d09effa/attachment.pgp>
More information about the ffmpeg-devel
mailing list