pa_converters.c does not convert float to int correctly

[RossBencina]

For example here:

https://github.com/PortAudio/portaudio/blob/5094303863e931432d2cd65eadda8851cc3676cd/src/common/pa_converters.c#L345

But also elsewhere.

The main problem is that float to int casts round to zero, not to negative infinity. So there is a discontinuity at 0.

Here's some ideas:

https://www.cs.cmu.edu/~rbd/papers/cmj-float-to-int.html

https://android.googlesource.com/platform/system/media/+/master/audio_utils/primitives.c

https://cs.android.com/android/platform/superproject/+/master:system/media/audio_utils/include/audio_utils/primitives.h;l=779;drc=fafe9eba3fdd3a94439ba5574d095fa915bc73aa

The next step is to agree on what action should be taken.

[RossBencina]

Any changes that we make should be benchmarked for performance.

[dmitrykos]

@RossBencina, you are correct indeed. I did not look for a looong time to these routines and in fact scaling values (0x7F..s) do not look correct. The clamp values are fine though.

I propose to replace hardcoded 0x7..s with such macro which provides the correct scaling value for all types we need:

#define PA_CONVERT_SCALE_VALUE_SIZE(BYTES) (1 << (((BYTES) * 8) - 1))
#define PA_CONVERT_SCALE_TYPE(TYPE) PA_CONVERT_SCALE_VALUE_SIZE(sizeof(TYPE))

It will give:

• int8: 0x80
• int16: 0x8000
• 24-bit packed int, 3 bytes: 0x800000
• int32: 0x80000000

[ClippingAudio]

Just want to add here that in using Port Audio we have also noticed that with clipping turned off the conversion from float to int can overflow for any maximum value sample (+1.0) during the scaling. This occurs due to a lack of precision in the float scalar which results in a rounding error that causes the sample to go out of range. A quick "fix" is to turn clipping back on which brings the offending value back in range.

One possible solution would be to cast the scaled value to a larger type (a double) so that we get better precision and no rounding errors e.g.

double scaled = *src * ((double) 0x7FFFFFFF);
PaInt32 scaledInt = (PaInt32)scaled;

[dmitrykos]

To my view Float32_To_Int32 and Float32_To_Int32_Dither as well as other Float32_To_XXX functions which are not doing clipping need to be aliases to their Float32_To_Int32_Clip and Float32_To_Int32_DitherClip and Float32_To_XXX_Clip friends. The flag paClipOff with this approach becomes obsolete and noop.

Not doing clipping during this conversion leads to an invalid values from time to time due to rounding, so basically users must always force clipping by using paClipOff flag, although if they don't do that PortAudio will deliver the erroneous result - periodic pops & clicks if audio data is spread in full range, e.g. [-1.0, 1.0].

E.g. my proposal - deprecate paClipOff by making it noop, and use only _Clip and _DitherClip for conversion from float to integer. I can take this task if it makes sense for all and there is an agreement for such change.

[RossBencina]

It will give:

• int8: 0x80
• int16: 0x8000
• 24-bit packed int, 3 bytes: 0x800000
• int32: 0x80000000

It's controversial, and maybe we want both options. However, historically, for PortAudio we want to scale by 0x7FFF (etc). Because we do not want float 1.0 to scale to an overflow (even when rounded correctly). I get that this is not the same issue that ClippingAudio has raised.