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

[Michael Niedermayer]

On Wed, Aug 27, 2008 at 01:00:38AM +0700, Vladimir Voroshilov wrote:
> 2008/8/27 Michael Niedermayer <michaelni at gmx.at>:
> > On Tue, Aug 26, 2008 at 10:05:12PM +0700, Vladimir Voroshilov wrote:
> >> 2008/8/26 Michael Niedermayer <michaelni at gmx.at>:
> >> > 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
> >>
> >> I'll remove entire comment.
> >>
> >> But i'm in doubt now.
> >> What is better:
> >
> >> 1. add (int) cast to MULL arguments in i386/mathops.h (as pengvado
> >> suggest in IRC after discussing bug together with mru and
> >> Dark_Shikari)
> >
> > ok
> 
> Please look into attached patch and tell what chunks are ok.
> I used (and tested) only MULL macro but others (due to used "l"
> suffix) can have the same bug.
> Unfortunately, i'm not know asm enough good to be sure.

all ok

and yes, i can confirm the unholy black registers need to be cast
as well

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

When the tyrant has disposed of foreign enemies by conquest or treaty, and
there is nothing more to fear from them, then he is always stirring up
some war or other, in order that the people may require a leader. -- Plato
-------------- 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/00f0f7d7/attachment.pgp>