plietar / librespot

Open Source Spotify client library
MIT License
1.14k stars 184 forks source link

[Question] Regarding the use of Sink.stop #224

Open leafy7382 opened 7 years ago

leafy7382 commented 7 years ago

My problem: I have a Rpi3 + Hifiberry Digi+ Pro running VolumioNext (the new build that supports Digi+ Pro) and use the volspotconnect2 that in turn uses librespot. Whether I am using TOSLINK or Coax, I get a mute relay click from my DAC (Schiit Yggdrasil) whenever

  1. Started playing after a stop
  2. Resumed playing after a stop
  3. Next track or previous track before 3 secs into play

Since in the Alsa backend, the format of the stream is fixed (signed 16,44100), the only way that make the DAC relay click is by stopping the output completely and restarting the output.

I've noticed that Sink.stop were called exactly 4 times in

PlayerCommand::Load
PlayerCommand::Stop
PlayerCommand::Pause

and when there is a Vorbis error

Would it be possible to change the stop behaviour to not cut off the whole backend? What I perceive the possible solutions could be one of the following(please correct me if I am wrong):

kingosticks commented 7 years ago

Releasing the ALSA device when stopped, like other applications do, is good, do you really want to lose that? If you don't release it you end up blocking any other applications form using it (unless you configure an ALSA dmix plugin). When you've got a bunch of audio playback applications trying to get along together this is quite important.

If I remember correctly (I might be wrong), the Android Spotify client doesn't actually have a stop button, so bizarrely in this case it's nice that pause has the same behaviour as stop in regards to this.

leafy7382 commented 7 years ago

In my setup, the RPi should not have other audio apps competing with librespot for audio, if it has, it would be very distracting. Another thought would be to not stop ALSA for as long as librespot is connected to Spotify, and only stop it when the connection is broken.

Or, another way to do it is to actually break out stop into pause and real stops, and implement track switches using pause instead of stops. This way the stops really release the device as behaving applications do and pause would hold on to the device dearly and not surprise the user by playing something else from another application.

kingosticks commented 7 years ago

They are not actually playing audio at the same time. Only one program is ever actually in a playing state. Generally you would have it setup so each program's on-play function sends stop commands to all the other programs. So that the one program that is actually currently using the alsa device will then stop and release it. For this setup, ensuring that stop always releases the device (like your 2nd paragraph) is important. I hope I've explained this a bit better now.

leafy7382 commented 7 years ago

So would it make more sense to implement a hard .stop and .pause and use .pause for Playercommand:{Load|Pause}? I think this would be required for gapless if it's implemented too

herrernst commented 7 years ago

In its early versions (when only the portaudio backend was available), librespot never closed the audio device. I wished for this to be changed, and it was merged. You can see relevant changes and discussion here: https://github.com/plietar/librespot/pull/107 Maybe you could even just revert this commit for yourself.

In the meantime, I have also learned that there are situations where it would be better to keep the audio device open, at least as long librespot is selected as speaker. @plietar, maybe this should be switchable with a command-line-switch like --close-audio-device-on-stop true(=default)|false?

leafy7382 commented 7 years ago

I went ahead to change .stop to do nothing (thus holding the audio device). As I am running from a plugin in Volumio, the only way right now is to restart the plugin in order to release the device. Not a huge hassle for me at the moment as Spotify is my only streaming source. I get sort of pseudo gapless playback as a bonus.

Another idea is to place a hook in handle_end_of_track to start a timer of X (like X=5 or 10) second, if there isn't any message coming in after timeout (either seek() or next()), it would probably mean that user has no interaction at the end of a playlist and we can release the device. But this is well beyond my understanding of Rust at this moment.