[FFmpeg-devel] [PATCH] Remove MULL macro from lsp.c

[Vladimir Voroshilov]

2008/8/26 Michael Niedermayer <michaelni at gmx.at>:
> On Mon, Aug 25, 2008 at 10:27:18PM +0700, Vladimir Voroshilov wrote:
>> 2008/8/25 Michael Niedermayer <michaelni at gmx.at>:
>> > On Sat, Aug 23, 2008 at 01:08:13AM +0700, Vladimir Voroshilov wrote:
>> >> Attached patch replaces MULL macro in lsp.c with multiplication and right shift.
>> >> Current code works wrong because one of macro's arguments is int16_t while asm
>> >> assumes both of them to be int.
>> >>
>> >> I'll also remove all occurences of MULL macro from my G.729 code.
>> >>
>> >> P.S. Another fix is leaving MULL there but explicitly cast the second
>> >> argument to (int).
>> >> I don't know which is better.
>> >
>> > I would say that the bug you are trying to fix is in MULL
>> > it should work when its arguments are not int.i
>>
>> i hope, somebody fix it then.
>>
>> >
>> > besides
>> >
>> > [...]
>> >>          f[i] = f[i-2];
>> >>          for(j=i; j>1; j--)
>> >> -            f[j] -= MULL(f[j-1], lsp[2*i-2]) - f[j-2]; // (3.22) * (0.15) * 2 -> (3.22)
>> >> +            f[j] -= (((int64_t)f[j-1] * lsp[2*i-2]) >> 14) - f[j-2]; // (3.22) * (0.15) * 2 -> (3.22)
>> >>
>> >
>> > please fix the comments
>>
>> I didn't see any error in it, though. If it looks confusing on not
>
> the comment says *2, there is nothing in the code that does *2

code should be read as (2 * f * lsp) >> 15 which is equal to  f * lsp >> 14
"2*" removed due to optimization.

>
>
>> clear, i should prefer delete it.
>>
>> > also please fix the comments in ff_acelp_high_pass_filter()
>> > they contradict the claim that cliping is needed. One of them has to
>> > be wrong.
>>
>> hm... looks like i'm not able to clearly describe allowed range for hpf_f
>> Simple calculations shows unlimited domain for it.
>> Anyway those comments  are confusing even for me now :) thus i'd
>> prefer remove them too.
>>
>> About clipping.
>> Input signal with repeating -16384,16383 values (sine signal with maximum
>> amplitude and sampling_rate/2 frequency) will cause int16_t overflow
>> in output even without rounding.
>
> please remove the clipping and all the comments and go back to
> elementary school

I'll be as happy as a prince to study there with you.
Just a joke. And now seriously.

> a quick calculation shows that the amplitude should grow by a factor
> of ~1.943

Floating point arithmetics does not require clipping here.

> an exact implementation shows that it reaches 31838 -31838 with
> -16384,16383
> which does not overflow the 16bit range

I'll be thankful if you point me where does this follow from.

Anyway. Let's check. Sorry if i am doing stupid things.

hpf_f[0]=hpf_f[1]=0
in[-2]=in[-1]=0
in[0]=-16384
in[1]=16383

first iteration:
tmp = (hpf_f[0]*15836LL)>>13 gives tmp=0
tmp+=(hpf_f[1]* -7667LL)>>13 gives tmp=0
tmp+=7699*(in[0]-2*in[-1]+in[-2]) gives tmp=-126140416
out[0] = tmp>>12 gives out[0] = -30796

Second iteration:
hpf_f[1]=0
hpf_f[0]=-125616128
in[-2]=0
in[-1]=-16384
in[0]=16383

tmp = (hpf_f[0]*15836LL)>>13 gives tmp=-243842728
tmp+=(hpf_f[1]* -7667LL)>>13 gives tmp=-243842728
tmp+=7699*(in[0]-2*in[-1]+in[-2]) gives tmp=134578520
out[0] = tmp>>12 gives out[0] = 32856

Where is my calculation wrong ?

Attached file is tiny testsuite which clearly shows overflow.
The reason is in fixed-point math rounding errors, imho.

-- 
Regards,
Vladimir Voroshilov mailto:voroshil at gmail.com
JID: voroshil at gmail.com, voroshil at jabber.ru
ICQ: 95587719
-------------- next part --------------
A non-text attachment was scrubbed...
Name: test_high_pass_filt_ovrfl.c
Type: text/x-csrc
Size: 907 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080826/1846abf4/attachment.c>