[FFmpeg-devel] [PATCH] Some ra144.c simplifications
Michael Niedermayer
michaelni
Sun Jun 29 12:53:19 CEST 2008
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.
>
> Also, I had to change the initial value of sum in
> ff_acelp_lp_synthesis_filter(). I don't know if I can just change it or
> if I should add a "round" boolean to the parameters of
> ff_acelp_lp_synthesis_filter...
an int rounder which is 0x800 / 0xFFF would make more sense
>
> One last thing, can I claim copyright on this file?
which file?
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
If a bugfix only changes things apparently unrelated to the bug with no
further explanation, that is a good sign that the bugfix is wrong.
-------------- 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/5db98fa8/attachment.pgp>
More information about the ffmpeg-devel
mailing list