[GH-ISSUE #68] [Question] Regarding the use of Sink.stop #57

Closed
opened 2026-02-27 19:28:34 +03:00 by kerem · 18 comments
Owner

Originally created by @sashahilton00 on GitHub (Jan 29, 2018).
Original GitHub issue: https://github.com/librespot-org/librespot/issues/68

Issue by leafy7382
Friday Jul 21, 2017 at 15:42 GMT
Originally opened as https://github.com/plietar/librespot/issues/224


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):

  • Don't set self.0 = None; in stop, just do nothing
  • Implement a Sink.mute function that outputs 0 constantly (or does nothing) and use Sink.mute instead of Sink.stop for the first 3 scenarios. Than we can keep the Sink.stop behavior for Vorbis errors.
Originally created by @sashahilton00 on GitHub (Jan 29, 2018). Original GitHub issue: https://github.com/librespot-org/librespot/issues/68 <a href="https://github.com/leafy7382"><img src="https://avatars2.githubusercontent.com/u/359465?v=4" align="left" width="96" height="96" hspace="10"></img></a> **Issue by [leafy7382](https://github.com/leafy7382)** _Friday Jul 21, 2017 at 15:42 GMT_ _Originally opened as https://github.com/plietar/librespot/issues/224_ ---- 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): - Don't set `self.0 = None;` in stop, just do nothing - Implement a `Sink.mute` function that outputs 0 constantly (or does nothing) and use `Sink.mute` instead of `Sink.stop` for the first 3 scenarios. Than we can keep the `Sink.stop` behavior for Vorbis errors.
kerem 2026-02-27 19:28:34 +03:00
Author
Owner

@sashahilton00 commented on GitHub (Jan 29, 2018):

Comment by kingosticks
Friday Jul 21, 2017 at 17:55 GMT


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.

<!-- gh-comment-id:361264253 --> @sashahilton00 commented on GitHub (Jan 29, 2018): <a href="https://github.com/kingosticks"><img src="https://avatars2.githubusercontent.com/u/934824?v=4" align="left" width="48" height="48" hspace="10"></img></a> **Comment by [kingosticks](https://github.com/kingosticks)** _Friday Jul 21, 2017 at 17:55 GMT_ ---- 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.
Author
Owner

@sashahilton00 commented on GitHub (Jan 29, 2018):

Comment by leafy7382
Friday Jul 28, 2017 at 08:01 GMT


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.

<!-- gh-comment-id:361264280 --> @sashahilton00 commented on GitHub (Jan 29, 2018): <a href="https://github.com/leafy7382"><img src="https://avatars2.githubusercontent.com/u/359465?v=4" align="left" width="48" height="48" hspace="10"></img></a> **Comment by [leafy7382](https://github.com/leafy7382)** _Friday Jul 28, 2017 at 08:01 GMT_ ---- 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.
Author
Owner

@sashahilton00 commented on GitHub (Jan 29, 2018):

Comment by kingosticks
Friday Jul 28, 2017 at 08:48 GMT


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.

<!-- gh-comment-id:361264299 --> @sashahilton00 commented on GitHub (Jan 29, 2018): <a href="https://github.com/kingosticks"><img src="https://avatars2.githubusercontent.com/u/934824?v=4" align="left" width="48" height="48" hspace="10"></img></a> **Comment by [kingosticks](https://github.com/kingosticks)** _Friday Jul 28, 2017 at 08:48 GMT_ ---- 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.
Author
Owner

@sashahilton00 commented on GitHub (Jan 29, 2018):

Comment by leafy7382
Friday Jul 28, 2017 at 09:56 GMT


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

<!-- gh-comment-id:361264327 --> @sashahilton00 commented on GitHub (Jan 29, 2018): <a href="https://github.com/leafy7382"><img src="https://avatars2.githubusercontent.com/u/359465?v=4" align="left" width="48" height="48" hspace="10"></img></a> **Comment by [leafy7382](https://github.com/leafy7382)** _Friday Jul 28, 2017 at 09:56 GMT_ ---- 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
Author
Owner

@sashahilton00 commented on GitHub (Jan 29, 2018):

Comment by herrernst
Tuesday Aug 01, 2017 at 20:36 GMT


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?

<!-- gh-comment-id:361264351 --> @sashahilton00 commented on GitHub (Jan 29, 2018): <a href="https://github.com/herrernst"><img src="https://avatars2.githubusercontent.com/u/20795?v=4" align="left" width="48" height="48" hspace="10"></img></a> **Comment by [herrernst](https://github.com/herrernst)** _Tuesday Aug 01, 2017 at 20:36 GMT_ ---- 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`?
Author
Owner

@sashahilton00 commented on GitHub (Jan 29, 2018):

Comment by leafy7382
Wednesday Aug 02, 2017 at 04:13 GMT


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.

<!-- gh-comment-id:361264374 --> @sashahilton00 commented on GitHub (Jan 29, 2018): <a href="https://github.com/leafy7382"><img src="https://avatars2.githubusercontent.com/u/359465?v=4" align="left" width="48" height="48" hspace="10"></img></a> **Comment by [leafy7382](https://github.com/leafy7382)** _Wednesday Aug 02, 2017 at 04:13 GMT_ ---- 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.
Author
Owner

@ComlOnline commented on GitHub (Jan 29, 2018):

@herrernst do you think a flag like the one you suggested is still needed?

<!-- gh-comment-id:361353655 --> @ComlOnline commented on GitHub (Jan 29, 2018): @herrernst do you think a flag like the one you suggested is still needed?
Author
Owner

@herrernst commented on GitHub (Jan 29, 2018):

I prefer the current behaviour (audio device is released when paused/stopped). If somebody wants to change that, I would wish for a switch to at least optionally get the current behaviour back.

<!-- gh-comment-id:361390069 --> @herrernst commented on GitHub (Jan 29, 2018): I prefer the current behaviour (audio device is released when paused/stopped). If somebody wants to change that, I would wish for a switch to at least optionally get the current behaviour back.
Author
Owner

@ComlOnline commented on GitHub (Jan 29, 2018):

Oh I agree the current behavior is what I would expect. I was meaning in respect to this:

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.

Are there still cases where it is needed? Or are these more edge cases

<!-- gh-comment-id:361391944 --> @ComlOnline commented on GitHub (Jan 29, 2018): Oh I agree the current behavior is what I would expect. I was meaning in respect to this: >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. Are there still cases where it is needed? Or are these more edge cases
Author
Owner

@herrernst commented on GitHub (Jan 29, 2018):

I personally don't need this edge case, but maybe @leafy7382 does.

<!-- gh-comment-id:361392887 --> @herrernst commented on GitHub (Jan 29, 2018): I personally don't need this edge case, but maybe @leafy7382 does.
Author
Owner

@sashahilton00 commented on GitHub (Feb 1, 2018):

I don't need it, but looking over the code (though I'm uncertain of this), it looks like https://github.com/plietar/librespot/pull/107/files#diff-f8239df44acb1007c7514a788ccc6ae6R91 is responsible for killing the audio device, so if we just add a runtime flag to disable this line, wouldn't that be enough? If so, it's a trivial change.

<!-- gh-comment-id:362125443 --> @sashahilton00 commented on GitHub (Feb 1, 2018): I don't need it, but looking over the code (though I'm uncertain of this), it looks like https://github.com/plietar/librespot/pull/107/files#diff-f8239df44acb1007c7514a788ccc6ae6R91 is responsible for killing the audio device, so if we just add a runtime flag to disable this line, wouldn't that be enough? If so, it's a trivial change.
Author
Owner

@awiouy commented on GitHub (Feb 1, 2018):

I use this for LibreELEC: github.com/awiouy/librespot@6e82353ba2

run_onstart() and run_onstop() are respectively called when a Spotify app switches to and from librespot. The sink could be opened and closed accordingly.

As a bonus, artist and track are provided as environment variables when a track is first loaded..

<!-- gh-comment-id:362168631 --> @awiouy commented on GitHub (Feb 1, 2018): I use this for LibreELEC: https://github.com/awiouy/librespot/commit/6e82353ba271427fe996a6d866bebc70ff96786b run_onstart() and run_onstop() are respectively called when a Spotify app switches to and from librespot. The sink could be opened and closed accordingly. As a bonus, artist and track are provided as environment variables when a track is first loaded..
Author
Owner

@leafy7382 commented on GitHub (Feb 18, 2018):

Wow, great to have this thread back. Yes I'd love to have this edge case even if it's via a switch. Default behaviour makes my DAC click between every track change (it's mechanical so unavoidable) and lose about 1 sec of music. With this mod and fast enough network, it's almost as good as gapless playback, this is very important for classical works like operas.

<!-- gh-comment-id:366516468 --> @leafy7382 commented on GitHub (Feb 18, 2018): Wow, great to have this thread back. Yes I'd love to have this edge case even if it's via a switch. Default behaviour makes my DAC click between every track change (it's mechanical so unavoidable) and lose about 1 sec of music. With this mod and fast enough network, it's almost as good as gapless playback, this is very important for classical works like operas.
Author
Owner

@vladvasiliu commented on GitHub (Mar 9, 2018):

Hi,

I've been having this same problem with a hifiberry digi+ (standard version) and the DAC clicking is extremely annoying.
However, I use other sources on Volumio (local drive) and the behaviour is the same: whenever I change a track, the DAC goes click. The volumio issue is here: https://github.com/volumio/Volumio2/issues/1492

My point is that this sink release business would be just a workaround, and the way librespot does it is the way other apps do it too. This might actually be a problem with the digi+ itself and / or the driver.
As detailed in the mentioned issue, the exact same DAC with the exact same optical connection but on a ICH sound card doesn't have the clicking. Direct USB input to the DAC doesn't click either.
I'm guessing the sink is handled the same way by the player, though, in all the cases.

<!-- gh-comment-id:371912917 --> @vladvasiliu commented on GitHub (Mar 9, 2018): Hi, I've been having this same problem with a hifiberry digi+ (standard version) and the DAC clicking is extremely annoying. However, I use other sources on Volumio (local drive) and the behaviour is the same: whenever I change a track, the DAC goes click. The volumio issue is here: https://github.com/volumio/Volumio2/issues/1492 My point is that this sink release business would be just a workaround, and the way librespot does it is the way other apps do it too. This might actually be a problem with the digi+ itself and / or the driver. As detailed in the mentioned issue, the exact same DAC with the exact same optical connection but on a ICH sound card doesn't have the clicking. Direct USB input to the DAC doesn't click either. I'm guessing the sink is handled the same way by the player, though, in all the cases.
Author
Owner

@sashahilton00 commented on GitHub (Mar 20, 2018):

If anyone would like to implement this runtime flag, I'd be happy to merge it, but the current behaviour should remain as default.

<!-- gh-comment-id:374657433 --> @sashahilton00 commented on GitHub (Mar 20, 2018): If anyone would like to implement this runtime flag, I'd be happy to merge it, but the current behaviour should remain as default.
Author
Owner

@benoitmahe commented on GitHub (May 1, 2018):

Same issue with Allo Digione and Bifrost, so a runtime flag to change this behaviour would be really appreciated :-)

<!-- gh-comment-id:385668287 --> @benoitmahe commented on GitHub (May 1, 2018): Same issue with Allo Digione and Bifrost, so a runtime flag to change this behaviour would be really appreciated :-)
Author
Owner

@ashthespy commented on GitHub (Jun 8, 2018):

Was also facing similar clicks/pops on each track change - so added a simple delay with a runtime flag.
github.com/ashthespy/librespot@ab48ac8802

Am testing it now - but it seems to be leading some buffer underruns with alsa on my setup.

<!-- gh-comment-id:395891683 --> @ashthespy commented on GitHub (Jun 8, 2018): Was also facing similar clicks/pops on each track change - so added a simple delay with a runtime flag. https://github.com/ashthespy/librespot/commit/ab48ac8802d037e6eefdf51661cafa05013640a2 Am testing it now - but it seems to be leading some buffer underruns with alsa on my setup.
Author
Owner

@ashthespy commented on GitHub (Mar 13, 2020):

This should be solved with gapless playback landing in #430. For people who for some reason want the old behaviour, check out #444.

<!-- gh-comment-id:598938700 --> @ashthespy commented on GitHub (Mar 13, 2020): This should be solved with gapless playback landing in #430. For people who for some reason want the old behaviour, check out #444.
Sign in to join this conversation.
No milestone
No project
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
starred/librespot#57
No description provided.