[FFmpeg-devel] [PATCH] Some ra144.c simplifications
Michael Niedermayer
michaelni
Sun Jun 29 23:14:35 CEST 2008
On Sun, Jun 29, 2008 at 03:00:33PM +0200, Vitor Sessak wrote:
> 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...
ok
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
The educated differ from the uneducated as much as the living from the
dead. -- Aristotle
-------------- 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/20080629/8e6354e4/attachment.pgp>
More information about the ffmpeg-devel
mailing list