A fix for #207 — Signal Generator distorts over long periods of time

[iluvcapra]

This fix changes SignalGenerator such that its internal time value to wrap-around for every period of the generated waveform. This requires more testing but is posted here for visibility.

(This PR was posted just before and closed due to an error by me, I was basing off the wrong HEAD.)

[dvdsk]

I think at large values of i the cast to float loses precision. f32 can store integers up to 16,777,216 exactly. At 44.1k sample rate distortion will start after 380 seconds, growing more extreme as time goes on.

[PetrGlad]

@dvdsk also floats' precision is relative while period is fixed, hence generator timing precision will drop gradually over time. f64 may have enough digits to make it unnoticeable, though.

I see wrapping may only be a problem with rather small periods (with frequencies that are close to the sample rate). Rounding error there may become comparable to sampling period. That means that rounded remainder may introduce slight timing error once per signal period.
If signal period is exact multiply of sampling rate, then the wrapping version is exact and definitely better than the current one.

[iluvcapra]

i needs to wrap around but I'm not sure how to negotiate this considering I've promised a f32 period in the interface, do I have to find a least common multiple of I and period or can I just dump out of i after it gets sufficienly high?

[dvdsk]

listing all related issues/pr's (so we can close em once this is done)

• Fix audio distortion bug with SineWave source #202
• SineWave has growing levels of distortion over time because of float round off errors #207

[dvdsk]

Current approach using sample counter will not work. You have to switch to adding an increment.

• period = 1/freq
• in 1s we have sample_rate samples

thus every sample advances we advance (1/sample_rate) * (1/period) of a full cycle (between 0 and 1)

Example:
sample-rate = 10, period = 10 seconds, each sample takes 1/100 of a full cycle

• now we want the full cycle to be between 0 and 2pi instead of 0 and 1 -> multiply by 2p
• we do not want the period but the frequency -> replace period by 1/frequency

gives: cycle_increment = freq/sample_rate * 2pi which is what #202 uses. To prevent cycle_increment (or a better name if you can think of it) from blowing up over time (and getting to the part where floats lose precision) just do: cycle_increment = cycle_increment mod 2pi

[PetrGlad]

It did not occur to me that floats support modulo too. Yes, float increment is better, since wrap-around will be calculated exactly. As I understand this requires to change trait Function. Also try_seek implementation will have to be adjusted accordingly.

So the implementation could:

fn next(&mut self) {
  let sample = self.f(self.t); //  t is cycle_pos
  self.t += self.dt;
  if self.t >= 1.0 { self.t -= 1.0; }  // or self.t = self.t.fract() or t %= 1.0
  sample
}

Then, sin generator is reduced to

fn render(t: f32) -> f32 {
  (t * 32::consts::TAU).sin()
}

This limits functions to be periodic, but it looks like fancier versions are not anticipated.

Regarding API, I am not sure it makes sense to do match Function on every iteration unless we want to switch those dynamically. Each function can be its own Source (maybe that is too verbose) or generator can be parametrized (SignalGenerator<Function>). The latter also lets users to provide their own functions for generation.

[dvdsk]

Regarding API, I am not sure it makes sense to do match Function on every iteration unless we want to switch those dynamically. Each function can be its own Source (maybe that is too verbose) or generator can be parametrized (SignalGenerator).

Making the function generic makes it a chore to pass the source around. Any struct the source is stored in, or function its passed too now must be generic too. Unless you use Box, but that is slower then the match that is there now.

The latter also lets users to provide their own functions for generation.

Are there enough other functions that you could want be passed in for this to make sense? If we miss a few then adding them to signal-generator makes more sense to me. Since that does not require the code to become more complex.

[iluvcapra]

We could implement the function as a fn, they're all simply enough. When the SignalGenerator is created could just use the function enum to pick which one and then we save this in a slot on the struct.

Could then also add a second initializer to the struct that would allow a client to write their own fn,

[dvdsk]

Could then also add a second initializer to the struct that would allow a client to write their own fn,

wouldn't that make the Signalgenerator generic over the function? Thats my main issue the custom function approach. Having generics in Source could become annoying for end users.

[iluvcapra]

No, because the fn would always have the same type. Essentially the struct becomes:

#[derive(Clone, Debug)]
pub struct SignalGenerator {
    sample_rate: cpal::SampleRate,
    period: f32,
    function: fn(f32) -> f32, //this was Function,
    t: f32,  // this was i: u32
}

Then in the initializer we set function to whatever it needs to be, probably with a closure that captures the period variable.

[dvdsk]

No, because the fn would always have the same type.

ahh yes I forgot fn is a function pointer. Wonder what that does to perf, we shall see. Given how fast signal generator is already I do not think it matters at all.

Kinda fun since you can throw f32::sin in there if you want a sine wave :)
We could ship standalone square and sawtooth wave functions too. Then the API would be something like: Signalgenerator::new(rodio::signals::square)

[PetrGlad]

Having a lot of cruft in type parameters is a price for not having dyn dispatch, if I understand this. I see existing Sink types may already get long. This is not strictly necessary unless we want absolutely top performance where most functions can be inlined.

User-provided function could be a wavetable synth? :) It does not have to be some analytically simple stateless function. But if making this generator flexible requires too much of a hassle its not worth it unless there is actual need for that.

If only a plain function is supported does it mean it cannot be a closure? A plain (stateless) fn would be enough for existing cases, though.

[dvdsk]

in order of my personal preference we can have

1. sine/sawtooth etc as seperate Sources, possible just aliasing SignalGenerator with F a function. This is as performant as it can be and pretty discoverable but not extendable.
2. (the current situation) a SignalGenerator which you pass an enum selecting the signal you want to generate. Its fine I think users might look for the words sine in the sources list and mis this but a search will turn up Function::Sine, I guess that could get a docs link to SignalGenerator.
3. a SignalGenerator which you pass a signal function (fn(f32) -> f32) as paramater. A sine wave is made with SignalGenerator::new(std::f32::sin). More signal functions are provided in a rodio module. This will be slower but maybe marginally so since the fn can not be inlined as its size is unknown to the compiler. Its fun :) I kinda like it, especially passing in std::f32::sin just feels fun. It will confuse users though so lets not do it. Also YAGNI.
4. a SignalGenerator which takes something implementing FnMut(f32) -> f32 and stores and uses it through dynamic dispatch (Box<dyn FnMut(f32) -> f32>). Can we provide ready made structs that implement FnMut? Or do we provide a lot of example code? Slower maybe more then the previous option, way harder to use as you need to know what closures are (rodio attracts quite a lot of novices). I see no reason for this since no one has presented a usecase and it just introduces complexity. Very much YAGNI.
5. a SignalGenerator taking a generic argument FnMut through static dispatch (impl<F: FnMut(f32) -> f32> SignalGenerator<F>) super versatile and can be fully optimized just like seperate sources (the first option). Still has the disadvantages of the previous option and also requires the generics to be named. Those generics spread through the users option. Honestly we should never implement this unless a very clear usecase is presented. Peak YAGNI

[PetrGlad]

Separate sources will have some duplicating code. But likely those can be extracted into helper functions. I would not object this version. We can have other implementations later when needed.

[iluvcapra]

Thanks all, let me get to work on this and I'll post changes. Per @dvdsk 's note I'll continue to implement (2) but it's easy to add interfaces along the lines of (1), either with some boilerplate or maybe a macro if we're being really nerdy.

[iluvcapra]

This addresses the guts of everything discussed here I think:

• SignalGenerator now uses an f32 to keep track of elapsed time, as a proportion of the completed period. This variable wraps around the interval of (0.0,1.0) to prevent loss of precision when the generator runs for long periods of time.
• We no longer perform a match for every rendered sample of the signal generator, the selected test signal is now generated by a function pointer that's selected at init-time.
• I have created some convenience sources for generating triangle, square and sawtooth waves to make these test signals more visible to new users.

[iluvcapra]

Looking good to me.

[dvdsk]

Looking good to me.

very clean nice 👍

I guess the little artifacts come from resampling or the Fourier itself?

[iluvcapra]

Looking good to me.

very clean nice 👍

I guess the little artifacts come from resampling or the Fourier itself?

The artifacts are mostly from the windowing function, here's the cos³ window of the same test.

There are odd harmonics but the first one is -80 dB below the fundamental, that's something like one part in 10 million, well below the f32::EPSILON and is probably spectral leakage in the fft anyways. (FFTs are very sensitive to window selection depending on what you're trying to do.)

[iluvcapra]

Looking good to me.

very clean nice 👍
I guess the little artifacts come from resampling or the Fourier itself?

The artifacts are mostly from the windowing function, here's the cos3 window of the same test.

You know what? When I make a test signal with iZotope or Pro Tools it's perfect, I'm only getting from the signal generated by SignalGenerator. I'm not sure exactly what's going on here.

[iluvcapra]

I figured it out, I was recording rodio's output through a virtual device and there was a sample-rate conversion happening. Once I set everything to 44.1 it cleans up.

[dvdsk]

let me know once this is ready (I think it still needs a note in the change-log it supports a custom function now?, or am I reading the source wrong?)

[iluvcapra]

Okay all ready I think.

[dvdsk]

very nice! thanks for all the hard work!

[iluvcapra]

Always a pleasure and very educational!

[PetrGlad]

Since SignalGenerator with a function callback option was chosen, and it already implements Source then separate structs for various function shapes are not necessary. Those differ only in initialization parameters. Functions can be passed directly. Do we need the function enum compatibility? So SawtoothWave could be implemented for example as

pub fn sawtooth(frequency: f32, sample_rate: u32) -> SignalGenerator {
    SignalGenerator::with_function(
        cpal::SampleRate(sample_rate), frequency, sawtooth_signal)
}

OK, can refine this later...

[dvdsk]

your right, its a little bit more complex then it needs to be. Feel free to address that directly on main.

edit: (main is still called master on this repo)

[iluvcapra]

Since SignalGenerator with a function callback option was chosen, and it already implements Source then separate structs for various function shapes are not necessary. Those differ only in initialization parameters. Functions can be passed directly. Do we need the function enum compatibility? So SawtoothWave could be implemented for example as

pub fn sawtooth(frequency: f32, sample_rate: u32) -> SignalGenerator {
    SignalGenerator::with_function(
        cpal::SampleRate(sample_rate), frequency, sawtooth_signal)
}

OK, can refine this later...

Yeah I didn't do this because it'd be a breaking change to remove the enum but please feel free if you want.

[dvdsk]

I might have misunderstood @PetrGlad so let me clarify my stance. I agree the individual sources (structs) Sine, Sawtooth and Square may use SignalGenerator::with_function() and that would be slightly neater. I do not think it is really needed. We can adress it when we remove SignalGenerator::new. What we can do is mark SignalGenerator::new as deprecated.