Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Chorus malfunctioning for single precision builds #1331

Closed
RoqueDeicide opened this issue Jun 4, 2024 · 11 comments · Fixed by #1339
Closed

Chorus malfunctioning for single precision builds #1331

RoqueDeicide opened this issue Jun 4, 2024 · 11 comments · Fixed by #1339
Labels
Milestone

Comments

@RoqueDeicide
Copy link

FluidSynth version

2.3.5 x64 and x86 Windows builds. Most likely affects previous versions as well.

Describe the bug

An issue had been bugging me for about half a year: playing MIDI tracks using FluidSynth would always result in a very left-heavy audio. I initially thought that my soundfont wasn't very well balanced, until I encountered a track with a channel that had center pan and a program which I had just centered. Realizing something was wrong with FluidSynth, I first turned off chorus, and audio became centered! Thinking that "synth.chorus.nr" being set to an odd number resulted in an extra output on the left side, I set it to 4 from default 3. The audio became centered! Then I switched back to 3, and audio went back to the left, then I switched it to 2... and audio stayed on the left side! After this I tested all "nr" settings from 1 to 20. Here is how they affected the audio balancing:

  • 1-3 and 9-10: audio stayed on the left.
  • 6 and 14 : audio stayed on the right.
  • 4-5, 7-8, 11-12, 15-16 and 19: audio stayed centered.
  • 13, 20: audio started on the right, but moved to the center after a few seconds.
  • 17: audio started on the left, but moved to the center after a few seconds.
  • 18: audio started centered, but moved to the left after a few seconds.

I've looked at the code, to see what's wrong, but I couldn't find anything wrong with it. So I cloned the repository and compiled the code myself to try to debug it. Neither debug nor release builds, that I've made however, were affected by the above issue! The only potential relevant difference between my builds and official ones, is that mine definitely don't use OpenMP. I don't know anything about the latter, so I cannot investigate further.

Expected behavior

There supposed to a stereophonic effect, where chorus output is moving from side to side. Prior to using my builds of FluidSynth I didn't even know what that sounded like.

Steps to reproduce

Play any MIDI track with at least some center panned channels, using official 2.3.5 x86 or x64 Windows builds. The chorus settings are defaults. The soundfont should have all of its programs use at lease 25% chorus. This can be achieved by selecting all presets in Polyphone editor, selecting a chorus row and typing 25, and saving the soundfont afterward.

@RoqueDeicide
Copy link
Author

I forgot to mention, I've built my FluidSynth using MSVC in Visual Studio 2022.

@derselbst
Copy link
Member

Thanks for the report. I have reproduced this and can confirm that something isn't correct. It doesn't seem to be related to OpenMP though:

On Linux, compiling with and without OpenMP enabled gives a nice modulating chorus effect.

On Windows, using the officially distributed MSVC binaries, synth.chorus.nr: 1 gives a strongly attenuated signal on the right channel, as if there's a standing wave in the buffer which causes wave cancellation or something. Chorus doesn't modulate either. But this is happens no matter if OpenMP is enabled or not.

Could be due to a compiler optimization. Can you pls. share the build logs when you compiled it with MSVC? I'd like to see the exact version of MSVC and the optimization flags used.

@derselbst derselbst added this to the 2.3 milestone Jun 16, 2024
@RoqueDeicide
Copy link
Author

I'm afraid, I don't know how to get a log with information you need. But, here is the version of the compiler: 14.40.33807.

Also, here is an excerpt from build.ninja file:

FLAGS = /DWIN32 /D_WINDOWS /W3 /Zi /O2 /Ob1 /DNDEBUG -MD -D _UNICODE -D UNICODE -D _WIN32_WINNT=0x0A00 -D WINVER=0x0A00 -DFLUIDSYNTH_DLL_EXPORTS

@derselbst
Copy link
Member

derselbst commented Jun 20, 2024

The issue seems to be caused by compiling with single precision floats. I.e. when calling cmake with -Denable-floats=1 (as the CI build does) the issue you've reported can be heard. When compiling manually, you probably kept the default (which is double precision) and the issue does not occur. I need to investigate...

@derselbst
Copy link
Member

I promoted all the floating point math in fluid_chorus.c to double by replacing fluid_real_t. To my surprise, the issue persists.

@jjceresa If you have some time, could you have a look into this? I've used the soundfont zipped below and then in fluidsynth's shell did a

prog 0 4
noteon 0 60 127

chorus.zip

The audible difference between compiling with enable-floats=1 and =0 is quite evident.

@derselbst derselbst changed the title Something is wrong with stereophonic chorus effect Chorus malfunctioning for single precision builds Jun 20, 2024
@derselbst
Copy link
Member

@jjceresa Never mind, I found it:

mod->a1 = 2 * FLUID_COS(w);

In single precision math, this gives 2.0f exactly. In double precision however 1.9999999543264151. Apparently that makes a difference when calculating the sine modulator a few lines later.

@RoqueDeicide Can you please test the fluidsynth-x64 binaries?

@derselbst
Copy link
Member

Unfortunately, the core problem persists: The code which is meant to handle numerical instabilities when the phase of the oscillator is near PI/2 doesn't work as intended:

static FLUID_INLINE fluid_real_t get_mod_sinus(sinus_modulator *mod)
{
fluid_real_t out;
out = mod->a1 * mod->buffer1 - mod->buffer2;
mod->buffer2 = mod->buffer1;
if(out >= 1.0f) /* reset in case of instability near PI/2 */
{
out = 1.0f; /* forces output to the right value */
mod->buffer2 = mod->reset_buffer2;
}

If the phase w is initially PI/2, a1 becomes 2 right from the beginning. buffer1 is always pretty equal to buffer2. Therefore, out will become 1 after some time when precision is exhausted.

buffer2 will now be set to buffer1, which is 1 in my case.

Since out is 1, we are entering the if clause and out is assigned 1. buffer2 will be set to reset_buffer2. But at PI/2, reset_buffer2 is 1.

buffer1 is also 1.

And that's it. Everything is 1. Modulation is now completely frozen.

So yes, promoting a1 to double precision makes this more unlikely to happen, but doesn't prevent this from happening.

@RoqueDeicide
Copy link
Author

I've downloaded and installed the binaries from x64 artifact from the link. The issue appears to persist: with nr of 3 pan is on the left, with 4 it appears centered and with 6 it's on the right.

@jjceresa
Copy link
Collaborator

jjceresa commented Jun 28, 2024

Hello,
I confirm that using single precision (float) in sinusoidal modulator (chorus.c) makes it sensitive to the following bad behaviour (b1,b2):
For clarity, following comments (L 247,L248) should be read as this:

  (247) fluid_real_t w = 2 * FLUID_M_PI * freq / sample_rate; // step phase between each sinus wave sample (in radian) 
  (248) fluid_real_t a; // initial phase at which the sinus wave must begin (in radian)

b1) The expected frequency "freq" minimum is 0.1Hz (min of synth.chorus.speed). Default "freq" is 0.3Hz.
Using a very low "freq" below a critical frequency fc ("freq" < fc Hz) makes the "step phase" "w" so small that the oscillator stay pretty at the same value "out" = sin(a). For example, "a" at 0 "out" stay to 0. For "a" at PI/2 "out" stay to 1. This correspond to continue signal.
Using single precision, the critical frequency fc is 1.8Hz. This causes the issue (b1) for default freq 0.3Hz.
Using double precision, the critical frequency fc is lowered to 0.05Hz which is lower than "freq" min and solves the issue.

b2)Using single precision, a very low initial phase (i.e "a" = 0), makes the output peak to peak [-0.777, +0.777] instead of expected [-1, +1] .
Using double precision , makes the output peak to peak very close or equal to [-1, +1] .

@derselbst

I promoted all the floating point math in fluid_chorus.c to double by replacing fluid_real_t. To my surprise, the issue persists.

To solve the issue , behaviour (b1) (for an expected frequency "freq" minimum = 0.1Hz) and behaviour (b2), we need the
following changes (c1, c2, c3):
Change c1 at line 170 :

/* Warning: for sufficient precision use double member in struct sinus_modulator ! 
(never use fluid_real_t). See https://github.com/FluidSynth/fluidsynth/issues/1331 */
typedef struct
{
    double   a1;          /* Coefficient: a1 = 2 * cos(w) */
    double   buffer1;     /* buffer1 */
    double   buffer2;     /* buffer2 */
    double   reset_buffer2;/* reset value of buffer2 */
} sinus_modulator;

Change c2 at line 244:

/* Warning: for sufficient precision use double  in set_sinus_frequency() computation !.
 Never use: fluid_real_t , cosf(), sinf(), FLUID_COS(), FLUID_SIN()). See https://github.com/FluidSynth/fluidsynth/issues/1331
*/
static void set_sinus_frequency(sinus_modulator *mod,
                                float freq, float sample_rate, float phase)
{
    double w = 2 * FLUID_M_PI * (double)freq / (double)sample_rate; /* step phase between each sinus wave sample (in radian) */
    double a; /* initial phase at which the sinus wave must begin (in radian) */

    mod->a1 = 2 * cos(w);

    a = (2 * FLUID_M_PI / 360) * phase;

    mod->buffer2 = sin(a - w); /* y(n-1) = sin(initial phase - step phase) */
    mod->buffer1 = sin(a); /* y(n) = sin(initial phase) */
    mod->reset_buffer2 = sin(FLUID_M_PI / 2 - w); /* reset value for PI/2 */
}

change c3 at line 267:

/* Warning: for sufficient precision use double  in get_mod_sinus() computation !. 
  See comment in set_sinus_frequency().
*/
static FLUID_INLINE double get_mod_sinus(sinus_modulator *mod)
{
    double out;
    ...
    ...
    return out
}

@derselbst
Copy link
Member

Good to hear from you JJC. I've committed your proposals.

The expected frequency "freq" minimum is 0.1Hz (min of synth.chorus.speed).

Right, that'll prevent us from hitting PI/2 exactly - I overlooked that.

@RoqueDeicide Please try these fluidsynth-x64 binaries again. It sounds absolutely fine to me now. If it still doesn't work for you, double-check that you have correctly replaced the binaries and provide more information on how to reproduce what you hear, esp. the Soundfont you've used, the commandline, and also an audio rendering of what you hear.

@RoqueDeicide
Copy link
Author

Chorus appears to work now. This issue can be closed by the relevant pull request merge.

Thanks everyone!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants