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

[Michael Niedermayer]

On Tue, Aug 26, 2008 at 11:39:53AM +0700, Vladimir Voroshilov wrote:
> 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.

when 2* is removed from the code it also has to be removed from the comment


> 
> >
> >
> >> 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.

I do not think there is any significant difference between fix and float


> 
> > 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.
[...]
> 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 ?

It is not wrong, it is calculating something quite different from what you
said:
"
 Input signal with repeating -16384,16383 values (sine signal with maximum
 amplitude and sampling_rate/2 frequency)
"
Thats what i tested and that has a max of 31838

What you tested was a signal of ..., 0, 0, 0, 0, -16384,16383,-16384,16383, ...

anyway, if you try it with ..., 16383,16383,16383,16383,-16384,16383,-16384,16383, ...
it goes to -61590

and with a signal that is designed to achive the max it reaches 78484

But at this point we do not know at all if such signals really are possible
as input to the filter.

If they are then the comments about the bits in the variables are wrong if
not then the claim of the clip() being required is wrong. As is the comments
are plain contradictionary. And i would like to see them fixed or removed.

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

Breaking DRM is a little like attempting to break through a door even
though the window is wide open and the only thing in the house is a bunch
of things you dont want and which you would get tomorrow for free anyway
-------------- 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/dab4533c/attachment.pgp>