udoprog / audio

A crate for working with audio in Rust
Apache License 2.0
78 stars 11 forks source link

add fill method to ChannelMut and BufMut #16

Closed Be-ing closed 1 year ago

udoprog commented 1 year ago

Try and implement the proposed API without requiring Sample in the Buf trait. Otherwise you'll inadvertently drag in the question of whether Sample should be required (which is more suitable for a separate issue).

The latter isn't strictly needed and might not be appropriate - say if you're writing a buf abstraction around native buffers where you can't guarantee sample appropriateness.

Be-ing commented 1 year ago

if you're writing a buf abstraction around native buffers where you can't guarantee sample appropriateness.

Could you elaborate? What would be a real use case for that? Sample is implemented for all of Rust's numeric primitives, so I'm struggling to come up with an example where this would actually be an issue.

udoprog commented 1 year ago

They're both hypothetical, but things I've been meaning to try:

Can dynamically typed containers impl Buf? If Sample is required it might put undue requirements on the impl. I even think a lifetime parameter might be appropriate if the sample is modelled as a pointer (which would need a lifetime).

Can OS buffers impl Buf / BufMut (without requiring type punting wrappers?). Very similar to the same question as above, would mean reworking the BufferMut wrapper type for wasapi (in audio-device).

Be-ing commented 1 year ago

It would be possible to implement fill without requiring Sample everywhere, but that would require duplicated code between the trait impls because a default implementation wouldn't be possible. Though I think a default implementation could be done if Copy is required for Buf::Sample.

Be-ing commented 1 year ago

Which of those approaches would you prefer?

udoprog commented 1 year ago

Not to require Copy for Buf::Sample.

Be-ing commented 1 year ago

I figured out a better way to do this by creating a supertrait of BufMut that provides a fill method only if the samples impl Copy with the power of generic associated types! :sparkles: I now understand why stabilizing them was such a Big Deal. It's very satisfying when the pieces fit together.

udoprog commented 1 year ago

You can add constraints to the trait directly instead of using a supertrait, like how it's done for Iterator here: https://doc.rust-lang.org/std/iter/trait.Iterator.html#method.flatten

Or for BufMut::copy_channels here: https://github.com/udoprog/audio/blob/main/audio-core/src/buf_mut.rs#L89

So you can add this directly to BufMut:

fn fill(&mut self, value: Self::Sample) where Self::Sample: Copy {
    for sample in self.iter_mut() {
        *sample = value;
    }
}

Don't forget to forward the implementation for the &mut BufMut impl as well!

Be-ing commented 1 year ago

Ah, I wasn't sure if that syntax required that all implementations of the trait satisfy the trait bounds for every function. So if an implementation doesn't satisfy the trait bound, it will still be able to use the rest of the methods?

udoprog commented 1 year ago

That's right! Just how Iterator::flatten is only available if you're iterating over something that can be coerced into an iterator.

Playground: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=bd39f6c284bb466351194d0ce046feb4

Be-ing commented 1 year ago

That was much simpler than I thought it would be! Well, trying those two previous implementations was still a great learning experience. Rust gets more awesome the better I understand it. :grin:

Be-ing commented 1 year ago

Doc tests added.

Be-ing commented 1 year ago

Ready to merge?

udoprog commented 1 year ago

Thanks for the reminder!