diff --git a/.travis.yml b/.travis.yml index 68aedd94..cbfe0d11 100644 --- a/.travis.yml +++ b/.travis.yml @@ -1,24 +1,21 @@ -sudo: false +sudo: required +dist: trusty language: python python: - "2.7_with_system_site_packages" -addons: - apt: - sources: - - mopidy-stable - packages: - - libffi-dev - - libspotify-dev - - mopidy - - python-all-dev - env: - TOX_ENV=py27 - TOX_ENV=flake8 +before_install: + - "wget -q -O - https://apt.mopidy.com/mopidy.gpg | sudo apt-key add -" + - "sudo wget -q -O /etc/apt/sources.list.d/mopidy.list https://apt.mopidy.com/jessie.list" + - "sudo apt-get update -qq" + - "sudo apt-get install -y python-gst0.10 libffi-dev libspotify-dev python-all-dev" + install: - "pip install tox" diff --git a/README.rst b/README.rst index 7e3019b3..9c2d8ee6 100644 --- a/README.rst +++ b/README.rst @@ -137,9 +137,41 @@ Project resources - `Issue tracker `_ +Credits +======= + +- Original author: `Stein Magnus Jodal `__ +- Current maintainer: `Stein Magnus Jodal `__ +- `Contributors `_ + + Changelog ========= +v2.3.0 (2016-02-06) +------------------- + +Feature release. + +- Ignore all audio data deliveries from libspotify when when a seek is in + progress. This ensures that we don't deliver audio data from before the seek + with timestamps from after the seek. + +- Ignore duplicate end of track callbacks. + +- Don't increase the audio buffer timestamp if the buffer is rejected by + Mopidy. This caused audio buffers delivered after one or more rejected audio + buffers to have too high timestamps. + +- When changing tracks, block until Mopidy completes the appsrc URI change. + Not blocking here might break gapless playback. + +- Lookup of a playlist you're not subscribed to will now properly load all of + the playlist's tracks. (Fixes: #81, PR: #82) + +- Workaround teardown race outputing lots of short stack traces on Mopidy + shutdown. (See #73 for details) + v2.2.0 (2015-11-15) ------------------- diff --git a/mopidy_spotify/__init__.py b/mopidy_spotify/__init__.py index de44ceec..4a5ddc52 100644 --- a/mopidy_spotify/__init__.py +++ b/mopidy_spotify/__init__.py @@ -5,7 +5,7 @@ from mopidy import config, ext -__version__ = '2.2.0' +__version__ = '2.3.0' class Extension(ext.Extension): diff --git a/mopidy_spotify/library.py b/mopidy_spotify/library.py index 8be624e1..3e95580b 100644 --- a/mopidy_spotify/library.py +++ b/mopidy_spotify/library.py @@ -4,8 +4,10 @@ from mopidy import backend -import mopidy_spotify -from mopidy_spotify import browse, distinct, images, lookup, search, utils +# Workaround https://github.com/public/flake8-import-order/issues/49: +from mopidy_spotify import Extension +from mopidy_spotify import ( + __version__, browse, distinct, images, lookup, search, utils) logger = logging.getLogger(__name__) @@ -19,9 +21,7 @@ def __init__(self, backend): self._config = backend._config['spotify'] self._requests_session = utils.get_requests_session( proxy_config=backend._config['proxy'], - user_agent='%s/%s' % ( - mopidy_spotify.Extension.dist_name, - mopidy_spotify.__version__)) + user_agent='%s/%s' % (Extension.dist_name, __version__)) def browse(self, uri): return browse.browse(self._config, self._backend._session, uri) diff --git a/mopidy_spotify/lookup.py b/mopidy_spotify/lookup.py index f1f1408e..48a50d00 100644 --- a/mopidy_spotify/lookup.py +++ b/mopidy_spotify/lookup.py @@ -94,6 +94,7 @@ def _lookup_playlist(config, sp_link): sp_playlist = sp_link.as_playlist() sp_playlist.load() for sp_track in sp_playlist.tracks: + sp_track.load() track = translator.to_track( sp_track, bitrate=config['bitrate']) if track is not None: diff --git a/mopidy_spotify/playback.py b/mopidy_spotify/playback.py index 5c888147..80f78cc5 100644 --- a/mopidy_spotify/playback.py +++ b/mopidy_spotify/playback.py @@ -29,9 +29,11 @@ def __init__(self, *args, **kwargs): self._timeout = self.backend._config['spotify']['timeout'] self._buffer_timestamp = BufferTimestamp(0) + self._seeking_event = threading.Event() self._first_seek = False self._push_audio_data_event = threading.Event() self._push_audio_data_event.set() + self._end_of_track_event = threading.Event() self._events_connected = False def _connect_events(self): @@ -39,11 +41,11 @@ def _connect_events(self): self._events_connected = True self.backend._session.on( spotify.SessionEvent.MUSIC_DELIVERY, music_delivery_callback, - self.audio, self._push_audio_data_event, + self.audio, self._seeking_event, self._push_audio_data_event, self._buffer_timestamp) self.backend._session.on( spotify.SessionEvent.END_OF_TRACK, end_of_track_callback, - self.audio) + self._end_of_track_event, self.audio) def change_track(self, track): self._connect_events() @@ -51,15 +53,21 @@ def change_track(self, track): if track.uri is None: return False + logger.debug( + 'Audio requested change of track; ' + 'loading and starting Spotify player') + need_data_callback_bound = functools.partial( need_data_callback, self._push_audio_data_event) enough_data_callback_bound = functools.partial( enough_data_callback, self._push_audio_data_event) seek_data_callback_bound = functools.partial( - seek_data_callback, self.backend._actor_proxy) + seek_data_callback, self._seeking_event, self.backend._actor_proxy) + self._buffer_timestamp.set(0) self._first_seek = True + self._end_of_track_event.clear() try: sp_track = self.backend._session.get_track(track.uri) @@ -67,31 +75,37 @@ def change_track(self, track): self.backend._session.player.load(sp_track) self.backend._session.player.play() - self._buffer_timestamp.set(0) - self.audio.set_appsrc( + future = self.audio.set_appsrc( LIBSPOTIFY_GST_CAPS, need_data=need_data_callback_bound, enough_data=enough_data_callback_bound, seek_data=seek_data_callback_bound) self.audio.set_metadata(track) + # Gapless playback requires that we block until URI change in + # mopidy.audio has completed before we return from change_track(). + future.get() + return True except spotify.Error as exc: logger.info('Playback of %s failed: %s', track.uri, exc) return False def resume(self): + logger.debug('Audio requested resume; starting Spotify player') self.backend._session.player.play() return super(SpotifyPlaybackProvider, self).resume() def stop(self): + logger.debug('Audio requested stop; pausing Spotify player') self.backend._session.player.pause() return super(SpotifyPlaybackProvider, self).stop() def on_seek_data(self, time_position): - logger.debug('Audio asked us to seek to %d', time_position) + logger.debug('Audio requested seek to %d', time_position) if time_position == 0 and self._first_seek: + self._seeking_event.clear() self._first_seek = False logger.debug('Skipping seek due to issue mopidy/mopidy#300') return @@ -105,7 +119,7 @@ def need_data_callback(push_audio_data_event, length_hint): # This callback is called from GStreamer/the GObject event loop. logger.log( TRACE_LOG_LEVEL, - 'Audio asked for more data (hint=%d); accepting deliveries', + 'Audio requested more data (hint=%d); accepting deliveries', length_hint) push_audio_data_event.set() @@ -113,24 +127,37 @@ def need_data_callback(push_audio_data_event, length_hint): def enough_data_callback(push_audio_data_event): # This callback is called from GStreamer/the GObject event loop. logger.log( - TRACE_LOG_LEVEL, 'Audio says it has enough data; rejecting deliveries') + TRACE_LOG_LEVEL, 'Audio has enough data; rejecting deliveries') push_audio_data_event.clear() -def seek_data_callback(spotify_backend, time_position): +def seek_data_callback(seeking_event, spotify_backend, time_position): # This callback is called from GStreamer/the GObject event loop. # It forwards the call to the backend actor. + seeking_event.set() spotify_backend.playback.on_seek_data(time_position) def music_delivery_callback( session, audio_format, frames, num_frames, - audio_actor, push_audio_data_event, buffer_timestamp): + audio_actor, seeking_event, push_audio_data_event, buffer_timestamp): # This is called from an internal libspotify thread. # Ideally, nothing here should block. + if seeking_event.is_set(): + # A seek has happened, but libspotify hasn't confirmed yet, so + # we're dropping all audio data from libspotify. + if num_frames == 0: + # libspotify signals that it has completed the seek. We'll accept + # the next audio data delivery. + seeking_event.clear() + return num_frames + if not push_audio_data_event.is_set(): - return 0 + return 0 # Reject the audio data. It will be redelivered later. + + if not frames: + return 0 # No audio data; return immediately. known_format = ( audio_format.sample_type == spotify.SampleType.INT16_NATIVE_ENDIAN) @@ -149,25 +176,30 @@ def music_delivery_callback( 'sample_rate': audio_format.sample_rate, } - duration = audio.calculate_duration( - num_frames, audio_format.sample_rate) + duration = audio.calculate_duration(num_frames, audio_format.sample_rate) buffer_ = audio.create_buffer( bytes(frames), capabilites=capabilites, timestamp=buffer_timestamp.get(), duration=duration) - buffer_timestamp.increase(duration) - # We must block here to know if the buffer was consumed successfully. - if audio_actor.emit_data(buffer_).get(): + consumed = audio_actor.emit_data(buffer_).get() + + if consumed: + buffer_timestamp.increase(duration) return num_frames else: return 0 -def end_of_track_callback(session, audio_actor): +def end_of_track_callback(session, end_of_track_event, audio_actor): # This callback is called from the pyspotify event loop. + if end_of_track_event.is_set(): + logger.debug('End of track already received; ignoring callback') + return + logger.debug('End of track reached') + end_of_track_event.set() audio_actor.emit_data(None) diff --git a/tests/test_lookup.py b/tests/test_lookup.py index 80112c53..b0b332b0 100644 --- a/tests/test_lookup.py +++ b/tests/test_lookup.py @@ -144,6 +144,7 @@ def test_lookup_of_playlist_uri(session_mock, sp_playlist_mock, provider): session_mock.get_link.assert_called_once_with('spotify:playlist:alice:foo') sp_playlist_mock.link.as_playlist.assert_called_once_with() sp_playlist_mock.load.assert_called_once_with() + sp_playlist_mock.tracks[0].load.assert_called_once_with() assert len(results) == 1 track = results[0] diff --git a/tests/test_playback.py b/tests/test_playback.py index 81743943..57f6b09e 100644 --- a/tests/test_playback.py +++ b/tests/test_playback.py @@ -61,6 +61,7 @@ def test_connect_events_adds_music_delivery_handler_to_session( spotify.SessionEvent.MUSIC_DELIVERY, playback.music_delivery_callback, audio_mock, + playback_provider._seeking_event, playback_provider._push_audio_data_event, playback_provider._buffer_timestamp) in session_mock.on.call_args_list) @@ -74,7 +75,8 @@ def test_connect_events_adds_end_of_track_handler_to_session( assert (mock.call( spotify.SessionEvent.END_OF_TRACK, - playback.end_of_track_callback, audio_mock) + playback.end_of_track_callback, + playback_provider._end_of_track_event, audio_mock) in session_mock.on.call_args_list) @@ -140,11 +142,13 @@ def test_on_seek_data_updates_timestamp_and_seeks_in_spotify( def test_on_seek_data_ignores_first_seek_to_zero_on_every_play( session_mock, provider): + provider._seeking_event.set() track = models.Track(uri='spotfy:track:test') provider.change_track(track) provider.on_seek_data(0) + assert not provider._seeking_event.is_set() assert session_mock.player.seek.call_count == 0 @@ -168,36 +172,103 @@ def test_enough_data_callback(): def test_seek_data_callback(): + seeking_event = threading.Event() backend_mock = mock.Mock() - playback.seek_data_callback(backend_mock, 1340) + playback.seek_data_callback(seeking_event, backend_mock, 1340) + assert seeking_event.is_set() backend_mock.playback.on_seek_data.assert_called_once_with(1340) -def test_music_delivery_rejects_data_depending_on_push_audio_data_event( - session_mock): +def test_music_delivery_rejects_data_when_seeking(session_mock, audio_mock): + audio_format = mock.Mock() + frames = b'123' + num_frames = 1 + seeking_event = threading.Event() + seeking_event.set() + push_audio_data_event = threading.Event() + push_audio_data_event.set() + buffer_timestamp = mock.Mock() + assert seeking_event.is_set() + + result = playback.music_delivery_callback( + session_mock, audio_format, frames, num_frames, + audio_mock, seeking_event, push_audio_data_event, buffer_timestamp) + + assert seeking_event.is_set() + assert audio_mock.emit_data.call_count == 0 + assert result == num_frames + + +def test_music_delivery_when_seeking_accepts_data_after_empty_delivery( + session_mock, audio_mock): audio_format = mock.Mock() frames = b'' num_frames = 0 + seeking_event = threading.Event() + seeking_event.set() + push_audio_data_event = threading.Event() + push_audio_data_event.set() + buffer_timestamp = mock.Mock() + assert seeking_event.is_set() + + result = playback.music_delivery_callback( + session_mock, audio_format, frames, num_frames, + audio_mock, seeking_event, push_audio_data_event, buffer_timestamp) + + assert not seeking_event.is_set() + assert audio_mock.emit_data.call_count == 0 + assert result == num_frames + + +def test_music_delivery_rejects_data_depending_on_push_audio_data_event( + session_mock, audio_mock): + + audio_format = mock.Mock() + frames = b'123' + num_frames = 1 + seeking_event = threading.Event() push_audio_data_event = threading.Event() buffer_timestamp = mock.Mock() assert not push_audio_data_event.is_set() result = playback.music_delivery_callback( session_mock, audio_format, frames, num_frames, - audio_mock, push_audio_data_event, buffer_timestamp) + audio_mock, seeking_event, push_audio_data_event, buffer_timestamp) + assert audio_mock.emit_data.call_count == 0 assert result == 0 +def test_music_delivery_shortcuts_if_no_data_in_frames( + session_mock, audio_lib_mock, audio_mock): + + audio_format = mock.Mock(channels=2, sample_rate=44100, sample_type=0) + frames = b'' + num_frames = 1 + seeking_event = threading.Event() + push_audio_data_event = threading.Event() + push_audio_data_event.set() + buffer_timestamp = mock.Mock() + + result = playback.music_delivery_callback( + session_mock, audio_format, frames, num_frames, + audio_mock, seeking_event, push_audio_data_event, buffer_timestamp) + + assert result == 0 + assert audio_lib_mock.create_buffer.call_count == 0 + assert audio_mock.emit_data.call_count == 0 + + def test_music_delivery_rejects_unknown_audio_formats( session_mock, audio_mock): audio_format = mock.Mock(sample_type=17) - frames = b'' - num_frames = 0 + frames = b'123' + num_frames = 1 + seeking_event = threading.Event() push_audio_data_event = threading.Event() push_audio_data_event.set() buffer_timestamp = mock.Mock() @@ -205,7 +276,7 @@ def test_music_delivery_rejects_unknown_audio_formats( with pytest.raises(AssertionError) as excinfo: playback.music_delivery_callback( session_mock, audio_format, frames, num_frames, - audio_mock, push_audio_data_event, buffer_timestamp) + audio_mock, seeking_event, push_audio_data_event, buffer_timestamp) assert 'Expects 16-bit signed integer samples' in str(excinfo.value) @@ -219,6 +290,7 @@ def test_music_delivery_creates_gstreamer_buffer_and_gives_it_to_audio( audio_format = mock.Mock(channels=2, sample_rate=44100, sample_type=0) frames = b'\x00\x00' num_frames = 1 + seeking_event = threading.Event() push_audio_data_event = threading.Event() push_audio_data_event.set() buffer_timestamp = mock.Mock() @@ -226,7 +298,7 @@ def test_music_delivery_creates_gstreamer_buffer_and_gives_it_to_audio( result = playback.music_delivery_callback( session_mock, audio_format, frames, num_frames, - audio_mock, push_audio_data_event, buffer_timestamp) + audio_mock, seeking_event, push_audio_data_event, buffer_timestamp) audio_lib_mock.calculate_duration.assert_called_once_with(1, 44100) audio_lib_mock.create_buffer.assert_called_once_with( @@ -245,6 +317,7 @@ def test_music_delivery_consumes_zero_frames_if_audio_fails( audio_format = mock.Mock(channels=2, sample_rate=44100, sample_type=0) frames = b'\x00\x00' num_frames = 1 + seeking_event = threading.Event() push_audio_data_event = threading.Event() push_audio_data_event.set() buffer_timestamp = mock.Mock() @@ -252,17 +325,33 @@ def test_music_delivery_consumes_zero_frames_if_audio_fails( result = playback.music_delivery_callback( session_mock, audio_format, frames, num_frames, - audio_mock, push_audio_data_event, buffer_timestamp) + audio_mock, seeking_event, push_audio_data_event, buffer_timestamp) + assert buffer_timestamp.increase.call_count == 0 assert result == 0 def test_end_of_track_callback(session_mock, audio_mock): - playback.end_of_track_callback(session_mock, audio_mock) + end_of_track_event = threading.Event() + + playback.end_of_track_callback( + session_mock, end_of_track_event, audio_mock) + assert end_of_track_event.is_set() audio_mock.emit_data.assert_called_once_with(None) +def test_duplicate_end_of_track_callback_is_ignored(session_mock, audio_mock): + end_of_track_event = threading.Event() + end_of_track_event.set() + + playback.end_of_track_callback( + session_mock, end_of_track_event, audio_mock) + + assert end_of_track_event.is_set() + assert audio_mock.emit_data.call_count == 0 + + def test_buffer_timestamp_wrapper(): wrapper = playback.BufferTimestamp(0) assert wrapper.get() == 0 diff --git a/tox.ini b/tox.ini index ae93a39f..c51729e4 100644 --- a/tox.ini +++ b/tox.ini @@ -4,9 +4,7 @@ envlist = py27, flake8 [testenv] sitepackages = true deps = - mopidy==dev -rdev-requirements.txt -install_command = pip install --allow-unverified=mopidy --pre {opts} {packages} commands = py.test \ --basetemp={envtmpdir} \