From e51f475a00fac40ebecfc45ee9190335895dbe9f Mon Sep 17 00:00:00 2001 From: Roderick van Domburg Date: Wed, 29 Dec 2021 22:18:38 +0100 Subject: [PATCH] Further initial loading improvements This should fix remaining cases of a client connecting, and failing to start playback from *beyond* the beginning when `librespot` is still loading that track. This undoes the `suppress_loading_status` workaround from #430, under the assumption that the race condition reported there has since been fixed on Spotify's end. --- Cargo.toml | 2 +- audio/src/fetch/receive.rs | 3 +- connect/src/spirc.rs | 67 +++++++++++++++++--------------------- playback/src/player.rs | 20 +++++------- 4 files changed, 42 insertions(+), 50 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 5a501ef5..3df50606 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -58,7 +58,7 @@ hyper = "0.14" log = "0.4" rpassword = "5.0" thiserror = "1.0" -tokio = { version = "1", features = ["rt", "rt-multi-thread", "macros", "signal", "sync", "parking_lot", "process"] } +tokio = { version = "1", features = ["rt", "macros", "signal", "sync", "parking_lot", "process"] } url = "2.2" sha-1 = "0.9" diff --git a/audio/src/fetch/receive.rs b/audio/src/fetch/receive.rs index e04c58d2..270a62c8 100644 --- a/audio/src/fetch/receive.rs +++ b/audio/src/fetch/receive.rs @@ -427,7 +427,8 @@ pub(super) async fn audio_file_fetch( } None => break, } - } + }, + else => (), } if fetch.get_download_strategy() == DownloadStrategy::Streaming() { diff --git a/connect/src/spirc.rs b/connect/src/spirc.rs index 144b9f24..d6692b51 100644 --- a/connect/src/spirc.rs +++ b/connect/src/spirc.rs @@ -550,7 +550,7 @@ impl SpircTask { SpircCommand::Play => { if active { self.handle_play(); - self.notify(None, true) + self.notify(None) } else { CommandSender::new(self, MessageType::kMessageTypePlay).send() } @@ -558,7 +558,7 @@ impl SpircTask { SpircCommand::PlayPause => { if active { self.handle_play_pause(); - self.notify(None, true) + self.notify(None) } else { CommandSender::new(self, MessageType::kMessageTypePlayPause).send() } @@ -566,7 +566,7 @@ impl SpircTask { SpircCommand::Pause => { if active { self.handle_pause(); - self.notify(None, true) + self.notify(None) } else { CommandSender::new(self, MessageType::kMessageTypePause).send() } @@ -574,7 +574,7 @@ impl SpircTask { SpircCommand::Prev => { if active { self.handle_prev(); - self.notify(None, true) + self.notify(None) } else { CommandSender::new(self, MessageType::kMessageTypePrev).send() } @@ -582,7 +582,7 @@ impl SpircTask { SpircCommand::Next => { if active { self.handle_next(); - self.notify(None, true) + self.notify(None) } else { CommandSender::new(self, MessageType::kMessageTypeNext).send() } @@ -590,7 +590,7 @@ impl SpircTask { SpircCommand::VolumeUp => { if active { self.handle_volume_up(); - self.notify(None, true) + self.notify(None) } else { CommandSender::new(self, MessageType::kMessageTypeVolumeUp).send() } @@ -598,7 +598,7 @@ impl SpircTask { SpircCommand::VolumeDown => { if active { self.handle_volume_down(); - self.notify(None, true) + self.notify(None) } else { CommandSender::new(self, MessageType::kMessageTypeVolumeDown).send() } @@ -629,7 +629,7 @@ impl SpircTask { PlayerEvent::Loading { .. } => { trace!("==> kPlayStatusLoading"); self.state.set_status(PlayStatus::kPlayStatusLoading); - self.notify(None, false) + self.notify(None) } PlayerEvent::Playing { position_ms, .. } => { trace!("==> kPlayStatusPlay"); @@ -642,7 +642,7 @@ impl SpircTask { if (*nominal_start_time - new_nominal_start_time).abs() > 100 { *nominal_start_time = new_nominal_start_time; self.update_state_position(position_ms); - self.notify(None, true) + self.notify(None) } else { Ok(()) } @@ -655,7 +655,7 @@ impl SpircTask { nominal_start_time: new_nominal_start_time, preloading_of_next_track_triggered: false, }; - self.notify(None, true) + self.notify(None) } _ => Ok(()), } @@ -673,7 +673,7 @@ impl SpircTask { if *position_ms != new_position_ms { *position_ms = new_position_ms; self.update_state_position(new_position_ms); - self.notify(None, true) + self.notify(None) } else { Ok(()) } @@ -686,7 +686,7 @@ impl SpircTask { position_ms: new_position_ms, preloading_of_next_track_triggered: false, }; - self.notify(None, true) + self.notify(None) } _ => Ok(()), } @@ -699,7 +699,7 @@ impl SpircTask { warn!("The player has stopped unexpectedly."); self.state.set_status(PlayStatus::kPlayStatusStop); self.play_status = SpircPlayStatus::Stopped; - self.notify(None, true) + self.notify(None) } } } @@ -788,7 +788,7 @@ impl SpircTask { } match update.get_typ() { - MessageType::kMessageTypeHello => self.notify(Some(ident), true), + MessageType::kMessageTypeHello => self.notify(Some(ident)), MessageType::kMessageTypeLoad => { if !self.device.get_is_active() { @@ -810,47 +810,47 @@ impl SpircTask { self.play_status = SpircPlayStatus::Stopped; } - self.notify(None, true) + self.notify(None) } MessageType::kMessageTypePlay => { self.handle_play(); - self.notify(None, true) + self.notify(None) } MessageType::kMessageTypePlayPause => { self.handle_play_pause(); - self.notify(None, true) + self.notify(None) } MessageType::kMessageTypePause => { self.handle_pause(); - self.notify(None, true) + self.notify(None) } MessageType::kMessageTypeNext => { self.handle_next(); - self.notify(None, true) + self.notify(None) } MessageType::kMessageTypePrev => { self.handle_prev(); - self.notify(None, true) + self.notify(None) } MessageType::kMessageTypeVolumeUp => { self.handle_volume_up(); - self.notify(None, true) + self.notify(None) } MessageType::kMessageTypeVolumeDown => { self.handle_volume_down(); - self.notify(None, true) + self.notify(None) } MessageType::kMessageTypeRepeat => { self.state.set_repeat(update.get_state().get_repeat()); - self.notify(None, true) + self.notify(None) } MessageType::kMessageTypeShuffle => { @@ -870,17 +870,16 @@ impl SpircTask { let context = self.state.get_context_uri(); debug!("{:?}", context); } - self.notify(None, true) + self.notify(None) } MessageType::kMessageTypeSeek => { self.handle_seek(update.get_position()); - self.notify(None, true) + self.notify(None) } MessageType::kMessageTypeReplace => { self.update_tracks(&update); - self.notify(None, true)?; if let SpircPlayStatus::Playing { preloading_of_next_track_triggered, @@ -898,12 +897,13 @@ impl SpircTask { } } } - Ok(()) + + self.notify(None) } MessageType::kMessageTypeVolume => { self.set_volume(update.get_volume() as u16); - self.notify(None, true) + self.notify(None) } MessageType::kMessageTypeNotify => { @@ -1172,7 +1172,7 @@ impl SpircTask { fn handle_end_of_track(&mut self) -> Result<(), Error> { self.handle_next(); - self.notify(None, true) + self.notify(None) } fn position(&mut self) -> u32 { @@ -1391,14 +1391,7 @@ impl SpircTask { CommandSender::new(self, MessageType::kMessageTypeHello).send() } - fn notify( - &mut self, - recipient: Option<&str>, - suppress_loading_status: bool, - ) -> Result<(), Error> { - if suppress_loading_status && (self.state.get_status() == PlayStatus::kPlayStatusLoading) { - return Ok(()); - }; + fn notify(&mut self, recipient: Option<&str>) -> Result<(), Error> { let status_string = match self.state.get_status() { PlayStatus::kPlayStatusLoading => "kPlayStatusLoading", PlayStatus::kPlayStatusPause => "kPlayStatusPause", diff --git a/playback/src/player.rs b/playback/src/player.rs index f7788fda..6e2933b2 100644 --- a/playback/src/player.rs +++ b/playback/src/player.rs @@ -857,14 +857,6 @@ impl PlayerTrackLoader { let stream_loader_controller = encrypted_file.get_stream_loader_controller().ok()?; - if play_from_beginning { - // No need to seek -> we stream from the beginning - stream_loader_controller.set_stream_mode(); - } else { - // we need to seek -> we set stream mode after the initial seek. - stream_loader_controller.set_random_access_mode(); - } - let key = match self.session.audio_key().request(spotify_id, file_id).await { Ok(key) => key, Err(e) => { @@ -875,6 +867,10 @@ impl PlayerTrackLoader { let mut decrypted_file = AudioDecrypt::new(key, encrypted_file); + // Parsing normalisation data and starting playback from *beyond* the beginning + // will trigger a seek() so always start in random access mode. + stream_loader_controller.set_random_access_mode(); + let normalisation_data = match NormalisationData::parse_from_file(&mut decrypted_file) { Ok(data) => data, Err(_) => { @@ -930,15 +926,17 @@ impl PlayerTrackLoader { let mut stream_position_pcm = 0; let position_pcm = PlayerInternal::position_ms_to_pcm(position_ms); - if position_pcm > 0 { - stream_loader_controller.set_random_access_mode(); + if !play_from_beginning { match decoder.seek(position_pcm) { Ok(_) => stream_position_pcm = position_pcm, Err(e) => error!("PlayerTrackLoader::load_track error seeking: {}", e), } - stream_loader_controller.set_stream_mode(); }; + // Transition from random access mode to streaming mode now that + // we are ready to play from the requested position. + stream_loader_controller.set_stream_mode(); + info!("<{}> ({} ms) loaded", audio.name, audio.duration); return Some(PlayerLoadedTrackData {