Merge rodio player and librespot crates into MuxPlayer#53
Draft
Merge rodio player and librespot crates into MuxPlayer#53
Conversation
This crates a - mux player (to abstract away all lower player calls) - Source resolver (to resolve playback urls if missing or invalid)
Reviewer's GuideIntroduces a new core/player crate that encapsulates a generic, multiplexed player abstraction (MuxPlayer + PlayerHandler) and moves rodio-specific functionality and Tauri wiring to the backend, including new source resolution and error types, while refactoring the FFMPEG decoder error type and simplifying Bazel targets. Class diagram for new core player abstraction and rodio integrationclassDiagram
class PlayerHandler {
-MuxPlayer mux
-SourceResolver source_resolver
+new(f: SourceResolverFn) PlayerHandler
+load_song(song: Song) Result~void, PlayerError~
+play() Result~void, PlayerError~
+pause() Result~void, PlayerError~
+set_volume(volume: f32) Result~void, PlayerError~
+seek(position: f64) Result~void, PlayerError~
}
class MuxPlayer {
-Vec~Arc<Box<dyn PlayerExt>>~ players
-Arc<Box<dyn PlayerExt>> active_player
+new() MuxPlayer
+load(song: Song) Result~void, PlayerError~
+set_active_player(player: Arc<Box<dyn PlayerExt>>) void
+find_best_player(song: Song) Result~void, PlayerError~
}
class PlayerExt {
<<interface>>
+play() Result~void, PlayerError~
+pause() Result~void, PlayerError~
+stop() Result~void, PlayerError~
+set_volume(volume: f32) Result~void, PlayerError~
+seek(position: f64) Result~void, PlayerError~
+set_src(src: ValidSrc) Result~void, PlayerError~
+can_play(src: ValidSrc) bool
}
class RodioPlayer {
-Sender~RodioCommand~ tx
-JoinHandle~()~? handle
+new() RodioPlayer
+get_volume() Result~f32, PlayerError~
+play() Result~void, PlayerError~
+pause() Result~void, PlayerError~
+stop() Result~void, PlayerError~
+set_volume(volume: f32) Result~void, PlayerError~
+seek(pos: f64) Result~void, PlayerError~
+set_src(src: ValidSrc) Result~void, PlayerError~
+can_play(src: ValidSrc) bool
+set_src_internal(src: String, sink: Arc<rodio::Player>) Result~void, MoosyncError~
}
class PlayerError {
<<enum>>
NoPlayerFound(details: String)
NoSrcFound(song: Song)
PlaybackUrlResolutionFailed(error: Box<dyn Error + Send + Sync>)
InvalidSong
}
class ValidSrc {
<<enum>>
Path(path: PathBuf)
Url(url: String)
+inner(self) String
}
class SourceResolverFn {
<<type_alias>>
Fn(song: Song) -> Result~String, Box<dyn Error + Send + Sync>~
}
class SourceResolver {
-SourceResolverFn resolver
+new(resolver: SourceResolverFn) SourceResolver
+resolve_playback_url(song: Song) Result~void, PlayerError~
}
class Song {
<<proto_message>>
+song: Option~InnerSong~
+get_path() Option~String~
+get_playback_url() Option~String~
+to_string() String
}
class SongsExt {
<<trait_implementation>>
+get_title(self) Option~String~
+get_path(self) Option~String~
+get_playback_url(self) Option~String~
+to_string(self) String
}
class DecoderError {
<<enum>>
NoAudioStream
WrongStream(expected: usize, got: i32)
DurationUnavailable
Rsmpeg(error: RsmpegError)
CString(error: NulError)
AV(code: c_int)
}
class FFMPEGDecoder {
-AVFormatContextInput format_ctx
-AVCodecContext codec_ctx
-usize stream_idx
-Option~SwrContext~ swr_ctx
-Vec~u8~ current_frame
-u64 current_frame_idx
-u64 requested_seek_timestamp
+open(path: str) Result~FFMPEGDecoder, DecoderError~
+initialize_swr_context(codec_ctx: AVCodecContext) Result~Option~SwrContext~, DecoderError~
+convert_and_store_frame(frame: AVFrame) Result~void, DecoderError~
+decode_next_packet() Result~Option~AVFrame~, DecoderError~
+process_next_packet() Result~void, DecoderError~
+resync_after_seek() Result~void, DecoderError~
}
PlayerHandler --> MuxPlayer : owns
PlayerHandler --> SourceResolver : owns
MuxPlayer --> PlayerExt : uses
RodioPlayer ..|> PlayerExt
MuxPlayer --> RodioPlayer : default player
PlayerHandler --> PlayerError : error_handling
MuxPlayer --> PlayerError : error_handling
SourceResolver --> PlayerError : error_handling
MuxPlayer --> ValidSrc : selection
RodioPlayer --> ValidSrc : playback_src
SourceResolver --> SourceResolverFn : wraps
SourceResolverFn --> Song : input
Song ..> SongsExt : implements
FFMPEGDecoder --> DecoderError : error_handling
RodioPlayer --> FFMPEGDecoder : uses
DecoderError --> MoosyncError : converts
DecoderError --> SeekError : converts
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
SourceResolver::resolve_playback_urlyou always returnErr(PlayerError::InvalidSong)even after successfully resolving and setting the playback URL, which meansPlayerHandler::load_songwill never reach the secondmux.loadcall as intended; this should returnOk(())(or a more specific result) on success instead. ValidSrc::innertakes ownership ofselfandDisplayworks around that by cloning, andPathis converted withto_str().unwrap(), which can panic; consider changinginnerto take&selfand return aCow<str>orResult<String, _>so you avoid unnecessary cloning and handle non‑UTF8 paths safely.MuxPlayerstores players asVec<Arc<Box<dyn PlayerExt>>>, which adds an extra layer of indirection; you can simplify this to something likeVec<Arc<dyn PlayerExt + Send + Sync>>(or similar bounds needed by your use sites) to reduce allocation/indirection overhead and make the type easier to work with.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `SourceResolver::resolve_playback_url` you always return `Err(PlayerError::InvalidSong)` even after successfully resolving and setting the playback URL, which means `PlayerHandler::load_song` will never reach the second `mux.load` call as intended; this should return `Ok(())` (or a more specific result) on success instead.
- `ValidSrc::inner` takes ownership of `self` and `Display` works around that by cloning, and `Path` is converted with `to_str().unwrap()`, which can panic; consider changing `inner` to take `&self` and return a `Cow<str>` or `Result<String, _>` so you avoid unnecessary cloning and handle non‑UTF8 paths safely.
- `MuxPlayer` stores players as `Vec<Arc<Box<dyn PlayerExt>>>`, which adds an extra layer of indirection; you can simplify this to something like `Vec<Arc<dyn PlayerExt + Send + Sync>>` (or similar bounds needed by your use sites) to reduce allocation/indirection overhead and make the type easier to work with.
## Individual Comments
### Comment 1
<location path="core/player/src/source.rs" line_range="53-60" />
<code_context>
+ }
+
+ // This method will ignore any existing playback url and path and try to find a new one
+ pub fn resolve_playback_url(&self, song: &mut Song) -> Result<(), PlayerError> {
+ let playback_url =
+ (self.resolver)(song).map_err(|e| PlayerError::PlaybackUrlResolutionFailed(e))?;
+
+ if let Some(inner_song) = song.song.as_mut() {
+ inner_song.playback_url = Some(playback_url);
+ };
+ Err(PlayerError::InvalidSong)
+ }
+}
</code_context>
<issue_to_address>
**issue (bug_risk):** resolve_playback_url always returns an error, even after successfully setting playback_url.
The function currently sets `playback_url` (when `song.song` is `Some`) but then always returns `Err(PlayerError::InvalidSong)`, so `PlayerHandler::load_song` will always treat resolution as a failure.
This should return `Ok(())` when a URL is successfully set, and only return `InvalidSong` when `song.song` is `None` (or other validation fails). For example:
```rust
pub fn resolve_playback_url(&self, song: &mut Song) -> Result<(), PlayerError> {
let playback_url = (self.resolver)(song)
.map_err(PlayerError::PlaybackUrlResolutionFailed)?;
if let Some(inner_song) = song.song.as_mut() {
inner_song.playback_url = Some(playback_url);
Ok(())
} else {
Err(PlayerError::InvalidSong)
}
}
```
</issue_to_address>
### Comment 2
<location path="core/player/src/source.rs" line_range="21-24" />
<code_context>
+}
+
+impl ValidSrc {
+ pub(crate) fn inner(self) -> String {
+ match self {
+ ValidSrc::Path(path) => path.to_str().unwrap().to_string(),
+ ValidSrc::Url(url) => url,
+ }
+ }
</code_context>
<issue_to_address>
**suggestion (bug_risk):** ValidSrc::inner consumes self, forcing clones in Display and panicking on non-UTF8 paths.
Because `inner` takes ownership, the `Display` impl has to call `self.clone().inner()`, which adds unnecessary cloning for a read-only operation. In addition, `PathBuf::to_str().unwrap()` can panic on non-UTF8 paths.
You can avoid both issues by borrowing and using a lossless conversion:
```rust
pub(crate) fn inner(&self) -> String {
match self {
ValidSrc::Path(path) => path.to_string_lossy().into_owned(),
ValidSrc::Url(url) => url.clone(),
}
}
impl Display for ValidSrc {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
write!(f, "{}", self.inner())
}
}
```
Suggested implementation:
```rust
impl Display for ValidSrc {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
write!(f, "{}", self.inner())
}
}
impl ValidSrc {
```
```rust
impl Display for ValidSrc {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
write!(f, "{}", self.inner())
}
}
impl ValidSrc {
pub(crate) fn inner(&self) -> String {
match self {
ValidSrc::Path(path) => path.to_string_lossy().into_owned(),
ValidSrc::Url(url) => url.clone(),
}
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+53
to
+60
| pub fn resolve_playback_url(&self, song: &mut Song) -> Result<(), PlayerError> { | ||
| let playback_url = | ||
| (self.resolver)(song).map_err(|e| PlayerError::PlaybackUrlResolutionFailed(e))?; | ||
|
|
||
| if let Some(inner_song) = song.song.as_mut() { | ||
| inner_song.playback_url = Some(playback_url); | ||
| }; | ||
| Err(PlayerError::InvalidSong) |
There was a problem hiding this comment.
issue (bug_risk): resolve_playback_url always returns an error, even after successfully setting playback_url.
The function currently sets playback_url (when song.song is Some) but then always returns Err(PlayerError::InvalidSong), so PlayerHandler::load_song will always treat resolution as a failure.
This should return Ok(()) when a URL is successfully set, and only return InvalidSong when song.song is None (or other validation fails). For example:
pub fn resolve_playback_url(&self, song: &mut Song) -> Result<(), PlayerError> {
let playback_url = (self.resolver)(song)
.map_err(PlayerError::PlaybackUrlResolutionFailed)?;
if let Some(inner_song) = song.song.as_mut() {
inner_song.playback_url = Some(playback_url);
Ok(())
} else {
Err(PlayerError::InvalidSong)
}
}
Comment on lines
+21
to
+24
| pub(crate) fn inner(self) -> String { | ||
| match self { | ||
| ValidSrc::Path(path) => path.to_str().unwrap().to_string(), | ||
| ValidSrc::Url(url) => url, |
There was a problem hiding this comment.
suggestion (bug_risk): ValidSrc::inner consumes self, forcing clones in Display and panicking on non-UTF8 paths.
Because inner takes ownership, the Display impl has to call self.clone().inner(), which adds unnecessary cloning for a read-only operation. In addition, PathBuf::to_str().unwrap() can panic on non-UTF8 paths.
You can avoid both issues by borrowing and using a lossless conversion:
pub(crate) fn inner(&self) -> String {
match self {
ValidSrc::Path(path) => path.to_string_lossy().into_owned(),
ValidSrc::Url(url) => url.clone(),
}
}
impl Display for ValidSrc {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
write!(f, "{}", self.inner())
}
}Suggested implementation:
impl Display for ValidSrc {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
write!(f, "{}", self.inner())
}
}
impl ValidSrc {impl Display for ValidSrc {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
write!(f, "{}", self.inner())
}
}
impl ValidSrc {
pub(crate) fn inner(&self) -> String {
match self {
ValidSrc::Path(path) => path.to_string_lossy().into_owned(),
ValidSrc::Url(url) => url.clone(),
}
}
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Introduce a new backend-centric player module that multiplexes playback between implementations, and migrate the existing Rodio-based playback logic and wiring to this shared core player.
New Features:
Enhancements:
Build:
Tests:
Chores: