[FFmpeg-devel] [PATCH] Some ra144.c simplifications
Michael Niedermayer
michaelni
Sun Jun 29 13:39:08 CEST 2008
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.
[...]
>>> One last thing, can I claim copyright on this file?
>> which file?
>
> See $subj ;-)
Well, if it makes you happy, i wont object.
>
> Btw, it should be ready for the next review cycle.
ok, ill do when i have time (might take a little though)
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
When the tyrant has disposed of foreign enemies by conquest or treaty, and
there is nothing more to fear from them, then he is always stirring up
some war or other, in order that the people may require a leader. -- Plato
-------------- 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/dddd8857/attachment.pgp>
More information about the ffmpeg-devel
mailing list