Skip to content

Added Support for Local Files#136

Closed
braydnm wants to merge 4 commits intojpochyla:masterfrom
braydnm:localfiles
Closed

Added Support for Local Files#136
braydnm wants to merge 4 commits intojpochyla:masterfrom
braydnm:localfiles

Conversation

@braydnm
Copy link
Copy Markdown

@braydnm braydnm commented Aug 22, 2021

Parsing of the Spotify configuration for local files and the JSON returned by the web API.

Changes

Local files imported by Spotify is stored in a custom binary format at <Spotify Config Dir>/Users/<username>-user/local-files.bnk. When playlist tracks are attempted to be loaded instead of discarding any local tracks attempt to find a local track in the local-files.bnk and if a matching title, artist, album, etc. are found then return a properly formatted Track structure

TODO:

  1. Test on platforms other than Linux
  2. Attempting to play a local files throws unimplemented
  3. Figure out how to decode and play formats other than Vorbis
  4. Continue reverse engineering the local-files.bnk so that local files can be added through Psst
  5. Maybe look into syncing local files with other devices on the network?

Parsing of the Spotify configuration for local files and the JSON returned by the web API
Copy link
Copy Markdown
Owner

@jpochyla jpochyla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, nice! Added some nitpicks. I'd like to do some of my own modifications later anyway, so if you don't feel like it, just leave it to me for until we're merging.

Comment thread psst-gui/src/data/config.rs Outdated
}

pub fn get_username(&self) -> Option<String> {
match &self.credentials {
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

map would be better here I think?

Comment thread psst-gui/src/main.rs Outdated
.init();

let config = Config::load().unwrap_or_default();
let username = config.get_username();
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This won't really work the first time user launches the app. Can we maybe inject the username (or the LocalTrackManager) into WebApi a bit later?

Comment thread psst-gui/src/webapi/client.rs Outdated
local: match username {
Some(u) => LocalTrackManager::new(u),
_ => None
}
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again map I personally find preferable here, but maybe we should re-think how the manager is initialized, see above.

// https://developer.spotify.com/documentation/web-api/reference/#endpoint-get-playlist
pub fn get_playlist(&self, id: &str) -> Result<Playlist, Error> {
let request = self.get(format!("v1/playlists/{}", id))?;
let request = self.get(format!("v1/me/playlists/{}", id))?;
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the endpoint really incorrect?

Comment thread psst-gui/src/webapi/local.rs Outdated
* If the varint size is 0 this is a null string
*
* The end of a trailer for a given track is signified by 0x78, 0x04 from what I can tell
*/
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really appreciate the docs!

Comment thread psst-gui/src/webapi/local.rs Outdated
Ok(s) => Ok(s.to_string()),
// TODO: This can definitely be done better
Err(_) => Err(io::Error::from(io::ErrorKind::Other))
}
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe using String::from_utf8 directly, with map_err, would be better?

Comment thread psst-gui/src/webapi/local.rs Outdated
let file_path = match Config::spotify_local_files(username) {
Some(p) => p,
_ => return None
};
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

? should cover this I think?

Comment thread psst-gui/src/data/track.rs Outdated
pub explicit: bool,
pub is_local: bool,
#[serde(skip_deserializing)]
pub local_path: Option<Arc<String>>,
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Arc<str> would be preferable.

Small changes to follow more Rustisms and fixed a bug where the username will not be known on the first run
@braydnm
Copy link
Copy Markdown
Author

braydnm commented Aug 22, 2021

I committed most of the changes you suggested. The problem I was running into was injecting the username into the WebApi after log in as it becomes an Arc. I ended up just making the LocalTrackManager a singleton similar to WebApi. I don't know if this is the best way to do it and there may be a better way with Rust features I'm not familiar with. Feel free to make any changes you see fit.

@jpochyla
Copy link
Copy Markdown
Owner

Hi @sleepychaser, I've made some more changes (mostly some style nits, functionality should be identical I think). It works on macOS. The playback is next!

@jpochyla
Copy link
Copy Markdown
Owner

jpochyla commented Aug 29, 2021

Re. multiple codecs support: this is definitely needed (not even all Spotify tracks are in Vorbis). Options:

  1. Get rid of minivorbis, use Symphonia (which seems excellent) for the decoding, and miniaudio just for the output.
  2. Wait until miniaudio hits a stable version with the highlevel API and vorbis codec included.
  3. Keep minivorbis and use the MP3 decoder from miniaudio -- I don't like this much.

It's sad the the rust audio output is in such a bad state (rodio can not even keep the buffers full in debug mode, cpal has >100 issues open and maintainers don't seem to care, etc.), so it seems like we're stuck with a C library for this.

@jpochyla
Copy link
Copy Markdown
Owner

jpochyla commented Dec 1, 2021

Merged to master, sorry about the delay :) Playback is still not supported, but now that we have Symphonia, it wouldn't be that hard to do.

@jpochyla jpochyla closed this Dec 1, 2021
@braydnm braydnm deleted the localfiles branch May 4, 2022 19:35
@Kyiro
Copy link
Copy Markdown

Kyiro commented Jun 29, 2022

Will playback ever be supported?

@jpochyla jpochyla mentioned this pull request Aug 15, 2022
@jpochyla
Copy link
Copy Markdown
Owner

Hi @Kyiro, I've opened an issue for this. No ETA, though :(

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants