From e00b590ae9757edbb565a8e788a54defd76a6bed Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Tue, 30 Oct 2012 22:30:13 +0100 Subject: [PATCH 01/10] Detangle EOT and EOS. This commit tries to detangle EOS from EOT which we have incorrectly intermingled. EOS events should only be happening at the end of the playlist when we are about to stop. EOT handling has been removed / broken in this change, and will need to be re-added with proper tests. --- mopidy/core/actor.py | 2 +- mopidy/core/playback.py | 7 +++++-- tests/backends/base/playback.py | 6 ++++++ 3 files changed, 12 insertions(+), 3 deletions(-) diff --git a/mopidy/core/actor.py b/mopidy/core/actor.py index cd4ba180..072a989b 100644 --- a/mopidy/core/actor.py +++ b/mopidy/core/actor.py @@ -56,7 +56,7 @@ class Core(pykka.ThreadingActor, AudioListener, BackendListener): """List of URI schemes we can handle""" def reached_end_of_stream(self): - self.playback.on_end_of_track() + self.playback.on_end_of_stream() def state_changed(self, old_state, new_state): # XXX: This is a temporary fix for issue #232 while we wait for a more diff --git a/mopidy/core/playback.py b/mopidy/core/playback.py index 4941ef0f..73227f68 100644 --- a/mopidy/core/playback.py +++ b/mopidy/core/playback.py @@ -312,6 +312,11 @@ class PlaybackController(object): elif old_state == PlaybackState.PAUSED: self.pause() + def on_end_of_stream(self): + self._trigger_track_playback_ended() + self.state = PlaybackState.STOPPED + self.current_cp_track = None + def on_end_of_track(self): """ Tell the playback controller that end of track is reached. @@ -326,8 +331,6 @@ class PlaybackController(object): if self.tl_track_at_eot: self._trigger_track_playback_ended() self.play(self.tl_track_at_eot) - else: - self.stop(clear_current_track=True) if self.consume: self.core.tracklist.remove(tlid=original_tl_track.tlid) diff --git a/tests/backends/base/playback.py b/tests/backends/base/playback.py index 09dffbab..ea6cbbf4 100644 --- a/tests/backends/base/playback.py +++ b/tests/backends/base/playback.py @@ -110,6 +110,7 @@ class PlaybackControllerTest(object): def test_current_track_after_completed_playlist(self): self.playback.play(self.tracklist.tl_tracks[-1]) self.playback.on_end_of_track() + self.playback.on_end_of_stream() self.assertEqual(self.playback.state, PlaybackState.STOPPED) self.assertEqual(self.playback.current_track, None) @@ -338,6 +339,8 @@ class PlaybackControllerTest(object): self.playback.on_end_of_track() + self.playback.on_end_of_stream() + self.assertEqual(self.playback.state, PlaybackState.STOPPED) @populate_tracklist @@ -346,6 +349,7 @@ class PlaybackControllerTest(object): for _ in self.tracks: self.playback.on_end_of_track() + self.playback.on_end_of_stream() self.assertEqual(self.playback.current_track, None) self.assertEqual(self.playback.state, PlaybackState.STOPPED) @@ -510,6 +514,7 @@ class PlaybackControllerTest(object): def test_tracklist_position_at_end_of_playlist(self): self.playback.play(self.tracklist.tl_tracks[-1]) self.playback.on_end_of_track() + self.playback.on_end_of_stream() self.assertEqual(self.playback.tracklist_position, None) def test_on_tracklist_change_gets_called(self): @@ -815,6 +820,7 @@ class PlaybackControllerTest(object): def test_end_of_playlist_stops(self): self.playback.play(self.tracklist.tl_tracks[-1]) self.playback.on_end_of_track() + self.playback.on_end_of_stream() self.assertEqual(self.playback.state, PlaybackState.STOPPED) def test_repeat_off_by_default(self): From 3a9ce05b7c8bed3e77255033b17c391de06bd8be Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Thu, 1 Nov 2012 22:28:57 +0100 Subject: [PATCH 02/10] Add change track to playback providers. This method is intended to make implementing EOT handling much more streamlined by adding a way to easily just change the URI and any other state without messing with gstreamer pipe states Naming of this is a bit unfortunate as it is not the same as the core.playback's concept of change_track. At least not yet --- mopidy/backends/base.py | 21 ++++++++++++++++++--- mopidy/backends/spotify/playback.py | 18 +++++------------- 2 files changed, 23 insertions(+), 16 deletions(-) diff --git a/mopidy/backends/base.py b/mopidy/backends/base.py index 8250a24c..ebef096f 100644 --- a/mopidy/backends/base.py +++ b/mopidy/backends/base.py @@ -88,8 +88,8 @@ class BaseLibraryProvider(object): class BasePlaybackProvider(object): """ - :param audio: the audio actor - :type audio: actor proxy to an instance of :class:`mopidy.audio.Audio` + :param audio: audio sub-system + :type audio: actor proxy to a :class:`mopidy.audio.Audio` actor. :param backend: the backend :type backend: :class:`mopidy.backends.base.Backend` """ @@ -121,9 +121,24 @@ class BasePlaybackProvider(object): :rtype: :class:`True` if successful, else :class:`False` """ self.audio.prepare_change() - self.audio.set_uri(track.uri).get() + self.change_track(track) return self.audio.start_playback().get() + def change_track(self, track): + """ + Swith to provided track. + + Used for handling of EOT and and in :meth:`play`. + + *MAY be reimplemented by subclass.* + + :param track: the track to play + :type track: :class:`mopidy.models.Track` + :rtype: :class:`True` if successful, else :class:`False` + """ + self.audio.set_uri(track.uri).get() + return True + def resume(self): """ Resume playback at the same time position playback was paused. diff --git a/mopidy/backends/spotify/playback.py b/mopidy/backends/spotify/playback.py index e4534172..2e41f072 100644 --- a/mopidy/backends/spotify/playback.py +++ b/mopidy/backends/spotify/playback.py @@ -23,26 +23,18 @@ class SpotifyPlaybackProvider(base.BasePlaybackProvider): return super(SpotifyPlaybackProvider, self).pause() - def play(self, track): - if track.uri is None: - return False - + def change_track(self, track): + self.audio.set_uri('appsrc://').get() + self.audio.set_metadata(track).get() try: self.backend.spotify.session.load( Link.from_string(track.uri).as_track()) self.backend.spotify.session.play(1) - - self.audio.prepare_change() - self.audio.set_uri('appsrc://') - self.audio.start_playback() - self.audio.set_metadata(track) - - self._timer.play() - - return True except SpotifyError as e: logger.info('Playback of %s failed: %s', track.uri, e) return False + self._timer.play() + return True def resume(self): time_position = self.get_time_position() From 2a391f23e1a11b84de0a2bb665989e49b9a7129a Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Thu, 8 Nov 2012 23:17:48 +0100 Subject: [PATCH 03/10] Use source's factory name to determine type. Checking the URI inside the new-source handler turned out to tickle some issue that gave us deadlocks. Getting the source and checking which factory created it is much safer. --- mopidy/audio/actor.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/mopidy/audio/actor.py b/mopidy/audio/actor.py index 7de98075..1f482301 100644 --- a/mopidy/audio/actor.py +++ b/mopidy/audio/actor.py @@ -78,8 +78,9 @@ class Audio(pykka.ThreadingActor): self._appsrc = None def _on_new_source(self, element, pad): - uri = element.get_property('uri') - if not uri or not uri.startswith('appsrc://'): + source = element.get_property('source') + + if source.get_factory().get_name() != 'appsrc': return # These caps matches the audio data provided by libspotify @@ -87,7 +88,6 @@ class Audio(pykka.ThreadingActor): b'audio/x-raw-int, endianness=(int)1234, channels=(int)2, ' b'width=(int)16, depth=(int)16, signed=(boolean)true, ' b'rate=(int)44100') - source = element.get_property('source') source.set_property('caps', default_caps) # GStreamer does not like unicode source.set_property('format', b'time') From 1f544038b046df8bc4652b6547b136acb33077af Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Thu, 8 Nov 2012 23:41:39 +0100 Subject: [PATCH 04/10] Setup about to finish handling for proper EOT support. With this change we ask the core.playback system to set the next tracks uri and otherwise update state as needed. With this change we should be able to support streaming oggs and gapless playback to mention a few of items this fixes. On the down side, this change breaks test_end_of_track_skips_to_next_track_on_failure as there is no clean way to handle this with the async nature of this EOT handling. --- mopidy/audio/actor.py | 7 +++++++ mopidy/core/playback.py | 6 ++++-- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/mopidy/audio/actor.py b/mopidy/audio/actor.py index 1f482301..0a0cb0be 100644 --- a/mopidy/audio/actor.py +++ b/mopidy/audio/actor.py @@ -77,6 +77,13 @@ class Audio(pykka.ThreadingActor): def _on_about_to_finish(self, element): self._appsrc = None + # TODO: this is just a horrible hack to get us started. the + # comunication is correct, but this way of hooking it up is not. + from mopidy.core import Core + logger.debug(u'Triggering reached end of track event') + core = pykka.ActorRegistry.get_by_class(Core)[0].proxy() + core.playback.on_end_of_track().get() + def _on_new_source(self, element, pad): source = element.get_property('source') diff --git a/mopidy/core/playback.py b/mopidy/core/playback.py index 73227f68..3ca2631f 100644 --- a/mopidy/core/playback.py +++ b/mopidy/core/playback.py @@ -315,7 +315,7 @@ class PlaybackController(object): def on_end_of_stream(self): self._trigger_track_playback_ended() self.state = PlaybackState.STOPPED - self.current_cp_track = None + self.current_tl_track = None def on_end_of_track(self): """ @@ -329,8 +329,10 @@ class PlaybackController(object): original_tl_track = self.current_tl_track if self.tl_track_at_eot: + self.current_tl_track = self.tl_track_at_eot self._trigger_track_playback_ended() - self.play(self.tl_track_at_eot) + self._get_backend().playback.change_track(self.current_track) + self._trigger_track_playback_started() if self.consume: self.core.tracklist.remove(tlid=original_tl_track.tlid) From 39580e19485b707fa0dc4e0a1deeecc9011c513a Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Sat, 5 Jan 2013 16:46:16 +0100 Subject: [PATCH 05/10] Use blocking calls when changing tracks on EOT. This ensures that we block about-to-finish and the pipeline until the next URI is set, alowing for at least EOS free playback. If the uri is set quickly enough this will also be gapless. --- mopidy/audio/actor.py | 5 +++++ mopidy/core/playback.py | 5 ++++- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/mopidy/audio/actor.py b/mopidy/audio/actor.py index e3d5ac87..701fe2ac 100644 --- a/mopidy/audio/actor.py +++ b/mopidy/audio/actor.py @@ -95,6 +95,11 @@ class Audio(pykka.ThreadingActor): from mopidy.core import Core logger.debug(u'Triggering reached end of track event') core = pykka.ActorRegistry.get_by_class(Core)[0].proxy() + + # Note that we can not let this function return until we have the next + # URI set for gapless / EOS free playback. This means all the + # subsequent remote calls to backends etc. down this code path need to + # block. core.playback.on_end_of_track().get() def _on_new_source(self, element, pad): diff --git a/mopidy/core/playback.py b/mopidy/core/playback.py index afd34394..1c105f0c 100644 --- a/mopidy/core/playback.py +++ b/mopidy/core/playback.py @@ -330,10 +330,13 @@ class PlaybackController(object): original_tl_track = self.current_tl_track + # As noted in mopidy.audio which calls this code, we need to make sure + # the calls to the backend are blocking or gapless / EOS free playback + # will break. if self.tl_track_at_eot: self.current_tl_track = self.tl_track_at_eot self._trigger_track_playback_ended() - self._get_backend().playback.change_track(self.current_track) + self._get_backend().playback.change_track(self.current_track).get() self._trigger_track_playback_started() if self.consume: From 81b6620799311d08233d8eaa77ff6279d1ff8388 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Sat, 5 Jan 2013 17:37:16 +0100 Subject: [PATCH 06/10] audio: Handle about to finish for non appsrc playback The appsrc cleanup code was still short circuiting the about to finish handler, this meant that EOT handling never happened for local files and playback stopped. With this change proper EOT handling works for all track types. --- mopidy/audio/actor.py | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/mopidy/audio/actor.py b/mopidy/audio/actor.py index 701fe2ac..f812d49f 100644 --- a/mopidy/audio/actor.py +++ b/mopidy/audio/actor.py @@ -82,13 +82,14 @@ class Audio(pykka.ThreadingActor): 'notify::source', self._on_new_source) def _on_about_to_finish(self, element): - source, self._appsrc = self._appsrc, None - if source is None: - return + # Cleanup appsrc related stuff. + old_appsrc, self._appsrc = self._appsrc, None + + if self._appsrc_seek_data_id is not None and old_appsrc: + old_appsrc.disconnect(self._appsrc_seek_data_id) + self._appsrc_caps = None - if self._appsrc_seek_data_id is not None: - source.disconnect(self._appsrc_seek_data_id) - self._appsrc_seek_data_id = None + self._appsrc_seek_data_id = None # TODO: this is just a horrible hack to get us started. the # comunication is correct, but this way of hooking it up is not. From 0d7b7e29a3594474e06d0649b3fe39a8c31aeecc Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Sat, 5 Jan 2013 21:35:41 +0100 Subject: [PATCH 07/10] audio: Remove test for error handling that no longer happens. --- tests/backends/base/playback.py | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/tests/backends/base/playback.py b/tests/backends/base/playback.py index ea6cbbf4..155fa661 100644 --- a/tests/backends/base/playback.py +++ b/tests/backends/base/playback.py @@ -362,16 +362,6 @@ class PlaybackControllerTest(object): self.playback.on_end_of_track() self.assertEqual(self.playback.state, PlaybackState.STOPPED) - @populate_tracklist - def test_end_of_track_skips_to_next_track_on_failure(self): - # If backend's play() returns False, it is a failure. - self.backend.playback.play = lambda track: track != self.tracks[1] - self.playback.play() - self.assertEqual(self.playback.current_track, self.tracks[0]) - self.playback.on_end_of_track() - self.assertNotEqual(self.playback.current_track, self.tracks[1]) - self.assertEqual(self.playback.current_track, self.tracks[2]) - @populate_tracklist def test_end_of_track_track_before_play(self): self.assertEqual(self.playback.tl_track_at_next, self.tl_tracks[0]) From 6b922066bb82553c8b9bbef8b46d73056309436b Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Sat, 5 Jan 2013 22:47:07 +0100 Subject: [PATCH 08/10] core: Trigger playback ended before switching tracks --- mopidy/core/playback.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mopidy/core/playback.py b/mopidy/core/playback.py index 1c105f0c..1f2e0384 100644 --- a/mopidy/core/playback.py +++ b/mopidy/core/playback.py @@ -334,8 +334,8 @@ class PlaybackController(object): # the calls to the backend are blocking or gapless / EOS free playback # will break. if self.tl_track_at_eot: - self.current_tl_track = self.tl_track_at_eot self._trigger_track_playback_ended() + self.current_tl_track = self.tl_track_at_eot self._get_backend().playback.change_track(self.current_track).get() self._trigger_track_playback_started() From fb8fba243ab4de10452c0c179eba76301e77dea5 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Sat, 5 Jan 2013 23:16:42 +0100 Subject: [PATCH 09/10] audio/core: Rework how we hook up end of track handling. --- mopidy/audio/actor.py | 15 ++++++++------- mopidy/core/actor.py | 4 ++++ 2 files changed, 12 insertions(+), 7 deletions(-) diff --git a/mopidy/audio/actor.py b/mopidy/audio/actor.py index 206eae09..04b0ab06 100644 --- a/mopidy/audio/actor.py +++ b/mopidy/audio/actor.py @@ -49,6 +49,8 @@ class Audio(pykka.ThreadingActor): self._software_mixing = False self._volume_set = None + self._end_of_track_callback = None + self._appsrc = None self._appsrc_caps = None self._appsrc_need_data_callback = None @@ -101,17 +103,12 @@ class Audio(pykka.ThreadingActor): self._appsrc_caps = None - # TODO: this is just a horrible hack to get us started. the - # comunication is correct, but this way of hooking it up is not. - from mopidy.core import Core - logger.debug(u'Triggering reached end of track event') - core = pykka.ActorRegistry.get_by_class(Core)[0].proxy() - # Note that we can not let this function return until we have the next # URI set for gapless / EOS free playback. This means all the # subsequent remote calls to backends etc. down this code path need to # block. - core.playback.on_end_of_track().get() + if self._end_of_track_callback: + self._end_of_track_callback() def _on_new_source(self, element, pad): source = element.get_property('source') @@ -291,6 +288,10 @@ class Audio(pykka.ThreadingActor): logger.debug('Triggering reached_end_of_stream event') AudioListener.send('reached_end_of_stream') + def set_on_end_of_track(self, callback): + """Set callback to be called on end of track.""" + self._end_of_track_callback = callback + def set_uri(self, uri): """ Set URI of audio to be played. diff --git a/mopidy/core/actor.py b/mopidy/core/actor.py index 072a989b..1c635799 100644 --- a/mopidy/core/actor.py +++ b/mopidy/core/actor.py @@ -46,6 +46,10 @@ class Core(pykka.ThreadingActor, AudioListener, BackendListener): self.tracklist = TracklistController(core=self) + # Hook up blocking on end of track handler to audio sub-system. + audio.set_on_end_of_track( + lambda: self.actor_ref.proxy().playback.on_end_of_track().get()) + def get_uri_schemes(self): futures = [b.uri_schemes for b in self.backends] results = pykka.get_all(futures) From 3857eaa8400a2fd9ca637365dd10935b7d96e19d Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Sat, 5 Jan 2013 23:32:35 +0100 Subject: [PATCH 10/10] core: Unbreak end of track related tests. We should only be hooking up end of track when an audio instance is passed in. Additionally the tracklist tests where wrongly sending in the audio module instead of the mock audio.Audio instance. --- mopidy/core/actor.py | 7 ++++--- tests/backends/base/tracklist.py | 2 +- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/mopidy/core/actor.py b/mopidy/core/actor.py index 1c635799..03ad2712 100644 --- a/mopidy/core/actor.py +++ b/mopidy/core/actor.py @@ -46,9 +46,10 @@ class Core(pykka.ThreadingActor, AudioListener, BackendListener): self.tracklist = TracklistController(core=self) - # Hook up blocking on end of track handler to audio sub-system. - audio.set_on_end_of_track( - lambda: self.actor_ref.proxy().playback.on_end_of_track().get()) + if audio: + # Hook up blocking on end of track handler to audio sub-system. + audio.set_on_end_of_track( + lambda: self.actor_ref.proxy().playback.on_end_of_track().get()) def get_uri_schemes(self): futures = [b.uri_schemes for b in self.backends] diff --git a/tests/backends/base/tracklist.py b/tests/backends/base/tracklist.py index 71f44018..39536edc 100644 --- a/tests/backends/base/tracklist.py +++ b/tests/backends/base/tracklist.py @@ -18,7 +18,7 @@ class TracklistControllerTest(object): def setUp(self): self.audio = mock.Mock(spec=audio.Audio) self.backend = self.backend_class.start(audio=self.audio).proxy() - self.core = core.Core(audio=audio, backends=[self.backend]) + self.core = core.Core(audio=self.audio, backends=[self.backend]) self.controller = self.core.tracklist self.playback = self.core.playback