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

[Reimar Döffinger]

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.

Have you actually checked how bad it is?
Assuming only the least significant bit of "2.0 * M_PI / n" is wrong,
and given we support up to nbits == 18, that means the 16 least
significant bits of alpha will be wrong at the end of the loop, even
if you do not include possible rounding errors in the addition.
That would mean 37 bit of precision before the sin/cos instead of 53,
even if that does work in this case (still 13 bit left to single
precision for errors in the addition, sin, cos, etc.), I think it is
justified to say that you should do this almost never.
Certainly not if there is basically no reason like in this case.