Merge pull request #1346 from adamcik/feature/eot-seek-handling

Gapless aware seek handling

docs/changelog.rst

@@ -16,6 +16,10 @@ Core API
 - Start ``tlid`` counting at 1 instead of 0 to keep in sync with MPD's
   ``songid``.
 
+- :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
 ------
 
@@ -83,6 +87,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. 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: :issue:`1346`)
+
 
 v1.1.2 (UNRELEASED)
 ===================

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

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
 
@@ -130,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()
@@ -211,14 +214,24 @@ 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()
+            else:
+                self._seek(self._pending_position)
+
+    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.
@@ -437,11 +450,6 @@ 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)
-
         # 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
@@ -455,14 +463,21 @@ 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
-
-        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."""

tests/core/test_playback.py

@@ -432,6 +432,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()
@@ -454,6 +455,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()
@@ -502,6 +504,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)
@@ -529,6 +532,24 @@ def test_seek_past_end_of_track_emits_events(self, listener_mock):
             ],
             listener_mock.send.mock_calls)
 
+    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):
 
@@ -821,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()
@@ -831,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)