[FFmpeg-devel] FATE & Regressions (and PPC is broken)

[Måns Rullgård]

M?ns Rullg?rd <mans at mansr.com> writes:

> Diego Biurrun <diego at biurrun.de> writes:
>
>> On Sat, Mar 15, 2008 at 02:24:41AM +0100, Michael Niedermayer wrote:
>>> On Sat, Mar 15, 2008 at 12:36:36AM +0000, M?ns Rullg?rd wrote:
>>> > Mike Melanson <mike at multimedia.cx> writes:
>>> > 
>>> > > The following regression test is broken on PowerPC (gcc 4.1.2):
>>> > >
>>> > > diff -u -w
>>> > > "/home/melanson/ffmpeg/ffmpeg-main"/tests/ffmpeg.regression.ref
>>> > > tests/data/vsynth.regression
>>> > > --- /home/melanson/ffmpeg/ffmpeg-main/tests/ffmpeg.regression.ref
>>> > > 2008-03-08 19:10:07.000000000 -0800
>>> > > +++ tests/data/vsynth.regression        2008-03-12 14:27:53.000000000 -0700
>>> > > @@ -197,7 +197,7 @@
>>> > >  353368 ./tests/data/a-flac.flac
>>> > >  c4228df189aad9567a037727d0e763e4 *./tests/data/flac.vsynth.out.wav
>>> > >  stddev: 33.31 PSNR:65.87 bytes:1040384
>>> > > -a1e56bb034c2773438ba1c830c4cea07 *./tests/data/a-wmav1.asf
>>> > > +59575b4756d674cd8c8d53d86c08e8cb *./tests/data/a-wmav1.asf
>>> > >  106004 ./tests/data/a-wmav1.asf
>>> > >  stddev:12251.50 PSNR:14.56 bytes:1056768
>>> > >  stddev:2106.00 PSNR:29.85 bytes:1048576
>>> > > make: *** [codectest] Error 1
>>> > 
>>> > OK, I tracked this one down.  It turns out lrint() rounds slightly
>>> > differently for whatever reason (bug or otherwise).  Attached patch
>>> > changes it to lround() (which has well-defined rounding behaviour).
>>> > With this change, the results are the same on x86_64 and ppc64.
>>> > 
>>> > This raises the broader question of lrint() usage in FFmpeg.  The
>>> > documentation for the lrint() family says:
>>> > 
>>> >   These functions shall round their argument to the nearest integer
>>> >   value, rounding according to the current rounding direction.
>>> > 
>>> > Clearly, in a library we do not know the current rounding direction,
>>> > and it would be rude of us to change it.  The lround() family, on the
>>> > other hand, always rounds the same way:
>>> > 
>>> >   These functions shall round their argument to the nearest integer
>>> >   value, rounding halfway cases away from zero, regardless of the
>>> >   current rounding direction.
>>> 
>>> We have to use lrint* because its faster see:
>>
>> Hmm, does speed really matter that much in that part of the WMA encoder?
>> Or is that just a comment for the general case and Mans' patch is OK?
>
> Yes, speed matters.  Besides, further investigation shows the
> inaccuracy arises elsewhere.  This change worked by accident.

I've tracked down the real reason for the difference.  It results from
lost precision by use of floats in mdct.c.  Attached patch makes a few
temporary values double instead.  I don't know what it might do to
performance, though.

-- 
M?ns Rullg?rd
mans at mansr.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: mdct-precision.patch
Type: text/x-patch
Size: 842 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080316/89557c43/attachment.bin>