[PR #430] [MERGED] Gapless play and improved notifications #921

New issue

Closed

opened 2026-02-27 20:00:28 +03:00 by kerem · 0 comments

kerem commented

2026-02-27 20:00:28 +03:00

Owner

📋 Pull Request Information

Original PR: https://github.com/librespot-org/librespot/pull/430
Author: @kaymes
Created: 2/2/2020
Status: ✅ Merged
Merged: 2/27/2020
Merged by: @sashahilton00

Base: dev ← Head: gapless_play

📝 Commits (10+)

5784b46 Prepare for gapless play.
9eef690 Some clean up
8756341 Remove debug message
349e182 Smarter handling of preloading and loading of tracks that are already loaded.
6fed8d0 Make preloading work.
981b76b Keep the sink open
499824a rust fmt
ead794f Correct notifications when loading the same track again.
18d1181 Clean up some code
3f111a9 Suppress sending loading state to Spotify unless we actually need to load a track.

📊 Changes

7 files changed (+1470 additions, -463 deletions)

View changed files

📝 audio/src/fetch.rs (+9 -0)
📝 connect/src/spirc.rs (+370 -123)
📝 examples/play.rs (+4 -2)
📝 metadata/src/lib.rs (+3 -0)
📝 playback/src/player.rs (+1067 -324)
📝 src/main.rs (+12 -10)
📝 src/player_event_handler.rs (+5 -4)

📄 Description

This PR allows librespot to perform gapless playback (issue #21). As far as I can see it's working well, but given the extent of the changes, it requires a good amount of testing.

Features of this PR so far:

Upgrade of plumbing between spirc and player.
Gapless playback: pre-loading and keeping the sink open.
Improve notifications to Spotify: while loading a track, the status kPlayStatusLoading is sent and while playing, the correct playback position is transmitted (after loading, seek and buffer under runs).
More and more detailed player events are produced.

The reason for having "additional features" in this one PR is that the necessary changes to the plumbing between the player and spirc made them come out naturally.

And now to the more technical details:
One of the first realisations was, that for gapless play there has to be better comunication between the player and spirc. Initially I thought about beefing up the future that notifies spirc about the end of a track, but then I realised, that this would kind of duplicate the player events channel that is already there. Thus, I decided to beef up the player events channel instead and get rid of the existing future. The player can now feed multiple player event channels and spirc takes one of them to listen to player events. This allows spirc to know about when to preload the next track. As a by product, it also allows spirc to have more detailed information about the player's status, in particular the current playback position.

To be able to preload tracks, loading of tracks has to happen in a non-blocking way. Initially I started to convert PlayerInternal::load_track() into a proper future. However, I eventually realised, that the call to seek() is blocking and can't be converted to proper async code easily. Thus load_track() now spawns a new (short lived) thread to load a track and put the result onto a one-shot channel. Thus, we effectively get a future that loads the next track.

I turned PlayerInternal into a future so run() is now replaced by poll(). But it still contains blocking code as before. Thus, it must not be spawned onto the tokio reactor but instead run via wait() in a dedicated thread. That's the same as before, the thread just does player.wait() instead of player.run(). The reason for this is to give PlayerInternal's main loop a context to poll multiple channels and futures as needed without having to resort to timed cycles and similar crutches.

The code is set up such that the player emits a TimeToPreloadNextTrack event when playback is 30 seconds or less to the end of the track AND the remainder of the track has been downloaded. This is the signal to SpircTask to examine the playlist (using preview_next_track()) and call player.preload_track() to have the player start with the download.

In the old code, spirc uses the protobuf frame it communicates with the server with to store its own state machine (e.g. store the playing status). This was no longer sufficient. E.g. when the status is kPlayStatusLoading, spirc must know whether it is loading to play or loading to pause. Thus, I gave SpircTask its own state (SpircPlayStatus) which allows it to accurately track what is going on.

Next steps:
The added events also allow to expose more information to external programs through run_program_on_events. E.g. volume changes now emit events on the channel which would make fixing #410 trivial.

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/librespot-org/librespot/pull/430 **Author:** [@kaymes](https://github.com/kaymes) **Created:** 2/2/2020 **Status:** ✅ Merged **Merged:** 2/27/2020 **Merged by:** [@sashahilton00](https://github.com/sashahilton00) **Base:** `dev` ← **Head:** `gapless_play` --- ### 📝 Commits (10+) - [`5784b46`](https://github.com/librespot-org/librespot/commit/5784b4652cc082536c2357e2f9da8096b3b488fe) Prepare for gapless play. - [`9eef690`](https://github.com/librespot-org/librespot/commit/9eef690a22d9943321aa06e19968336d243b4241) Some clean up - [`8756341`](https://github.com/librespot-org/librespot/commit/87563412013304ea88f4b356f148d78a1f86ffae) Remove debug message - [`349e182`](https://github.com/librespot-org/librespot/commit/349e182d419bc3a7337be9c485b6f15fe42c25ac) Smarter handling of preloading and loading of tracks that are already loaded. - [`6fed8d0`](https://github.com/librespot-org/librespot/commit/6fed8d0413ddb82fde1f6f61c83a201d83f454bb) Make preloading work. - [`981b76b`](https://github.com/librespot-org/librespot/commit/981b76bace9f2df6db391c677bce34cadad70653) Keep the sink open - [`499824a`](https://github.com/librespot-org/librespot/commit/499824a6ba1bbc6963d711606e96ed53909d42aa) rust fmt - [`ead794f`](https://github.com/librespot-org/librespot/commit/ead794f4fd4f06d87b2c2037aa1f8a31f67aa4db) Correct notifications when loading the same track again. - [`18d1181`](https://github.com/librespot-org/librespot/commit/18d1181bf511e7fd7bd3ab1f04613cc4c4b28d2d) Clean up some code - [`3f111a9`](https://github.com/librespot-org/librespot/commit/3f111a97785c1c1d5a77d833a98e5fb085a85399) Suppress sending loading state to Spotify unless we actually need to load a track. ### 📊 Changes **7 files changed** (+1470 additions, -463 deletions) <details> <summary>View changed files</summary> 📝 `audio/src/fetch.rs` (+9 -0) 📝 `connect/src/spirc.rs` (+370 -123) 📝 `examples/play.rs` (+4 -2) 📝 `metadata/src/lib.rs` (+3 -0) 📝 `playback/src/player.rs` (+1067 -324) 📝 `src/main.rs` (+12 -10) 📝 `src/player_event_handler.rs` (+5 -4) </details> ### 📄 Description This PR allows librespot to perform gapless playback (issue #21). As far as I can see it's working well, but given the extent of the changes, it requires a good amount of testing. **Features of this PR so far:** - Upgrade of plumbing between spirc and player. - Gapless playback: pre-loading and keeping the sink open. - Improve notifications to Spotify: while loading a track, the status kPlayStatusLoading is sent and while playing, the correct playback position is transmitted (after loading, seek and buffer under runs). - More and more detailed player events are produced. The reason for having "additional features" in this one PR is that the necessary changes to the plumbing between the player and spirc made them come out naturally. **And now to the more technical details:** One of the first realisations was, that for gapless play there has to be better comunication between the player and spirc. Initially I thought about beefing up the future that notifies spirc about the end of a track, but then I realised, that this would kind of duplicate the player events channel that is already there. Thus, I decided to beef up the player events channel instead and get rid of the existing future. The player can now feed multiple player event channels and spirc takes one of them to listen to player events. This allows spirc to know about when to preload the next track. As a by product, it also allows spirc to have more detailed information about the player's status, in particular the current playback position. To be able to preload tracks, loading of tracks has to happen in a non-blocking way. Initially I started to convert PlayerInternal::load_track() into a proper future. However, I eventually realised, that the call to seek() is blocking and can't be converted to proper async code easily. Thus load_track() now spawns a new (short lived) thread to load a track and put the result onto a one-shot channel. Thus, we effectively get a future that loads the next track. I turned PlayerInternal into a future so run() is now replaced by poll(). But it still contains blocking code as before. Thus, it must not be spawned onto the tokio reactor but instead run via wait() in a dedicated thread. That's the same as before, the thread just does player.wait() instead of player.run(). The reason for this is to give PlayerInternal's main loop a context to poll multiple channels and futures as needed without having to resort to timed cycles and similar crutches. The code is set up such that the player emits a TimeToPreloadNextTrack event when playback is 30 seconds or less to the end of the track AND the remainder of the track has been downloaded. This is the signal to SpircTask to examine the playlist (using preview_next_track()) and call player.preload_track() to have the player start with the download. In the old code, spirc uses the protobuf frame it communicates with the server with to store its own state machine (e.g. store the playing status). This was no longer sufficient. E.g. when the status is kPlayStatusLoading, spirc must know whether it is loading to play or loading to pause. Thus, I gave SpircTask its own state (SpircPlayStatus) which allows it to accurately track what is going on. **Next steps:** The added events also allow to expose more information to external programs through run_program_on_events. E.g. volume changes now emit events on the channel which would make fixing #410 trivial. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

kerem

2026-02-27 20:00:28 +03:00

closed this issue
added the
pull-request
label

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#921

No description provided.

Rows
Columns