Merge pull request #1440 from adamcik/fix/1425-about-to-finish-skip-unplayable

audio: Make sure about to finish skips unplayable tracks

mopidy/core/playback.py

@@ -135,6 +135,7 @@ class PlaybackController(object):
             return self._pending_position
         backend = self._get_backend(self.get_current_tl_track())
         if backend:
+            # TODO: Wrap backend call in error handling.
             return backend.playback.get_time_position().get()
         else:
             return 0
@@ -250,17 +251,23 @@ class PlaybackController(object):
         if self._state == PlaybackState.STOPPED:
             return
 
-        # TODO: check that we always have a current track
+        pending = self.core.tracklist.eot_track(self._current_tl_track)
-        original_tl_track = self.get_current_tl_track()
+        while pending:
-        next_tl_track = self.core.tracklist.eot_track(original_tl_track)
+            # TODO: Avoid infinite loops if all tracks are unplayable.
+            backend = self._get_backend(pending)
+            if not backend:
+                continue
 
-        # TODO: only set pending if we have a backend that can play it?
+            try:
-        # TODO: skip tracks that don't have a backend?
+                if backend.playback.change_track(pending.track).get():
-        self._pending_tl_track = next_tl_track
+                    self._pending_tl_track = pending
-        backend = self._get_backend(next_tl_track)
+                    break
+            except Exception:
+                logger.exception('%s backend caused an exception.',
+                                 backend.actor_ref.actor_class.__name__)
 
-        if backend:
+            self.core.tracklist._mark_unplayable(pending)
-            backend.playback.change_track(next_tl_track.track).get()
+            pending = self.core.tracklist.eot_track(pending)
 
     def _on_tracklist_change(self):
         """
@@ -300,6 +307,7 @@ class PlaybackController(object):
     def pause(self):
         """Pause playback."""
         backend = self._get_backend(self.get_current_tl_track())
+        # TODO: Wrap backend call in error handling.
         if not backend or backend.playback.pause().get():
             # TODO: switch to:
             # backend.track(pause)
@@ -367,10 +375,18 @@ class PlaybackController(object):
         if not backend:
             return False
 
+        # TODO: Wrap backend call in error handling.
         backend.playback.prepare_change()
-        if not backend.playback.change_track(pending_tl_track.track).get():
-            return False  # TODO: test for this path
 
+        try:
+            if not backend.playback.change_track(pending_tl_track.track).get():
+                return False
+        except Exception:
+            logger.exception('%s backend caused an exception.',
+                             backend.actor_ref.actor_class.__name__)
+            return False
+
+        # TODO: Wrap backend calls in error handling.
         if state == PlaybackState.PLAYING:
             try:
                 return backend.playback.play().get()
@@ -419,6 +435,7 @@ class PlaybackController(object):
         if self.get_state() != PlaybackState.PAUSED:
             return
         backend = self._get_backend(self.get_current_tl_track())
+        # TODO: Wrap backend call in error handling.
         if backend and backend.playback.resume().get():
             self.set_state(PlaybackState.PLAYING)
             # TODO: trigger via gst messages
@@ -476,6 +493,7 @@ class PlaybackController(object):
         backend = self._get_backend(self.get_current_tl_track())
         if not backend:
             return False
+        # TODO: Wrap backend call in error handling.
         return backend.playback.seek(time_position).get()
 
     def stop(self):
@@ -483,6 +501,7 @@ class PlaybackController(object):
         if self.get_state() != PlaybackState.STOPPED:
             self._last_position = self.get_time_position()
             backend = self._get_backend(self.get_current_tl_track())
+            # TODO: Wrap backend call in error handling.
             if not backend or backend.playback.stop().get():
                 self.set_state(PlaybackState.STOPPED)
 

tests/core/test_playback.py

@@ -13,6 +13,16 @@ from mopidy.models import Track
 from tests import dummy_audio
 
 
+class TestPlaybackProvider(backend.PlaybackProvider):
+    def translate_uri(self, uri):
+        if 'error' in uri:
+            raise Exception(uri)
+        elif 'unplayable' in uri:
+            return None
+        else:
+            return uri
+
+
 # TODO: Replace this with dummy_backend now that it uses a real
 # playbackprovider Since we rely on our DummyAudio to actually emit events we
 # need a "real" backend and not a mock so the right calls make it through to
@@ -22,7 +32,7 @@ class TestBackend(pykka.ThreadingActor, backend.Backend):
 
     def __init__(self, config, audio):
         super(TestBackend, self).__init__()
-        self.playback = backend.PlaybackProvider(audio=audio, backend=self)
+        self.playback = TestPlaybackProvider(audio=audio, backend=self)
 
 
 class BaseTest(unittest.TestCase):
@@ -196,6 +206,36 @@ class TestNextHandling(BaseTest):
 
         assert self.core.playback.get_current_tl_track() == tl_tracks[2]
 
+    def test_next_skips_over_change_track_error(self):
+        # Trigger an exception in translate_uri.
+        track = Track(uri='dummy:error', length=1234)
+        self.core.tracklist.add(tracks=[track], at_position=1)
+
+        tl_tracks = self.core.tracklist.get_tl_tracks()
+
+        self.core.playback.play()
+        self.replay_events()
+
+        self.core.playback.next()
+        self.replay_events()
+
+        assert self.core.playback.get_current_tl_track() == tl_tracks[2]
+
+    def test_next_skips_over_change_track_unplayable(self):
+        # Make translate_uri return None.
+        track = Track(uri='dummy:unplayable', length=1234)
+        self.core.tracklist.add(tracks=[track], at_position=1)
+
+        tl_tracks = self.core.tracklist.get_tl_tracks()
+
+        self.core.playback.play()
+        self.replay_events()
+
+        self.core.playback.next()
+        self.replay_events()
+
+        assert self.core.playback.get_current_tl_track() == tl_tracks[2]
+
 
 class TestPreviousHandling(BaseTest):
     # TODO Test previous() more
@@ -252,8 +292,38 @@ class TestPreviousHandling(BaseTest):
 
         assert self.core.playback.get_current_tl_track() == tl_tracks[0]
 
+    def test_previous_skips_over_change_track_error(self):
+        # Trigger an exception in translate_uri.
+        track = Track(uri='dummy:error', length=1234)
+        self.core.tracklist.add(tracks=[track], at_position=1)
 
-class OnAboutToFinishTest(BaseTest):
+        tl_tracks = self.core.tracklist.get_tl_tracks()
+
+        self.core.playback.play(tl_tracks[2])
+        self.replay_events()
+
+        self.core.playback.previous()
+        self.replay_events()
+
+        assert self.core.playback.get_current_tl_track() == tl_tracks[0]
+
+    def test_previous_skips_over_change_track_unplayable(self):
+        # Makes translate_uri return None.
+        track = Track(uri='dummy:unplayable', length=1234)
+        self.core.tracklist.add(tracks=[track], at_position=1)
+
+        tl_tracks = self.core.tracklist.get_tl_tracks()
+
+        self.core.playback.play(tl_tracks[2])
+        self.replay_events()
+
+        self.core.playback.previous()
+        self.replay_events()
+
+        assert self.core.playback.get_current_tl_track() == tl_tracks[0]
+
+
+class TestOnAboutToFinish(BaseTest):
 
     def test_on_about_to_finish_keeps_finished_track_in_tracklist(self):
         tl_track = self.core.tracklist.get_tl_tracks()[0]
@@ -263,6 +333,34 @@ class OnAboutToFinishTest(BaseTest):
 
         self.assertIn(tl_track, self.core.tracklist.tl_tracks)
 
+    def test_on_about_to_finish_skips_over_change_track_error(self):
+        # Trigger an exception in translate_uri.
+        track = Track(uri='dummy:error', length=1234)
+        self.core.tracklist.add(tracks=[track], at_position=1)
+
+        tl_tracks = self.core.tracklist.get_tl_tracks()
+
+        self.core.playback.play(tl_tracks[0])
+        self.replay_events()
+
+        self.trigger_about_to_finish()
+
+        assert self.core.playback.get_current_tl_track() == tl_tracks[2]
+
+    def test_on_about_to_finish_skips_over_change_track_unplayable(self):
+        # Makes translate_uri return None.
+        track = Track(uri='dummy:unplayable', length=1234)
+        self.core.tracklist.add(tracks=[track], at_position=1)
+
+        tl_tracks = self.core.tracklist.get_tl_tracks()
+
+        self.core.playback.play(tl_tracks[0])
+        self.replay_events()
+
+        self.trigger_about_to_finish()
+
+        assert self.core.playback.get_current_tl_track() == tl_tracks[2]
+
 
 class TestConsumeHandling(BaseTest):