-
Notifications
You must be signed in to change notification settings - Fork 223
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
Synthesizer Waveforms #602
base: master
Are you sure you want to change the base?
Conversation
...and upstream.
Can we hold on this for a moment? I'd like to add white and pink noise and I don't think it'd be too difficult. |
Need to write an example app to test them.
Okay check that out. |
This resolves a PR code review note and should make the source run much faster.
What if I just called it “TestSignal”, that’s why I did it, I wanted to make signals as test sources for other sources.On Aug 13, 2024, at 7:22 AM, David Kleingeld ***@***.***> wrote:
@dvdsk commented on this pull request.
In src/source/noise.rs:
+ sample_rate: cpal::SampleRate,
+}
+
+impl WhiteNoise {
+ /// Create a new white noise generator.
+ pub fn new(sample_rate: cpal::SampleRate) -> Self {
+ Self { sample_rate }
+ }
+}
+
+impl Iterator for WhiteNoise {
+ type Item = f32;
+
+ #[inline]
+ fn next(&mut self) -> Option<Self::Item> {
+ let randf = rand::random::<f32>();
In a015eb4
small_rng looks good! We might wanna put the whole synthesizer (and rand dependency) behind a cargo feature. Rodio is used in a ton of large projects that do not need synthesizers but without a feature the will pay the cost of compiling the rand crate.
—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you authored the thread.Message ID: ***@***.***>
|
Could you clearify?
Did you mean you made this (white and pink noise) as a test signal? Since if
that is the only use case for those noises rodio might not be the best place for
them. Given rodio already has a source for testing already (Sine) and I do not
think we need three sources for testing. The more code there is in rodio the more
chance for bugs and the harder the library gets to maintain.
Or are you proposing to rename Sine to TestSource? A sine (wave) is a known
concept in audio processing, if we call it Test people might not find it. We
could mention the sine source in the docs somewhere though.
…On Tue, Aug 13, 2024 at 09:04:04AM -0700, Jamie Hardt wrote:
What if I just called it “TestSignal”, that’s why I did it, I wanted to make signals as test sources for other sources.On Aug 13, 2024, at 7:22 AM, David Kleingeld ***@***.***> wrote:
@dvdsk commented on this pull request.
In src/source/noise.rs:
> + sample_rate: cpal::SampleRate,
+}
+
+impl WhiteNoise {
+ /// Create a new white noise generator.
+ pub fn new(sample_rate: cpal::SampleRate) -> Self {
+ Self { sample_rate }
+ }
+}
+
+impl Iterator for WhiteNoise {
+ type Item = f32;
+
+ #[inline]
+ fn next(&mut self) -> Option<Self::Item> {
+ let randf = rand::random::<f32>();
In a015eb4
small_rng looks good! We might wanna put the whole synthesizer (and rand dependency) behind a cargo feature. Rodio is used in a ton of large projects that do not need synthesizers but without a feature the will pay the cost of compiling the rand crate.
—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you authored the thread.Message ID: ***@***.***>
--
Reply to this email directly or view it on GitHub:
#602 (comment)
You are receiving this because you were mentioned.
Message ID: ***@***.***>
|
What I'd propose is just renaming I definitely think there should at least be a Square and triangle waves I'm more equivocal on. A lot of signal generator plugins make them, and not in a particularly musical context. It's definitely useful to be able to see them on a scope, square waves can tell you about slew rate in a DA, triangle waves have a known spectrum and you can see aberrant harmonics easily. It's useful that these signals generate a clear known spectrum with a countable number of overtones. If we're talking about test signals maybe I'm missing one, we could definitely use a chirp signal generator, that's really useful for evaluating frequency response. |
you clearly know what you are talking about which is very nice since I do not 😅. I happily defer to your judgement 👍. And I agree on the Synth -> TestSignal rename, that is clearer. I see you have also already reimplemented the Sine source as Synth/TestSignal 👍 . We could deprecate it in the future (the Sine source), that would clean up the API a bit. |
Actually |
Modules, files and types renamed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've left my opinion on the code here and there its almost exclusively about code style. Let me know if you disagree with anything, then we can discuss that in more detail.
and again thanks for the solid work!
|
||
#[inline] | ||
fn try_seek(&mut self, duration: Duration) -> Result<(), SeekError> { | ||
self.i = (self.sample_rate.0 as f32 * duration.as_secs_f32()) as u64; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it make sense to implement try_seek
? Seek was meant as a rough way to go to a point in time. The duration is measured from the start of playback.
You seek in a music file a podcast etc usually based on imprecise instructions from the user (go forward 5s go to 13m:23s). If you need precise narrow adjustments to playback (35ms forward) skipping samples seems the better approach (see the skip duration method which is part of Source).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a precise seeking method? I noticed this when I was chaining together sources for crossfades, sometimes you need to split a source/skip a source to a precise sample.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unfortunately not, the seek implementation is left over to the decoder. The decoder can be really precise, but it could also be off for a bit. Think off by milliseconds not seconds.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Jus getting back into this, would you like me to delete this from the implementation?
I'll get on these in a few days, I'm finishing a Chirp source to complete the suite. |
Noise source and example guarded by it.
This is all passing again, it looks like all of the failures in nightly are outside the scope of the PR. |
//! | ||
//! ## Features | ||
//! | ||
//! Rodio provides several optional features that are guarded with feature gates. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the only thats not a decoder that lives behind a feature gate. So its not several
right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dunno I expected all the features to eventually be in here, I'll change the language to make it singular.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just noticed the todo!()
. So I take it you are not done yet. Take your time! its looking good!
Oh that shouldn't be there, I thought I'd deleted that, I'm not going to support try_seek with any of the signal generators based on our previous convo. |
Why isn't this building in |
Its building fine its just that the test builds are failing as there are undocumented methods. The wierd thing is, that lint never fired in the past... My guess is that the lint was broken and has been fixed in the latest nightly. |
I'll disable the lint or fix the docs |
if you merge main all the lint errors not related to this PR should disappear 👍 |
Adds a new Source,
Synth
, that can generate square, triangle, sawtooth and sine waveforms useful for additive music sound synthesis. Waveforms can be generated with arbitrary sample rates and frequencies.Also the existing
SineWave
Source has been reimplemented as a client ofSynth
.