-
Notifications
You must be signed in to change notification settings - Fork 756
feat: Basic local file support #1595
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev
Are you sure you want to change the base?
feat: Basic local file support #1595
Conversation
9f8be59 to
d0efe21
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the late review. Overall looks good to me, for the audio part I would like to have @roderickvd opinion about it (mainly symphonia and the player itself). From my point of view it seems to do the job but I might miss some crucial details.
| Some(StandardTagKey::AlbumArtist) => metadata.album_artists = value.clone(), | ||
| Some(StandardTagKey::Album) => metadata.album = value.clone(), | ||
| Some(StandardTagKey::TrackNumber) => { | ||
| metadata.number = value.parse::<u32>().unwrap_or_default() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That'll set "0" when an empty string or something malformed. Make it None instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair point, made the numeric fields optional and set these to None if parsing fails. Are you fine with the string fields being set to an empty string if those aren't found?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer for them to also be an Option. When I think of media players, I think they do differentiate between "field not set" and "field empty".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, changed all fields to Option.
On that note, in player_event_handler I have been just using unwrap_or_default to deal with the now optional fields when we start playing a local file. But it just occurred to me there is technically a difference between having an environment variable as an empty string versus not setting it (I speculate in C land it equates to getenv returning "\0" versus a null pointer). Should we be checking and only setting the variable for Some or is that inconsequential in this case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that in this case it’s inconsequential. Audio distributions that use it like moOde check for empty strings. That’s the main purpose, I think: to display the active track.
|
|
||
| pub fn normalisation_data(&mut self) -> Option<NormalisationData> { | ||
| let mut metadata = self.format.metadata(); | ||
| pub(crate) fn new_with_probe<R>(src: R, extension: Option<&str>) -> DecoderResult<Self> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm gonna merge #1589 and then I think it'd be good to merge new and new_with_probe for less redundant code. The current / to be merged new is quite specific to open a reader for a certain file format, but it doesn't need to be. It may as well probe for all supported formats and then use Symphonia's hint system to make it try the right demuxer and decoder faster.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I've taken a crack at merging them and it seems to work from smoke testing with both regular tracks and local tracks. The only thing I am not sure about is
let mss_opts = MediaSourceStreamOptions {
buffer_len: librespot_audio::AudioFetchParams::get().minimum_download_size,
};The download size is not strictly relevant to local files, but I assume this is just basically an arbitrary buffer size and it's fine to set it to this even in the local file case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From top of mind I think it needs to be 32 kB or higher (or was it 64 kB?) and a power of two. And from further memory - but it's a bit vague - I remember that this is enough for Symphonia to seek effectively, and much more doesn't make it much better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it just so happens to be the exact same value as the default, funnily enough:
librespot/audio/src/fetch/mod.rs
Line 103 in 961e2f4
| let minimum_download_size = 64 * 1024; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I did that to have a bit more efficient HTTP block sizes.
Spotify Connect does not allow you to move playback of a local file to the librespot device as it says that it "can't play this track". Note that this is slightly inconsistent as Spotify allows you to switch to a local file if librespot is already playing a non-local file, which currently fails with an error. However, it is possible for the desktop and iOS client to accept playback of local files. In looking at the PUT request sent to `connect-state/v1/devices/<id>` from the iOS client, it can be seen that it includes `audio/local` as an entry in the `supported_types` capability field. This commit introduces this field to the capabilities that librespot sends. For now, it is a complete lie as we do not support local file playback, but it will make the ongoing development of this feature easier, as we will not have to queue up a non-local track and attempt to switch to a local one. Testing shows that with this flag the "can't play this track" message disappears and allows librespot to (attempt) to play a local file before erroring out.
2888930 to
961e2f4
Compare
|
Wait, if the tracks that will be found in the path added by |
Each device will have its own Local Files playlist based on what folders have been configured. I don't think we can get a list of sources from Spotify since a connect play request could come from an entirely different device. Even if there is file path information in the request, it may not be valid on the current device. In terms of live-updating the local file content, maybe we could use something like https://github.com/notify-rs/notify to update the lookup in the background, or only update it if we get a request to play a file we fail to find a path for. |
|
@roderickvd @photovoltex thanks for all the reviews so far, I was just wondering if you had any more feedback? I may make a start on adding some of the extra functionality in subsequent PRs, namely lifting the sample rate restriction and making the local files playlist work. Would you like me to create issues for these tasks? |
Discussed in #1525. Adds a basic level of support for playing back local files from a connect device. The
librespotbinary must be launched with--local-file-dir/-lflags specifying search paths for local files. If a request is received to play a local file, and a file match is made based on the file's artist, album, track title, and duration, thenlibrespotwill now be able to play it.This support has the following caveats:
spotify:local-fileswhich currently causes an error in the context handling code, aslibrespotattempts to fetch it from the API and appears to get a 404. I tried to make this work, but it feels like an abstraction for context would be beneficial so we could generate context from the filesystem (currently I think we use the proto models which are obviously tightly coupled to the Spotify API). Also, I do not understand enough about the context handling code to do a good job 😅