Improved Oscillators #4397

curlymorphic · 2018-06-03T15:57:15Z

LMMS used the same oscillators in many places for audio generation. This pull request provides a band limited option, so no aliasing, and a cleaner signal. The Implementation has been via TripleOscillator, will the addition of a new button per oscillator allowing switching between the classic sounds, and the new. Currently the sin, triangle, saw, square, moog saw and the exponential waveforms have a band limited version. The user wave shape is still WIP

The clearness of the waveforms can be clearly heard, and become more apparent when using FM or phase mode, backward compatibility is retained.

I have a wiki page with explanations images and audio files
https://github.com/curlymorphic/lmms/wiki/LMMS-Oscillator-Class

Prototype of using multiband wavetables in the Oscillator class. This implementation hijacks the original getsample functions, as such replaces the original waveforms of the square, triangle and saw waves. This is not intended to be merged in it’s current format. The additional code is manly a port from a separate GPL project I have developed and requires refactoring to LMMS coding conventions. https://github.com/curlymorphic/involve.git

Wavetables are held in in array of tables accessed s_waveTables[WaveShape][band][frame]

sine triange saw square implemented

curlymorphic · 2018-06-03T16:05:23Z

I am aware it appears to fail to link the fftw3 library , and am investigating.
I have pushed a temporary fix,to allow review of the concepts

karmux · 2018-06-08T20:30:30Z

It plays silence using these settings. There are many more settings that play silence when wt buttons are active. When I deactivate last wt button it starts to make sound again.

karmux · 2018-06-08T20:54:20Z

One more issue.

When default (wt is off) TripleOscillator/SEGuitar.xpf is played at the same time with simple beat made of kick01.ogg then LMMS always crashes very quickly. I can't crash LMMS when playing SEGuitar.xpf in solo.

SecondFlight · 2018-06-08T20:56:50Z

Thanks for testing this, @karmux!

refactored the fft code

curlymorphic · 2018-06-09T18:40:36Z

@karmux
Thank you for your testing, you reveled a bug where the moogsaw and the exp waveforms were not generated. I have updated the code.

karmux · 2018-06-10T01:26:11Z

Thanks you @curlymorphic. Both issues are fixed now.

Wallacoloo · 2018-06-10T22:47:17Z

Added the in progress label since the end of the description ("This code is not being presented as finished, but I would like to present the concept for comments") makes it sound like it's not quite finalized (and there are still some TODOs sprinkled in the code). But please feel free to remove that label once you're happy with it & ready for the code to be reviewed/merged 👍

qnebra · 2018-06-13T09:23:19Z

This with @SecondFlight 3xOSC randomization patches will be great.

curlymorphic · 2018-06-24T16:45:21Z

I feel this is ready for both testing and code review.
I shall of course squash once all amendments are finalized, before merging

The adding of the WT oscillators to additional internal plugins would be better handled in additional pull requests, once any issues are ironed out.

Wallacoloo

The math looks sound. But there are a lot of missing delete statements that need to be addressed (also, do you have to do something like fftw_free a plan once you're done using it?) and most importantly, all of the code and variables used to set up the wavetables should be static.

include/Oscillator.h

plugins/triple_oscillator/TripleOscillator.cpp

src/core/Oscillator.cpp

Wallacoloo · 2018-06-24T20:57:32Z

include/Oscillator.h

+	sample_t *m_generatedWaveTable;
+	static float *s_waveTableBandFreqs;
+	static int s_waveTablesPerWaveformCount;
+	static sample_t ***s_waveTables;


Accessing an element in s_waveTables requires 3 levels of indirection. But we know the size of this array at compile time. I think we should declare it as something like:

static sample_t s_waveTables[WaveShapes::NumWaveShapes][32 /*s_waveTablesPerWaveformCount*/][WAVETABLE_LENGTH];

That way all of these are sitting next to each other in memory & the compiler can translate indices into memory addresses much more easily.

This has the additional advantage of removing the untidy memory allocations for the generated tables, that were never correctly deleted.

Wallacoloo · 2018-06-24T21:03:42Z

src/core/Oscillator.cpp

+void Oscillator::generateSawWaveTable(int bands)
+{
+	float max = 0;
+	m_generatedWaveTable = new sample_t[WAVETABLE_LENGTH];


These generate<X>WaveTable() functions all leak the allocation associated with m_generatedWaveTable (i.e. there's no delete matching this new statement). I would advise just returning a std::array<sample_t, WAVETABLE_LENGTH> type rather than returning the data through a heap-allocated member variable. i.e.

std::array<sample_t, WAVETABLE_LENGTH> Oscillator::generateSawWaveTable(int bands) { std::array<sample_t, WAVETABLE_LENGTH> wave; // populate wave[i] for each index i. return wave; }

A few more alternatives:

Pass the address to write results

Use arrays and/or some smart pointers for s_waveTables

Also, I think you may reduce duplications by using passing waveform type/pointer to a function that returns coefficient.

The generation functions have been revised to take a pointer to the relevant location in s_waveTables. This addresses the issue of the memory leakage, and removes unnecessarily verbose code.

src/core/Oscillator.cpp

Wallacoloo · 2018-06-24T21:25:37Z

include/Oscillator.h

+					f1 + 1 :
+					0;
+		return linearInterpolate(s_waveTables[shape][waveTableBandFromFreq(m_freq)][f1],
+				s_waveTables[shape][waveTableBandFromFreq(m_freq)][f2], fraction(frame));


Right now, the compiler can't always see into waveTableBandFromFreq(m_freq) since that's defined in the source file. So I'll bet it's going to actually call that function twice, since it won't be able to verify it has no side-effects. This might change if you move that function into the header, but I'd feel more comfortable if you assigned that result to a variable, e.g. something like

int band = waveTableBandFromFreq(m_freq); return linearInterpolate(s_waveTables[shape][band][f1], s_waveTables[shape][band][f2], fraction(frame));

Done.

On a side note do you have any references or key words to type into google, so I can do some reading on this matter please.

@curlymorphic I don't know any specific resources to point you to regarding compiler optimizations. If you can read assembly code, you can figure out what kinds of things compilers are capable of by typing code into http://godbolt.org and then trying to figure out what the assembly code is doing. You can even select old compiler versions and see how the code they generate and the optimization they can do has improved over time. But that can be a lot of work.

@curlymorphic The optimization @Wallacoloo is describing is called common subexpression elimination.

src/core/Oscillator.cpp

PhysSong · 2018-06-24T22:57:13Z

src/core/Oscillator.cpp

@@ -88,6 +118,206 @@ void Oscillator::update( sampleFrame * _ab, const fpp_t _frames,
 	}
 }

+void Oscillator::generateSineWaveTable(int bands)
+{
+	bands = bands;


Could you please use Q_UNUSED instead ? Currently it's unclear why this statement exists. Also, some compilers will complain about it.

noted, seeing as a sinewave has no harmonics, as the generation is independent of the number of bands, bands has been removed from the function signature.

plugins/triple_oscillator/TripleOscillator.cpp

PhysSong · 2018-06-24T23:03:07Z

include/Oscillator.h

+	int  waveTableBandFromFreq(float freq);
+	void generateWaveTables();
+	void allocWaveTables();
+	void createFFtPlans();


Somewhat odd choice of cases, I guess either FFT or Fft would be better.

PhysSong · 2018-06-24T23:15:44Z

src/core/Oscillator.cpp

+void Oscillator::generateSawWaveTable(int bands)
+{
+	float max = 0;
+	m_generatedWaveTable = new sample_t[WAVETABLE_LENGTH];


A few more alternatives:

Pass the address to write results

Use arrays and/or some smart pointers for s_waveTables

PhysSong · 2018-06-24T23:20:31Z

src/core/Oscillator.cpp

+void Oscillator::generateSawWaveTable(int bands)
+{
+	float max = 0;
+	m_generatedWaveTable = new sample_t[WAVETABLE_LENGTH];


Also, I think you may reduce duplications by using passing waveform type/pointer to a function that returns coefficient.

src/core/Oscillator.cpp

PhysSong · 2018-06-24T23:37:26Z

src/core/Oscillator.cpp

+		s_waveTables[WaveShapes::TriangleWave][i] = m_generatedWaveTable;
+	}
+	//generate moogSaw tables
+	//generate signal buffer


I think it's possible to generate moog saw/exponential(actually parabolic) without FFT. Though their Fourier decompositions are not known well, but I can calculate it if you want. 😉

I was intending to keep the fft, As I am intending to use the same functionality for the user wave shapes, However, I would prefer to get the current scope implemented correctly first.

The FFT analysis has remained, as the functionality has now been extended to the user waveforms

curlymorphic · 2018-06-25T21:34:14Z

@Wallacoloo @PhysSong Thanks for the review, I shall work through the changes over the coming days

SecondFlight · 2018-06-25T23:25:10Z

It would appear that playing a note too high has some unintended side effects. If you're going to test this, turn your volume down.

Steps to reproduce:

Set 3osc to the saw or square waveforms
Move the tuning control above the piano to the lowest possible setting
Click the highest note you can

This produces a genuinely horrifying noise. I've gotten it to segfault by moving my mouse back and forth, but I can't seem to reproduce this consistently.

SecondFlight · 2018-06-25T23:30:09Z

Also, I remember noticing that the FFT on some notes doesn't extend all the way to the nyquist rate. How far up it would go varied per-note if I'm remembering correctly. It's possible that this isn't realistic to fix, and it doesn't sound audibly bad to me. Still would be nice to get that precision if it's not out of your way and doesn't have a huge performance impact.

Or I might be remembering things wrong. I don't have an easy analysis setup on Ubuntu so I can't test it here.

SecondFlight

See my recent comments on the PR, especially the first one.

PhysSong · 2018-06-26T00:09:44Z

This produces a genuinely horrifying noise. I've gotten it to segfault by moving my mouse back and forth, but I can't seem to reproduce this consistently.

I guess it's an out-of-range error. Current implementation seems to handle pitch within MIDI standard correctly, but it doesn't seem like for too high or low notes.

Wallacoloo · 2018-07-23T21:30:26Z

@curlymorphic I'm sorry I didn't get to this sooner. Everything looks ready to merge, except for the two new comments I just left.

PhysSong

There are some minor issues which may be ignored. You can ignore this review and leave them as-is if you want.

PhysSong · 2018-08-23T05:19:46Z

include/Oscillator.h

@@ -79,6 +83,14 @@ class EXPORT Oscillator
 		delete m_subOsc;
 	}

+	static void waveTableInit();
+	static void destroyFFTPlans();


Can you move this line after createFFTPlans for consistency?

The destroyFftPlans is public so it can be called from Engine::destroy() on clean up, createFftPlans is private, I feel this is currently in the correct location.

src/CMakeLists.txt

src/core/SampleBuffer.cpp

src/core/main.cpp

LmmsBot · 2018-08-25T11:44:34Z

Downloads for this pull request

Linux: download.
Windows 32 Bit: download.
Windows 64 Bit: download.
Mac: download.

Generated by the LMMS pull requests bot.

curlymorphic · 2018-08-25T16:07:29Z

Updated the handling of the wavetables for the userWaveShape, the buffer is now dynamically allocated only when used by Oscillator greatly reducing memory usage.

@Wallacoloo, @PhysSong I feel I have addressed all outstanding comments

PhysSong · 2018-08-26T02:28:06Z

src/core/Oscillator.cpp

+
+void Oscillator::generateAntiAliasUserWaveTable(SampleBuffer *sampleBuffer)
+{
+	delete sampleBuffer->m_userAntiAliasWaveTable;


Possible memory leaks: sampleBuffer->m_userAntiAliasWaveTable[i](0 <= i < WAVE_TABLES_PER_WAVEFORM_COUNT) are not deleted.

I also think you can move allocation/deallocation code to SampleBuffer instead. If so, you can share the deletion code with SampleBuffer destructor.
Alternatively, you may use std::unique_ptr as suggested by @Wallacoloo. I can provide an example code if you want.

PhysSong · 2018-10-26T06:25:37Z

@curlymorphic Do you have time to address the memory issue? I think that's the only remaining issue here.

curlymorphic · 2018-10-30T10:32:27Z

@PhysSong Yes I shall address, I am nearing the end of a busy project, and will have some time to finish this issue.

PhysSong · 2019-04-24T04:23:25Z

@curlymorphic Are you still here?

curlymorphic · 2019-12-28T11:54:28Z

I Shall not be issuing any additional commits to this pull request, please feel free to make any amendments if any developer wishes

he29-net · 2020-06-02T18:53:55Z

Hi; I would be interested in getting this PR finished and merged, so I started to play around with it in some free moments. It seems to work fine, although it had to be rebased to master before it even compiled (some headers missing in Zyn, a missing Qt include somewhere else, ladspa libraries failing dynamic linking.. apparently this old version does not like my up-to-date system?).

One thing I noticed though: it seems that the band-limited waveform is selected purely based on the pressed key, with no regard to detuning. In other words, the harmonic series of any (non-sine) tone played with the default triple-osc preset abruptly ends at 10 kHz for osc. 2 and at 5 kHz for osc. 3. This obviously makes a big difference to the final sound so I assume it was not intended?

EDIT: To clarify, I meant the coarse-detune in triple-osc itself; the master pitch and instrument-wide pitch shift both work fine. It has also another effect: when the coarse-detune is turned up, the waveform used is no longer "band limited enough" and folds back, doing exactly what this PR meant to prevent. So I take it as a sign this behavior was not intentional and should be fixed if possible.

Are there any other known issues I missed, apart from this and the memory leak? I also noticed the wave tables take some time to generate when LMMS starts, and only a single CPU core was utilized. So spawning a few threads to take care of it instead could be a good idea as well (although at this point I didn't even see the code yet so I guess I'll decide when it comes to that).

SecondFlight · 2020-06-02T19:56:00Z

Good find. That sounds like a bug to me. I don't remember seeing any other issues, but that doesn't mean there aren't any.

he29-net · 2020-06-04T19:57:33Z

So, I think I have the memory issues resolved (yes, plural -- apart from the leak, valgrind also found some uninitialized values used in conditions -- one in this code and one in my spectrum analyzer, so I fixed that as well).

I tried to use unique_ptr as suggested in the discussion, but I could not get it working. One problem is that std::make_unique is only available with C++14, but even after trying to work around that, it seems using unique_ptr for 2D arrays is very tricky and problematic. At least for me. So I gave up and kept the ordinary pointers.

Now, the question is: how do I get my changes here? The only way I can think of is making a new PR based on my modified branch; all the commits should remain visible, so curlymorphic should properly get the credit. Or someone could cherry-pick it the changes from my branch and add them here. https://github.com/he29-net/lmms/tree/oscillator-fix

I also found 2 more issues:

if I enable wavetable mode in tripleosc, load a user waveform into osc 1 and then switch osc. 2 or 3 into user waveform mode without loading a waveform into it, osc 1 becomes silent;
the threshold used to discard weak spectral bands in the user wave table may be still too high: when loading fmsine2.flac (I assume it is one of the defaults?), all harmonic content suddenly disappears when the amplitude drops below approximately -42 dB. I know there is not much to be heard there, but it is bound to raise some questions, since the new spectrum analyzer goes down to -50 dB by default and the band-limited waveforms are so clean that this sharp drop won't be hidden by noise.. :)

EDIT: Solved. The first issue happened because the IFFT result of an empty user waveform was normalized by dividing all samples by the maximum value, i.e. zero. This produced a buffer full of NaN-s, which then mixed with the lower-index oscillator and "poisoned" its signal with NaN-s as well. I replaced the max-search + divide with a normalization function from fft_helpers. As for the second issue, I reduced the "silent" threshold for user waveform from 0.1 to 0.001, which seems to keep the signal at least down to -50dB. Hopefully that will be enough and nobody complains. :) That leaves the detuning issue; I'll try to look into that tomorrow.

he29-net · 2020-06-05T10:07:11Z

The detuning issue should be fixed now as well. All commits are available at:
https://github.com/he29-net/lmms/commits/oscillator-fix

I also parallelized the initialization for platforms that properly support std multi-threading (i.e. not mingw) and the wave table generation time dropped from around 4 s to around 2 s on my machine. The saw wave is the bottleneck, for some reason it takes 2 seconds (square and triangle takes each around 1 s, sine and FFT are practically instant).

And I also reduced the cutoff threshold for the exponential wave (reasoning same as with the user wave). That should be about it, so far I don't know about any other issues that would block this PR.

curlymorphic · 2020-06-05T11:06:09Z

Thanks for finishing and fixing the bugs in this pull request. I can not speak for LMMS as I am no longer active, but as for the pull request, if it makes life easier and the commit logs cleaner I would not object if this was to be squashed into a single new commit and pull request, no need to worry about keeping my name in the logs. @he29-net You deserve the credit for this.

@PhysSong would probably be best to ask on how to handle the pull request.

JohannesLorenz · 2020-11-30T18:35:33Z

Can anyone summarize what needs to be done next with this PR?

he29-net · 2020-11-30T20:39:39Z

Someone with write access should merge my branch from:
https://github.com/he29-net/lmms/commits/oscillator-fix
into this PR (or get the patch in here by other means). It fixes the outstanding memory leak and a few other issues. It is a few months old though, so I'm not sure if it resolves all the current conflicts with master.

And a final round of testing may be needed as well, I'm not sure how much testing was done in the original review.

JohannesLorenz · 2020-12-05T15:56:49Z

@he29-net Normally, you are allowed to push on other users' PRs (unless they explicitly forbid it). Did you try? Otherwise I'll merge it and push for you.

JohannesLorenz · 2020-12-05T19:37:41Z

@he29-net Does your branch make sense on its own? If yes, I'd suggest you make a PR against master and then we rebase that PR on master. Otherwise, I would cherry-pick it all from your branch, i.e. just copy the commits.

he29-net · 2020-12-05T20:52:44Z

@JohannesLorenz I was worried it could somehow mess up Dave's attribution, so I did not even try. But perhaps it won't be so bad; from what I saw in the mean time, the squash commits tend to credit multiple authors, so hopefully that includes all the commits in the PR?

Anyway, Dave wrote earlier that we should not worry too much about keeping his name in the logs if it makes things easier, so I'll open a new PR for the fixed version as you suggest, since this seems to be the case that "makes things easier". :)

JohannesLorenz · 2020-12-05T21:03:53Z

This was not my suggestion, but it seems OK, too 😄

he29-net · 2020-12-05T21:10:44Z

This was not my suggestion, but it seems OK, too smile

Wait, what? :D Why else would you suggest to make a PR using my branch then?

I'm just running a test build to see if anything broke after I merged current master into the branch. If it works, I'll open the PR and see how it looks..

JohannesLorenz · 2020-12-05T21:22:37Z

Oh, I just thought you could make

a PR with only your fix against master, but it sounded like
you wanted a PR with dave's commits + your fix?

he29-net · 2020-12-05T21:26:36Z

Well, "PR with only my fix against master" makes zero sense, since the code that I fixed does not exist in master. :) My fixed branch = Dave's branch + a few extra commits (my fixes).

PhysSong · 2020-12-29T04:58:53Z

Continued in #5826, closing.

curlymorphic added 5 commits May 27, 2018 11:31

Revised to LMMS coding standards

b116e2d

Wavetables are held in in array of tables accessed s_waveTables[WaveShape][band][frame]

LMMS coding conventions

a4b04af

Add WT button to triple oscillator

010ac84

sine triange saw square implemented

Band Limted Wavetables for the MoogSaw and Exp waveforms

e725c95

fftw3 linking

e9f5310

Revised to work with the moogsaw and exp waveforms

fe61c40

refactored the fft code

Wallacoloo added the in progress label Jun 10, 2018

curlymorphic added 2 commits June 24, 2018 16:37

Oscillator Wave Table, move initilization code to main.cpp

20684bf

WT oscillator - tidy

2d6ebfe

curlymorphic removed the in progress label Jun 24, 2018

curlymorphic requested review from Wallacoloo and SecondFlight June 24, 2018 16:45

Wallacoloo requested changes Jun 24, 2018

View reviewed changes

PhysSong reviewed Jun 24, 2018

View reviewed changes

SecondFlight requested changes Jun 25, 2018

View reviewed changes

SecondFlight mentioned this pull request Jun 29, 2018

FM or heavy PM in TripleOscillator makes outputs odd for some target waveforms #3816

Closed

PhysSong reviewed Aug 23, 2018

View reviewed changes

WT Oscillator code formatting

1b74605

WTOscillator use dynamic allocation for user waves,

44714ca

PhysSong reviewed Aug 26, 2018

View reviewed changes

ryuukumar mentioned this pull request Jul 21, 2020

Reorganising LMMS, renaming editors and classes and everything in between (cont. #120) #5592

Closed

56 tasks

he29-net mentioned this pull request Oct 30, 2020

TripleOscillator with hi-pass produces different output depending on the export settings #5749

Open

he29-net mentioned this pull request Dec 5, 2020

Improved Oscillators -- fixed #5826

Merged

PhysSong closed this Dec 29, 2020

Improved Oscillators #4397

Improved Oscillators #4397

Conversation

curlymorphic commented Jun 3, 2018 • edited Loading

curlymorphic commented Jun 3, 2018 • edited Loading

karmux commented Jun 8, 2018

karmux commented Jun 8, 2018 • edited Loading

SecondFlight commented Jun 8, 2018

curlymorphic commented Jun 9, 2018

karmux commented Jun 10, 2018

Wallacoloo commented Jun 10, 2018

qnebra commented Jun 13, 2018

curlymorphic commented Jun 24, 2018

Wallacoloo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

curlymorphic Jul 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

curlymorphic commented Jun 25, 2018

SecondFlight commented Jun 25, 2018

SecondFlight commented Jun 25, 2018 • edited Loading

SecondFlight left a comment

Choose a reason for hiding this comment

PhysSong commented Jun 26, 2018

Wallacoloo commented Jul 23, 2018

PhysSong left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LmmsBot commented Aug 25, 2018 • edited Loading

Downloads for this pull request

curlymorphic commented Aug 25, 2018

Choose a reason for hiding this comment

PhysSong commented Oct 26, 2018

curlymorphic commented Oct 30, 2018

PhysSong commented Apr 24, 2019

curlymorphic commented Dec 28, 2019

he29-net commented Jun 2, 2020 • edited Loading

SecondFlight commented Jun 2, 2020

he29-net commented Jun 4, 2020 • edited Loading

he29-net commented Jun 5, 2020

curlymorphic commented Jun 5, 2020

JohannesLorenz commented Nov 30, 2020

he29-net commented Nov 30, 2020

JohannesLorenz commented Dec 5, 2020 • edited Loading

JohannesLorenz commented Dec 5, 2020

he29-net commented Dec 5, 2020

JohannesLorenz commented Dec 5, 2020

he29-net commented Dec 5, 2020

JohannesLorenz commented Dec 5, 2020

he29-net commented Dec 5, 2020

PhysSong commented Dec 29, 2020

curlymorphic commented Jun 3, 2018 •

edited

Loading

curlymorphic commented Jun 3, 2018 •

edited

Loading

karmux commented Jun 8, 2018 •

edited

Loading

curlymorphic Jul 5, 2018 •

edited

Loading

SecondFlight commented Jun 25, 2018 •

edited

Loading

LmmsBot commented Aug 25, 2018 •

edited

Loading

he29-net commented Jun 2, 2020 •

edited

Loading

he29-net commented Jun 4, 2020 •

edited

Loading

JohannesLorenz commented Dec 5, 2020 •

edited

Loading