Change the Signal trait to return Self::Frame rather than Option<Self::Item>

[mitchmindtree]

Closes #54.

cc @andrewcsmith would you mind reviewing this? The Signal changes affect quite a lot of the lib, but I think it turned out well. For now I must 💤

Non-sleepy Edit:

This also removes the need for the Interpolator::is_exhausted method and simplifies some of the Interpolator implementations now that Signals are infinite.

[mitchmindtree]

I'm going to go ahead and merge this now as I'd like to make some progress that builds on this, but will give it a few more days before publishing.

[andrewcsmith]

Sorry, I've been out of programming mode for a few days. Trying to wrap my brain around what exactly this API change entails. First, I fully support the semantics of Signal being infinite. I agree that simplifies an enormous number of implementations.

Main question - why not use the default From and Into traits? It would be great to be able to use Rust's type inference. That could potentially be the greatest asset for this library -- declarative typing, basically.

Second, more abstract concern - The from_slice method is too presumptuous for what I see as a default API. To me, from_iter and from_slice are two very different use-cases: we definitely know the length of the slice, and the possible (even likely) uses go beyond just iterating over the frames. Plus, this implementation still cycles through Option<F>, which could potentially be a performance hit -- maybe we could benchmark that to see whether there's a burden to using Iterator as an intermediary. In short, I think we could do a lot more with slice, beyond just turning it into an Iterator, such as looping it, indexing (for wavetable distortion), one-shot, etc.

I'll integrate all this into my existing projects as well, which use windowing and FFT analysis pretty heavily.

[mitchmindtree]

why not use the default From and Into traits?

Which types in particular would you like to see this for? It might be helpful if you could give an example of the ergonomic gain too.

this implementation still cycles through Option, which could potentially be a performance hit -- maybe we could benchmark that to see whether there's a burden to using Iterator as an intermediary

I agree it would be nice add some benchmarks to the lib, although I'd be surprised if we could get much faster than the std::slice::Iter implementation ourselves - I believe the std slice iterator types are highly specialised (e.g. they know the length of their slice and have optimised Iterator implementations accordingly).

In short, I think we could do a lot more with slice, beyond just turning it into an Iterator, such as looping it, indexing (for wavetable distortion), one-shot, etc

I guess I'm not sure I understand why the current implementation would stop us from doing any of these things? E.g. if a user wants to cycle over a slice, they could do signal::from_iter(slice.iter().cloned().cycle()). I mainly added the signal::from_slice function to reduce some of the common boilerplate involved with slices e.g. slice.iter().cloned().

[mitchmindtree]

Ahh, I just realised I've been assuming that the Cloned adaptor would be optimised for Cloned<std::slice::Iter>, however this does not seem to be the case! Actually now that I think about it this makes sense as .cloned().nth(5) implies that all 5 elements are actually cloned, whether or not it would be more efficient to just index ahead!

I think we can avoid this issue by removing the signal::from_slice function in favour of adding something along the lines of signal::from_ref_iter along with a signal::FromRefIterator type which takes an IntoIterator<Item=&F>, and clones the frames in the implementation of the Signal::next method instead. This way we can reap the benefits of the highly optimised std::slice::Iter while keeping a nicely ergonomic way of creating signals from slices, e.g. signal::from_ref_iter(&slice).

[andrewcsmith]

This feels suspicious to me. I pretty quickly got stuck in an infinite loop when I just called collect() on a Windowed signal in some existing code. I think I'd rather have API compilation breakage than silent code failure. Here's an example of my usage:

let bin = 2048;
for chunk in window::Windower::hanning(&vector[..], bin, 1024) {
    // Why do we need to pass bin again, after we've already told the Window how long it should be?
    let chunk_data: Vec<[f64; 1]> = chunk.take(bin).collect();
}

[mitchmindtree]

Yeah this is a good point. I have a couple uncertainties with the window API atm, maybe I'll open another issue.

[andrewcsmith]

Right, although LLVM might optimize that away since Sample: Copy so Cloned might be a moot point anyway.

I see your point that from_slice doesn't preclude other uses, but to me it's just that the slice-to-signal conversion is more complicated, and specifying a default glosses over that. Something like:

/// Plays a slice once, and then outputs F::equilibrium()
struct OneShot { ... }

/// Cycles a slice for eternity
struct Cycle { ... }

pub fn one_shot_from_slice( ... );
pub fn cycle_from_slice( ... );

signal::from_slice::<OneShot>(slice);
let one_shot: OneShot = signal::from_slice(slice);
let one_shot: OneShot = slice.into(); // if we impl From<&[S: Sample]> for OneShot
signal::from_slice::<Cycle>(slice);

Unfortunately, std::iter::Cycle clones the Iterator, so we would be probably better off creating our own implementation of a cycling wavetable oscillator.

Which types in particular would you like to see this for? It might be helpful if you could give an example of the ergonomic gain too.

My mistake, sorry. First off, turns out I was thinking of another part of the library (src/lib.rs), and second off From<f64> for f64 is clearly already implemented. So now I see why we have all these separate traits.

I'm still working through things, but somewhat gradually because there's silent breakage in the windowing functionality now that we have infinite signals. I'm wondering whether it makes sense to move away from using Iterator in mod window, and just do something like .each_window(f: FnMut()), moving away from the for ... { } syntax. That would allow for streaming iterators as well, and make the API change explicit.

[mitchmindtree]

Right, although LLVM might optimize that away since Sample: Copy so Cloned might be a moot point anyway.

True true, the more I think about it and look at the Cloned src, it really should not affect the performance at all. Perhaps we can add some benchmarks at some point to double check that this is the case.

Unfortunately, std::iter::Cycle clones the Iterator

Hmmm I'm struggling to think of a more efficient implementation than this - do you have one in mind?

it's just that the slice-to-signal conversion is more complicated, and specifying a default glosses over that

I can see where you're coming from. I'm happy to just remove the from_slice function altogether for now in favour of just using the from_iter API directly. This way at least users will know exactly how their frames will be consumed without any ambiguity.

[andrewcsmith]

Sure, if it's not too much feature creep for you. Really, the from_slice specificity is my 2¢ and I totally won't be offended if you ditch it. But give me a week and I should have a PR with a number of slice-to-signal generator options. I like the signal::from_slice<Type> syntax the best, I think, because it lets people decide whether they want to annotate or turbofish.

As far as Cycle goes, I don't know whether it would be more efficient to just index the same immutable buffer. That's a question for benchmarking. It does seem like an iterator turned into a slice should just re-borrow that same immutable slice, so maybe it's not a big deal either way. But it'll be a fun benchmarking project anyway.