From b293a116b6055bbb28b1df1b6fd936169a67aac8 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Sat, 13 Feb 2016 23:16:42 +0100 Subject: [PATCH 1/3] audio: Make sure about to finish skips unplayable tracks --- mopidy/core/playback.py | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/mopidy/core/playback.py b/mopidy/core/playback.py index e0eab403..76e80afd 100644 --- a/mopidy/core/playback.py +++ b/mopidy/core/playback.py @@ -250,17 +250,16 @@ class PlaybackController(object): if self._state == PlaybackState.STOPPED: return - # TODO: check that we always have a current track - original_tl_track = self.get_current_tl_track() - next_tl_track = self.core.tracklist.eot_track(original_tl_track) - - # TODO: only set pending if we have a backend that can play it? - # TODO: skip tracks that don't have a backend? - self._pending_tl_track = next_tl_track - backend = self._get_backend(next_tl_track) - - if backend: - backend.playback.change_track(next_tl_track.track).get() + pending = self.core.tracklist.eot_track(self._current_tl_track) + while pending: + # TODO: Avoid infinite loops if all tracks are unplayable. + backend = self._get_backend(pending) + if backend and backend.playback.change_track(pending.track).get(): + self._pending_tl_track = pending + break + else: + self.core.tracklist._mark_unplayable(pending) + pending = self.core.tracklist.eot_track(pending) def _on_tracklist_change(self): """ From 76ab5ffb041a6b26dc9fa890976f25fc8fcef2d5 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Sun, 14 Feb 2016 17:16:31 +0100 Subject: [PATCH 2/3] core: Make sure exceptions from backend's change_track is handled Also adds TODOs for the rest of the backend calls in playback which all need to assume backends can and will screw up. --- mopidy/core/playback.py | 18 ++++++++- tests/core/test_playback.py | 74 ++++++++++++++++++++++++++++++++++++- 2 files changed, 88 insertions(+), 4 deletions(-) diff --git a/mopidy/core/playback.py b/mopidy/core/playback.py index 76e80afd..3597f920 100644 --- a/mopidy/core/playback.py +++ b/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 @@ -253,6 +254,7 @@ class PlaybackController(object): pending = self.core.tracklist.eot_track(self._current_tl_track) while pending: # TODO: Avoid infinite loops if all tracks are unplayable. + # TODO: Wrap backend call in error handling. backend = self._get_backend(pending) if backend and backend.playback.change_track(pending.track).get(): self._pending_tl_track = pending @@ -299,6 +301,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) @@ -366,10 +369,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() @@ -418,6 +429,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 @@ -475,6 +487,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): @@ -482,6 +495,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) diff --git a/tests/core/test_playback.py b/tests/core/test_playback.py index a43724f0..860ce556 100644 --- a/tests/core/test_playback.py +++ b/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] From 79a4835e4eb43024153fe2eb7185791e24a591a2 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Sun, 14 Feb 2016 17:23:20 +0100 Subject: [PATCH 3/3] core: Add tests for change_track failing in about-to-finish --- mopidy/core/playback.py | 18 ++++++++++++------ tests/core/test_playback.py | 28 ++++++++++++++++++++++++++++ 2 files changed, 40 insertions(+), 6 deletions(-) diff --git a/mopidy/core/playback.py b/mopidy/core/playback.py index 3597f920..d6c470f2 100644 --- a/mopidy/core/playback.py +++ b/mopidy/core/playback.py @@ -254,13 +254,19 @@ class PlaybackController(object): pending = self.core.tracklist.eot_track(self._current_tl_track) while pending: # TODO: Avoid infinite loops if all tracks are unplayable. - # TODO: Wrap backend call in error handling. backend = self._get_backend(pending) - if backend and backend.playback.change_track(pending.track).get(): - self._pending_tl_track = pending - break - else: - self.core.tracklist._mark_unplayable(pending) + if not backend: + continue + + try: + if backend.playback.change_track(pending.track).get(): + self._pending_tl_track = pending + break + except Exception: + logger.exception('%s backend caused an exception.', + backend.actor_ref.actor_class.__name__) + + self.core.tracklist._mark_unplayable(pending) pending = self.core.tracklist.eot_track(pending) def _on_tracklist_change(self): diff --git a/tests/core/test_playback.py b/tests/core/test_playback.py index 860ce556..cfd58793 100644 --- a/tests/core/test_playback.py +++ b/tests/core/test_playback.py @@ -333,6 +333,34 @@ class TestOnAboutToFinish(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):