-
Notifications
You must be signed in to change notification settings - Fork 231
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
base: master
Are you sure you want to change the base?
Conversation
Well, that shouldn't be a problem. If the sine wave doesn't match the expected frequency, it should get converted automatically. |
I agree with using |
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? |
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. 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. |
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:
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. |
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:
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). |
b10961c
to
a7f67b3
Compare
0ee0413
to
073fdb1
Compare
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.