Feature: ChannelRouter #656
Conversation
Implementation in progress.
Undid an unnecesary refactor.
Can't try to grab a lock on the audio thread.
…rodio into feature-channelrouter
examples/channel_routing.rs
Outdated
| 0 => println!("Left speaker ramp up"), | ||
| 1..10 => { | ||
| _ = controller.map(0, 0, n as f32 / 10.0); | ||
| _ = controller.map(0, 1, 0f32); |
There was a problem hiding this comment.
this is not very readable right? I can guess its channel from, channel too, gain? But we might be able to do better. I see a number of options, there might well be more:
1️⃣ make map take a struct like: controller.map(Mapping { from: 0, to: 1, gain: 0f32 })
2️⃣ make map a builder: controller.map().from(0).to(1).with_gain(0.f32).appy(). Here you could make the gain optional and do a default of 1.0
3️⃣ rename it so the argument names are in the name: controller.map_from_to_with_gain(0, 1, 0f32).
I think 2️⃣ is my favorite, though its a little more code.
There was a problem hiding this comment.
I like the builder too.
|
BTW I'm working on a separate project right now to try to profile all these different approaches, once I have that I'll post what I find and then we can move forward with this. |
|
|
||
| self.current_channel = 0; | ||
|
|
||
| for change in self.receiver.try_iter() { |
There was a problem hiding this comment.
Yes, it may do as first version, but polling channel is relatively expensive. In other places an additional flag is used to skip the loop altogether when there are no changes (e.g. mixer::Mixer::has_pending). See also #658.
There was a problem hiding this comment.
This only checks the receiver once if there are no changes in it right? Is a single channel poll slower then checking an atomic flag?
edit: Nevermind I checked the source for mpsc and its kinda complex. Still I would like to have a benchmark for this source before making such a change. If only to enjoy the speedup, but it might also show there to be no effect. Maybe that complexity for mpsc compiles down to something real lean?
| let gain = input_gains[self.current_channel as usize]; | ||
| in_sample.amplify(gain) | ||
| }) | ||
| .reduce(|a, b| a.saturating_add(b)); |
There was a problem hiding this comment.
Since this is real-time I'd prefer this to be more explicit. Maybe the matrix should be transposed to streamline this. This way instead of zip it would select a gain vector and then do the dot product of coefficients by input_buffer (it can be even optimized with SIMD).
There was a problem hiding this comment.
I do not want to overburden @iluvcapra, this is already quite a big effort! So we can always leave optimizations for later and merge this first. Someone else can work on them too (maybe @PetrGlad). Let me know what you prefer @iluvcapra.
| from: u16, | ||
| to: u16, | ||
| gain: f32, | ||
| ) -> Result<(), ChannelRouterControllerError> { |
There was a problem hiding this comment.
❓ If the routing has to change one will have to remember to clear all the gains that are not needed anymore.
Maybe a "reset" function that sets all the gains to zero would be helpful.
❓ I wonder if we should allow specifying matrix at the initialization time, or let users to pass a list of coefficients.
There was a problem hiding this comment.
Probably that is why I wanted to set the whole gain set at once: users may have the matrix on their side process and inspect it a they prefer and then use its to update the router.
| /// - The first dimension is respective to the input channels | ||
| /// - The second is respective to the output channels | ||
| /// | ||
| /// Thus, if a value at `map[1,1]` is 0.2, this signifies that the signal on |
There was a problem hiding this comment.
It is still confusing, I'd picked different numbers for input and output, or maybe just use names instead of constants (e.g. map[in,out])
| // | ||
| // Doing it as a HashMap<(u16,u16), f32> is an option too but there's a penalty hashing these | ||
| // values, there's ways to speed that up though. It'd be great if the object upgraded its | ||
| // implementation if it got sufficiently big. |
There was a problem hiding this comment.
No we do not need complex containers, if a sparse matrix is preferable I have an example how that can be implemented.
|
|
||
| // This might be too fussy, a source should never break a frame in the middle of an | ||
| // audio frame. | ||
| let samples_to_take = min( |
There was a problem hiding this comment.
Yes, I'd prefer to require all the sources to produce only complete frames. But I think it is not actually enforced anywhere so far. This can be an assertion, though.
| channel_router::channel_router(self, 2, mapping).1 | ||
| } | ||
|
|
||
| /// Creates a [`ChannelRouter`] that mixes a 5.1 source in SMPTE channel order (L, R, C, Lfe, Ls, |
There was a problem hiding this comment.
very nice example use-case 👍
very cool, I'm curious to see if what ends up as best lines up with our suspicions |
|
For mixing several input streams we can use similar class that uses the same algorithm but accepts several Sources at once. Our existing mixer only sums all inputs without any volume adjustments, |
Isn't the better way to do this to have users wrap the streams they are mixing in a |
|
Yes, it is a simpler solution. Maybe we could add and example or make some kind of cookbook. I was just pondering whether an über-mixer might be useful (the one like in this PR but allowing multiple inputs). For sparse matrix representation this would be straightforward to adapt (or have another more advances version). By he way, the router in this PR should also handle situation when number of input channels change. At least for current API this is required. |
I really like that idea! Sink/Player will be easy to use but the power of rodio lies in connecting the sources in an audio pipeline. We could have a bunch of examples but a cookbook is way nicer! |
This is an implementation of a multichannel router/mixer source. See #653.