Fix channel count upscaling

[ideka]

This changes channel upscaling so all input samples are spread out across the output channels, instead of repeating the last sample over and over. This approach should yield more accurate results. Fixes #558.

[dvdsk]

It seems like there is no standard order for 5.1 and 7.1 surround sound instead it differs across file formats and can even be configurable. For example see channel assignment for flac. That is gonna make correct channel conversion quite challenging. As it might need work on the decoders to unify how they provide channels. Symphonia does at least provide a consistent order it seems: symphonia docs.

It makes perfect sense to turn mono to stereo by assigning both speakers the same sample. Scaling stereo to 5.1 and 7.1 should probably be done by recognizing which of the output channels is the left and which the right channel and sending the rest zero's/nothing.

Now back to this PR and stereo to 5.1/7.1 up-scaling :)
From this cpal PR it seems cpal uses the WAVEFORMATEXTENSIBLE order (see winapi docs) just like symphonia does. That means you should be able to figure out which is the front left and front right channel for any device. Then we can have rodio send stereo to only those channels.

Does that sound good to you?

[ideka]

So it can be safely assumed that the first 18 channels will always be in the WAVEFORMATEXTENSIBLE order? Like in this table here?

So the way it should be is...

• 2 to 4: [1, 2, 3, 4] -> [1, 2, 0, 0, 3, 4, 0, 0]
• 2 to 6: [1, 2, 3, 4] -> [1, 2, 0, 0, 1, 2, 3, 4, 0, 0, 3, 4]
• 2 to 8: [1, 2, 3, 4] -> [1, 2, 0, 0, 1, 2, 1, 2, 3, 4, 0, 0, 3, 4, 3, 4]
• 4 to 6: [1, 2, 3, 4, 5, 6, 7, 8] -> [1, 2, 3, 4, 1, 2, 5, 6, 7, 8, 5, 6]
• 4 to 8: [1, 2, 3, 4, 5, 6, 7, 8] -> [1, 2, 3, 4, 1, 2, 1, 2, 5, 6, 7, 8, 5, 6, 5, 6]
• 6 to 8: [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12] -> [1, 2, 3, 4, 5, 6, 1, 2, 7, 8, 9, 10, 11, 12, 1, 2]

For channels from 9 to 18 it'd get more complicated because they don't alternate left/right anymore, but the idea would be the same. Match left with left, right with right, center with center.

This kinda begs the question though. As you said, when going from mono to stereo it makes sense to duplicate the sample, but should that be done as well when going higher? maybe for 2 to 8 for example, this makes more sense? [1, 2, 0, 0, 0, 0, 0, 0, 3, 4, 0, 0, 0, 0, 0, 0]

Let me know what you think.

[dvdsk]

So it can be safely assumed that the first 18 channels will always be in the WAVEFORMATEXTENSIBLE order? Like in this table here?

As far as I can deduce from the cpal source thats correct. I would say anything beyond 8 channels it too nice for rodio right now (though that's not my call to make).

Match left with left, right with right, center with center.

That would ensure audio to most speaker. Though that is not what (a quick google shows) is recommended. The tldr being:

While you can attempt to stream music in surround sound, this will often just make the audio louder without improving clarity, and often results in sound distortion.

I think we can safely generalize that to: do not use more channels then the source is offers as that will decrease clarity. With an exception for going from mono to stereo (it is quite annoying to have sound only left or right).

This kinda begs the question though. As you said, when going from mono to stereo it makes sense to duplicate the sample, but should that be done as well when going higher? maybe for 2 to 8 for example, this makes more sense? [1, 2, 0, 0, 0, 0, 0, 0, 3, 4, 0, 0, 0, 0, 0, 0]

So yes given the source above to me that makes more sense. Just for clarity that would correspond to this table:

7.1 speaker position	interleaved stereo sample numb	non interleaved stereo sample
SPEAKER_FRONT_LEFT	1	sample 1 left channel
SPEAKER_FRONT_RIGHT	2	sample 1 right channel
SPEAKER_FRONT_CENTER	0	silent
SPEAKER_LOW_FREQUENCY	0	silent
SPEAKER_BACK_LEFT	0	silent
SPEAKER_BACK_RIGHT	0	silent
SPEAKER_FRONT_LEFT_OF_CENTER	0	silent
SPEAKER_FRONT_RIGHT_OF_CENTER	0	silent
SPEAKER_FRONT_LEFT	3	sample 2 left channel
SPEAKER_FRONT_RIGHT	4	sample 2 right channel
SPEAKER_FRONT_CENTER	0	silent
SPEAKER_LOW_FREQUENCY	0	silent
SPEAKER_BACK_LEFT	0	silent
SPEAKER_BACK_RIGHT	0	silent
SPEAKER_FRONT_LEFT_OF_CENTER	0	silent
SPEAKER_FRONT_RIGHT_OF_CENTER	0	silent

[dvdsk]

oh and while your at would you mind doing a tiny refactor :)

At line 74 (right now) I think this:

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

would be clearer like this

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

and still work right? Would you agree?

[dvdsk]

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).

[ideka]

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.

[dvdsk]

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.

[ideka]

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.

[dvdsk]

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!

[ideka]

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.

[xMAC94x]

thanks for seeing a fix here :) Greetings from the veloren team

[dvdsk]

Since this is a non trivial PR and touches an essential part of rodio I've asked @est32 to take a look too. As soon as that's done we are ready to merge! 🎉

Thanks a lot for adding proper multi channel support @ideka to rodio! As you've seen its already appreciated by the community!