TripleOscillator: Oscillators are getting out of sync

[musikBear]

I have spend some time trying to figure out, why lmms sounds less 'punchy/ agressive' than other daw's during that i have seen something strange To reproduce insert a std 3oc set all 3 osc to saw insert a 18 bar long note in D3 play The sound variates in a sweep-like fashion:

Why? There are no

• envelope settings
• filters
• reso

why is this note not completely uniform in spektrum? If i do exactly the same with 3oc in fls-demo, i get a completely uniform spectrum

[Sti2nd]

why lmms sounds less 'punchy/ agressive' than other daw's

Maybe you haven't removed the default limiter from FL? I haven't got a clue how synthesis really work, but I can easily imagine that there are different algorithms for making a sine wave... TripleOscillator for LMMS is not the same instrument as 3OSC for FL! If you had used a VSTi and no effects in both maybe we could discuss something.

[michaelgregorius]

@Sti2nd I think it's really a bug (if it is not an intended feature).

To get an idea of what might be causing this behaviour and to simplify the conditions I have set all three oscillators to play a sine wave instead of a saw. The same effect can still be observed which means that we can likely eliminate the aliasing in the saw oscillators as a possible cause. My guess is that the three oscillators go out of phase over time and that the observed lowering of amplitude is the effect of some destructive interference due to the phasing.

Here is the sum of all three sines (fundamental + octave + 2. octave) at the beginning of the note (also the first samples of the test render):

Please note that the waveform is symmetric at this point in time and that it looks like expected. And now here is the waveform towards the end of my rendered wav file:

Under normal conditions it should look like in the first screenshot.

I noticed in the code that the TripleOscillator is composed of three OscillatorObjects and all of these instances have their own phase offset which means that it is perfectly possible for them to go out of phase, i.e. have different values at certain points in time. I would have assumed that the TripeOscillator only keeps one phase accumulator for all its oscillators (at least in mix mode). However, it's also possible that all this is by design and therefore an intended feature.

[Umcaruje]

Yeah, this is a bug. The oscillators should stay in phase if you do not change the phase offset manually.

[grejppi]

This is likely to be a floating point precision thing. Getting the frequency for a note involves a lot of divisions, multiplications, powf() calls and whatnot. The tiniest errors add up and result in the oscillators getting out of phase.

I made a test project with two oscillators. The first one has no detuning and plays the notes D4, D4, D4. The second one has its coarse detuning automated as 0, -12, -24 and playing the notes D4, D5, D6.

This is their difference.

[softrabbit]

Rendering that test project, I get a perfectly silent .wav. Can't see any action on the master output "scope" when playing it either. @grejppi, am I doing something wrong? This on master from approx. yesterday (Linux x86_64, Qt 5.4.1, GCC 4.9.2).

Update: looks like I get silence exporting this from the command line, from the GUI I get those bursts of noise. I have a feeling that this problem might come from some automation SNAFU or somehing like that. Possibly worth an issue of its own.

[michaelgregorius]

@grejppi The file works for me but I think the problem is not about using two instances of the Triple Osc. If I change the Triple Oscs in your example to only play a sine wave and move the phase of one of the synths by 180° I get perfect silence when playing in LMMS as is expected. In other words: the phases of the two Triple Oscs do not go out of sync (at least in that small example).

I also realized that export seems to be broken because when I render the perfect silence described above I get something that is definitively not silence: So it seems that currently we should not rely on rendered output when analyzing these problems. But I guess that's something for another issue.

If the problem is really caused by floating point precision then the code definitively needs refactoring. Normally you calculate the frequency from the MIDI note value and then initialize a phasor using the frequency and then only increment the phasor using an addition (and a boundary check) with each sample tick. Doing so should work for quite some time without any precision issues.

I also noted that I wrote nonsense in the last section of my comment from 12.05.2015. If the oscillators are tuned differently we obviously need separate phase accumulators for each oscillator. However, if they are tuned in octaves apart they should always sync up once the oscillator with the lowest tuning has finished a cycle.

[softrabbit]

I get phase drift when trying the original recipe from @musikBear, an 18 bar D3. And it looks to me like the drift increases with increasing sampling frequency; can't notice anything when exporting 44100x1, but with more oversampling the problem gets stronger. Smells like precision, rounding or something of that kind.

[softrabbit]

OK, now I'm sure. It's a precision problem, see https://gist.github.com/softrabbit/d8232cda4bbc3b61cdbc

Download, compile and run. Change the one #if 0 to #if 1, compile and run again. The last line shows the phases of the oscillators at 8x oversampling at the end of a 40 second note.

To quote the final lines from both runs on my 32-bit system:

0.831466       0.07414  0.0766979 (floats)
2.85105e-10    1        1         (doubles, NB. 0==1 when it comes to phase here)

Looks like this is one of the places where a 32-bit float isn't quite adequate.

[musikBear]

Very nice bughunt there :+1:

[karmux]

Which parts of core are affected by this bug? Oscillator class for sure but what else? How much work do you think is to fix it? Would be interesting to hear if fixing this makes sound more punchy for other instruments also.

[musikBear]

@karmux good question! But is this fix added to Master? The ticket is still open I see this on LMMS 1.1.90-g33d8c44 (last win-binary) But this is over 72 bar! I tried twice. Same shape. __ Settings:

Can anyone confirm for Master.

[karmux]

@musikBear I'm on master and this is what very long D3 note made in TripleOscillator (using settings on screenshot above) gives:

Exported using default settings.

Exported using highest settings.

[michaelgregorius]

This should be fixed with PR #3145. Here's the result of a long D3 as described in @musikBear's original report. Top is before the changes with float precision. Bottom is the fixed version where the phase variables have been replaced with doubles:

I have also repeated my test with the three added sine waves and they now also reproduce correctly after longer time spans.

Kudos to @grejppi and @softrabbit for their investigations! :)

[musikBear]

@karmux That is almost as weird as mine.. @michaelgregorius Yours looks perfect! If that are current Master then @grejppi and @softrabbit has crushed this bug, and the the ticket should be closed.

[Umcaruje]

@musikbear no, the screen shot is from a PR that @michaelgregorious proposed yesterday and it has not yet have been merged.

On 7 Dec 2016 13:29, "musikBear" notifications@github.com wrote:

@karmux https://github.com/karmux That is almost as weird as mine.. @michaelgregorius https://github.com/michaelgregorius Yours looks perfect! If that are current Master then @grejppi https://github.com/grejppi and @softrabbit https://github.com/softrabbit has crushed this bug, and the the ticket should be closed.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/LMMS/lmms/issues/2047#issuecomment-265435290, or mute the thread https://github.com/notifications/unsubscribe-auth/AF_bPUoGw1ohlagL2RfIUIpwW7V-w7nJks5rFqaxgaJpZM4EX2C7 .

[michaelgregorius]

Please note that using double values will not solve the problem completely. I was curious what caused the triangle pattern in @karmux' screenshot at high quality settings and it seems that its caused by the high sample rate which leads to even more phase updates during a given time span. Here you can see a comparison for the test tone with float (top) and double (bottom) phase updates using a sample rate of 192kHz during the export:

I you look at the screenshot at the original size you will see that the bottom waveform also starts to fade. With float the phases just go out of phase quicker than with double but in the long run it's also happening when using double types. However, this seems to be normal. For comparison here's five minutes of the test tone created with the Spire VST and exported at 192kHz:

Last but not least, playing a single tone for that long does not seem to be that musical anyway. Unless you are an artist in the spirit of John Cage I guess. ;)

[musikBear]

@michaelgregorius Again the bottom screenie using double, looks perfect. That said, i cant help wondering how different existing project using old 3oc will sound, with the repaired 3oc version

[michaelgregorius]

For some patches the difference in sound is already quite noticeable for not so long notes. For example if you take the default preset of 3OSC and set all three oscillators to saw then the current master will sound phasey and weak very quick. With the patch applied it sounds rock solid even if you hold the note for a long time. I have created a quick demo for this. The first two notes are a rather current master without the patch. The next two notes are the same master with the patch applied: https://soundcloud.com/blitbit/techdemo-2047/s-349L7

I have also performed a test with the song "Tectonic" by Farbro from the demo folder as it uses several instances of 3OSC. I have rendered it once with the patch applied and once without. I have then inverted the phase on one of the renders and mixed them down. If there was no difference between the tracks this would yield perfect silence. However, this is the result of the action: https://soundcloud.com/blitbit/difference-of-tectonic-by-farbro/s-AWQBk

As you can hear the bass seems to be quite affected by the change. To get an idea of how much random elements could influence the result I have also rendered the double version twice and then performed the actions. The result was that only few elements are affected by the randomness. For example the bass did not sound in this case.

So the patch would definitively have an effect on some songs. For example if a song author used the "phaseyness" created by the oscillators as part of his sound design then this aspect of the sound will be gone with the patch.

The following instruments use the class Oscillator and might be affected by the changes:

• LB302
• Monstro
• NES (but only for noise it seems)
• Organic
• 3OSC

[softrabbit]

So the patch would definitively have an effect on some songs. For example if a song author used the "phaseyness" created by the oscillators as part of his sound design then this aspect of the sound will be gone with the patch.

OTOH, that phaseyness would've behaved differently for different export settings. So I'd say it's no big loss.

[musikBear]

@michaelgregorius super job! 🎆
I really like how your double fix has made the 3oc completely solid in expression, the fact that it will influence existing songs is ofcause a bit problematic. @softrabbit conclusion is correct, but still it is a problem, especially because so many instruments are using the changed class. @tresf -perhaps add a paragraph on this in the release-notes.

[tresf]

the fact that it will influence existing songs is ofcause a bit problematic @tresf -perhaps add a paragraph on this in the release-notes.

It's the cost of fixing bugs. I wouldn't consider it problematic.

I'm not sure how to explain this in a way that would properly warn people. The fix will be mentioned, but the end user effect of the fix, that will just have to be observed first hand. :\

[softrabbit]

The following instruments use the class Oscillator and might be affected by the changes:

LB302 and Monstro should be unaffected, they handle the phase by themselves and just look up values from Oscillator. NES is probably okay if it only uses the noise waveform.

That leaves Organic and TripleOscillator, which most certainly are affected.

[musikBear]

I have an idea. I will just share it. Preserve the old oscillator class, and then let lmms select the old class, if the project time-stamp is older, than the 1.2 release-date. Old projects would then be preserved, unless the user saves it in 1.2 New projects would be with @michaelgregorius 👍 good fix

[Spekular]

People who need the old sound can finish their projects in an old version or bounce to audio. I don't think it's worth it to make the code vase messier for something like this.

[tresf]

Preserve the old oscillator class

No. Sometimes we choose to preserve bugs for backwards compat. This one is a terrible idea. It adds unnecessary complexity to our codebase. As @Spekular said, if people have a problem with this fix, they need to revert to an older version which exhibited the bug.

[michaelgregorius]

LB302 and Monstro should be unaffected, they handle the phase by themselves and just look up values from Oscillator. NES is probably okay if it only uses the noise waveform.

That's correct. However, LB302 and Mostro also store their phase information in float types which means that they are still susceptible to the same problems as 3OSC even with the fix applied. If we want to change this I propose that we create new issues for these instruments.

Concerning the backwards compatibility: I also think that keeping both versions of the class would only bloat and complicate the code. Perhaps it makes sense to add some warning to the dialog that pops up when you open an older file with a newer version. Something along the lines of: "You are loading a file created with version X in version Y. Some instruments might sound different." Although I am not sure whether this dialog still exists as I was not able to trigger it with any of the demo songs in a development build.

[michaelgregorius]

I have done some more testing with LB302 and Monstro and interestingly they do not exhibit the same problems as 3OSC although they use float types for their phase information. So it seems like the problem with 3OSC might in part be caused by the way the phase information is updated and not only due to the usage of float instead of double.

This also means that there is no need for new issues. :)

[tresf]

You are loading a file created with version X in version Y. Some instruments might sound different

432, #2656 ;).

[tresf]

Closed via #3145.

[michaelgregorius]

Reopening this one as I have reverted the changes from e3e14bb730440fd577b6800c0928f255ac52df63 due to the problems described in #3292.

[PhysSong]

I found something was wrong in #3145: m_ext_phaseOffset is const double &, but some plugins still pass float variable to the constructor. If I change them to double, that works great and #3292 doesn't occur anymore.

[michaelgregorius]

Great find @PhysSong! Please create a pull request so that we can double check for #3292.

[PhysSong]

Okay... Target to master, or stable-1.2? I guess this issue isn't serious.

[zonkmachine]

stable-1.2 I think. This is a regression that many have pointed out.

[lukas-w]

Are you sure it is a regression? AFAIK we've always been using float for this.

Btw, if we want to increase precision further (as even double will have measurable error eventually especially with high sample rates as @michaelgregorius pointed out in https://github.com/LMMS/lmms/issues/2047#issuecomment-265543960), we could make use of a special algorithm such as Kahan summation. This keeps the error down to a constant value (meaning the error is indepent from the number of samples and will not therefore not increase in time), so float is probably sufficient if we use that algorithm. Boost provides an implementation in its Accumulators library.

[PhysSong]

@lukas-w Good point, but there are some more issues that should be fixed(mostly powf() precision issue I guess). Relative error in 2^-24 is small, but it may break sync if accumulated enough. So I think it might be the best not to use float for every calculations related to phases. It will resolve sync issues almost perfectly, I guess.

[PhysSong]

as even double will have measurable error eventually especially with high sample rates

I think it can be fixed by using the method I mentioned above. I will make a quick test for that.

[PhysSong]

I've tested with double and it showed perfect sync for 1 minute note with 7040Hz(A8), sampling rate 192000Hz, oversampling x8. Theoretically, the error from double is negligiby small, even for one-day-long notes, if I remember correctly.

[zonkmachine]

Are you sure it is a regression? AFAIK we've always been using float for this.

No. I took it from memory. Memory wrong.

[zonkmachine]

@PhysSong Since it looks like you've basically solved this, maybe bump it to 1.2.0 ?

[PhysSong]

Note that fixing this issue may cause performance degrade since it replaces single-precision(float) calculations with double-precision(double) ones.

[SecondFlight]

@PhysSong What's the status on this? I don't see a linked PR for the fix you mentioned, but if the fix is in master or stable 1.2 I'd like to close this issue.

[PhysSong]

I haven't added the patch because it may lead to performance degrades due to the change from float to double.

[softrabbit]

Performance probably doesn't suffer much. See https://stackoverflow.com/questions/4584637/double-or-float-which-is-faster for a discussion. My proof of concept code in a comment above, https://github.com/LMMS/lmms/issues/2047#issuecomment-120009714, with slight modifications to run longer and print less gave results like this (seconds of user time):

NB. It could be that doubles are slower than floats on e.g. ARM systems, but even so I think this phase accumulator should be a small enough part of the whole CPU usage that it wouldn't hurt much.

[michaelgregorius]

I think the phase update code is only a very small part of what LMMS is doing when it's playing a song and I am pretty sure that the code with the double implementation will never show up in profiler runs.

To quote Donald Knuth: "The real problem is that programmers have spent far too much time worrying about efficiency in the wrong places and at the wrong times; premature optimization is the root of all evil (or at least most of it) in programming."

Concerning potentially weaker hardware like ARM: If a user wants to run LMMS on a weaker hardware then that user should pay the price for that and not all of LMMS' users.

[DeRobyJ]

If we really don't want to switch to double, we could at least work around the LFO syncing problem by secretly faking it.

Like resetting the value to initial phase every time we get to a new measure. This behavior would be activated once the Sync to Tempo feature is used, and it would be deactivated as soon as the LFO speed is changed in any other way.

A more proper, but quite longer, fix would be to refactor this feature to calculate values based on position rather than adding it up over time...

[michaelgregorius]

Hi @PhysSong,

can you please create a pull request of your changes so that we can put them through a profiler?

To be frank, I find it a bit sad that this issue is five years old now and that it's still open although we know what the problem is, what the solution is and are very close to fixing it. I am pretty sure that if the oscillator had been implemented from the start using double precision no one would have gotten the idea to replace the double with float for performance reasons, especially if it worsens the quality of the oscillator by that much.

[zonkmachine]

__

Concerning potentially weaker hardware like ARM: If a user wants to run LMMS on a weaker hardware then that user should pay the price for that and not all of LMMS' users.

I'm one of those users on a weaker machine. I vote double precision.

[DigArtRoks]

I stumbled in this long thread and was intrigued by it.

I did some experiment as well with monstro as that one is said to be stable in previous posts using phases stored in floats. Put osc2 and osc3 to a saw waveform set coarse value to -12 respectively -24 semitone. Play F3. It takes a while (longer than 3osc) but you start to hear changes in the sound as well. By tearing down the volume of one the oscillators and panning the other one to the left and one to the right, you can see in audacity that the phases of left and right also start to shift versus each other.

The phenomenon is accumulation of errors due to limited precision. Going from float to double will make it appear later, but eventually it will be there as well. Especially if you consider the whole range of notes and different sampling rates. Depending on the note, the errors accumulate with another speed and become more or less audible.

It is like real analog oscillators, they also are never perfectly aligned (apart from the fact that they may even drift), except if you sync them. Unfortunately in 3osc, the oscillator used to sync the one above does not sound.

[softrabbit]

The phenomenon is accumulation of errors due to limited precision. Going from float to double will make it appear later, but eventually it will be there as well.

A hell of a lot later. The fraction part in single precision floats is 23 bits, in double precision you get 52 bits. If those 29 additional bits translate straight to oscillator drift time it should be the same drift in 17 years as you get in a second with single precision.