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

Replace sample generics with fixed f32 #678

Open
dvdsk opened this issue Jan 13, 2025 · 11 comments
Open

Replace sample generics with fixed f32 #678

dvdsk opened this issue Jan 13, 2025 · 11 comments
Labels
breaking Proposed change that would break the public API enhancement help wanted

Comments

@dvdsk
Copy link
Collaborator

dvdsk commented Jan 13, 2025

Idea by @roderickvd see #669 (comment).

The single argument for this change is how much simpler the rodio code base will be with it applied. That leads to better maintainability and that means more time for adding fun stuff :)

We will lose the ability to losslessly support audio using more then 24 bits (f32 supports integers up to 2^24 without loss of precision). This is a non argument since there is no purpose to such precision in audio playback, 16bit already being transparent to human hearing. The 24 bits provided by f32 give ample room for additional effects that could affect the noise floor.

As an example of the extra complexity induced by having a generic sample argument see: #670 (comment).

We would still need a sample convertor in the interface to cpal, though f32 outputs should be preferred.

This will be a big though simple effort, maybe we can take turns on working on this on a separate branch?

@dvdsk dvdsk added enhancement help wanted breaking Proposed change that would break the public API labels Jan 13, 2025
@roderickvd
Copy link

✋ I can help.

Devil's advocate: today, users can have a Source with i16 samples and simply append them to a Sink. That may even be a common case. How sure are we about removing those ergonomics?

@dvdsk
Copy link
Collaborator Author

dvdsk commented Jan 14, 2025

users can have a Source with i16 samples and simply append them to a Sink. That may even be a common case. How sure are we about removing those ergonomics?

I expect the number of users that use non float Source's to be small since almost any effect would use f32. Of course effects are not the only use for Source you can also use it to generate audio or load from disk. For those use-cases rodio offers ready made sources.

So yes there will be a few users hit by this, thats why its breaking. We have an upgrade guide to help them out. Specifically I would point them to dasp::Sample. Would you, as advocate of the devil 😛, agree thats enough?

@roderickvd
Copy link

My personal view:

I like the flexible, hassle-free approach that Rodio offers. 16-bit PCM is at the basis of digital audio and I'd expect out-of-the-box support.

I can see it depending on crate positioning. If cpal is bare metal, awedio light-weight, then I regard Rodio as the extended yet no-frills toolkit.

I don't shy away from generics, particularly in Rust whose type system makes they robust and usually without any performance hit. But, if you were to take it out to settle on only a f32 pipeline internally, then I would advocate having a pluggable "adapter" for taking some input and converting it.

Done well, I can imagine it could be the same struct as you would need at the output to convert into whatever cpal expects.

@dvdsk
Copy link
Collaborator Author

dvdsk commented Jan 15, 2025

If cpal is bare metal, awedio light-weight, then I regard Rodio as the extended yet no-frills toolkit.

More like: cpal is the abstraction layer over OS-interfaces, so the opposite of bare metal. awedio is rodio without generics settled on i16 and with a different approach to controlling the source layers its impressive but not light-weight. Regarding rodio, I do not yet know what we are (still being discussed https://github.com/RustAudio/rodio/pull/654/files).

I don't shy away from generics

Thats quite a generic statement 😛. For me it depends where they are used, its fine to use impl AsRef<Path> in a function. Its also not a problem if you expect the generics to be qualified by the direct user of your struct or if they will use dynamic dispatch. I do shy away from statically dispatched code in chains of structs. Statically dispatched generics then generally make the code hard to change. That makes it a chore to refactor or add new stuff. These generics distract while writing filters/effects. While easily writing those is one of rodio's (proposed feedback always appreciated) goals.

I would advocate having a pluggable "adapter" for taking some input and converting it.

I was thinking about pointing users to dasp's conversion traits, maybe we should provide some examples with that?

@PetrGlad
Copy link
Collaborator

If we settle on f32 rodio Sample is not really necessary anymore. Having custom sample trait in rodio limits where dasp_sample conversions can be used since we are requiring rorio::Sample in the API.

@roderickvd
Copy link

If cpal is bare metal, awedio light-weight, then I regard Rodio as the extended yet no-frills toolkit.

More like: cpal is the abstraction layer over OS-interfaces, so the opposite of bare metal. awedio is rodio without generics settled on i16 and with a different approach to controlling the source layers its impressive but not light-weight. Regarding rodio, I do not yet know what we are (still being discussed https://github.com/RustAudio/rodio/pull/654/files).

I was trying to put Rodio into perspective.

I'll add to #654.

I would advocate having a pluggable "adapter" for taking some input and converting it.

I was thinking about pointing users to dasp's conversion traits, maybe we should provide some examples with that?

You have my view - it has not yet changed.

Well, when this is implemented then this whole thing will look a lot like dasp... https://github.com/RustAudio/dasp/blob/master/examples/synth.rs

fn main() -> Result<(), anyhow::Error> {
    // ...
    match config.sample_format() {
        cpal::SampleFormat::F32 => run::<f32>(&device, &config.into())?,
        cpal::SampleFormat::I16 => run::<i16>(&device, &config.into())?,
        cpal::SampleFormat::U16 => run::<u16>(&device, &config.into())?,
    }
    // ...
}

fn run<T>(device: &cpal::Device, config: &cpal::StreamConfig) -> Result<(), anyhow::Error>
where
    T: cpal::Sample,
{
    // ...
}

With or without <T: Sample> from some namespace?
Asking to understand - not be a wise guy ☮️

@dvdsk
Copy link
Collaborator Author

dvdsk commented Jan 15, 2025

Well, when this is implemented then this whole thing will look a lot like dasp...
https://github.com/RustAudio/dasp/blob/master/examples/synth.rs

I do not understand what you mean exactly, thats probably on me, I am kinda tired today 😅

@dvdsk
Copy link
Collaborator Author

dvdsk commented Jan 15, 2025

I would advocate having a pluggable "adapter" for taking some input and converting it.

I've quickly tried a few options, I can think of two options:

  • introducting some kind of Source trait like the current with an adapter layer between:
trait AnySource: Iterator<Item = Into<dasp::Sample>>;

struct SampleAdaptor<S> {
     inner_source: S,
}

impl<S> SampleAdaptor<S> {
    fn new(source: S) -> Self {
    }
}

impl<S: AnySource> Iterator for SampleAdaptor<S> {
    type Item = f32;
    fn next(&mut self) -> Option<f32> {
         f32::from(self.inner_source.next()?.into())
    }
}

impl<S> Source for SampleAdaptor<S> {
    ..
}
  • requiring users to change their existing sources to the new f32 non generic Source type but providing functions in rodio to the sample conversion
fn to_rodio_sample(sample: Into<Sample>) -> f32 {
    sample.into().to_f32()
}

My preference goes to the latter, I think it will be easier to explain in the upgrade guide.

@PetrGlad
Copy link
Collaborator

Regarding dasp, if sample format is limited, and we also do not have to convert sample rates then the API would look a lot like dasp. However, I have looked at the dasp API closer and it does not seem to have any channel manipulation logic (like mixing/routing).

@dvdsk
Copy link
Collaborator Author

dvdsk commented Jan 16, 2025

Regarding dasp, if sample format is limited, and we also do not have to convert sample rates then the API would look a lot like dasp. However, I have looked at the dasp API closer and it does not seem to have any channel manipulation logic (like mixing/routing).

and you can not use dasp without setting up cpal right? Dasp is cool, and if users want it we could add something in the future to make integration easier?

Though right now the biggest wants from users seem to be around cross-fade and easier 'playlist' management (queue stuff). Once we have those the difference between rodio and dasp will be clearer..

@roderickvd
Copy link

roderickvd commented Jan 17, 2025

I've quickly tried a few options, I can think of two options:

  • introducting some kind of Source trait like the current with an adapter layer between:

  • requiring users to change their existing sources to the new f32 non generic Source type but providing functions in rodio to the sample conversion

My preference goes to the latter, I think it will be easier to explain in the upgrade guide.

For most use cases that I can entertain I would think the first is easier, because generally users work with a collection of samples and not just one.

Something like this you will anyway need (?) at the end of the pipeline to cpal. If so, why not expose it to users also?

Thinking out loud: instead of an iterator it could also return a converted collection. Scrap that: won't work for streaming sources.

Brainfart 2: it does not need to be one or the other. The utility method in your latter example could be used in the iterator in the former.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Proposed change that would break the public API enhancement help wanted
Projects
None yet
Development

No branches or pull requests

3 participants