Fix(484) - ChannelVolume misbehaving

[DivineGod]

This PR will probably collect a few fixes that relate to #484 as I go through rodio to find all the issues that seem to prevent proper use of ChannelVolume.

I think I've identified a few areas that need attention, first being the ChannelCountConverter which is started with an Empty source when Sink::try_new is called. This would cause an issue where the channel converter had the wrong to count, as the actual source input channel count might not necessarily be 1 as the empty source was specifying. By setting Empty channels to 0 and handling the cases where they show up, we can more gracefully handle these issues.

Next issue is an issue with the sample rate converter where if there's a mismatch the ChannelVolume once again seem to be messed up and not produce the right results. This might also have been identified in other issues; I've not triaged them all yet.

[dvdsk]

First thanks a lot for going after this issue. Does the example your PR adds reproduce the issue? I would love to have a test that reproduces it.

If I understand correctly your approach modifies the channel converter and sample rate converter to recognize the Empty source. They do so by checking for its now nonsensical channel count and sample rate of zero.

An alternative would be removing the use of empty in queue (used by Sink::try_new) here:

rodio/src/queue.rs

Line 110 in a904566

current: Box<dyn Source<Item = S> + Send>,

We could use a Option<()> there and make the None branch play 'empty' sounds.

On the other hand, Empty probably has other valid uses....

Quite a difficult one this. I do not have any more time right now, ill come back to this.

[dvdsk]

I am not sure about this change. None means the frame length will not change going forward. I do not know what Some(0) means. Filters/wrappers around the Empty source will treat every new sample as a new frame and run code paths to handle that.

[DivineGod]

I don't recall why I had that in there. I don't think it needs to be there. So I'll happily revert that change

[DivineGod]

[...] Does the example your PR adds reproduce the issue? I would love to have a test that reproduces it.

Yes, it does, running the example on master produces a nasty clipped sound on all channels and then only output on channel 0. Note for reproducing is that the sample rate of the output device seems to have to be 48kHz on my setup. If it's not then I run into the sample rate issue that I mentioned.

[dvdsk]

Yes, it does, running the example on master produces a nasty clipped sound on all channels and then only output on channel 0.

Nice, then I can try it out and play around a little with the PR

[dvdsk]

I do not yet understand why ChannelVolume is misbehaving. I am gonna have to play with it a bit to find that out and to understand it better.

I can not merge this until I understand everything around it.

Unfortunately I do not have a lot of time allocated for this, so it could a bit. I'll keep you updated.

[DivineGod]

That’s totally understandable. I am happy to help out getting it to a state where it fixes things and there’s an understanding of why it all works out

[dvdsk]

I have some time now :)

If you still have time to help out, could you rebase or merge master to resolve the conflict?

As I understand the PR sample-rate & channel-count zero function as a sort of flag/placeholder. Then the behavior of various rodio parts change when they see it. Specifically the converters pick the output instead of the input as target count/rate. Would Option not work better for that role then zero?

[dvdsk]

Oh and the changelog needs an update marking this is fixed now

[DivineGod]

If you still have time to help out, could you rebase or merge master to resolve the conflict?

Rebased to sort merge conflict

[DivineGod]

Just rested the example after rebase; it has regressed to not sorting out the channel volumes correctly. I'll try to investigate and come up with some more in-depth unit tests as well to see if I can better isolate the issue.

A preliminary test in source/channel_volume is showing that that part is iterating correctly. But running a 6 channel output with the example distributes the first two provided volumes over the 6 total outputs.

[DivineGod]

I've added a test to sink.rs which reproduces my issue with the following assert:

---- sink::tests::test_channel_volume stdout ----
thread 'sink::tests::test_channel_volume' panicked at src/sink.rs:462:9:
assertion `left == right` failed
  left: Some(3.0517578e-5)
 right: Some(1.0)

I thought it repro'd but it didn't. I've pushed a working test for now.

[dvdsk]

I thought it repro'd but it didn't. I've pushed a working test for now.

thanks! and thank you very much for hunting for the bug!

[will3942]

Any update on this bug? I fear we're about to run into this issue as we try to use a 10 channel interface with our app that uses ChannelVolume.

[DivineGod]

Not yet, I’m afraid.

I’ve been busy with life events. I’ll have time to look into it more next week, hopefully

[PetrGlad]

Can you explain this comment?

[DivineGod]

Honestly, no idea. I'll be spending the day today going through this throughly to actually try to understand what I did.

[DivineGod]

I'm going to remove the test here. I don't think it's actually helpful to have there

[DivineGod]

I've been testing this by using Loopback on macOS to setup a virtual interface with 6 output channels and setting it as the default output device and noting that the volume meters are displaying correctly the output levels needed.

[dvdsk]

I've been testing this by using Loopback on macOS to setup a virtual interface with 6 output channels and setting it as the default output device and noting that the volume meters are displaying correctly the output levels needed.

very nice! Thanks for putting in all this effort (reporting, finding fixing & testing!). I left a few comments on the source. It mainly comes down to this:

I am afraid we might screw up ChannelVolume again in the future because the match { ... 0 => to } on their own seem puzzeling. We might remove them in the future without realizing. An integration test would catch it, alternatively a short comment at everyone of those matches might prevent such a thing too. We are playing with the idea of outer/later sources being able to instruct their inner sources. They can then pass them a preferred sample_rate. Would this get fixed if empty could get instructed to copy the sample_rate from an outer source? Or does that do nothing?

[DivineGod]

I've implemented Option for channels and sample_rate and fixed all the files. I've left a bunch of FIX-ME comments in places where I wasn't too sure what the correct path forward might be

[dvdsk]

I was afraid adding all the options might reduce performance, but I am measuring a slight increase.. thats pretty neat.

[PetrGlad]

Folks, this change worries me because I do not understand where the actual problem is. Number of channels in Empty should not affect anything if downstream sinks are implemented properly. If reducing Empty channels to 0 helps, that means the actual problem is elsewhere downstream and this change only masks it.

Or did I miss something?

I do not see any code that handles even number of channels differently, except Spatial. If I had to guess, I still think that that maybe chunks are not handled correctly somewhere so after a change (chunk end) the input number of channels change but the resulting stream is shifted by some samples, which in turn confuses output channels.

[PetrGlad]

// Side-notes:

Also, I could not see any tests that check conversion to mono in ChannelVolume (I still find this down-mixing confusing, but that is an other issue). Tests for ChannelVolume always use only single channel input.

I think Spatial should also adjust relative phases for left and right ear, not only loudness to properly simulate spatial position.

[PetrGlad]

And this is just theoretical, I am wondering if it makes sense to have typed streams, that are sequences of fixed size slices each containing all channel samples for a particular moment. This will make confusion of channels in a stream unlikely.

[DivineGod]

Folks, this change worries me because I do not understand where the actual problem is. Number of channels in Empty should not affect anything if downstream sinks are implemented properly. If reducing Empty channels to 0 helps, that means the actual problem is elsewhere downstream and this change only masks it.

Or did I miss something?

I do not see any code that handles even number of channels differently, except Spatial. If I had to guess, I still think that that maybe chunks are not handled correctly somewhere so after a change (chunk end) the input number of channels change but the resulting stream is shifted by some samples, which in turn confuses output channels.

So looking through the actual meat of the change here again. It looks like source/uniform.rs is the place where the channel count fix is crucial. And having an option makes it a little more clear.

I am on the same page as you with concerns about the fragility of this.

[DivineGod]

// Side-notes:

Also, I could not see any tests that check conversion to mono in ChannelVolume (I still find this down-mixing confusing, but that is an other issue). Tests for ChannelVolume always use only single channel input.

I think Spatial should also adjust relative phases for left and right ear, not only loudness to properly simulate spatial position.

FWIW, the use case I was using channel volume for was having multiple mono-sources spatially located using distance-based amplitude panning (DBAP) which doesn't require phase-shifting. So I would setup an output with n-channels and have m-sources each with a channel volume backed DBAP Source based on the same principle as SpatialSink

[PetrGlad]

@DivineGod Feel free to file a ticket if you have particular suggestions on how to improve spatial.

I think a test that demonstrates the problem is necessary for this issue. Do you use ChannelCountConverter? It is only piece that I am aware of that does have specific logic for stereo channels.

[will3942]

@PetrGlad you're right in that this should work correctly and that the issue is downstream.

I believe the issue is in

rodio/src/queue.rs

Line 120 in 8993f67

const THRESHOLD: usize = 512;

/ the THRESHOLD value.

For Empty source we use the THRESHOLD value, as Empty declares a current_span_len of None and also returns 0 to size_hint() which leaves us in (to quote the queue code) "the worst case scenario".

If this value is not divisible by the number of channels in the current Source Input then the bug occurs (this is the case for none power of 2 numbers as THRESHOLD is 512).

I'm still tracing exactly why but I believe it is as you say where we have shifted the input by a number of samples (those left over from the Empty source).

I'm trying to understand this more and understand a fix but as I'm not very familiar with the Rodio source code this may take a while so any suggestions would be really helpful.

[will3942]

I can't craft a test here to make it work but I believe the issue is shared/across the Queue/SourcesQueueOutput and UniformSourceIterator when used with a mixer.

I believe a test along these lines should uncover the issue but I don't understand Mixer well enough to do this. @dvdsk can you input at all in light of these findings?

#[test]
    fn mixer_different_channels() {
        use crate::mixer;

        println!("mixer_different_channels");

        let (mixer_tx, mut mixer_rx) = mixer::mixer(6, 44100);
        let (queue_tx, queue_rx) = queue::queue(true);

        mixer_tx.add(queue_rx);

        assert_eq!(mixer_rx.next(), Some(0));

        queue_tx.append(SamplesBuffer::new(6, 44100, vec![10i16, 10, 0, 0, 0, 0]));

        assert_eq!(mixer_rx.channels(), 6);
        assert_eq!(mixer_rx.sample_rate(), 1);
        assert_eq!(mixer_rx.next(), Some(10));
        assert_eq!(mixer_rx.next(), Some(10));
        assert_eq!(mixer_rx.next(), Some(0));
        assert_eq!(mixer_rx.next(), Some(0));
        assert_eq!(mixer_rx.next(), Some(0));
        assert_eq!(mixer_rx.next(), Some(0));
    }

[dvdsk]

I believe a test along these lines should uncover the issue but I don't understand Mixer well enough to do this. @dvdsk can you input at all in light of these findings?

I do not have a lot of time the next few days but sure, I'll take a look next week, if I dont report feel free to remind me. You have all done a lot of work here, it feels like we are close now. It would be very nice to close fix this bug, it has plagued is for a while now.

[PetrGlad]

@will3942 rodio's mixer should sum respective channels from each source. So it should handle all the cases (different span lengths and channel counts) properly.
I think we will have to simplify this (I would like to not use spans except on sources that do need to produce them).

Now I think it was a mistake to define span length in samples, not frames... Yes, it looks like ideally THRESHOLD should define number of output frames not number of samples.

I will have a look at this once more.

[will3942]

This test is showing some bizarre behaviour where if Empty has 1 channel the mixer (according to the test) outputs on the correct channel - whereas when actually running Rodio this is not the case. When Empty has 0 channels the mixer outputs on the wrong channel (3 and 4).

This could just be a bad test or perhaps the issue is further downstream but maybe this will help tracing the root cause.

#[test]
    fn mixer_different_channels() {
        use crate::mixer;

        let (mixer_tx, mut mixer_rx) = mixer::mixer(6, 48000);
        let mut current_channel: usize = 1;

        let inc_current_channel = |current_channel: &mut usize| {
            *current_channel = (*current_channel + 1) % 6;
        };

        let mixer_next = |mixer_rx: &mut MixerSource<i16>, current_channel: &mut usize| {
            let next = mixer_rx.next();
            inc_current_channel(current_channel);
            next
        };

        assert_eq!(current_channel, 1);
        assert_eq!(mixer_next(&mut mixer_rx, &mut current_channel), None);     
        assert_eq!(current_channel, 2);
        assert_eq!(mixer_next(&mut mixer_rx, &mut current_channel), None);
        assert_eq!(current_channel, 3);

        let (queue_tx, queue_rx) = queue::queue(true);

        assert_eq!(mixer_next(&mut mixer_rx, &mut current_channel), None);
        assert_eq!(mixer_next(&mut mixer_rx, &mut current_channel), None);
        assert_eq!(mixer_next(&mut mixer_rx, &mut current_channel), None);
        assert_eq!(mixer_next(&mut mixer_rx, &mut current_channel), None);

        mixer_tx.add(queue_rx);

        assert_eq!(current_channel, 1);
        assert_eq!(mixer_next(&mut mixer_rx, &mut current_channel), Some(0));

        queue_tx.append(SamplesBuffer::new(6, 48000, vec![10i16, 10, 0, 0, 0, 0]));

        // Empty with 1 channels - this is seemingly 6 * (512-1)
        for _ in 0..3071 {
            assert_eq!(mixer_next(&mut mixer_rx, &mut current_channel), Some(0));
        }

        // // // Empty with 0 channels
        // for _ in 0..511 {
        //     assert_eq!(mixer_next(&mut mixer_rx, &mut current_channel), Some(0));
        // }

        assert_eq!(mixer_rx.channels(), 6);
        assert_eq!(mixer_rx.sample_rate(), 48000);
        assert_eq!(current_channel, 1);
        assert_eq!(mixer_next(&mut mixer_rx, &mut current_channel), Some(10));
        assert_eq!(current_channel, 2);
        assert_eq!(mixer_next(&mut mixer_rx, &mut current_channel), Some(10));
        assert_eq!(mixer_next(&mut mixer_rx, &mut current_channel), Some(0));
        assert_eq!(mixer_next(&mut mixer_rx, &mut current_channel), Some(0));
        assert_eq!(mixer_next(&mut mixer_rx, &mut current_channel), Some(0));
        assert_eq!(mixer_next(&mut mixer_rx, &mut current_channel), Some(0));
        assert_eq!(mixer_next(&mut mixer_rx, &mut current_channel), Some(10));
        assert_eq!(mixer_next(&mut mixer_rx, &mut current_channel), Some(10));
        assert_eq!(mixer_next(&mut mixer_rx, &mut current_channel), Some(0));
        assert_eq!(mixer_next(&mut mixer_rx, &mut current_channel), Some(0));
        assert_eq!(mixer_next(&mut mixer_rx, &mut current_channel), Some(0));
        assert_eq!(mixer_next(&mut mixer_rx, &mut current_channel), Some(0));
    }

[dvdsk]

This test is showing some bizarre behaviour where if Empty has 1 channel the mixer (according to the test) outputs on the correct channel - whereas when actually running Rodio this is not the case. When Empty has 0 channels the mixer outputs on the wrong channel (3 and 4).

This could just be a bad test or perhaps the issue is further downstream but maybe this will help tracing the root cause.

I do not really understand what this test is actually testing. The use of the two closures makes it pretty hard to understand for me. The tests also passes without problem on my system, that seems intentional? Maybe a test that fails without the callbacks would be clearer. One note, the first channel will be channel zero, your counter starts at one?

[will3942]

@dvdsk apologies for the bad test - it was designed to check if the output was on the correct channel. The test does indeed pass even though in reality the audio comes out on the wrong channel.

I was intrigued that the fix in this pull makes the test fail as it comes out on the wrong channel although in reality comes out on the correct channel.

[dvdsk]

@dvdsk apologies for the bad test - it was designed to check if the output was on the correct channel. The test does indeed pass even though in reality the audio comes out on the wrong channel.

No worries, I took a close look at queue and I think I have a fix, you can check it out here: #684

[DivineGod]

Looks like there's been some good progress in figuring out the core underlying issue here and this PR is going to be obsolete? I am happy to close this if the new PR is superceding this.

[dvdsk]

Looks like there's been some good progress in figuring out the core underlying issue here and this PR is going to be obsolete? I am happy to close this if the new PR is superceding this.

progress but we are not done yet 😢 , its a difficult one this. But seeing the way we go this pr is going to be obsolete. Thanks for the valuable discussion that helped a lot!