[FFmpeg-devel] [PATCH] Some ra144.c simplifications

[Vitor Sessak]

Michael Niedermayer wrote:
> 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 ...

I'll see it later.

> 
> 
>> 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 ...)

It was my masochistic side that made that question =)


[...]

> 
>> /* 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?

Yes. Just rotate_block() is a misleading name. According to the only 
freely available documentation:

> The VSELP
> algorithm supports lags from 20 to 147; therefore, a special situation exists when the lag (L) is less than
> NSF. In this case, the bL vector is placed such that a portion of it hangs over the adaptive code book. These
> elements of the adaptive code book (long-term filter state) do not exist yet. The flr function of equation [2]
> remedies this by doubling the lag (code book index value). This results in copying the first NSF ? L
> elements of the bL vector to the ending NSF ? L elements.


[...]

> 
>> 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)(

No, they are slightly different (ff_acelp_lp_synthesis_filter use the 
last n input values to predict out[n] and this function use the last 10).

>> 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?

I removed the check, but I can't be sure the code is correct, it is not 
documented anywhere...

[...]

>> 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?

It occurs in valid streams.

All other remarks fixed as suggested.

-Vitor