starred/librespot

Fork 0

mirror of https://github.com/librespot-org/librespot.git synced 2026-04-27 08:15:50 +03:00

[GH-ISSUE #584] [Tokio migration] Sendness of Sinks #371

New issue

Closed

opened 2026-02-27 19:30:16 +03:00 by kerem · 1 comment

kerem commented

2026-02-27 19:30:16 +03:00

Owner

Originally created by @Johannesd3 on GitHub (Jan 26, 2021).
Original GitHub issue: https://github.com/librespot-org/librespot/issues/584

In the current version, Sinks are allowed not to be Send. While this is currently only the case for Rodio, it seems to have a reason. Therefore, the PlayerInternal future can't be executed by Tokio but must run in its own thread. Previously this was possible with Future::wait, but apparently they removed it.

Perhaps it would be the best to rewrite PlayerInternal in a synchronous manner. It's confusing anyway to have a blocking future. There are still a few places where an interaction with async functions is needed. I don't know how to solve this best...

Originally created by @Johannesd3 on GitHub (Jan 26, 2021). Original GitHub issue: https://github.com/librespot-org/librespot/issues/584 In the current version, `Sink`s are allowed not to be `Send`. While this is currently only the case for Rodio, it seems to have a [reason](https://github.com/RustAudio/cpal/blob/master/src/platform/mod.rs#L63-L65). Therefore, the `PlayerInternal` future can't be executed by Tokio but must run in its own thread. Previously this was possible with `Future::wait`, but apparently they removed it. Perhaps it would be the best to rewrite PlayerInternal in a synchronous manner. It's confusing anyway to have a blocking future. There are still a few places where an interaction with async functions is needed. I don't know how to solve this best...

kerem closed this issue

2026-02-27 19:30:16 +03:00

kerem commented

2026-02-27 19:30:16 +03:00

Author

Owner

@Johannesd3 commented on GitHub (Feb 3, 2021):

First of all, there is a solution to make the RodioSink Send: a RodioSink consists of a rodio::Sink and a rodio::OutputStream. The Sink is Send, but the OutputStream isn't. The OutputStream is only needed at creation of the Sink, but must not be dropped while the sound is playing.

The solution: spawn a thread, create OutputStream and Sink in there, send the Sink via oneshot back. Next, the thread waits for another oneshot in the other direction which is fired when the RodioSink is dropped. After receiving this, the OutputStream gets dropped and the background thread ends. Since the thread is sleeping most of the time, it wouldn't impact performance.

Nevertheless, I think the current player doesn't work well with Rodio. Currently, the player stuffs the audio as fast as possible into the sink. Rodio works different, it wants an iterator of samples (i16) as Source and polls this iterator from a background thread. I want to suggest a rewrite of the playback crate that is better compatible with rodio:

Introduce some traits:

Track: Iterator<Item = Vec<i16>>, which represents a loaded track. As iterator it yields the decoded audio, and it has some other functions, e.g. seek or set_stream_mode.
Queue, which has a function load_next_track returning a Track. There might also be functions to trigger preloading, to skip to the previous track, etc.
Sink, which can be played, paused and stopped, e.g. a rodio sink or a stdout/pipe sink.

The struct PlayerInternal, which was previously a Future, shall become an Iterator of i16s or Vec<i16>s instead. It contains a Queue and the currently playing Track. The audio it yields as iterator comes from the current Track. If a track is over, it loads the next track from its Queue. It is able to receive commands and emit events via channels. Seeking and skipping, but not playing and pausing. It also keeps track of the time: If the playback is behind, an event with the corrected time is sent.

A Sink is now created by passing a PlayerInternal to it. If it is playing, the Sink "polls" the PlayerInternal, or does nothing if it is paused. So the PlayerInternal is responsible for what is played, and the Sink is responsible for when it is played.

Now, a Player is just a small wrapper containing a Sink and a sender for PlayerInternal's commands. It supports already the usual commands play, pause, seek etc., but there are still some drawbacks regarding events, since the player has no own thread/task. A pause event wouldn't be possible, since the PlayerInternal wouldn't get polled if the Sink is paused. It also wouldn't be possible for the Player to stop the Sink when the PlayerInternal signals that the Queue is over, which should be done to avoid waste of resources.

So the next step would be a wrapper around Player that spawns its own thread/task, has channels to receive commands and send events, and handles the events of PlayerInternal in the right way. It could either be async or blocking.

This would be totally independent of Librespot, so this could happen in its own crate. Only the Track and Queue traits have to be implemented by librespot.

However, I'm not sure if this is better or simpler than the current solution, so what do you think of this idea?

@Johannesd3 commented on GitHub (Feb 3, 2021): First of all, there is a solution to make the `RodioSink` `Send`: a `RodioSink` consists of a `rodio::Sink` and a `rodio::OutputStream`. The `Sink` is `Send`, but the `OutputStream` isn't. The `OutputStream` is only needed at creation of the `Sink`, but must not be dropped while the sound is playing. The solution: spawn a thread, create `OutputStream` and `Sink` in there, send the `Sink` via `oneshot` back. Next, the thread waits for another `oneshot` in the other direction which is fired when the `RodioSink` is dropped. After receiving this, the `OutputStream` gets dropped and the background thread ends. Since the thread is sleeping most of the time, it wouldn't impact performance. *** Nevertheless, I think the current player doesn't work well with Rodio. Currently, the player stuffs the audio as fast as possible into the sink. Rodio works different, it wants an iterator of samples (`i16`) as `Source` and polls this iterator from a background thread. I want to suggest a rewrite of the playback crate that is better compatible with rodio: Introduce some traits: * `Track: Iterator<Item = Vec<i16>>`, which represents a loaded track. As iterator it yields the decoded audio, and it has some other functions, e.g. `seek` or `set_stream_mode`. * `Queue`, which has a function `load_next_track` returning a `Track`. There might also be functions to trigger preloading, to skip to the previous track, etc. * `Sink`, which can be `play`ed, `pause`d and `stop`ped, e.g. a rodio sink or a stdout/pipe sink. The `struct PlayerInternal`, which was previously a `Future`, shall become an `Iterator` of `i16`s or `Vec<i16>`s instead. It contains a `Queue` and the currently playing `Track`. The audio it yields as iterator comes from the current `Track`. If a track is over, it loads the next track from its `Queue`. It is able to receive commands and emit events via channels. Seeking and skipping, but not playing and pausing. It also keeps track of the time: If the playback is behind, an event with the corrected time is sent. A `Sink` is now created by passing a `PlayerInternal` to it. If it is playing, the `Sink` "polls" the `PlayerInternal`, or does nothing if it is paused. So the `PlayerInternal` is responsible for what is played, and the `Sink` is responsible for when it is played. Now, a `Player` is just a small wrapper containing a `Sink` and a sender for `PlayerInternal`'s commands. It supports already the usual commands `play`, `pause`, `seek` etc., but there are still some drawbacks regarding events, since the player has no own thread/task. A pause event wouldn't be possible, since the `PlayerInternal` wouldn't get polled if the `Sink` is paused. It also wouldn't be possible for the `Player` to stop the `Sink` when the `PlayerInternal` signals that the `Queue` is over, which should be done to avoid waste of resources. So the next step would be a wrapper around `Player` that spawns its own thread/task, has channels to receive commands and send events, and handles the events of `PlayerInternal` in the right way. It could either be async or blocking. This would be totally independent of Librespot, so this could happen in its own crate. Only the `Track` and `Queue` traits have to be implemented by librespot. However, I'm not sure if this is better or simpler than the current solution, so what do you think of this idea?

kerem referenced this issue

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

[PR #371] [MERGED] Fixed nightly build conflict with core #890

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

No description provided.

Rows
Columns