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

Fix audio distortion bug with SineWave source #202

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mlindner
Copy link

@mlindner mlindner commented Nov 7, 2018

Fixes #207

This fixes a audio distortion bug caused by float rounding errors and by the sinewave frequency not matching the frequency of the driver.

@tomaka
Copy link
Collaborator

tomaka commented Nov 10, 2018

the sinewave frequency not matching the frequency of the driver.

Well, that shouldn't be a problem. If the sine wave doesn't match the expected frequency, it should get converted automatically.

@tomaka
Copy link
Collaborator

tomaka commented Nov 10, 2018

I agree with using cur_val and iterating this way, but the change to the frequency is probably a different problem.

@mlindner
Copy link
Author

mlindner commented Nov 15, 2018

I can only assume that the algorithm being used to subsample 48khz to 44.1khz is incorrect. Any audible signal here would be below the Nyquist frequency so there shouldn't be any distortion. Rodio always uses 44.1khz as the output format type on my system because it picks the default format given by cpal device which on my system is a 44.1khz output.

One of the things I want to fix in a future pull request is have rodio pick the format that best approximates the sampling rate of the input signal so resampling isn't required, or punt the format selection up to the application user. Do you have a preference?

@tomaka
Copy link
Collaborator

tomaka commented Nov 16, 2018

One of the things I want to fix in a future pull request is have rodio pick the format that best approximates the sampling rate of the input signal so resampling isn't required, or punt the format selection up to the application user. Do you have a preference?

I'm a bit conflicted, because originally I wanted cpal and rodio to be able to handle hardware that doesn't come with any way to resample. Which is why it selects the frequency based on the output device, and not the input.
The other problem is that all inputs are mixed together, so if you have multiple different inputs of multiple frequencies, you have to convert at least one of them.

Ideally I'd say that there should be two modes of action: either we interface with a software engine (eg. ALSA), in which case we can open one new backend voice per input and thus pass the data through with the original frequency, or we are interfacing with some hardware, in which case we should select the most appropriate frequency based on the hardware, and do all the conversions ourselves.

@mlindner
Copy link
Author

mlindner commented Nov 16, 2018

Many games even let you set the output device type and the output sample rate. I think limiting it to simply picking it automatically isn't helpful. It's trying to be too helpful. We can have the default value be automatic but it should also be allowed to specify the format.

Thoughts:

  • By default I'd have rodio pick something automatically by looking at the sample rate of the output. (The default setting is the audio device's default format.) However it should be configurable if the user of rodio wishes to force a specific output format.
  • At the next level up rodio should inspect the format that was selected either automatically or manually and insert resample filters for all incoming signals to that sample rate, but do no re-sample filtering for any samples that already have the correct sample rate.

This basically establishes two API layers. Later on down the line we should be able to transparently swap the "rodio picks the default format of the audio device" to "rodio passes raw samples to the lower level hardware layer and lets the hardware do the mixing".

Another thought, what I would suggest need to happen would have the lower level drivers like ALSA supply a list of traits that the driver implements up to cpal and cpal can then propagate those traits up to rodio and other applications. The traits would specify whether they can resample, whether they can mix, how many channels they can support (if for example we need to combine channels in software), etc. That should not conflict though with the other implementation of rodio that I just discussed. It will optionally fall back on doing some things itself if the lower level hardware doesn't support those features.

@jayanderson
Copy link

Is there a reason this change wasn't merged? I just found this project and started poking around and was about to submit a very similar pull request.

Two comments:

  • Instead of creating its own constant PI_2 it could use std::f32::consts::TAU .
  • No multiplication or division is needed within the loop. Instead the structure can maintain an increment value set at creation time: increment: TAU * sample_rate.recip() * freq. Then within the iterator: self.cur_val = self.increment.

That said, I agree that this change wouldn't fix the distortion issue. This is however a simpler, slightly more efficient implementation of a sine wave. It also makes it easier to change the frequency after it's started without pops and cracks (if such a feature is desired in the future).

@dvdsk dvdsk force-pushed the master branch 3 times, most recently from b10961c to a7f67b3 Compare October 10, 2024 00:14
@dvdsk dvdsk force-pushed the master branch 4 times, most recently from 0ee0413 to 073fdb1 Compare November 8, 2024 00:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SineWave has growing levels of distortion over time because of float round off errors
3 participants