Merge pull request #474 from ashthespy/skip_unplayable

Skip unplayable tracks instead of stopping

connect/src/spirc.rs

@@ -457,8 +457,8 @@ impl SpircTask {
             Ok(dur) => dur,
             Err(err) => err.duration(),
         };
-        ((dur.as_secs() as i64 + self.session.time_delta()) * 1000
-            + (dur.subsec_nanos() / 1000_000) as i64)
+        (dur.as_secs() as i64 + self.session.time_delta()) * 1000
+            + (dur.subsec_nanos() / 1000_000) as i64
     }
 
     fn ensure_mixer_started(&mut self) {
@@ -624,24 +624,8 @@ impl SpircTask {
                             self.play_status = SpircPlayStatus::Stopped;
                         }
                     },
-                    PlayerEvent::TimeToPreloadNextTrack { .. } => match self.play_status {
-                        SpircPlayStatus::Paused {
-                            ref mut preloading_of_next_track_triggered,
-                            ..
-                        }
-                        | SpircPlayStatus::Playing {
-                            ref mut preloading_of_next_track_triggered,
-                            ..
-                        } => {
-                            *preloading_of_next_track_triggered = true;
-                            if let Some(track_id) = self.preview_next_track() {
-                                self.player.preload(track_id);
-                            }
-                        }
-                        SpircPlayStatus::LoadingPause { .. }
-                        | SpircPlayStatus::LoadingPlay { .. }
-                        | SpircPlayStatus::Stopped => (),
-                    },
+                    PlayerEvent::TimeToPreloadNextTrack { .. } => self.handle_preload_next_track(),
+                    PlayerEvent::Unavailable { track_id, .. } => self.handle_unavailable(track_id),
                     _ => (),
                 }
             }
@@ -780,6 +764,7 @@ impl SpircTask {
                 } = self.play_status
                 {
                     if preloading_of_next_track_triggered {
+                        // Get the next track_id in the playlist
                         if let Some(track_id) = self.preview_next_track() {
                             self.player.preload(track_id);
                         }
@@ -911,6 +896,50 @@ impl SpircTask {
             .and_then(|(track_id, _)| Some(track_id))
     }
 
+    fn handle_preload_next_track(&mut self) {
+        // Requests the player thread to preload the next track
+        match self.play_status {
+            SpircPlayStatus::Paused {
+                ref mut preloading_of_next_track_triggered,
+                ..
+            }
+            | SpircPlayStatus::Playing {
+                ref mut preloading_of_next_track_triggered,
+                ..
+            } => {
+                *preloading_of_next_track_triggered = true;
+                if let Some(track_id) = self.preview_next_track() {
+                    self.player.preload(track_id);
+                }
+            }
+            SpircPlayStatus::LoadingPause { .. }
+            | SpircPlayStatus::LoadingPlay { .. }
+            | SpircPlayStatus::Stopped => (),
+        }
+    }
+
+    // Mark unavailable tracks so we can skip them later
+    fn handle_unavailable(&mut self, track_id: SpotifyId) {
+        let unavailables = self.get_track_index_for_spotify_id(&track_id, 0);
+        for &index in unavailables.iter() {
+            debug_assert_eq!(self.state.get_track()[index].get_gid(), track_id.to_raw());
+            let mut unplayable_track_ref = TrackRef::new();
+            unplayable_track_ref.set_gid(self.state.get_track()[index].get_gid().to_vec());
+            // Misuse context field to flag the track
+            unplayable_track_ref.set_context(String::from("NonPlayable"));
+            std::mem::swap(
+                &mut self.state.mut_track()[index],
+                &mut unplayable_track_ref,
+            );
+            debug!(
+                "Marked <{:?}> at {:?} as NonPlayable",
+                self.state.get_track()[index],
+                index,
+            );
+        }
+        self.handle_preload_next_track();
+    }
+
     fn handle_next(&mut self) {
         let mut new_index = self.consume_queued_track() as u32;
         let mut continue_playing = true;
@@ -1144,10 +1173,32 @@ impl SpircTask {
         })
     }
 
+    // Helper to find corresponding index(s) for track_id
+    fn get_track_index_for_spotify_id(
+        &self,
+        track_id: &SpotifyId,
+        start_index: usize,
+    ) -> Vec<usize> {
+        let index: Vec<usize> = self.state.get_track()[start_index..]
+            .iter()
+            .enumerate()
+            .filter(|&(_, track_ref)| track_ref.get_gid() == track_id.to_raw())
+            .map(|(idx, _)| start_index + idx)
+            .collect();
+        // Sanity check
+        debug_assert!(!index.is_empty());
+        index
+    }
+
+    // Broken out here so we can refactor this later when we move to SpotifyObjectID or similar
+    fn track_ref_is_unavailable(&self, track_ref: &TrackRef) -> bool {
+        track_ref.get_context() == "NonPlayable"
+    }
+
     fn get_track_id_to_play_from_playlist(&self, index: u32) -> Option<(SpotifyId, u32)> {
-        let tracks_len = self.state.get_track().len() as u32;
+        let tracks_len = self.state.get_track().len();
 
-        let mut new_playlist_index = index;
+        let mut new_playlist_index = index as usize;
 
         if new_playlist_index >= tracks_len {
             new_playlist_index = 0;
@@ -1159,14 +1210,15 @@ impl SpircTask {
         // tracks in each frame either have a gid or uri (that may or may not be a valid track)
         // E.g - context based frames sometimes contain tracks with <spotify:meta:page:>
 
-        let mut track_ref = self.state.get_track()[new_playlist_index as usize].clone();
+        let mut track_ref = self.state.get_track()[new_playlist_index].clone();
         let mut track_id = self.get_spotify_id_for_track(&track_ref);
-        while track_id.is_err() || track_id.unwrap().audio_type == SpotifyAudioType::NonPlayable {
+        while self.track_ref_is_unavailable(&track_ref)
+            || track_id.is_err()
+            || track_id.unwrap().audio_type == SpotifyAudioType::NonPlayable
+        {
             warn!(
                 "Skipping track <{:?}> at position [{}] of {}",
-                track_ref.get_uri(),
-                new_playlist_index,
-                tracks_len
+                track_ref, new_playlist_index, tracks_len
             );
 
             new_playlist_index += 1;
@@ -1178,12 +1230,12 @@ impl SpircTask {
                 warn!("No playable track found in state: {:?}", self.state);
                 return None;
             }
-            track_ref = self.state.get_track()[index as usize].clone();
+            track_ref = self.state.get_track()[new_playlist_index].clone();
             track_id = self.get_spotify_id_for_track(&track_ref);
         }
 
         match track_id {
-            Ok(track_id) => Some((track_id, new_playlist_index)),
+            Ok(track_id) => Some((track_id, new_playlist_index as u32)),
             Err(_) => None,
         }
     }

playback/src/player.rs

@@ -123,6 +123,11 @@ pub enum PlayerEvent {
         play_request_id: u64,
         track_id: SpotifyId,
     },
+    // The player was unable to load the requested track.
+    Unavailable {
+        play_request_id: u64,
+        track_id: SpotifyId,
+    },
     // The mixer volume was set to a new level.
     VolumeSet {
         volume: u16,
@@ -136,6 +141,9 @@ impl PlayerEvent {
             Loading {
                 play_request_id, ..
             }
+            | Unavailable {
+                play_request_id, ..
+            }
             | Started {
                 play_request_id, ..
             }
@@ -398,6 +406,7 @@ impl PlayerState {
         }
     }
 
+    #[allow(dead_code)]
     fn is_stopped(&self) -> bool {
         use self::PlayerState::*;
         match *self {
@@ -406,6 +415,14 @@ impl PlayerState {
         }
     }
 
+    fn is_loading(&self) -> bool {
+        use self::PlayerState::*;
+        match *self {
+            Loading { .. } => true,
+            _ => false,
+        }
+    }
+
     fn decoder(&mut self) -> Option<&mut Decoder> {
         use self::PlayerState::*;
         match *self {
@@ -748,8 +765,12 @@ impl Future for PlayerInternal {
                     }
                     Ok(Async::NotReady) => (),
                     Err(_) => {
-                        self.handle_player_stop();
-                        assert!(self.state.is_stopped());
+                        warn!("Unable to load <{:?}>\nSkipping to next track", track_id);
+                        assert!(self.state.is_loading());
+                        self.send_event(PlayerEvent::EndOfTrack {
+                            track_id,
+                            play_request_id,
+                        })
                     }
                 }
             }
@@ -769,7 +790,21 @@ impl Future for PlayerInternal {
                     }
                     Ok(Async::NotReady) => (),
                     Err(_) => {
+                        debug!("Unable to preload {:?}", track_id);
                         self.preload = PlayerPreload::None;
+                        // Let Spirc know that the track was unavailable.
+                        if let PlayerState::Playing {
+                            play_request_id, ..
+                        }
+                        | PlayerState::Paused {
+                            play_request_id, ..
+                        } = self.state
+                        {
+                            self.send_event(PlayerEvent::Unavailable {
+                                track_id,
+                                play_request_id,
+                            });
+                        }
                     }
                 }
             }
@@ -1302,7 +1337,6 @@ impl PlayerInternal {
     fn handle_command_preload(&mut self, track_id: SpotifyId) {
         debug!("Preloading track");
         let mut preload_track = true;
-
         // check whether the track is already loaded somewhere or being loaded.
         if let PlayerPreload::Loading {
             track_id: currently_loading,
@@ -1547,6 +1581,51 @@ impl ::std::fmt::Debug for PlayerCommand {
     }
 }
 
+impl ::std::fmt::Debug for PlayerState {
+    fn fmt(&self, f: &mut ::std::fmt::Formatter<'_>) -> ::std::fmt::Result {
+        use PlayerState::*;
+        match *self {
+            Stopped => f.debug_struct("Stopped").finish(),
+            Loading {
+                track_id,
+                play_request_id,
+                ..
+            } => f
+                .debug_struct("Loading")
+                .field("track_id", &track_id)
+                .field("play_request_id", &play_request_id)
+                .finish(),
+            Paused {
+                track_id,
+                play_request_id,
+                ..
+            } => f
+                .debug_struct("Paused")
+                .field("track_id", &track_id)
+                .field("play_request_id", &play_request_id)
+                .finish(),
+            Playing {
+                track_id,
+                play_request_id,
+                ..
+            } => f
+                .debug_struct("Playing")
+                .field("track_id", &track_id)
+                .field("play_request_id", &play_request_id)
+                .finish(),
+            EndOfTrack {
+                track_id,
+                play_request_id,
+                ..
+            } => f
+                .debug_struct("EndOfTrack")
+                .field("track_id", &track_id)
+                .field("play_request_id", &play_request_id)
+                .finish(),
+            Invalid => f.debug_struct("Invalid").finish(),
+        }
+    }
+}
 struct Subfile<T: Read + Seek> {
     stream: T,
     offset: u64,