From e74eafb38a75b43bd1d7408339d255f4f5442fd9 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Thu, 3 Dec 2015 22:27:22 +0100 Subject: [PATCH 1/8] core: Switch back to correct track if seek happens before stream changed Technically the seek still needs to be postponed for this to work right, but it's a step closer. --- mopidy/core/playback.py | 8 ++++---- tests/core/test_playback.py | 34 ++++++++++++++++++++++++++++++++++ 2 files changed, 38 insertions(+), 4 deletions(-) diff --git a/mopidy/core/playback.py b/mopidy/core/playback.py index 45e1b4ba11..96fac4d9a5 100644 --- a/mopidy/core/playback.py +++ b/mopidy/core/playback.py @@ -437,10 +437,10 @@ def seek(self, time_position): if self.get_state() == PlaybackState.STOPPED: self.play() - # TODO: uncomment once we have tests for this. Should fix seek after - # about to finish doing wrong track. - # if self._current_tl_track and self._pending_tl_track: - # self.play(self._current_tl_track) + # Make sure we switch back to previous track if we get a seek while we + # have a pending track. + if self._current_tl_track and self._pending_tl_track: + self._change(self._current_tl_track, self.get_state()) # We need to prefer the still playing track, but if nothing is playing # we fall back to the pending one. diff --git a/tests/core/test_playback.py b/tests/core/test_playback.py index 4ae3b4ef87..6ea5313ff8 100644 --- a/tests/core/test_playback.py +++ b/tests/core/test_playback.py @@ -6,6 +6,8 @@ import pykka +import pytest + from mopidy import backend, core from mopidy.internal import deprecation from mopidy.models import Track @@ -529,6 +531,25 @@ def test_seek_past_end_of_track_emits_events(self, listener_mock): ], listener_mock.send.mock_calls) + @pytest.mark.xfail + def test_seek_race_condition_emits_events(self, listener_mock): + tl_tracks = self.core.tracklist.get_tl_tracks() + + self.core.playback.play(tl_tracks[0]) + self.trigger_about_to_finish(replay_until='stream_changed') + listener_mock.reset_mock() + + self.core.playback.seek(1000) + self.replay_events() + + # When we trigger seek after an about to finish the other code that + # emits track stopped/started and playback state changed events gets + # triggered as we have to switch back to the previous track. + # The correct behavior would be to only emit seeked. + self.assertListEqual( + [mock.call('seeked', time_position=1000)], + listener_mock.send.mock_calls) + def test_previous_emits_events(self, listener_mock): tl_tracks = self.core.tracklist.get_tl_tracks() @@ -632,6 +653,19 @@ def test_seek_paused_stay_paused(self): self.core.playback.seek(1000) self.assertEqual(self.core.playback.state, core.PlaybackState.PAUSED) + def test_seek_race_condition_after_about_to_finish(self): + tl_tracks = self.core.tracklist.get_tl_tracks() + + self.core.playback.play(tl_tracks[0]) + self.replay_events() + + self.trigger_about_to_finish(replay_until='stream_changed') + self.core.playback.seek(1000) + self.replay_events() + + current_tl_track = self.core.playback.get_current_tl_track() + self.assertEqual(current_tl_track, tl_tracks[0]) + class TestStream(BaseTest): From aeb881896b16f2f627a3d3aade1c47d982a34f81 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Thu, 3 Dec 2015 23:00:52 +0100 Subject: [PATCH 2/8] core: Trigger position changed from audio events. Makes sure to only fire when the position changed to our intended seek target. Otherwise we would also be triggering this when playback starts. --- mopidy/core/actor.py | 3 +++ mopidy/core/playback.py | 12 ++++++++---- tests/core/test_playback.py | 3 +++ 3 files changed, 14 insertions(+), 4 deletions(-) diff --git a/mopidy/core/actor.py b/mopidy/core/actor.py index e365e4b79a..93cb814e4a 100644 --- a/mopidy/core/actor.py +++ b/mopidy/core/actor.py @@ -90,6 +90,9 @@ def reached_end_of_stream(self): def stream_changed(self, uri): self.playback._on_stream_changed(uri) + def position_changed(self, position): + self.playback._on_position_changed(position) + def state_changed(self, old_state, new_state, target_state): # XXX: This is a temporary fix for issue #232 while we wait for a more # permanent solution with the implementation of issue #234. When the diff --git a/mopidy/core/playback.py b/mopidy/core/playback.py index 96fac4d9a5..ea72500468 100644 --- a/mopidy/core/playback.py +++ b/mopidy/core/playback.py @@ -26,6 +26,7 @@ def __init__(self, audio, backends, core): self._current_tl_track = None self._pending_tl_track = None + self._pending_position = None self._last_position = None self._previous = False @@ -220,6 +221,11 @@ def _on_stream_changed(self, uri): self.set_state(PlaybackState.PLAYING) self._trigger_track_playback_started() + def _on_position_changed(self, position): + if self._pending_position == position: + self._trigger_seeked(position) + self._pending_position = None + def _on_about_to_finish_callback(self): """Callback that performs a blocking actor call to the real callback. @@ -455,14 +461,12 @@ def seek(self, time_position): self.next() return True + self._pending_position = time_position backend = self._get_backend(self.get_current_tl_track()) if not backend: return False - success = backend.playback.seek(time_position).get() - if success: - self._trigger_seeked(time_position) - return success + return backend.playback.seek(time_position).get() def stop(self): """Stop playing.""" diff --git a/tests/core/test_playback.py b/tests/core/test_playback.py index 6ea5313ff8..8ff378618d 100644 --- a/tests/core/test_playback.py +++ b/tests/core/test_playback.py @@ -434,6 +434,7 @@ def test_stop_emits_events(self, listener_mock): self.core.playback.play(tl_tracks[0]) self.replay_events() self.core.playback.seek(1000) + self.replay_events() listener_mock.reset_mock() self.core.playback.stop() @@ -456,6 +457,7 @@ def test_next_emits_events(self, listener_mock): self.core.playback.play(tl_tracks[0]) self.replay_events() self.core.playback.seek(1000) + self.replay_events() listener_mock.reset_mock() self.core.playback.next() @@ -504,6 +506,7 @@ def test_seek_emits_seeked_event(self, listener_mock): listener_mock.reset_mock() self.core.playback.seek(1000) + self.replay_events() listener_mock.send.assert_called_once_with( 'seeked', time_position=1000) From 454077afebeab1d0acb0ae230328f386b68db364 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Thu, 3 Dec 2015 23:23:55 +0100 Subject: [PATCH 3/8] core: Make sure certain events are ignored when doing eot-seeks --- mopidy/core/playback.py | 9 ++++++--- tests/core/test_playback.py | 3 --- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/mopidy/core/playback.py b/mopidy/core/playback.py index ea72500468..f9c9295a46 100644 --- a/mopidy/core/playback.py +++ b/mopidy/core/playback.py @@ -212,14 +212,17 @@ def _on_stream_changed(self, uri): # This code path handles the stop() case, uri should be none. position, self._last_position = self._last_position, None - self._trigger_track_playback_ended(position) + if self._pending_position is None: + self._trigger_track_playback_ended(position) self._stream_title = None if self._pending_tl_track: self._set_current_tl_track(self._pending_tl_track) self._pending_tl_track = None - self.set_state(PlaybackState.PLAYING) - self._trigger_track_playback_started() + + if self._pending_position is None: + self.set_state(PlaybackState.PLAYING) + self._trigger_track_playback_started() def _on_position_changed(self, position): if self._pending_position == position: diff --git a/tests/core/test_playback.py b/tests/core/test_playback.py index 8ff378618d..f0d6d47712 100644 --- a/tests/core/test_playback.py +++ b/tests/core/test_playback.py @@ -6,8 +6,6 @@ import pykka -import pytest - from mopidy import backend, core from mopidy.internal import deprecation from mopidy.models import Track @@ -534,7 +532,6 @@ def test_seek_past_end_of_track_emits_events(self, listener_mock): ], listener_mock.send.mock_calls) - @pytest.mark.xfail def test_seek_race_condition_emits_events(self, listener_mock): tl_tracks = self.core.tracklist.get_tl_tracks() From eeb1f91ed1e83f2e7da5e4e8fe37ef8e403f2a8f Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Thu, 3 Dec 2015 23:33:48 +0100 Subject: [PATCH 4/8] core: Actually perform delayed "eot-seek" on stream changed --- mopidy/core/playback.py | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/mopidy/core/playback.py b/mopidy/core/playback.py index f9c9295a46..e877866fa6 100644 --- a/mopidy/core/playback.py +++ b/mopidy/core/playback.py @@ -223,6 +223,8 @@ def _on_stream_changed(self, uri): if self._pending_position is None: self.set_state(PlaybackState.PLAYING) self._trigger_track_playback_started() + else: + self._seek(self._pending_position) def _on_position_changed(self, position): if self._pending_position == position: @@ -446,11 +448,6 @@ def seek(self, time_position): if self.get_state() == PlaybackState.STOPPED: self.play() - # Make sure we switch back to previous track if we get a seek while we - # have a pending track. - if self._current_tl_track and self._pending_tl_track: - self._change(self._current_tl_track, self.get_state()) - # We need to prefer the still playing track, but if nothing is playing # we fall back to the pending one. tl_track = self._current_tl_track or self._pending_tl_track @@ -464,11 +461,20 @@ def seek(self, time_position): self.next() return True + # Store our target position. self._pending_position = time_position + + # Make sure we switch back to previous track if we get a seek while we + # have a pending track. + if self._current_tl_track and self._pending_tl_track: + self._change(self._current_tl_track, self.get_state()) + else: + return self._seek(time_position) + + def _seek(self, time_position): backend = self._get_backend(self.get_current_tl_track()) if not backend: return False - return backend.playback.seek(time_position).get() def stop(self): From 9f23757cc3f9f7fc3b48eabeeccef720cc03fb49 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Fri, 4 Dec 2015 20:59:01 +0100 Subject: [PATCH 5/8] core: Return pending position during active seek. This covers over that audio will fail query position while a seek is in progress. It also means that instead of returning zero we at least return something which is much closer to the time that we will soon end up playing from. --- mopidy/core/playback.py | 2 ++ tests/core/test_playback.py | 2 -- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/mopidy/core/playback.py b/mopidy/core/playback.py index e877866fa6..89bd92ee73 100644 --- a/mopidy/core/playback.py +++ b/mopidy/core/playback.py @@ -131,6 +131,8 @@ def set_state(self, new_state): def get_time_position(self): """Get time position in milliseconds.""" + if self._pending_position is not None: + return self._pending_position backend = self._get_backend(self.get_current_tl_track()) if backend: return backend.playback.get_time_position().get() diff --git a/tests/core/test_playback.py b/tests/core/test_playback.py index f0d6d47712..0da59b4d67 100644 --- a/tests/core/test_playback.py +++ b/tests/core/test_playback.py @@ -855,7 +855,6 @@ def test_time_position_selects_dummy1_backend(self): self.core.playback.play(self.tl_tracks[0]) self.trigger_stream_changed() - self.core.playback.seek(10000) self.core.playback.time_position self.playback1.get_time_position.assert_called_once_with() @@ -865,7 +864,6 @@ def test_time_position_selects_dummy2_backend(self): self.core.playback.play(self.tl_tracks[1]) self.trigger_stream_changed() - self.core.playback.seek(10000) self.core.playback.time_position self.assertFalse(self.playback1.get_time_position.called) From 438b4b7a35681ddefdfcf5318a50324e663055f0 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Fri, 4 Dec 2015 23:09:48 +0100 Subject: [PATCH 6/8] docs: Update changelog with seek related changes --- docs/changelog.rst | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/docs/changelog.rst b/docs/changelog.rst index 3bda9237c1..610788d672 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -16,6 +16,11 @@ Core API - Start ``tlid`` counting at 1 instead of 0 to keep in sync with MPD's ``songid``. +- `get_time_position` now returns the seek target while a seek is in progress. + This gives better results than just failing the position query. + + (Fixes: :issue:`312` PR: :pr:`1346`) + Models ------ @@ -83,6 +88,11 @@ Gapless - Tests have been updated to always use a core actor so async state changes don't trip us up. +- Seek events are now triggered when the seek completes. Further changes have + been made to make seek work correctly for gapless related corner cases. + + (Fixes: :issue:`1305` PR: :pr:`1346`) + v1.1.2 (UNRELEASED) =================== From 8c6bb639636725bfc9e2cd1d21668fecd3aa4e82 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Sat, 5 Dec 2015 11:34:15 +0100 Subject: [PATCH 7/8] docs: Improve changelog PR#1346 per review comments --- docs/changelog.rst | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/docs/changelog.rst b/docs/changelog.rst index 610788d672..87e7470efd 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -16,10 +16,9 @@ Core API - Start ``tlid`` counting at 1 instead of 0 to keep in sync with MPD's ``songid``. -- `get_time_position` now returns the seek target while a seek is in progress. - This gives better results than just failing the position query. - - (Fixes: :issue:`312` PR: :pr:`1346`) +- :meth:`~mopidy.core.PlaybackController.get_time_position` now returns the + seek target while a seek is in progress. This gives better results than just + failing the position query. (Fixes: :issue:`312` PR: :issue:`1346`) Models ------ @@ -88,10 +87,10 @@ Gapless - Tests have been updated to always use a core actor so async state changes don't trip us up. -- Seek events are now triggered when the seek completes. Further changes have - been made to make seek work correctly for gapless related corner cases. - - (Fixes: :issue:`1305` PR: :pr:`1346`) +- Seek events are now triggered when the seek completes. Previously the event + was emitted when the seek was requested, not when it completed. Further + changes have been made to make seek work correctly for gapless related corner + cases. (Fixes: :issue:`1305` PR: :pr:`1346`) v1.1.2 (UNRELEASED) From 1c2850bc3eb0bab9d0624be0eadb7fcdd74fbf59 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Sat, 5 Dec 2015 21:24:18 +0100 Subject: [PATCH 8/8] docs: Use :issue: instead of :pr: --- docs/changelog.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/changelog.rst b/docs/changelog.rst index 87e7470efd..8cc886ce1b 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -90,7 +90,7 @@ Gapless - Seek events are now triggered when the seek completes. Previously the event was emitted when the seek was requested, not when it completed. Further changes have been made to make seek work correctly for gapless related corner - cases. (Fixes: :issue:`1305` PR: :pr:`1346`) + cases. (Fixes: :issue:`1305` PR: :issue:`1346`) v1.1.2 (UNRELEASED)