Issue 1737----Add s24/s32 write; extend WASAPI to use s24/s32; extend enumeration to report s24/s32#1746
Conversation
|
Fixes #1737 |
derselbst
left a comment
There was a problem hiding this comment.
I've completed a first commit by commit review. Very impressive! I need to play around with that test/test_synth_render_s16.c a bit (@spessasus you might want to check out that file, since you asked about the testing suite recently)
There's one commit I do not like so much: 97e6476 - Sry, but I have an ultra wide screen, and this causes 2/3 of my screen becoming unused. I prefer using dynamic line wraps, so it nicely adjusts to whatever window size my editor is using. So pls. revert / drop that commit.
Rest looks good for the moment. I still need to test this locally, to get a bit more familiar with the changes, and also fix the CI builds.
|
I rewrote some commits, so that it now compiles successfully with every commit. The test_synth_render_s16 fails with the same output: at commits a29ed22 and 831cba1. So that's looking good. I'll look through the CI builds to see what their test output and then make this test tolerant as necessary. |
|
Getting an interesting compile error if OSAL is glib: Seems like |
It requires fluid_synth_get_ticks to be declared
1da54e1 to
725ff7f
Compare
|
Ok, so I've fixed all compilation issues so far. The only remaining problem is the failing unit tests for the two mingw32 CI builds. Looks like some float rounding error problem. In trying to reproduce them (by adding more mingw builds which use an older version though), I'm seeing strange errors that I never saw before in any CI build: it complains about the SF2 file being corrupted. Looks like an unrelated and very rare regression in the soundfont loader. So for the moment, I'm done. I need to think about this... |
|
Thank you for your Herculean efforts to deal with all the compilation issues. The s16 test is irksome because it is testing for bit identity against a pre-change snapshot. I was more surprised that it passed on the Release build with my configuration than I was with the failure on the Debug build. It is the sort of situation where close can be good enough. But that is a hard standard to put into a test. Best wishes for figuring out the soundfont loader regression. |
|
Smth. goes wrong during I don't yet understand why. I wrote a simple test for it and it didn't break. I extended the test with all the freads and seeks the sfloader does on the test soundfont and it breaks.
That s16 test actually turns out to be pretty stable, now that I've permitted an abs diff of two. The s24 and s32 tests cause all the trouble I'm currently dealing with: https://github.com/FluidSynth/fluidsynth/actions/runs/21564899911/job/62134706057#step:6:148 Sorry that I'm doing all that in this PR. I'll clean it up once I (give up or) understand what's going on. Anyway, enough for today. |
|
I don't have a mingw build environment. The following is my attempt at a dry lab analysis of your build log with the s24 and s32 test errors. The failing job is MINGW32 (32-bit) with GCC 15.2.0. Both the tests and fluid_synth_write_int.cpp are built with -O2 and no FP-stabilizing flags (-fexcess-precision=standard, -mfpmath=sse, etc.). The deltas (s24: 256 = 1<<8, s32: 8) look like a single-quantization-step rounding difference, consistent with x87/excess precision on 32-bit. Quick proof: try adding -fexcess-precision=standard for mingw32 only; if needed try -msse2 -mfpmath=sse or (diagnostic) -ffloat-store. If any of those fix it, we can decide whether to keep a mingw32-only flag or make the conversion code explicitly round/spill before int conversion. Hope this helps. |
|
The mingw32 builds indeed implicitly use an x87 FPU. Switching to SSE2 explicitly passes the test. https://github.com/FluidSynth/fluidsynth/actions/runs/21604127754/job/62320667267 I need to think about whether using |
|
Thanks for running the tests to identify that the issue is the use of an x87 FPU. Skipping the s24 and s32 tests for x87 FPUs is certainly a possibility. It is easy and there probably is not much risk of a regression that would affect only x87 FPUs. If you go that way, I think you want to use If you think it is worth some effort to keep the s24 and s32 tests for all builds, I could modify it to make it strict by default, but relax it only on x87. On x87 the comparison could be changed to:
For s32, you observed a delta of 8, so a tolerance of 8 is reasonable if it’s stable. Here is a possible modification for the s32 test: Let me know if you would like me to submit a commit with the s24 and s32 tests revised to include these x87 FPU special cases. I propose a tolerance of 256 for the s24 test. Is 16 a good limit be for the number of mismatches tolerated before reporting too many mismatches? |
|
I'd be open to both approaches. I have reverted all the recent debug commits, I'll pursue that in a separate branch. If you have some time, you're welcome to make the tests more tolerant for x87. |
|
Based on the CI results of my first commit (f7e707b), I did further work. I believe my revisions now address the x87 FPU issues appropriately. Based on my tests, I set the tolerance on the s32 test to a delta of 16, which is very small for a 32 bit sound level. I set the count tolerance to 512 based on an observed count of 414, roughly 10% of the sample frames. e8e7f66 is the good commit. |
|
I added a unit test for |
derselbst
left a comment
There was a problem hiding this comment.
I just had a final look into it and I think it's good enough to merge it now. Thanks for contributing!
|
And thanks for all the work you did to push this over the finish line! |
Summary
Refactors synth integer output rendering to a shared C++ implementation (preserving existing s16 behavior bit-for-bit).
Centralizes float→PCM conversion + dithering kernels (s16/s24/s32) in shared internal helpers used by both synth-write and driver paths.
Adds 24-bit and 32-bit integer output support to the WASAPI driver and device enumeration.
Commits, commit-by-commit review suggested
https://github.com/jimhen3ry/fluidsynth_old/commit/12c48c3d05cfe054f9b238b40e5529c59b1e182f synth: refactor integer output rendering core (templated s16/s24/s32)
https://github.com/jimhen3ry/fluidsynth_old/commit/fe7c36560b09474f1d90557d50637c645bfd22f3 tests: add render identity tests for s16/s24/s32 integer paths
https://github.com/jimhen3ry/fluidsynth_old/commit/19252727896617ef31db5f554d10d8b84999d0b0 wasapi: 24/32-bit output via shared planar-float→PCM conversion (centralizes dither/convert kernels)
https://github.com/jimhen3ry/fluidsynth_old/commit/b1d83486620dab823b75f68af8923a209960e329 wasapi: extend device enumeration for 24/32-bit formats
https://github.com/jimhen3ry/fluidsynth_old/commit/97e6476e695518be38ff73e7017eb5421be4d608 docs: fluidsettings.xml formatting--only reflow, no substantive changes
https://github.com/jimhen3ry/fluidsynth_old/commit/dd702455d6149773a3a38743b19757c1f4d8d930 docs: update audio.sample-format and WASAPI notes
Files changed
Synth/core:
Drivers/WASAPI:
Device enumeration:
src/CMakeLists.txt
Tests:
Docs:
Testing
Builds in Debug + Release (MSVC/VS) and fluidsynth -Q runs.
All synth render tests pass except: s16 identity test fails by 1 LSB in Debug only (Release passes).
The ⓉⒺⓈⓉ.mid file plays using the VintageDreamsWaves-v2.sf2 for all sample formats using the WASAPI driver in both shared and exclusive modes.