[FFmpeg-devel] [PATCH] Remove MULL macro from lsp.c
Tue Aug 26 13:37:02 CEST 2008
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*15836LL)>>13 gives tmp=-243842728
> tmp+=(hpf_f* -7667LL)>>13 gives tmp=-243842728
> tmp+=7699*(in-2*in[-1]+in[-2]) gives tmp=134578520
> out = tmp>>12 gives out = 32856
> Where is my calculation wrong ?
It is not wrong, it is calculating something quite different from what you
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
Size: 189 bytes
Desc: Digital signature
More information about the ffmpeg-devel