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.

Sign up for GitHub

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

Fix channel count upscaling #559

Merged
merged 5 commits into from
Apr 1, 2024
Merged

Fix channel count upscaling #559

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6de6f22
Fix channel count upscaling
ideka Mar 25, 2024
41604cf
Avoid duplicating signals when upscaling channels
ideka Mar 26, 2024
e6424ff
Update size_hint to work with previous changes
ideka Mar 27, 2024
a5f0675
Clarify some code, fix small docs typo
ideka Mar 27, 2024
2f3daa9
Fix size_hint bug, improve size_hint test
ideka Mar 31, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
91 changes: 62 additions & 29 deletions src/conversions/channels.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
use cpal::Sample;

/// Iterator that converts from a certain channel count to another.
#[derive(Clone, Debug)]
pub struct ChannelCountConverter<I>
Expand All @@ -19,7 +21,7 @@ where
///
/// # Panic
///
/// Panicks if `from` or `to` are equal to 0.
/// Panics if `from` or `to` are equal to 0.
///
#[inline]
pub fn new(
Expand Down Expand Up @@ -49,25 +51,29 @@ where
impl<I> Iterator for ChannelCountConverter<I>
where
I: Iterator,
I::Item: Clone,
I::Item: Sample,
{
type Item = I::Item;

fn next(&mut self) -> Option<I::Item> {
let result = if self.next_output_sample_pos == self.from - 1 {
let value = self.input.next();
self.sample_repeat = value.clone();
value
} else if self.next_output_sample_pos < self.from {
self.input.next()
} else {
self.sample_repeat.clone()
let result = match self.next_output_sample_pos {
0 => {
// save first sample for mono -> stereo conversion
let value = self.input.next();
self.sample_repeat = value;
value
}
x if x < self.from => self.input.next(),
1 => self.sample_repeat,
_ => Some(I::Item::EQUILIBRIUM),
};

self.next_output_sample_pos += 1;
if result.is_some() {
self.next_output_sample_pos += 1;
}

if self.next_output_sample_pos == self.to {
self.next_output_sample_pos -= self.to;
self.next_output_sample_pos = 0;

if self.from > self.to {
for _ in self.to..self.from {
Expand All @@ -83,11 +89,14 @@ where
fn size_hint(&self) -> (usize, Option<usize>) {
let (min, max) = self.input.size_hint();

let min =
(min / self.from as usize) * self.to as usize + self.next_output_sample_pos as usize;
let max = max.map(|max| {
(max / self.from as usize) * self.to as usize + self.next_output_sample_pos as usize
});
let consumed = std::cmp::min(self.from, self.next_output_sample_pos) as usize;
let calculate = |size| {
(size + consumed) / self.from as usize * self.to as usize
- self.next_output_sample_pos as usize
};

let min = calculate(min);
let max = max.map(calculate);

(min, max)
}
Expand All @@ -96,7 +105,7 @@ where
impl<I> ExactSizeIterator for ChannelCountConverter<I>
where
I: ExactSizeIterator,
I::Item: Clone,
I::Item: Sample,
{
}

Expand All @@ -106,36 +115,60 @@ mod test {

#[test]
fn remove_channels() {
let input = vec![1u16, 2, 3, 1, 2, 3];
let input = vec![1u16, 2, 3, 4, 5, 6];
let output = ChannelCountConverter::new(input.into_iter(), 3, 2).collect::<Vec<_>>();
assert_eq!(output, [1, 2, 1, 2]);
assert_eq!(output, [1, 2, 4, 5]);

let input = vec![1u16, 2, 3, 4, 1, 2, 3, 4];
let input = vec![1u16, 2, 3, 4, 5, 6, 7, 8];
let output = ChannelCountConverter::new(input.into_iter(), 4, 1).collect::<Vec<_>>();
assert_eq!(output, [1, 1]);
assert_eq!(output, [1, 5]);
}

#[test]
fn add_channels() {
let input = vec![1u16, 2, 1, 2];
let output = ChannelCountConverter::new(input.into_iter(), 2, 3).collect::<Vec<_>>();
assert_eq!(output, [1, 2, 2, 1, 2, 2]);
let input = vec![1i16, 2, 3, 4];
let output = ChannelCountConverter::new(input.into_iter(), 1, 2).collect::<Vec<_>>();
assert_eq!(output, [1, 1, 2, 2, 3, 3, 4, 4]);

let input = vec![1i16, 2];
let output = ChannelCountConverter::new(input.into_iter(), 1, 4).collect::<Vec<_>>();
assert_eq!(output, [1, 1, 0, 0, 2, 2, 0, 0]);

let input = vec![1u16, 2, 1, 2];
let input = vec![1i16, 2, 3, 4];
let output = ChannelCountConverter::new(input.into_iter(), 2, 4).collect::<Vec<_>>();
assert_eq!(output, [1, 2, 2, 2, 1, 2, 2, 2]);
assert_eq!(output, [1, 2, 0, 0, 3, 4, 0, 0]);
}

#[test]
fn size_hint() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this test seems quite complex and coupled to the implementation. How about something along these lines:

fn test(iter, from, to) {
    size_hint = iter.size_hint();
    actual size = iter.count() // from Iterator trait
    assert( is actual size contained in size_hint? )
}

If we wanted (might be overkill) we could define a custom Test iterator with a custom uncertain size_hint (I think the min and max size hints are the same for Vec since the len is known).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have a point, but the thing is the tricky/most fallible part of the size_hint calculation is when some of the items have already been consumed, so that's the part that is most valuable to test I think.

What about something like this?

fn test(input: &[i16], from: cpal::ChannelCount, to: cpal::ChannelCount) {
    let count = ChannelCountConverter::new(input.iter().copied(), from, to).count();
    let mut converter = ChannelCountConverter::new(input.iter().copied(), from, to);
    for i in 0..count {
        assert_eq!(converter.size_hint(), (count - i, Some(count - i)));
        converter.next();
    }
}

But I am okay with your version if you think this is still overkill.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can make the test function generic and clone the iterator: fn test<T>(iter: impl Iterator<Item=T> + Clone, from, to). Then you can pass in iterators in various states.

The overkill option would be passing in a custom iterator specifically made for this test. That would look something like this:

struct TestIter {
  bounds_to_report: (usize, Option<usize>),
  length: usize,
  consumed: usize,
}

impl Iterator for TestIter {
  Item = ();
  fn next(&mut self) -> Self::item {
      if self.consumed >= self.length {
         None
      } else 
         Some(())
      }
  }

  fn size_hint(&self) -> (usize, Option<usize) {
    self.bounds_to_report
  }
}

This would allow us to test the behavior when the upper and lower bound are not the same. But I have looked around and no one implementing Iterator does something like this 😅, So lets not.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see the appeal of the + Clone version. It seems like it'd require a lot of calls to be very useful...

I think these two options are sound:

fn test_1(input: &[i16], from: cpal::ChannelCount, to: cpal::ChannelCount) {
    let converter = ChannelCountConverter::new(input.iter().copied(), from, to);
    let size_hint = converter.size_hint();
    let count = converter.count();
    assert_eq!(size_hint, (count, Some(count)));
}

fn test_2(input: &[i16], from: cpal::ChannelCount, to: cpal::ChannelCount) {
    let mut converter = ChannelCountConverter::new(input.iter().copied(), from, to);
    let count = converter.clone().count();
    for i in 0..count {
        assert_eq!(converter.size_hint(), (count - i, Some(count - i)));
        converter.next();
    }
}

Let me know which one you think is best and I'll commit and push the changes.

Copy link
Collaborator

@dvdsk dvdsk Mar 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see the appeal of the + Clone version. It seems like it'd require a lot of calls to be very useful...

oh seems like that was just me being silly there. I had the count before the size_hint (and thus consuming the iterator) which I fixed using clone. Swapping the order like you do is obv superior 👍

The idea of passing in an iterator instead of a slice is that you can pass in partially consumed iterators. You could pass in a [1,2,3,4].into_iter().skip(3). Though the loop works too!

The whole i in 0..count and then count - i, do you agree with replacing that with for left_in_iter in (0..count).rev() and (left_in_iter, Some(left_in_iter))?

I don't see any way this could improve further so push it (test_2) I'll ask est31 to take a quick look and will get it merged!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yeah that is better. It'll have to be (0..=count).rev() though.
I'm also fixing a bug that caused the size_hint to go up again if calling .next() after the iterator was exhausted, and adding one final assert_eq! to test for it.

fn test(input: &[i16], from: cpal::ChannelCount, to: cpal::ChannelCount) {
let mut converter = ChannelCountConverter::new(input.iter().copied(), from, to);
let count = converter.clone().count();
for left_in_iter in (0..=count).rev() {
println!("left_in_iter = {}", left_in_iter);
assert_eq!(converter.size_hint(), (left_in_iter, Some(left_in_iter)));
converter.next();
}
assert_eq!(converter.size_hint(), (0, Some(0)));
}

test(&[1i16, 2, 3], 1, 2);
test(&[1i16, 2, 3, 4], 2, 4);
test(&[1i16, 2, 3, 4], 4, 2);
test(&[1i16, 2, 3, 4, 5, 6], 3, 8);
test(&[1i16, 2, 3, 4, 5, 6, 7, 8], 4, 1);
}

#[test]
fn len_more() {
let input = vec![1u16, 2, 1, 2];
let input = vec![1i16, 2, 3, 4];
let output = ChannelCountConverter::new(input.into_iter(), 2, 3);
assert_eq!(output.len(), 6);
}

#[test]
fn len_less() {
let input = vec![1u16, 2, 1, 2];
let input = vec![1i16, 2, 3, 4];
let output = ChannelCountConverter::new(input.into_iter(), 2, 1);
assert_eq!(output.len(), 2);
}
Expand Down
Loading