[FFmpeg-devel] [PATCH] Some ra144.c simplifications
Vitor Sessak
vitor1001
Sun Jun 29 15:00:33 CEST 2008
Michael Niedermayer wrote:
> On Sun, Jun 29, 2008 at 01:28:14PM +0200, Vitor Sessak wrote:
>> Michael Niedermayer wrote:
>>> On Sat, Jun 28, 2008 at 11:27:40PM +0200, Vitor Sessak wrote:
>>>> Michael Niedermayer wrote:
>>>>> On Tue, Jun 24, 2008 at 11:35:12PM +0200, Vitor Sessak wrote:
>>>>>> 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 =)
>>>>> good, please remind me to do another review when you are finished
>>>>> with that one :)
>>>>> With your help ra144.c will soon be pretty nice and clean, next comes
>>>>> ra288.c i assume :)
>>>> Why not? By the way, can I just clean it and you flame me in -cvslogs?
>>> yes, but id like to do a review of what remains when you run out of ideas
>>> ...
>>> (iam saying that so you can remind me as i will certainly forget ...)
>>>>> [...]
>>>>>>>> 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]
>>>>> no it does not, it uses filter_length which you could set to 10
>>>> Ok, I did it (patch attached). But there were two things I was not happy
>>>> with. First, ff_acelp_lp_synthesis_filter completely ignores
>>>> filter_coeffs[0] and uses filter_coeffs[1..filter_length]. I have no
>>>> idea why it is like that, but I think the correct inner loop would be
>>>>
>>>>> for(i=0; i<filter_length; i++)
>>>>> sum -= filter_coeffs[i] * out[n-i-1];
>>>> where now filter_length is 10 (and not 11!) for the 10th order filter.
>> To be more precise, are you ok with the following patch? Vectors in C
>
> no because there is a -1 now more to be done in the inner loop.
Ok, what about this one? I really don't like the way it is now. I need
to pass a pointer to non-allocated memory in ra144.c and I think a
common function should receive more meaningful input...
-Vitor
-------------- next part --------------
A non-text attachment was scrubbed...
Name: acelp3.diff
Type: text/x-patch
Size: 1829 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080629/774e0691/attachment.bin>
More information about the ffmpeg-devel
mailing list