Real-time: Replace slices of vectors with slices of references #50
Conversation
| wave_out.push(Vec::with_capacity(frames)); | ||
| for chan in 0..channels { | ||
| let chan_out = if active_channels_mask.map(|mask| mask[chan]).unwrap_or(true) | ||
| // An empty input buffer is implicitly interpreted as if the |
There was a problem hiding this comment.
I recommend to remove such implicit special case handling from the API.
There was a problem hiding this comment.
Yeah, it's convenient but probably not a great idea to keep it. It's from before I added the active channels mask. Let's remove.
There was a problem hiding this comment.
Thanks for confirming my concerns.
It should be fixed in a separate PR, because multiple tests need to be updated or deleted.
uklotzde
force-pushed
the
rt-slice-instead-of-vec
branch
from
6b653ae
to
a864599
Compare
src/lib.rs
Outdated
| if wave.len() != needed_len && mask[chan] { | ||
| for (chan, wave_in) in wave_in.iter().enumerate().filter(|(chan, _)| mask[*chan]) { | ||
| let actual_len = wave_in.as_ref().len(); | ||
| if actual_len != input_len { |
There was a problem hiding this comment.
Should we allow the input to be longer than needed? In a way it makes sense since we allow this for the output. But then the process methods should probably also return both how many input frames it read.
There was a problem hiding this comment.
I don't think so. The input length is fixed and consumed completely. Otherwise it would be the the callers responsibility to shorten it. Only the output capacity might be longer than needed.
There was a problem hiding this comment.
This is exactly what was bothering me. Why not apply the same rules for input and output? For the resampler it's perfectly fine to get more input samples than needed. If both input and output are allowed to be longer, then one can easily resample a longer sound clip by re-slicing the input until the whole clip has been processed. Something like this:
fn process(indata: &[&[usize]], outdata: &[&[usize]]) -> (usize, usize) {
println!("processing");
return (100, 100);
}
fn main() {
let ch1in = vec![0usize; 1024];
let ch2in = vec![0usize; 1024];
let mut indata = vec![&ch1in[..], &ch2in[..]];
let ch1out = vec![0usize; 10000];
let ch2out = vec![0usize; 10000];
let mut outdata = vec![&ch1out[..], &ch2out[..]];
while indata[0].len() >= 100 && outdata[0].len() >= 100 {
let (nbr_in, nbr_out) = process(&indata, &outdata);
for chan in indata.iter_mut() {
*chan = &chan[nbr_in..];
}
for chan in outdata.iter_mut() {
*chan = &chan[nbr_out..];
}
}
}
This is a quite common use case for this library, and this way of doing it is easier than what must be done today.
| @@ -183,6 +188,17 @@ impl fmt::Display for ResampleError { | |||
| actual, channel, expected | |||
| ) | |||
| } | |||
| Self::InsufficientOutputBufferSize { | |||
There was a problem hiding this comment.
This error variant is fine, but maybe WringNumberOfInputFrames should be renamed. See the other comment about input length.
There was a problem hiding this comment.
See my comment. The input is expected to match and it is the caller's responsibility to pass the expected parameters. Otherwise the API would probably get too complicated. KISS, unless someone provides a use case that doesn't fit yet.
|
This looks good now, many thanks! I'll take a proper look and merge it as soon as I can. |
|
Ping. Any news on this PR? |
|
Sorry it's taking so long. My plan is to make a little campaign and merge this, remove the implicit mask stuff, simplify the examples, and make the next release. I'll hopefully get to it soon. |
|
I didn't check if this PR is an intermediate step for the solution proposed by #51. Only noticed the request recently. |
|
The solution in #51 is quite different. That one will take some time to work out. We'll see, maybe the |
|
Incidentally this change helped me find an issue in the audio crate: udoprog/audio#18 |
Fixes #49.