PortAudio / portaudio

PortAudio is a cross-platform, open-source C language library for real-time audio input and output.
Other
1.43k stars 296 forks source link

Improve float to int truncation precision #835

Open dmitrykos opened 1 year ago

dmitrykos commented 1 year ago

Improve float to int truncation precision by forcing floating-point rounding mode to rounding towards zero or by using CPU SIMD which truncates by rounding towards zero to nearest.

This PR addresses issues discussed in #390 and PR #403 which removed lrint in favor of C-style cast of floating point value to integer.

Currently, conversion via C-style cast works as expected, i.e. truncates floating point value to integer with rounding towards zero to nearest, only if CPU is set to rounding towards zero to nearest mode. But, if user code or compilation flags set some other rounding mode the conversion via C-style cast becomes erroneous results in unexpected result.

To circumvent it this PR proposes to set needed required rounding mode via fesetround API and reset it back to previous user-set rounding mode when conversion operation completes.

Besides fesetround API it is also possible to use specialized CPU SIMD which truncates with rounding towards zero, for example it can be x86 SSE and SSE2, ARMv8. By using CPU SIMD it is possible to avoid calling fesetround API completely that improves run-time performance.

This PR implements these 2 approaches:

  1. Use fesetround to set required rounding mode when no CPU SIMD is present for the truncation of float and double to int for the required rounding mode
  2. Use CPU SIMD for truncation of float and double to int and in this case code related fesetround is optimized away by the compiler in Release build, so no overhead happens during run-time
RossBencina commented 12 months ago

Hi Dmitry,

Phil and I have reviewed your change together. Improving the converter code is definitely something we want to do. We have the following critical questions which need to be covered first:

Then there are lesser issues that will need to be discussed/addressed prior merge:

RossBencina commented 12 months ago

note so I don't forget: any change here may affect the relevance of pa_x86_plain_converters.c/.h

dmitrykos commented 12 months ago

Hi Ross!

Thank you for your comments.

  • I know it changes the current behavior, but we think that round-to-nearest is the best choice numerically, mostly because it doesn't introduce zero-crossing distortion. Is it easy and/or possible to change your code to round-to-nearest?

Yes it is indeed possible by setting fesetround to FE_TONEAREST but in this case the optimization with SIMD will be lost because SSE and ARMv8 SIMD is truncating always towards zero. So SIMD part of implementation needs to be removed. Also, I did not change scaling of float to integer but for FE_TOWARDZERO scaling has to be updated to: 0x80000000 for int32, 0x800000 for int24, 0x8000 for int16, 0x80 for int8 in order to reach full scale of integer representation for -1 and 1.

What tests have you run? Have you tested on ARM? if so, which compilers and platforms?

Windows platform but PA GitHub actions also compile for Ubuntu and OSX. fesetround has consistent behavior across platforms, so one would be sufficient. In case of FE_TONEAREST we would just guarantee C-style cast happens with default FE_TONEAREST rounding mode.

I did not implement PA tests for assembler inserts for SSE and ARMv8 but that code is taken from my project with >10 years of operation including ARMv8 on Linux and iOS. It makes sense to make tests if we agree on FE_TOWARDZERO with SIMD optimization.

C89 and MSVC backward compatibility considerations

I am able to compile for Windows XP with MinGW but do not have older MSVC than VS2019. MSDN has this API available for vs140 tools which if I am not mistaken can be used to target Windows XP builds. If anybody reports incompatibility I could add workaround in the future.

Compiler compatibility of SSE intrinsics?

Must be fairly portable as that intrinsic is available on all compilers.

Compiler compatibility of ARM inline asm syntax (this isn't going to work in MSVC is it? does it work in Clang?)

I updated implementation to limit to GCC and Clang but utilized platforms flags were set by GCC or Clang only, so it was fine already.

Function naming

Yes, adjusted.

To preserve backwards compatibility with older PA versions I probably need to change implementation to FE_TONEAREST and remove SIMD implementation. Or, update scaling to integer as I commented earlier and rely on current implementation. I will try to find time and implement zero crossing distortion comparison of current PA's implementation + FE_TONEAREST with a newly proposed implementation + FE_TOWARDZERO.

dmitrykos commented 10 months ago

After double checking PA implementation and specifically pa_x86_plain_converters.c I realized that expected by the library rounding mode is actually rounding to nearest and not rounding towards zero as I proposed initialy.

Therefore, I updated description and implementation to set rounding to nearest mode and removed all SIMD-related code which was doing rounding towards zero. That simplified PR and made it similar to pa_x86_plain_converters.c which forces rounding to nearest mode via fpuControlWord_ https://github.com/PortAudio/portaudio/blob/7e62dfc832fcdbcf249cf2a05721c4512fd59fef/src/os/win/pa_x86_plain_converters.c#L129

@RossBencina, @philburk would you please check implementation again as it looks quite straight forward now.