Remove UnknownTypeBuffer in favour of specifying sample type.

[mitchmindtree]

This is an implementation of the planned changes described in #119.

For a quick overview of how the API has changed, check out the updated
examples.

TODO:

• Update API.
• Update examples.
• Remove data_type field from Format? See here.
• Update docs.
• Update backends:
  • null
  • ALSA
  • ASIO
  • WASAPI
  • CoreAudio
  • Emscripten

Closes #119
Closes #260

[mitchmindtree]

OK this should be good to go!

I'll leave it up for another couple days before merging in case anyone has any feedback.

[msiglreith]

probably a good point in the timeline to get CI rustfmt checks in 👀

[mitchmindtree]

Thanks for the review @msiglreith!

I've been reflecting on this PR a little more the past couple hours. One issue to note about this approach is that it will likely require generating a fair bit more code, as the entire call to build_input/output_stream must be generated for each of the unique sample formats the user wishes to support.

One potential alternative could be to revert back to a dynamically dispatched approach, though rather than the unwieldy Unknown{Input/Output}BufferTypes we instead pass &Data/&mut Data to the callback, where Data provides a method for safely casting to a slice of the expected sample type.

Building the output stream in the beep.rs example might look something like this instead:

let data_fn = move |data| match sample_format {
    cpal::SampleFormat::F32 => write_data::<f32>(data, channels, &mut next_value),
    cpal::SampleFormat::I16 => write_data::<i16>(data, channels, &mut next_value),
    cpal::SampleFormat::U16 => write_data::<u16>(data, channels, &mut next_value),
};
let stream = device.build_output_stream(&format, data_fn, err_fn)?;

// ...

fn write_data<T>(output: &mut cpal::Data, channels: usize, next_sample: &mut dyn FnMut() -> f32)
where
    T: cpal::Sample,
{
    let output = output.as_slice_mut::<T>().expect("unexpected sample type");
    for frame in output.chunks_mut(channels) {
        let value: T = cpal::Sample::from(&next_sample());
        for sample in frame.iter_mut() {
            *sample = value;
        }
    }
}

Thoughts?

[msiglreith]

I'm generally in favor of more (kinda) unopinionated APIs at the abstraction layer (down to just passing memory ranges around), tends to be easier to agree on 👍
On open point for me is how to potentially distinguish between de/interleaved formats? IIRC the ASIO backend manually does these deinterleaving operations right now.

[mitchmindtree]

On open point for me is how to potentially distinguish between de/interleaved formats?

Aye this has been crossing my mind too, I think coreaudio is also interleaving/de-interleaving behind the scenes atm. It would be nice to know whether or not all platforms providing non-interleaved data do so with all channels in contiguous memory (one after the other). I think I'll leave adding an API to distinguish between non/interleaved for a future PR and look into this some more then. I agree it's worth keeping in mind.

[mitchmindtree]

@msiglreith (and any other reviewers), I'm leaning towards going down this route mitchmindtree#3.

[Ralith]

I'm disappointed by the way this turned out. The new approach strikes me as being semantically equivalent to the original UnknownBufferType, but more error-prone/less idiomatic, since now instead of pattern-matching to get a slice that's guaranteed to be of the right type, you have to make sure you get your SampleFormat and as_slice_mut uses matched up by hand. The SampleFormat check is still stuck in the main loop, so I'm not sure what problem this has solved.

[mitchmindtree]

@Ralith originally I had opted for passing the sample type as a type parameter via the build_input/output_stream methods as in the original issue I opened. You can see how the API looked at this point by checking out the commit before this comment. However, soon after I wrote the comment, I realised that this would result in a much larger amount of code generation in each of the host implementations as a result for little to no practical performance or safety gain beyond what landed in this PR. Please see this comment and this PR where I shared my thoughts on this.

With the changes in this PR, the user can still move the check outside of the main loop - here's an example of an alternative way the beep.rs example could be written that moves the SampleFormat match outside of the main audio loop:

// ...

let stream = match format.data_type {
    cpal::SampleFormat::F32 => device.build_output_stream(
        &format,
        move |data| write_data::<f32>(data, channels, &mut next_value),
        err_fn,
    ),
    cpal::SampleFormat::I16 => device.build_output_stream(
        &format,
        move |data| write_data::<i16>(data, channels, &mut next_value),
        err_fn,
    ),
    cpal::SampleFormat::U16 => device.build_output_stream(
        &format,
        move |data| write_data::<u16>(data, channels, &mut next_value),
        err_fn,
    ),
}?;

// ...

fn write_data<T>(output: &mut cpal::Data, channels: usize, next_sample: &mut dyn FnMut() -> f32)
where
    T: cpal::Sample,
{
    let output = output.as_slice_mut::<T>().expect("unexpected sample type");
    for frame in output.chunks_mut(channels) {
        let value: T = cpal::Sample::from(&next_sample());
        for sample in frame.iter_mut() {
            *sample = value;
        }
    }
}

If you're concerned about the necessity for this line:

    let output = output.as_slice_mut::<T>().expect("unexpected sample type");

Perhaps we could add an unsafe, unchecked version of this method that omits the check and returns the slice directly rather than an Option?

I hope this helps to clarify my thinking a little. If not let me know, and perhaps share a rough code snippet of what you would have liked to have seen instead.

Also I'm trying to leave a couple of days or so for review in my bigger PRs atm as there are quite a few things I'd like to address while I have the chance, but perhaps this isn't long enough. Let me know if you think there should be more time left for review.

[Ralith]

However, soon after I wrote the comment, I realised that this would result in a much larger amount of code generation in each of the host implementations as a result for little to no practical performance or safety gain beyond what landed in this PR. Please see this comment and this PR where I shared my thoughts on this.

I reviewed those prior to commenting. I agree that causing large amounts of monomorphization is not something that should be done lightly. A polymorphic API could be provided which did not incur significant monomorphization by, for example, constructing user-facing generic functions as trivial wrappers around internal monomorphic functions; this is sometimes referred to as an "outlining". I'm willing to attempt a draft of this approach if there's interest.

With the changes in this PR, the user can still move the check outside of the main loop

The apparent removal of the check is only cosmetic; each iteration still must internally perform a type-check via expect, and worse still, all static typechecking has been lost so e.g. a rarely-used format combination in more complex code could easily grow a lurking panic.

Perhaps we could add an unsafe, unchecked version of this method that omits the check and returns the slice directly rather than an Option?

I'm not really concerned about performance, but rather robustness and how easy it is to use the API incorrectly. This change looks like a performance noop and a step backwards in those other regards.

Also I'm trying to leave a couple of days or so for review in my bigger PRs atm as there are quite a few things I'd like to address while I have the chance, but perhaps this isn't long enough

I would have liked to weigh in before the merge, and two days would have been adequate time to do so had I been aware of the PR. I'm not subscribed to every change that's made in the repository, only individual issues that I care about. As a subscriber to #119, I was only notified of the PR's existence when it was merged; a comment there when the PR was opened would have been helpful.

[mitchmindtree]

A statically-typed (i.e. generic-based) API could be provided which did not incur significant monomorphization by, for example, constructing user-facing generic functions as trivial wrappers around internal monomorphic functions; this is sometimes referred to as an "outlining". I'm willing to attempt a draft of this approach if there's interest.

That would be greatly appreciated if you get the chance! I spent a while trying to work out how to allow for a user to submit a generic function that could be called by the Host implementation based on the sample format, though could not quite manage to find a nice solution that did not require a fancier type system. Perhaps I was getting too caught up on trying to keep the API that allows the user to pass a closure or function - are you thinking perhaps the user passes a type implementing some trait that allows for processing the data instead?

As a subscriber to #119, I was only notified of the PR's existence when it was merged; a comment there when the PR was opened would have been helpful.

That's a great point - I'll make a note to ping the relevant issues as I open each PR in the future, sorry for the oversight!

[Ralith]

That would be greatly appreciated if you get the chance!

Will do. I appreciate that rich polymorphism is sometimes more easily said than done. I'll definitely be trying something closurey first, as an explicit trait implementation is certainly much less ergonomic.

[MOZGIII]

Hey! I love this, had pretty much the same issue with not being notified, but I think it turned up good.
I think we should consider using sample crate and it's Format and Sample types - or at least provide good compatibility for using it.

[MOZGIII]

The patch's a bit unreadable with all the formatting changes, we really need to enforce cargo fmt everywhere once and for all.

[Ralith]

I noticed there's a .rustfmt.toml that thoroughly breaks my stable rustfmt, I'm guessing due to nightly options.

[mitchmindtree]

I think we should consider using sample crate and it's Format and Sample types - or at least provide good compatibility for using it.

@MOZGIII I tend to use the sample crate in all my downstream audio stuff, I'm glad to see it suggested as a useful tool for this sort of thing!

That said, CPAL feels like a special case where we want to have the minimal set of dependencies possible and only the absolutely necessary abstractions to provide a steady cross-platform experience, as I imagine it will serve as the foundation for a lot of different kinds of audio applications/projects.

As a side note - there has been some talk of stripping down the sample crate a bit further lately, splitting it into multiple crates and feature-gating the different features. Perhaps it might become a better candidate for CPAL after this process, though even then I'm not sure CPAL would gain much. Users should still be able to use the sample crate to work pretty seemlessly with the audio data slices provided by CPAL, as sample::Sample is quite a vast superset of cpal::Sample.

[MOZGIII]

That said, CPAL feels like a special case where we want to have the minimal set of dependencies possible and only the absolutely necessary abstractions to provide a steady cross-platform experience, as I imagine it will serve as the foundation for a lot of different kinds of audio applications/projects.

Oh, yeah, I remember this was discussed earlier. So, it's still the same.

As a side note - there has been some talk of stripping down the sample crate a bit further lately, splitting it into multiple crates and feature-gating the different features. Perhaps it might become a better candidate for CPAL after this process, though even then I'm not sure CPAL would gain much. Users should still be able to use the sample crate to work pretty seemlessly with the audio data slices provided by CPAL, as sample::Sample is quite a vast superset of cpal::Sample.

I like this. I wasn't aware of this, but this sort of things is why I'm bringing up using sample crate again.
I'm also using it downstream. I didn't work on my project for quite a while now, I'll need to get up to date with the latest changes with cpal and sample crate.

[MOZGIII]

The compelling reason to use sample crate for cpal is sample::Format rather than sample::Sample - but I'd say usability experiments have to be conducted before this can be confirmed as a good (or bad 😄) idea.