[FFmpeg-devel] [PATCH] Fix for inverted sign in ffvorbis?audio?output (resubmit)

[Siarhei Siamashka]

On Thursday 14 May 2009, Michael Niedermayer wrote:
> On Thu, May 14, 2009 at 12:09:31PM +0300, Siarhei Siamashka wrote:
> > On Thu, May 14, 2009 at 11:29 AM, Reimar D?ffinger wrote:
> > > On Thu, May 14, 2009 at 11:06:21AM +0300, Siarhei Siamashka wrote:
> > >> And what about this variant?
> > >>
> > >> + ? ?alpha = (scale < 0 ? 0.5 * M_PI : 0) + 0.25 * M_PI / n;
> > >> + ? ?scale = -sqrt(fabs(scale));
> > >> ? ? ?for(i=0;i<n4;i++) {
> > >> - ? ? ? ?alpha = 2 * M_PI * (i + 1.0 / 8.0) / n;
> > >> - ? ? ? ?s->tcos[i] = -cos(alpha);
> > >> - ? ? ? ?s->tsin[i] = -sin(alpha);
> > >> + ? ? ? ?s->tcos[i] = cos(alpha) * scale;
> > >> + ? ? ? ?s->tsin[i] = sin(alpha) * scale;
> > >> + ? ? ? ?alpha += 2.0 * M_PI / n;
> > >
> > > Rounding errors accumulate like that, IMO you really should never do
> > > that unless a) you know the value you add can be represented exactly
> > > or b) you absolutely can't avoid it for performance reasons.
> >
> > Everything is calculated in double precision floating point, with the
> > final results stored in single precision. Additionally, transcendental
> > functions generally have their precision unspecified unless I'm
> > mistaken. There are plenty of extra bits of precision in intermediate
> > calculations. Regression test does not detect any problems, this
> > probably means that accumulated rounding errors never had any real
> > impact and the final result of double->float conversion was always the
> > same (of course it could affect the results theoretically, but this
> > probability is rather low).
>
> We did spend enough time debuging cos/sin rounding errors, in the past
> already iam not going to take any chances when theres no benefit from it at
> all

Yes, I know this. Also "cos(x)" is not exactly equal to "sin(x + 0.5 * M_PI)"
in practice. So there is still a small theoretical chance that having scale
factor set to "-1.0" can produce results not completely equal to using scale
factor "1.0" initially and changing sign of IMDCT output manually. This does
not seem to happen in tests with real audio files though (and this does not
happen even with my test variant of the code quoted above, which had precision
for intermediate calculations intentionally relaxed).

Would this patch be finally OK (kind of blend of your and Reimar's
suggestions)?

-- 
Best regards,
Siarhei Siamashka
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Support-for-getting-i-MDCT-output-multiplied-by-a-c.patch
Type: text/x-diff
Size: 9932 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090515/48405b64/attachment.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090515/48405b64/attachment.pgp>