[FFmpeg-devel] [PATCH] Some ra144.c simplifications
Michael Niedermayer
michaelni
Sat Jun 21 18:16:46 CEST 2008
On Sat, Jun 21, 2008 at 07:53:09AM +0200, Vitor Sessak wrote:
> Michael Niedermayer wrote:
>> On Wed, Jun 04, 2008 at 08:18:10PM +0200, Vitor Sessak wrote:
>>> Michael Niedermayer wrote:
>>>> On Wed, May 28, 2008 at 09:23:02PM +0200, Vitor Sessak wrote:
>>>>> Michael Niedermayer wrote:
>>>>>> On Wed, May 28, 2008 at 06:56:45PM +0200, Vitor Sessak wrote:
>>>>>>> Michael Niedermayer wrote:
>>>>>>>> On Tue, May 27, 2008 at 09:16:09PM +0200, Vitor Sessak wrote:
>>>>>>>>> Michael Niedermayer wrote:
>>>>>>>>>> On Sun, May 25, 2008 at 07:11:52PM +0200, Vitor Sessak wrote:
>>>>>>>>>>> Michael Niedermayer wrote:
>>>>>>>>>>>> On Sun, May 25, 2008 at 06:05:15PM +0200, Vitor Sessak wrote:
>>>>>>>>>>>> [...]
>>>>>>>>>>>>>>> ok
>>>>>>>>>>>>>> One more...
>>>>>>>>>>>>> ... and some more cleanup:
>>>>>>>>>>>>>
>>>>>>>>>>>>> ra144_vector_add_wav.diff: Make add_wav() receive a vector
>>>>>>>>>>>>> instead of three integers
>>>>>>>>>>>>>
>>>>>>>>>>>>> ra144_params_dec2.diff: Do not calculate anything based in l,
>>>>>>>>>>>>> it is unrolled in the loop anyway
>>>>>>>>>>>> ok
>>>>>>>>>>> Now s/(unsigned) short/(u)int16_t.
>>>>>>>>>> ok
>>>>>>>>> Next one. dec2() interpolates the block coefficients from the
>>>>>>>>> previous one and fall back to a block-dependent schema if the
>>>>>>>>> interpolation results in an unstable filter...
>>>>>>>> [...]
>>>>>>>>> + // Interpolate block coefficients from the this frame forth
>>>>>>>>> block and
>>>>>>>>> + // last frame forth block
>>>>>>>>> for (x=0; x<30; x++)
>>>>>>>>> - decsp[x] = (a * inp[x] + b * inp2[x]) >> 2;
>>>>>>>>> + decsp[x] = (a * ractx->lpc_coef[x] + b *
>>>>>>>>> ractx->lpc_coef_old[x])>> 2;
>>>>>>>> ff_acelp_weighted_vector_sum()
>>>>>>> Ok, but to do that I need to use int16_t. So I propose to apply my
>>>>>>> original patch and then the attached one.
>>>>>> hmm, ok
>>>>> Done. Now remove the dec1() function (that was memcpy + 1 line of
>>>>> code). As a side effect, it removes the need of a memcpy (the dec1()
>>>>> call at decode_frame()).
>>>> ok
>>> Now the first patch (ra144_rescale_energy.diff) split the energy
>>> rescaling out of the rms() function. The next patch remove *lpc_refl from
>>> the context, since the only thing needed from the last frame is the non
>>> rescaled output of rms().
>> ok
>
> Now, I'm almost finished with this. Two things remains:
>
> 1- When decoding a ra144 encoded file, ffmpeg produces lots of "Multiple
> frames in a packet from stream 0" (see
> http://fate.multimedia.cx/index.php?test_result=1911120 for an example).
> This is because the decoder receives a 1000 byte sample and decode only 20
> bytes. The attached patch fix this (it decode all the 50 blocks).
wrong solution, we need a AVParser that splits the 1000bytes in 20byte
packets. A generic one that works based on block_align might be usefull
for other cases as well ...
>
> 2- There are lots of unused table entries. Ok to remove them or do you
> thing they can useful for anything (another codec?)?
remove
>
> Finally, if there is anything else you don't like about ra144.{c,h}, now is
> the time to say if you want me to have a look at it...
1st pass review of ra144.c is below :)
(yes you regret now that you asked, i know ...)
[...]
> #define NBLOCKS 4 /* number of segments within a block */
> #define BLOCKSIZE 40 /* (quarter) block size in 16-bit words (80 bytes) */
> #define HALFBLOCK 20 /* BLOCKSIZE/2 */
i think BLOCKSIZE/2 is better
> #define BUFFERSIZE 146 /* for do_output */
doxygen compatible commenst
>
>
> typedef struct {
> unsigned int old_energy; ///< previous frame energy
>
> /* the swapped buffers */
> unsigned int lpc_tables[2][10];
> unsigned int *lpc_coef; ///< LPC coefficients
> unsigned int *lpc_coef_old; ///< previous frame LPC coefficients
> unsigned int lpc_refl_rms;
> unsigned int lpc_refl_rms_old;
proper doxygen grouping comments missing
[...]
> /**
> * Evaluate sqrt(x << 24). x must fit in 20 bits. This value is evaluated in an
> * odd way to make the output identical to the binary decoder.
> */
> static int t_sqrt(unsigned int x)
> {
> int s = 0;
> while (x > 0xfff) {
> s++;
> x = x >> 2;
> }
>
> return (ff_sqrt(x << 20) << s) << 2;
int s = 2;
and the << 2 can be droped
> }
>
> /**
> * Evaluate the LPC filter coefficients from the reflection coefficients.
> * Does the inverse of the eval_refl() function.
> */
> static void eval_coefs(const int *refl, int *coefs)
> {
> int buffer[10];
> int *b1 = buffer;
> int *b2 = coefs;
> int x, y;
>
> for (x=0; x < 10; x++) {
> b1[x] = refl[x] << 4;
>
> for (y=0; y < x; y++)
> b1[y] = ((refl[x] * b2[x-y-1]) >> 12) + b2[y];
>
> FFSWAP(int *, b1, b2);
> }
>
> for (x=0; x < 10; x++)
> coefs[x] >>= 4;
> }
>
> /* rotate block */
> static void rotate_block(const int16_t *source, int16_t *target, int offset)
useless comment
> {
> int i=0, k=0;
> source += BUFFERSIZE - offset;
>
> while (i<BLOCKSIZE) {
> target[i++] = source[k++];
>
> if (k == offset)
> k = 0;
> }
are you sure this is correct?
this does not rotate to the begin of the block but repeats its end
because source has been modified by (BUFFERSIZE - offset)
> }
>
> /* inverse root mean square */
> static int irms(const int16_t *data, int factor)
> {
> unsigned int i, sum = 0;
>
> for (i=0; i < BLOCKSIZE; i++)
> sum += data[i] * data[i];
>
> if (sum == 0)
> return 0; /* OOPS - division by zero */
>
> return (0x20000000 / (t_sqrt(sum) >> 8)) * factor;
> }
I think factor can be removed out of this function and multiplied outside
irms() is used just once ...
>
> /* multiply/add wavetable */
> static void add_wav(int n, int skip_first, int *m, const int16_t *s1,
> const int8_t *s2, const int8_t *s3, int16_t *dest)
I think dest should be consistently first (or last) argument of functions.
> {
> int i;
> int v[3];
>
> v[0] = 0;
> for (i=!skip_first; i<3; i++)
> v[i] = (gain_val_tab[n][i] * m[i]) >> (gain_exp_tab[n][i] + 1);
>
> for (i=0; i < BLOCKSIZE; i++)
> dest[i] = ((*(s1++))*v[0] + (*(s2++))*v[1] + (*(s3++))*v[2]) >> 12;
s1[i]*v[0] + s2[i]*v[1] ...
> }
>
> static void lpc_filter(const int16_t *lpc_coefs, const int16_t *adapt_coef,
> void *out, int *statbuf, int len)
> {
> int x, i;
> uint16_t work[50];
> int16_t *ptr = work;
>
> memcpy(work, statbuf,20);
10*sizeof(int16_t)
> memcpy(work + 10, adapt_coef, len * 2);
>
> for (i=0; i<len; i++) {
> int sum = 0;
> int new_val;
>
> for(x=0; x<10; x++)
> sum += lpc_coefs[9-x] * ptr[x];
>
> sum >>= 12;
>
> new_val = ptr[10] - sum;
>
> if (new_val < -32768 || new_val > 32767) {
> memset(out, 0, len * 2);
> memset(statbuf, 0, 20);
> return;
> }
>
> ptr[10] = new_val;
> ptr++;
> }
>
> memcpy(out, work+10, len * 2);
> memcpy(statbuf, work + 40, 20);
> }
duplicate of ff_acelp_lp_synthesis_filter)(
>
> static unsigned int rescale_rms(int rms, int energy)
> {
> return (rms * energy) >> 10;
> }
signed in unsigned out ?
>
> static unsigned int rms(const int *data)
> {
> int x;
> unsigned int res = 0x10000;
> int b = 0;
>
> for (x=0; x<10; x++) {
> res = (((0x1000000 - (*data) * (*data)) >> 12) * res) >> 12;
>
> if (res == 0)
> return 0;
>
> while (res <= 0x3fff) {
> b++;
> res <<= 2;
> }
> data++;
> }
*data -> data[x] which avoid data++
>
> if (res > 0)
> res = t_sqrt(res);
this looks odd, res is unsigned thus cannot be <0 and ==0 should not
be affected by the sqrt, also ==0 has already been checked above.
Are you sure res should be unsigned and that this code is correct?
>
> res >>= (b + 10);
> return res;
> }
>
> /* do quarter-block output */
> static void do_output_subblock(RA144Context *ractx,
doxy or remove and change function name
> const uint16_t *lpc_coefs, unsigned int gval,
> int16_t *output_buffer, 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 += HALFBLOCK - 1;
> rotate_block(ractx->adapt_cb, buffer_a, cba_idx);
> m[0] = irms(buffer_a, gval) >> 12;
> } else {
> m[0] = 0;
> }
>
> m[1] = ((cb1_base[cb1_idx] >> 4) * gval) >> 8;
> m[2] = ((cb2_base[cb2_idx] >> 4) * gval) >> 8;
the >> 4 can be merged in the table
>
> memmove(ractx->adapt_cb, ractx->adapt_cb + BLOCKSIZE,
> (BUFFERSIZE - BLOCKSIZE) * 2);
sizeof(int16)
[...]
> if (u + 0x1000 > 0x1fff) {
> av_log(ractx, AV_LOG_ERROR, "Overflow. Broken sample?\n");
> return 0;
> }
>
> for (c=8; c >= 0; c--) {
> if (u == 0x1000)
> u++;
>
> if (u == 0xfffff000)
> u--;
>
> b = 0x1000-((u * u) >> 12);
>
> if (b == 0)
> b++;
>
> 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;
retval differs between here and the above overflow check, why?
>
> FFSWAP(int *, bp1, bp2);
> }
> return retval;
> }
>
> static int interp(RA144Context *ractx, int16_t *out, int block_num,
> int copynew, int energy)
> {
> int work[10];
> int a = block_num + 1;
> int b = NBLOCKS - a;
> int x;
>
> // Interpolate block coefficients from the this frame forth block and
> // last frame forth block
> for (x=0; x<30; x++)
> out[x] = (a * ractx->lpc_coef[x] + b * ractx->lpc_coef_old[x])>> 2;
>
> if (eval_refl(out, work, ractx)) {
> // The interpolated coefficients are unstable, copy either new or old
> // coefficients
Is this an error condition or is the occuring on valid streams?
> if (copynew) {
> int_to_int16(out, ractx->lpc_coef);
> return rescale_rms(ractx->lpc_refl_rms, energy);
> } else {
> int_to_int16(out, ractx->lpc_coef_old);
> return rescale_rms(ractx->lpc_refl_rms_old, energy);
> }
int_to_int16(out, ractx->lpc_coef[copynew]);
return rescale_rms(ractx->lpc_refl_rms[copynew], energy);
> } else {
> return rescale_rms(rms(work), energy);
> }
> }
>
> /* Uncompress one block (20 bytes -> 160*2 bytes) */
> static int ra144_decode_frame(AVCodecContext * avctx,
> void *vdata, int *data_size,
> const uint8_t * buf, int buf_size)
doxy
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Observe your enemies, for they first find out your faults. -- Antisthenes
-------------- 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/20080621/57d26ec0/attachment.pgp>
More information about the ffmpeg-devel
mailing list