OutputStream: expose the underlying cpal::Stream or provide wrapper for pause()/play()

[martinetd]

Hello,

thanks for rodio!

I've been playing with rodio in https://github.com/martinetd/fuurin and one thing that's been annoying me is that even with all sources paused, just having an OutputStream present consumes a non-negligible amount of CPU sending silence to alsa.
(for what it's worth I tried the pipewire output provided in RustAudio/cpal#938 and it's very slightly better, but sensibly the same)

With the current code, I see no other way around dropping the whole output stream which means dropping all the sinks I've opened first then the mixer and the stream itself, but I tried just exposing the _stream and using outputstream.stream().pause() and it reduces cpu to zero, so I think it's a much better approach to this problem.

Would you accept a patch such as this?

diff --git a/src/stream.rs b/src/stream.rs
index f6c8bcf550c0..2df5d7307b49 100644
--- a/src/stream.rs
+++ b/src/stream.rs
@@ -44,7 +44,7 @@ pub struct OutputStream {
     config: OutputStreamConfig,
     mixer: Mixer,
     log_on_drop: bool,
-    _stream: cpal::Stream,
+    stream: cpal::Stream,
 }
 
 impl OutputStream {
@@ -53,6 +53,11 @@ pub fn mixer(&self) -> &Mixer {
         &self.mixer
     }
 
+    /// Access the output stream's underlying cpal Stream.
+    pub fn stream(&self) -> &cpal::Stream {
+        &self.stream
+    }
+
     /// Access the output stream's config.
     pub fn config(&self) -> &OutputStreamConfig {
         &self.config
@@ -487,7 +492,7 @@ fn open<E>(
         Self::init_stream(device, config, source, error_callback).and_then(|stream| {
             stream.play().map_err(StreamError::PlayStreamError)?;
             Ok(Self {
-                _stream: stream,
+                stream: stream,
                 mixer: controller,
                 config: *config,
                 log_on_drop: true,

Or would it be better to expose pause() and play() method directly that'd just call self.stream.pause()/play() ?

I think allowing access to the stream is more flexible ultimately, but happy either way.

I'll open a PR once there's an agreement on which is better :)

Thanks!

[dvdsk]

just having an OutputStream present consumes a non-negligible amount of CPU sending silence to alsa.

This is a worth solving! Thanks for bringing it to our attention with ideas on how to approach it.

I wonder if this is something we could do automatically in Rodio instead of requiring the user to mess with the cpal::Stream. The mixer should know when its empty same for the queue, they could then call pause on the cpal::Stream for you.

If that's something your willing to look into that would be very nice.

[martinetd]

just having an OutputStream present consumes a non-negligible amount of CPU sending silence to alsa.

FWIW as I had another look I'm not sure if it's the sending to alsa part that takes cpu, or just the fact that I have many paused sources so just iterating over all of them to sum zero for every sample takes cpu... But when cpal::Stream is paused then the outputstream mixer iterator isn't consumed and cpu usage falls to zero.
It's more obvious in non-release builds, with just one file my program consumes 4-5% cpu, with 25 files it's up to 40% cpu (respectively ~0.7 and 8% in release mode so much better but not all optimized away)

So, more standard clients that don't have lots of streams probably don't feel it as much -- I just happened to look when testing with many streams and probably in a debug build

I wonder if this is something we could do automatically in Rodio instead of requiring the user to mess with the cpal::Stream. The mixer should know when its empty same for the queue, they could then call pause on the cpal::Stream for you.

That's an idea!

I've played with cpal::Stream::pause and it's interesting for me even if the stream isn't empty (I have many sources playing at once, and a key to toggle them all, and confirmed that pausing the output stream properly pauses all the sources -- so it's slightly more elegant for me to not have to iterate through all the sources here and just pause the output steram)

But I can live with pausing all the streams and have this toggle automatically when there is no input:

• this is definitely nicer for current users (no need to change anything)
• if we start messing with a global output stream play/pause on content then having an explicit toggle will be hard to use, as it could unpause unexpectedly when adding new content or similar, so if we do this we definitely don't want to expose cpal::Stream::pause.

I've just had a close look and I can see how a Mixer can know there are no source at all, but I can't see how it can tell appart "all sources are paused" vs. "the sources present really provide silence" -- in paused case each source still is present and happen to have self.sum_current_sources() = 0 (in MixerSource::next()), but that doesn't mean anything.

This would require adding a method to the trait Source to query paused state, and query this for all sources every time (or every n times, it doesn't have to be immediate) we get a sample?
(Or I guess Source could become an iterator over Option and return None if paused? but that's an even bigger incompatible change so I don't think that's appropriate)

An advantage would be that we could put the paused streams aside to not iterate over them everytime if we can notify the mixer when they resume, which would save cpu even for partial pausing, but I'm not sure it's worth the change of the Source trait API -- I assume there are some external Sources out there?

So, hm, not so sure it's worth it?
If the goal is just to abstract cpal away (to e.g. allow using another implementation later) then I'm sure we'll always find a way to stop consuming the mixer iterator, so my current opinion is that having OutputStream::pause (and play) methods sounds like the simplest/robust way forward short term

[martinetd]

Correcting myself: it's not Source I was pausing,but Sink (I have a hard time thinking about it as a Sink because I have one per source)... That's a bit closer to the Mixer and perhaps something can be done; at least no need to modify the Source trait :)

An advantage would be that we could put the paused streams aside to not iterate over them everytime if we can notify the mixer when they resume, which would save cpu even for partial pausing, but I'm not sure it's worth the change of the Source trait API -- I assume there are some external Sources out there?

Actually thinking a bit further, if we can notify the Mixer on resume, we can notify on pause too, and there's no need to check at all...
So the question is can the Sink::pause/Sink::play somehow notify the parent sink?

I've tried quickly to add an Option<Mixer> to the Sink (which works thanks to Mixer being cheaply clonable, yay for Arc types), and I think this can work.
I don't think I'll finish tonight but I'll give it a shot (move paused Sinks to a different list in mixer so it can tell if everything is paused)

[martinetd]

I don't think I'll finish tonight but I'll give it a shot (move paused Sinks to a different list in mixer so it can tell if everything is paused)

Well, nothing is so easy...
I thought just keeping a clone of the Mixer in the Sink would be enough, but we need to mess with current_sources and not just pending_sources, and that one is harder to get.
I've moved it under lock to the Mixer and got this part working, but that comes at a cpu expanse to lock current sources on every iteration which is almost as big as the saving we get from it during pause.

And even then now we can tell when there is no pending or current source (everything paused or no stream) in the mixer, but I don't see how the mixer could now tell the OutputStream's stream that without adding more locks everywhere, so I'm giving up.
Even if that is possible I don't think the cost in nominal case is worth it... It might be possible to do better but I don't see how.

So back to square 1, I've been at it too long but unless you have a better idea I'll open a PR just exposing play/pause in OutputStream tomorrow.

[martinetd]

FWIW, auto-suspend start of code to be scrapped if someone wants to play with it: martinetd@3a79309

I'm not opening PR yet with martinetd@73148d1 (exposing pause/play) to keep discussions in one place for now

[dvdsk]

btw love all the thought your putting into this! I don't have time to give it a detailed look now but I'll take a look saturday! If I dont remind me please.

[martinetd]

There's absolutely no hurry on my end, but since you've asked I think it's Saturday again :)
Thank you for your time!

[dvdsk]

It is nontrivial indeed! Took me a couple of hours but I figured something out. See #791 for the details and some code. @martinetd if you want to take this off my hands and finish it that would be lovely. Though, while the idea is there, it is not going to be easy.