Implement Rodio as default playback backend #277
Conversation
I think that is the best course of action. The less audio code we have to maintain, the better. If you want to have a go at adding Rodio support, that would be most welcome. |
|
So I've switched the code to use Rodio which makes it far far simpler. It now plays audio on my Windows machine regardless of the audio device's native format which is nice. There are some pretty significant playback issues on my windows machine. The sink accepts audio without ever blocking the playback thread, and I think that might cause Librespot to just plow through songs as fast as it can download them... I will do some more testing soon, including on other platforms. I haven't found much time to spend on this, so I've just swapped in rodio as an optional back-end currently, rather than removing support for any of the others. |
|
It seems, based on the RustAudio/rodio#220 pull request, that the next step is using the |
|
Yes I have a completely working implementation at home. I was tempted to wait till the next Rodio release to keep going. But I'll see if I can get this working with a git dependency for the time being. The len method is only needed to provide the same functionality as the other backends. I'm using it the same way as #283 uses queue.size() |
|
I'd personally love to see the implementation. As you can see above I referenced above, there is an |
|
I just tested it, and it does work -- kinda. I don't know if I'm experiencing a bug in your implementation, or in librespot as a whole. When I try and play a song in librespot I can see it loads in the CLI, but nothing is played. Then when I press "next song" in the Spotify client the next song is loaded and played as expected, but the Spotify client disconnects from librespot. EDIT: It disconnects, but continues to play the queue at the time it connected. |
|
I've don't think I've ever experienced that.
…On Thu, 7 Mar 2019, 19:07 Rasmus Larsen, ***@***.***> wrote:
I just tested it, and it does work -- kinda. I don't know if I'm
experiencing a bug in your implementation, or in librespot as a whole.
When I try and play a song in librespot I can see it loads in the CLI, but
nothing is played. Then when I press "next song" in the Spotify client the
next song is loaded and played as expected, but the Spotify client
disconnects from librespot.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#277 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAUdMBp8GeZR_M5-DfD2rD9fi7DbEAWUks5vUWNrgaJpZM4ZEqNI>
.
|
|
I recorded a short example of what I mean. https://gfycat.com/thunderousbrightcuckoo It actually worked as intended the first time I tried to record it, but the second time it stopped again. Have tried a couple of times since, and it has only worked as intended that once. I'm suspecting this isn't a bug with your code, but it's a bi-product of you enabling native Windows support. |
|
I don't know if you've locally patched the default features but to run this currently I have to specify the rodio backend.
|
|
I modified the |
|
thanks for your work on this @willstott101 will give it a go later and let you know how it goes. We need to think a bit about how to handle this, as ideally rodio will become the only backend bundled with the librespot library once librespot-daemon is written, but in the meantime I have a feeling that dumping the other backends is going to break someone's workflow somewhere, and without a gauge of who is using what, it's hard to say. I think the optimal short-mid term solution might be to make sure this is working across platforms, then set rodio as the default backend, and then slowly phase the other ones out once we start introducing the daemon. |
|
Have just tested it and it seems to be working well, thanks for the work. With regards to previous comment, I suggest you also update this PR to set rodio as the default audio backend, (and rebase it to master so that Cargo.lock files match). Also, if some people here are running on Windows, it would be great if you could build it with |
willstott101
force-pushed
the
cpal-backend
branch
from
f257998
to
9630874
Compare
|
Managed to test this out on Windows. A bit of tweaking required to get it running (PR #293 was merged w/ this one), but built pretty easily, and sound is good. A quick vid of me testing it (loading, volume control, seeking, skipping, etc.), response times <1s, so pretty happy there. https://streamable.com/h15n1 Is there anything else in this PR that is still |
|
Not really.
I think it would be nice to get the stop method to actually do something.
In the current Rodio API I think that'd have to be reinitialising the Rodio
Sink. I am tempted to look into trying to add to the Rodio APIs so the
buffer is handled in there (some sync channel like Source). And make sure
we can clear that buffer/the sink's queue. I think that would be cleaner.
None of that is particularly important to me but I might look into it
fairly soon.
I reckon it's best to try and get this merged sooner. And have some more
people trying out rodio/cpal
…On Mon, 11 Mar 2019, 02:32 Sasha Hilton, ***@***.***> wrote:
Managed to test this out on Windows. A bit of tweaking required to get it
running (PR #293 <#293>
was merged w/ this one), but built pretty easily, and sound is good. A
quick vid of me testing it (loading, volume control, seeking, skipping,
etc.), response times <1s, so pretty happy there.
https://streamable.com/h15n1 Is there anything else in this PR that is
still WIP so to speak, or should we just review what's here and get this
merged?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#277 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAUdMNX352Yy5YUnDcXwsJ8kdzI19dIEks5vVcBPgaJpZM4ZEqNI>
.
|
willstott101
changed the title
|
Ok, if you're happy to merge this as is, we'll get it reviewed and merged sooner rather than later. With regards to the |
| } | ||
|
|
||
| fn stop(&mut self) -> io::Result<()> { | ||
| // This will immediately stop playback, but the sink is then unusable. |
There was a problem hiding this comment.
I haven't looked too closely at rodio, but can we not use the stop method to release the sink, and the start method to capture it again? Or is it a case of once closed, it's closed until librespot is reinitialised?
There was a problem hiding this comment.
Once a sink is closed it seems to set a flag in rodio that means it can't be used anymore. It's also currently the only API to immediately stop playback. The sink can be re-initialized from the rodio device... which seems to work quite well actually. It makes pausing/skipping feel much more responsive. However when using .stop() there is consistently a fuzz. I can't figure out any implementation of fn stop that doesn't cause the fuzz. Even setting the volume to 0 makes the sink go fuzzy (regardless of if it's stopped).
I suspect some digging into Rodio/CPAL is needed to get this to work. I may try to find out if the fuzz is windows specific or not tomorrow.
For reference:
fn start(&mut self) -> io::Result<()> {
// Really an "unpause". Doesn't undo "stop".
// self.rodio_sink.play();
// Will cause the existing sink to get finalized (and stop it if it
// hasn't already... which makes a fuzz sound).
// self.rodio_sink = rodio::Sink::new(&self.rodio_device);
Ok(())
}
fn stop(&mut self) -> io::Result<()> {
// This will immediately stop playback, but the sink is then unusable.
// It also makes a short fuzz noise when used.
// self.rodio_sink.stop();
// Just garbles the output, doesn't mute it properly.
// self.rodio_sink.set_volume(0.0);
// This will still fuzz when it gets finalized (by being replaced in play()).
// self.rodio_sink.pause();
// We just have to let the current buffer play till the end.
Ok(())
}
There was a problem hiding this comment.
What do you mean when you say fuzzy? As in it clicks when the sink is opened/closed, or switching back to librespot causes audio distortion?
There was a problem hiding this comment.
also, I get a compile error if using that line with rodio_device from the comment above:
error[E0609]: no field `rodio_device` on type `&mut audio_backend::rodio::RodioSink`
--> playback/src/audio_backend/rodio.rs:79:50
|
79 | self.rodio_sink = rodio::Sink::new(&self.rodio_device);
There was a problem hiding this comment.
Here's a windows buildable branch that reproduces the fuzz problem for me on both Ubuntu and Windows if you're curious.
I think I'll have to dig into Rodio and try some smaller examples to hunt this down. Setting the volume seems completely broken too. It would appear Sinks are not the ideal way to interact with Rodio which suprises me.
I could have a go at implementing using some lower level APIs in Rodio instead I guess... but at least the implementation in this PR functions ok as far as I know.
There was a problem hiding this comment.
Ah interesting, yeah it's whenever the stop method is called for me. So pausing does it.
There was a problem hiding this comment.
Have you tried on a different PC?
There was a problem hiding this comment.
Yeah I've tried on several PCs on both windows and ubuntu. I'll test on a macbook at some point. I've been working on a CPAL backend using the sample crate instead of using Rodio for resampling. I've managed to get that working but it's much more code. Hopefully I can use that as a learning tool to find the fuzz in upstream Rodio though.
There was a problem hiding this comment.
CPAL backend using the
samplecrate
would this be an option for me to try to get a librespot with jack + sample rate conversion?
There was a problem hiding this comment.
The audio code base has changed a fair bit since 2019. Let’s let this PR be.
playback/src/audio_backend/rodio.rs
Outdated
| let default_fmt = match device.default_output_format() { | ||
| Ok(fmt) => cpal::SupportedFormat::from(fmt), | ||
| Err(e) => { | ||
| info!("Error getting default rodio::Sink format: {:?}", e); |
There was a problem hiding this comment.
Should this be warn! instead?
playback/src/audio_backend/rodio.rs
Outdated
| let mut output_formats = match device.supported_output_formats() { | ||
| Ok(f) => f.peekable(), | ||
| Err(e) => { | ||
| info!("Error getting supported rodio::Sink formats: {:?}", e); |
|
Other than small formatting change above, looks good and works well for me. If someone else could also compile this and confirm whether they do or don't hear fuzzing when stop method is called, that would be great, as we can merge this if it's machine specific. |
|
I'm getting a panic when the rodio sink is created on an Odroid C2 using the HDMI output. The following trace is from running:
The alsa backend works ok so I'm guessing this is a rodio/cpal issue? I have a couple of other devices I will try and get running with the rodio backend so I can test for any audio issues with the rodio-fuzz branch. |
|
@chrisbp have you got |
|
So I got this running on an armv7 device and noticed a couple of things. Comparing the alsa backend to the rodio one, alsa definitely feels more responsive when pausing than rodio. I assume this is because alsa is clearing its queue and stopping playback immediately, where as rodio has to wait for the queue to empty via playback. So then I decided to try out the rodio-fuzz branch to see if the stop call helps make rodio more responsive on pause. It does indeed make it more responsive, but I get a short burst of audible junk output whenever stop is called on the sink. I decided to investigate by increasing the amount of data queued to the sink by upping the queue size check from 26 chunks to 135 chunks. With this change, the duration of the audible junk increases and begins to sound like part of the currently playing track albeit quite garbled. This is a guess, but it sounds like rodio is clearing the queue by deleting from the head of the queue while it is still being read from for playback. Regardless, it means calling stop on the sink seems to be a bad option. The other thing I noticed while swapping between alsa and rodio backends is that rodio seems to be consuming quite a bit more CPU. Here is the output from top while playing the same track: rodio alsa The rodio backend seems to average around 30% CPU usage while spiking up to 45% at times. Where as the alsa backend typical sits around 12% CPU, spiking up to 22%. |
No, the Odroid C2 is running Arch and I can't see an equivalent dev package there. It already has alsa-lib and alsa-utils though. Having said that I have rodio working on another armv7 device running Ubuntu 16.04 which only has libasound2 installed, not libasound2-dev. |
try |
|
Yeah it seems calling stop or releasing a Rodio sink reliably causes that fuzz for me. I've come to decide releasing the audio device is reasonably important for me, as switching default device during playback is something I'd like to work. Your comments @chrisbp inspired me to try pausing the sink, and sleeping the thread for a second. As pausing in Rodio makes a Source simply return 0 in it's iterator. However as soon as I eventually then call stop I get a small bit of fuzz. Even if the Sink should have seen nothing but 0 for 3 seconds. Honestly I think Rodio's architecture isn't really suited for this use-case. As far as I can tell we always have to convert to f32 samples. I'd be tempted to re-construct this Sink using CPAL and https://docs.rs/sample/0.10.0/sample/interpolate/struct.Converter.html There is no guaranteeing that we wouldn't see the same fuzz when releasing a device in CPAL though. |
|
@willstott101 do the |
|
@willstott101 @chrisbp unless there's an objection, I'm going to merge this, for a couple of reasons:
Whilst there are issues apparently with fuzz or sink creation, this backend has been added such that the other backends can still be used. Thus, I would rather get people using librespot out of the box to move over to rodio, and gather any feedback on potential problems, and provide a note in the readme that points people to manually specifying an alternative backend at compile time if rodio is problematic. @willstott101 if you're ok to rebase to master, will get this merged later today. |
Doesn't work that well.
willstott101
force-pushed
the
cpal-backend
branch
from
ead1d8a
to
3548917
Compare
|
The fuzz is not present in this branch so I'm happy for you to merge it. It's only when trying to release the Rodio Sink that we see the fuzz. I'll look into this more in other branches. No worries. |
|
Thanks for your work on this. Feel free to open another PR if you work out what causes the fuzz and come up with a fix. |
|
Sooo.. is there a guide somewhere how to install, use and connect rodio (to jack)? |
|
I haven't touched any of this since before Jack was added to CPAL (rodio's audio host abstraction layer). https://github.com/librespot-org/librespot/blob/dev/playback/src/audio_backend/mod.rs#L135 Implies to me: build with the See the docs here: https://github.com/librespot-org/librespot/wiki/Audio-Backends#usage |
|
Thank you very much for your quick answer! how do I build with like this |
|
|
|
Thanks man! hmm... seems like I need to change alsa to jack or something? What are my config options? |
|
Perhaps check out the chat: https://gitter.im/librespot-org/spotify-connect-resources I don't think this issue is really the right forum for this kind of help. Better to keep Pull Requests to conversation about their specific changes. |
|
hmm..
I started a new discussion – maybe you could help out there? Would be fantastic! Cheers! |
Hello, I'm new to Rust and librespot but I thought this #145 would be a reasonable first thing to look-at, as a vague attempt at helping to improve the cross-platform audio ecosystem and encourage projects such as Tomahawk.
I managed to get music playing with the back-end in this state... however there's a buzz when nothing is playing and the sink (in windows at least) has to be 44100 Hz for playback to work as cpal doesn't consider resampling to be in it's remit.
So I've decided to pause here, share what I've got and consider #195
Basically, would you prefer I had at go swapping in Rodio to handle everything it can, and reduce code here in librespot as much as possible, or perhaps use another approach... such as a Rodio back-end in the current structure with the intention of slowly removing the others.