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

[Ivan Kalvachev]

On 7/5/11, Reimar Döffinger <Reimar.Doeffinger at gmx.de> wrote:
> 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));


I'll write more detailed explanation tomorrow.
I tried benchmarking some of the implementations.
So far llrint is the slowest on all CPU.
ldexp is also slower than multiply on all CPU.
(all cpu = Intel Clarkdale and AMD TBird).