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

[Michael Niedermayer]

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


> 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

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

an exact implementation shows that it reaches 31838 -31838 with
-16384,16383

which does not overflow the 16bit range

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

I wish the Xiph folks would stop pretending they've got something they
do not.  Somehow I fear this will remain a wish. -- M?ns Rullg?rd
-------------- 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/20080826/e312322d/attachment.pgp>