[FFmpeg-devel] [PATCH] Some ra144.c simplifications
Vitor Sessak
vitor1001
Tue Jun 24 23:35:12 CEST 2008
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
More information about the ffmpeg-devel
mailing list