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

[Michael Niedermayer]

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>