[MPlayer-cvslog] r33811 - trunk/libaf/af_format.c

[Reimar Döffinger]

On Tue, Jul 05, 2011 at 12:19:41PM +0300, Ivan Kalvachev wrote:
> On 7/5/11, Reimar Döffinger <Reimar.Doeffinger at gmx.de> wrote:
> >
> >
> > On 5 Jul 2011, at 00:24, iive <subversion at mplayerhq.hu> wrote:
> >
> >> Author: iive
> >> Date: Tue Jul  5 00:24:52 2011
> >> New Revision: 33811
> >>
> >> Log:
> >> Audio format conversion from float to int is not checked for overflow.
> >> The float may be slightly off the [-1;1] range thus causing audible
> >> crackling noise.
> >>
> >> Add proper clipping using av_clip functions.
> >
> > That is not all you did.
> > You changed the multipliers, you added a use of ldexp for half the cases and
> > you made the code throw away up to 8 bit precision when converting to int
> > (for files decoding to float in the range [-1/256;1/256]).
> > Also float mantissa has 24 bits precision not 23.
> 
> I really missed to describe this in the message.
> 
> The exact multiples of 2 constants are the proper ones. You can see
> them used in integet->float conversion case.
> The decreased by 1 constants were used as a mean to avoid overflow
> when we have 1.0, however they shrink the range and it could  lead to
> distortion/fadedown if there are multiple back and forth conversions.
> Indeed the 1.0 would be clipped to 0.996093475, but all other values
> would be stable.

However that clipping introduces a DC bias.
Not that it really matters though.

> The ldexp() is used only as a faster way to multiply, as it changes
> only the exponent. It have same requirement as lrint (C99,
> _OPEN_SOURCE 600, etc). Do you prefer constant multiply or do you want
> all cases replaced with ldexp()?

I don't really care unless someone finds a significant performance
difference.
But doing it inconsistently is a major maintenance issue since you
can never be sure whether there is maybe some minor but critical difference
between the cases that must be preserved.

> The float is 32 bits total, 1 bit sign, 8 bits exponent and 23 bits
> mantissa. But you are right, there is one bit that is not stored, but
> assumed. You are also correct for the loss of precision in this case.

The loss of precision is 8 or 9 bits if the exponent is -8, not just
one.

> 3. Doing the multiply of 2^31 first, and then clipping float numbers
> before doing rounding. Thanks to lrint default rounding we'd have to
> use (1<<31)-(1<<8) again. Either use trunc() or simple i=(int)f;
> conversion to avoid it.

There is no problem with clipping to (1<<31)-(1<<8), that does not
lose precision in general. The problem is with extracting a 24 bit
int and left-shifting by 8 bit.
Also the max value is 1<<31 - 1 so that's what it should be clipped to.
There's also no real point in doing the multiply for the clipping case,
unless you can use that to save some cycles.
Not particularly fast, but I think it should cover everything except
probably NaNs (of course must be absolutely sure the compiler evaluates
"bits" to a constant):
#define CONV_F2S(in, out, bits)
if (in <= -1)
  out = -(1ULL << (bits - 1));
else if (in >= 1 - 1.0/(1ULL << bits))
  out = (1ULL << (bits - 1)) - 1;
else
  out = lrint(ldexp(in, bits - 1));