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

Michael Niedermayer michaelni
Wed Sep 24 15:54:37 CEST 2008


On Sat, Sep 20, 2008 at 06:05:45PM +0200, Vitor Sessak wrote:
> Vitor Sessak wrote:
> > Michael Niedermayer wrote:
> >> On Fri, Sep 05, 2008 at 12:23:58AM +0200, Vitor Sessak wrote:
> >>> 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.
> >>> I think now they can go through another review cycle.
> >>
> >> ra288 review, iam not sure if this one is the last pass though
> 
> [...]
> 
> >> I think this table would look prettier when its all multiplied by 2048 
> >> and
> >> maybe it would then fit in int16_t
> > 
> > Done. Should be ready for next round.
> 
> Last pointed problem (float->int conversion) solved, ping.

> static void decode(RA288Context *ractx, float gain, int cb_coef)
> {
>     int i, j;
>     double sumsum;
>     float sum, buffer[5];
>     float *block = ractx->sp_hist + 70 + 36; // current block
>     float *gain_block = ractx->gain_hist + 28;
> 
>     memmove(ractx->sp_hist + 70, ractx->sp_hist + 75, 36*sizeof(*block));
> 
>     /* block 46 of G.728 spec */
>     sum = 32.;
>     for (i=0; i < 10; i++)
>         sum -= gain_block[9-i] * ractx->gain_lpc[i];
> 
>     /* block 47 of G.728 spec */
>     sum = av_clipf(sum, 0, 60);
> 
>     /* block 48 of G.728 spec */
>     sumsum = exp(sum * 0.1151292546497) * gain; /* pow(10.0,sum/20)*gain */
> 
>     for (i=0; i < 5; i++)
>         buffer[i] = codetable[cb_coef][i] * sumsum * (1./2048.);

the constant can be multiplied into sumsum


[...]
> static void convolve(float *tgt, const float *src, int len, int n)
> {
>     for (; n >= 0; n--)
>         tgt[n] = scalar_product_float(src, src - n, len);
> 
> }

maybe that should be moved closer to the other small utility functions
above.


[...]
> /**
>  * Backward synthesis filter, find the LPC coefficients from past speech data.
>  */
> static void backward_filter(RA288Context *ractx)
> {
>     float temp1[37]; // RTMP in the spec
>     float temp2[11]; // GPTPMP in the spec
> 
>     do_hybrid_window(36, 40, 35, temp1, ractx->sp_hist,
>                      ractx->sp_rec, syn_window);
> 
>     if (!compute_lpc_coefs(temp1, 36, ractx->sp_lpc, 0, 1, 1))
>         apply_window(ractx->sp_lpc, ractx->sp_lpc, syn_bw_tab, 36);
> 
>     do_hybrid_window(10, 8, 20, temp2, ractx->gain_hist,
>                      ractx->gain_rec, gain_window);
> 
>     if (!compute_lpc_coefs(temp2, 10, ractx->gain_lpc, 0, 1, 1))
>         apply_window(ractx->gain_lpc, ractx->gain_lpc, gain_bw_tab, 10);
> 
>     memmove(ractx->gain_hist, ractx->gain_hist + 8,
>                                  28*sizeof(*ractx->gain_hist));
> 
>     memmove(ractx->sp_hist  , ractx->sp_hist   + 40,
>                                  70*sizeof(*ractx->sp_hist  ));

This code looks duplicated ...

if backward_filter did just one of the 2 and took the various arguments
as parameters half of the code here could be droped and backward_filter()
calls be replaced by 2 calls.


> }
> 
> static int ra288_decode_frame(AVCodecContext * avctx, void *data,
>                               int *data_size, const uint8_t * buf,
>                               int buf_size)
> {
>     float *out = data;
>     int i, j;
>     RA288Context *ractx = avctx->priv_data;
>     GetBitContext gb;
> 
>     if (buf_size < avctx->block_align) {
>         av_log(avctx, AV_LOG_ERROR,
>                "Error! Input buffer is too small [%d<%d]\n",
>                buf_size, avctx->block_align);
>         return 0;
>     }
> 
>     if (*data_size < 32*5*4)
>         return -1;
> 
>     init_get_bits(&gb, buf, avctx->block_align * 8);
> 
>     for (i=0; i < 32; i++) {
>         float gain = amptable[get_bits(&gb, 3)];
>         int cb_coef = get_bits(&gb, 6 + (i&1));
> 
>         decode(ractx, gain, cb_coef);
> 
>         for (j=0; j < 5; j++)
>             *(out++) = (1/4096.) * ractx->sp_hist[70 + 36 + j];

I suspect the (1/4096.) can be avoided by changing a few constants in the
code.

> static const float amptable[8]={
>      0.515625,  0.90234375,  1.57910156,  2.76342773,
>     -0.515625, -0.90234375, -1.57910156, -2.76342773
> };

I dont know if it makes any sense but these are integers when multiplied by 4096

Besides these, i think the code is pretty much ok and should not need another
review.

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Many that live deserve death. And some that die deserve life. Can you give
it to them? Then do not be too eager to deal out death in judgement. For
even the very wise cannot see all ends. -- Gandalf
-------------- 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/20080924/7bdb5ae0/attachment.pgp>



More information about the ffmpeg-devel mailing list