Fix(484) - ChannelVolume misbehaving #493

DivineGod · 2023-04-15T07:22:06Z

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 · 2024-04-20T10:31:06Z

src/source/empty.rs

@@ -36,17 +39,17 @@ where
 {
    #[inline]
    fn current_frame_len(&self) -> Option<usize> {
-        None
+        Some(0)


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.

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 · 2024-04-21T02:37:46Z

[...] 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 · 2024-04-21T08:41:34Z

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 · 2024-04-21T09:07:49Z

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 · 2024-04-21T09:13:51Z

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 · 2024-10-17T13:13:07Z

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 · 2024-10-17T13:14:08Z

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

DivineGod · 2024-10-22T23:09:00Z

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

Rebased to sort merge conflict

DivineGod · 2024-10-22T23:43:24Z

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 · 2024-10-23T04:58:34Z

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 · 2024-10-23T16:19:06Z

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 · 2024-11-29T16:01:31Z

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 · 2024-11-29T16:04:53Z

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 · 2024-12-01T14:32:30Z

src/sink.rs

+        let v = vec![100.0f32];
+        let input = SamplesBuffer::new(1, 48000, v.clone());
+
+        // High rate to avoid immediate control.


Can you explain this comment?

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

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

PetrGlad · 2024-12-01T17:06:58Z

@dvdsk BTW, what is the use case for ChannelVolume? It does simple mono conversion (with saturating_add) and only then splits it into output channels applying volume to each output channel. Including mono conversion looks odd to me. And this mono conversion also means there is no way to adjust channel volumes independently while keeping the stereo scene,

I would split ChannelVolume into a Mono converter and a per channel volume converter.

Looks like rodio does not have a limiter? I'd want one to muffle saturation distortions and make sound more "analog".

dvdsk · 2024-12-02T11:40:24Z

@dvdsk BTW, what is the use case for ChannelVolume?

its only used in the spatial source (and indirectly in spatial sink):

/// A simple spatial audio source. The underlying source is transformed to Mono
/// and then played in stereo. The left and right channel's volume are amplified
/// differently depending on the distance of the left and right ear to the source.

dvdsk · 2024-12-02T11:42:38Z

Looks like rodio does not have a limiter? I'd want one to muffle saturation distortions and make sound more "analog".

If a limiter is different then a low pass/high pass/band pass filter then we do not have one atm. Might be a good add.

will3942 · 2024-12-02T15:23:14Z

@dvdsk BTW, what is the use case for ChannelVolume? It does simple mono conversion (with saturating_add) and only then splits it into output channels applying volume to each output channel. Including mono conversion looks odd to me. And this mono conversion also means there is no way to adjust channel volumes independently while keeping the stereo scene,

I would split ChannelVolume into a Mono converter and a per channel volume converter.

Looks like rodio does not have a limiter? I'd want one to muffle saturation distortions and make sound more "analog".

We're currently using this to be able to output different sources onto different channels on a device, we have a scenario where we want to be able to do the following using a 10 channel USB audio interface:

Output Track A onto Channel 1, 4, 7
Output Track B onto Channel 2, 3, 5, 6
Output Track C Onto Channel 8

with channels 9 and 10 silent.

move channel from fix to constructor, add test Empty Source should not have a sample rate. Sample Rate Conversion to handle Empty source properly add more doc around Empty

DivineGod · 2024-12-04T03:52:37Z

Having some time to look at this. I've rebased the branch and re-run my channel_volume example on both the master branch and this branch. It looks to me that my branch is fixing the issue I'm seeing compared to what happens on master

DivineGod · 2024-12-04T03:59:29Z

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 · 2024-12-04T11:40:12Z

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 · 2024-12-05T02:26:46Z

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

This is a major refactor touching all the parts. The idea behind it is that for the exceptional case of `Empty` we can supply None to either and otherwise we either have supplied values or some "sane" defaults available to use

dvdsk · 2024-12-05T11:51:55Z

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

PetrGlad · 2024-12-05T18:46:14Z

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 · 2024-12-05T18:46:27Z

// 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 · 2024-12-05T19:04:23Z

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 · 2024-12-05T21:59:32Z

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 · 2024-12-05T22:05:19Z

// 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 · 2024-12-08T19:55:12Z

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

DivineGod force-pushed the fix/channel-volume branch 2 times, most recently from 0f059bd to 50b6f40 Compare April 15, 2023 08:59

DivineGod marked this pull request as ready for review April 16, 2023 23:18

DivineGod mentioned this pull request Apr 16, 2023

ChannelVolume misbehaving with non power of 2 number of channels #484

Open

DivineGod force-pushed the fix/channel-volume branch from 50b6f40 to 1167092 Compare May 1, 2023 00:50

dvdsk reviewed Apr 20, 2024

View reviewed changes

dvdsk force-pushed the master branch 3 times, most recently from b10961c to a7f67b3 Compare October 10, 2024 00:14

DivineGod force-pushed the fix/channel-volume branch from fbe90ab to f296cc9 Compare October 22, 2024 23:07

This comment was marked as resolved.

Sign in to view

dvdsk added this to the 0.20 milestone Oct 23, 2024

dvdsk removed this from the 0.20 milestone Nov 2, 2024

dvdsk force-pushed the master branch 4 times, most recently from 0ee0413 to 073fdb1 Compare November 8, 2024 00:55

PetrGlad reviewed Dec 1, 2024

View reviewed changes

DivineGod added 6 commits December 4, 2024 14:48

Empty Source type should report 0 channels

e988355

move channel from fix to constructor, add test Empty Source should not have a sample rate. Sample Rate Conversion to handle Empty source properly add more doc around Empty

clean match arm

62e7689

not sure None is better than Some(0)

51a5b6f

Add test to channel_volume and sink to try to catch this weird issue

0e8ec2e

fix the test to work correctly. Still on the hunt for this bug

1591e2a

fix channel_volume example to new api

75f9f92

DivineGod force-pushed the fix/channel-volume branch from b382da2 to 75f9f92 Compare December 4, 2024 03:49

fmt

86de41b

fix test

ccfbeb6

DivineGod force-pushed the fix/channel-volume branch 3 times, most recently from 9ea54c1 to 9f95a58 Compare December 5, 2024 02:38

See what happens if channels and sample_rate are options.

39434d0

This is a major refactor touching all the parts. The idea behind it is that for the exceptional case of `Empty` we can supply None to either and otherwise we either have supplied values or some "sane" defaults available to use

DivineGod force-pushed the fix/channel-volume branch from 9f95a58 to 39434d0 Compare December 5, 2024 02:54

dvdsk mentioned this pull request Dec 5, 2024

[Discussion] Minimize unnecessary resampling #646

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix(484) - ChannelVolume misbehaving #493

Fix(484) - ChannelVolume misbehaving #493

DivineGod commented Apr 15, 2023

dvdsk left a comment

dvdsk Apr 20, 2024

DivineGod Apr 21, 2024

DivineGod commented Apr 21, 2024

dvdsk commented Apr 21, 2024

dvdsk commented Apr 21, 2024

DivineGod commented Apr 21, 2024

dvdsk commented Oct 17, 2024

dvdsk commented Oct 17, 2024

DivineGod commented Oct 22, 2024

DivineGod commented Oct 22, 2024

This comment was marked as resolved.

DivineGod commented Oct 23, 2024

dvdsk commented Oct 23, 2024

will3942 commented Nov 29, 2024

DivineGod commented Nov 29, 2024

PetrGlad Dec 1, 2024

DivineGod Dec 5, 2024

DivineGod Dec 5, 2024

PetrGlad commented Dec 1, 2024 •

edited

Loading

dvdsk commented Dec 2, 2024

dvdsk commented Dec 2, 2024

will3942 commented Dec 2, 2024

DivineGod commented Dec 4, 2024

DivineGod commented Dec 4, 2024

dvdsk commented Dec 4, 2024

DivineGod commented Dec 5, 2024

dvdsk commented Dec 5, 2024

PetrGlad commented Dec 5, 2024

PetrGlad commented Dec 5, 2024

PetrGlad commented Dec 5, 2024

DivineGod commented Dec 5, 2024

DivineGod commented Dec 5, 2024

PetrGlad commented Dec 8, 2024

Fix(484) - ChannelVolume misbehaving #493

Are you sure you want to change the base?

Fix(484) - ChannelVolume misbehaving #493

Conversation

DivineGod commented Apr 15, 2023

dvdsk left a comment

Choose a reason for hiding this comment

dvdsk Apr 20, 2024

Choose a reason for hiding this comment

DivineGod Apr 21, 2024

Choose a reason for hiding this comment

DivineGod commented Apr 21, 2024

dvdsk commented Apr 21, 2024

dvdsk commented Apr 21, 2024

DivineGod commented Apr 21, 2024

dvdsk commented Oct 17, 2024

dvdsk commented Oct 17, 2024

DivineGod commented Oct 22, 2024

DivineGod commented Oct 22, 2024

This comment was marked as resolved.

DivineGod commented Oct 23, 2024

dvdsk commented Oct 23, 2024

will3942 commented Nov 29, 2024

DivineGod commented Nov 29, 2024

PetrGlad Dec 1, 2024

Choose a reason for hiding this comment

DivineGod Dec 5, 2024

Choose a reason for hiding this comment

DivineGod Dec 5, 2024

Choose a reason for hiding this comment

PetrGlad commented Dec 1, 2024 • edited Loading

dvdsk commented Dec 2, 2024

dvdsk commented Dec 2, 2024

will3942 commented Dec 2, 2024

DivineGod commented Dec 4, 2024

DivineGod commented Dec 4, 2024

dvdsk commented Dec 4, 2024

DivineGod commented Dec 5, 2024

dvdsk commented Dec 5, 2024

PetrGlad commented Dec 5, 2024

PetrGlad commented Dec 5, 2024

PetrGlad commented Dec 5, 2024

DivineGod commented Dec 5, 2024

DivineGod commented Dec 5, 2024

PetrGlad commented Dec 8, 2024

PetrGlad commented Dec 1, 2024 •

edited

Loading