From e0e6e390bd5a96b2edc048e458075a0ed44f83c2 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Fri, 20 Jun 2014 23:37:55 +0200 Subject: [PATCH 1/6] audio: Refactor message handling to delegate work more. This should allow for easier testing since we can send the parsed result of the messages for all message types. --- mopidy/audio/actor.py | 53 +++++++++++++++++++++++-------------------- 1 file changed, 29 insertions(+), 24 deletions(-) diff --git a/mopidy/audio/actor.py b/mopidy/audio/actor.py index 08c634e9..37223544 100644 --- a/mopidy/audio/actor.py +++ b/mopidy/audio/actor.py @@ -294,31 +294,17 @@ class Audio(pykka.ThreadingActor): self._disconnect(bus, 'message') bus.remove_signal_watch() - def _on_message(self, bus, message): - if (message.type == gst.MESSAGE_STATE_CHANGED - and message.src == self._playbin): - old_state, new_state, pending_state = message.parse_state_changed() - self._on_playbin_state_changed(old_state, new_state, pending_state) - elif message.type == gst.MESSAGE_BUFFERING: - percent = message.parse_buffering() - if percent < 10: - self._playbin.set_state(gst.STATE_PAUSED) - if percent == 100 and self._target_state == gst.STATE_PLAYING: - self._playbin.set_state(gst.STATE_PLAYING) - logger.debug('Buffer %d%% full', percent) - elif message.type == gst.MESSAGE_EOS: + def _on_message(self, bus, msg): + if msg.type == gst.MESSAGE_STATE_CHANGED and msg.src == self._playbin: + self._on_playbin_state_changed(*msg.parse_state_changed()) + elif msg.type == gst.MESSAGE_BUFFERING: + self._on_buffering(msg.parse_buffering()) + elif msg.type == gst.MESSAGE_EOS: self._on_end_of_stream() - elif message.type == gst.MESSAGE_ERROR: - error, debug = message.parse_error() - logger.error( - '%s Debug message: %s', - str(error).decode('utf-8'), debug.decode('utf-8') or 'None') - self.stop_playback() - elif message.type == gst.MESSAGE_WARNING: - error, debug = message.parse_warning() - logger.warning( - '%s Debug message: %s', - str(error).decode('utf-8'), debug.decode('utf-8') or 'None') + elif msg.type == gst.MESSAGE_ERROR: + self._on_error(*msg.parse_error()) + elif msg.type == gst.MESSAGE_WARNING: + self._on_warning(*msg.parse_warning()) def _on_playbin_state_changed(self, old_state, new_state, pending_state): if new_state == gst.STATE_READY and pending_state == gst.STATE_NULL: @@ -349,10 +335,29 @@ class Audio(pykka.ThreadingActor): AudioListener.send( 'state_changed', old_state=old_state, new_state=new_state) + def _on_buffering(self, percent): + if percent < 10: + self._playbin.set_state(gst.STATE_PAUSED) + if percent == 100 and self._target_state == gst.STATE_PLAYING: + self._playbin.set_state(gst.STATE_PLAYING) + + logger.debug('Buffer %d%% full', percent) + def _on_end_of_stream(self): logger.debug('Triggering reached_end_of_stream event') AudioListener.send('reached_end_of_stream') + def _on_error(self, error, debug): + logger.error( + '%s Debug message: %s', + str(error).decode('utf-8'), debug.decode('utf-8') or 'None') + self.stop_playback() + + def _on_warning(self, error, debug): + logger.warning( + '%s Debug message: %s', + str(error).decode('utf-8'), debug.decode('utf-8') or 'None') + def set_uri(self, uri): """ Set URI of audio to be played. From 2e1ca08fef3a463793984e751f94498bda13906d Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Sat, 21 Jun 2014 00:05:59 +0200 Subject: [PATCH 2/6] audio: Use on buffering internal in buferring tests --- tests/audio/test_actor.py | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/tests/audio/test_actor.py b/tests/audio/test_actor.py index 1df4ff18..f767318e 100644 --- a/tests/audio/test_actor.py +++ b/tests/audio/test_actor.py @@ -170,21 +170,13 @@ class AudioBufferingTest(unittest.TestCase): self.audio = audio.Audio(config=None) self.audio._playbin = mock.Mock(spec=['set_state']) - self.buffer_full_message = mock.Mock() - self.buffer_full_message.type = gst.MESSAGE_BUFFERING - self.buffer_full_message.parse_buffering = mock.Mock(return_value=100) - - self.buffer_empty_message = mock.Mock() - self.buffer_empty_message.type = gst.MESSAGE_BUFFERING - self.buffer_empty_message.parse_buffering = mock.Mock(return_value=0) - def test_pause_when_buffer_empty(self): playbin = self.audio._playbin self.audio.start_playback() playbin.set_state.assert_called_with(gst.STATE_PLAYING) playbin.set_state.reset_mock() - self.audio._on_message(None, self.buffer_empty_message) + self.audio._on_buffering(0) playbin.set_state.assert_called_with(gst.STATE_PAUSED) def test_stay_paused_when_buffering_finished(self): @@ -193,5 +185,5 @@ class AudioBufferingTest(unittest.TestCase): playbin.set_state.assert_called_with(gst.STATE_PAUSED) playbin.set_state.reset_mock() - self.audio._on_message(None, self.buffer_full_message) + self.audio._on_buffering(100) self.assertEqual(playbin.set_state.call_count, 0) From 168aa432aac68da2b5b6bf8e3a6c3cd4e074adac Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Sat, 21 Jun 2014 16:30:12 +0200 Subject: [PATCH 3/6] audio: Only trigger buffer pause once. --- mopidy/audio/actor.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/mopidy/audio/actor.py b/mopidy/audio/actor.py index 37223544..68fbe42c 100644 --- a/mopidy/audio/actor.py +++ b/mopidy/audio/actor.py @@ -50,12 +50,13 @@ class Audio(pykka.ThreadingActor): #: The GStreamer state mapped to :class:`mopidy.audio.PlaybackState` state = PlaybackState.STOPPED - _target_state = gst.STATE_NULL def __init__(self, config): super(Audio, self).__init__() self._config = config + self._target_state = gst.STATE_NULL + self._buffering = False self._playbin = None self._signal_ids = {} # {(element, event): signal_id} @@ -336,10 +337,12 @@ class Audio(pykka.ThreadingActor): 'state_changed', old_state=old_state, new_state=new_state) def _on_buffering(self, percent): - if percent < 10: + if percent < 10 and not self._buffering: self._playbin.set_state(gst.STATE_PAUSED) + self._buffering = True if percent == 100 and self._target_state == gst.STATE_PLAYING: self._playbin.set_state(gst.STATE_PLAYING) + self._buffering = False logger.debug('Buffer %d%% full', percent) From 7975815cdea61811408d7fb6e94a6ef09d67bae2 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Sat, 21 Jun 2014 17:28:25 +0200 Subject: [PATCH 4/6] audio: Expand audio state changed with target state --- mopidy/audio/actor.py | 24 +++++++++++++----------- mopidy/audio/listener.py | 16 +++++++++++++++- mopidy/core/actor.py | 2 +- tests/audio/test_listener.py | 7 ++++--- 4 files changed, 33 insertions(+), 16 deletions(-) diff --git a/mopidy/audio/actor.py b/mopidy/audio/actor.py index 68fbe42c..1f8dc746 100644 --- a/mopidy/audio/actor.py +++ b/mopidy/audio/actor.py @@ -23,6 +23,10 @@ mixers.register_mixers() playlists.register_typefinders() playlists.register_elements() +_GST_STATE_MAPPING = { + gst.STATE_PLAYING: PlaybackState.PLAYING, + gst.STATE_PAUSED: PlaybackState.PAUSED, + gst.STATE_NULL: PlaybackState.STOPPED} MB = 1 << 20 @@ -321,20 +325,18 @@ class Audio(pykka.ThreadingActor): if new_state == gst.STATE_READY: return # Ignore READY state as it's GStreamer specific - if new_state == gst.STATE_PLAYING: - new_state = PlaybackState.PLAYING - elif new_state == gst.STATE_PAUSED: - new_state = PlaybackState.PAUSED - elif new_state == gst.STATE_NULL: - new_state = PlaybackState.STOPPED - + new_state = _GST_STATE_MAPPING[new_state] old_state, self.state = self.state, new_state + target_state = _GST_STATE_MAPPING[self._target_state] + if target_state == new_state: + target_state = None + logger.debug( - 'Triggering event: state_changed(old_state=%s, new_state=%s)', - old_state, new_state) - AudioListener.send( - 'state_changed', old_state=old_state, new_state=new_state) + 'Triggering event: state_changed(old_state=%s, new_state=%s, ' + 'target_state=%s)', old_state, new_state, target_state) + AudioListener.send('state_changed', old_state=old_state, + new_state=new_state, target_state=target_state) def _on_buffering(self, percent): if percent < 10 and not self._buffering: diff --git a/mopidy/audio/listener.py b/mopidy/audio/listener.py index 537a81dd..d5203ab9 100644 --- a/mopidy/audio/listener.py +++ b/mopidy/audio/listener.py @@ -27,17 +27,31 @@ class AudioListener(listener.Listener): """ pass - def state_changed(self, old_state, new_state): + def state_changed(self, old_state, new_state, target_state): """ Called after the playback state have changed. Will be called for both immediate and async state changes in GStreamer. + Target state is used to when we should be in the target state, but + temporarily need to switch to an other state. A typical example of this + is buffering. When this happens an event with + `old=PLAYING, new=PAUSED, target=PLAYING` will be emitted. Once we have + caught up a `old=PAUSED, new=PLAYING, target=None` event will be + be generated. + + Regular state changes will not have target state set as they are final + states which should be stable. + *MAY* be implemented by actor. :param old_state: the state before the change :type old_state: string from :class:`mopidy.core.PlaybackState` field :param new_state: the state after the change + :type new_state: A :class:`mopidy.core.PlaybackState` field :type new_state: string from :class:`mopidy.core.PlaybackState` field + :param target_state: the intended state + :type target_state: string from :class:`mopidy.core.PlaybackState` + field or :class:`None` if this is a final state. """ pass diff --git a/mopidy/core/actor.py b/mopidy/core/actor.py index a3dba245..9d80d04c 100644 --- a/mopidy/core/actor.py +++ b/mopidy/core/actor.py @@ -65,7 +65,7 @@ class Core(pykka.ThreadingActor, audio.AudioListener, backend.BackendListener): def reached_end_of_stream(self): self.playback.on_end_of_track() - def state_changed(self, old_state, new_state): + 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 # Spotify play token is lost, the Spotify backend pauses audio diff --git a/tests/audio/test_listener.py b/tests/audio/test_listener.py index 1690ab80..6e0366cf 100644 --- a/tests/audio/test_listener.py +++ b/tests/audio/test_listener.py @@ -15,13 +15,14 @@ class AudioListenerTest(unittest.TestCase): self.listener.state_changed = mock.Mock() self.listener.on_event( - 'state_changed', old_state='stopped', new_state='playing') + 'state_changed', old_state='stopped', new_state='playing', + target_state=None) self.listener.state_changed.assert_called_with( - old_state='stopped', new_state='playing') + old_state='stopped', new_state='playing', target_state=None) def test_listener_has_default_impl_for_reached_end_of_stream(self): self.listener.reached_end_of_stream() def test_listener_has_default_impl_for_state_changed(self): - self.listener.state_changed(None, None) + self.listener.state_changed(None, None, None) From d8c41de2f70fce8526ce913c6bb75202f60cf912 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Mon, 14 Jul 2014 23:15:40 +0200 Subject: [PATCH 5/6] audio: Add tests for state changes while buffering --- mopidy/audio/actor.py | 6 ++++-- tests/audio/test_actor.py | 31 +++++++++++++++++++++++++++++++ 2 files changed, 35 insertions(+), 2 deletions(-) diff --git a/mopidy/audio/actor.py b/mopidy/audio/actor.py index 1f8dc746..68bc367b 100644 --- a/mopidy/audio/actor.py +++ b/mopidy/audio/actor.py @@ -342,9 +342,10 @@ class Audio(pykka.ThreadingActor): if percent < 10 and not self._buffering: self._playbin.set_state(gst.STATE_PAUSED) self._buffering = True - if percent == 100 and self._target_state == gst.STATE_PLAYING: - self._playbin.set_state(gst.STATE_PLAYING) + if percent == 100: self._buffering = False + if self._target_state == gst.STATE_PLAYING: + self._playbin.set_state(gst.STATE_PLAYING) logger.debug('Buffer %d%% full', percent) @@ -484,6 +485,7 @@ class Audio(pykka.ThreadingActor): :rtype: :class:`True` if successfull, else :class:`False` """ + self._buffering = False return self._set_state(gst.STATE_NULL) def _set_state(self, state): diff --git a/tests/audio/test_actor.py b/tests/audio/test_actor.py index f767318e..900c5188 100644 --- a/tests/audio/test_actor.py +++ b/tests/audio/test_actor.py @@ -178,6 +178,7 @@ class AudioBufferingTest(unittest.TestCase): self.audio._on_buffering(0) playbin.set_state.assert_called_with(gst.STATE_PAUSED) + self.assertTrue(self.audio._buffering) def test_stay_paused_when_buffering_finished(self): playbin = self.audio._playbin @@ -187,3 +188,33 @@ class AudioBufferingTest(unittest.TestCase): self.audio._on_buffering(100) self.assertEqual(playbin.set_state.call_count, 0) + self.assertFalse(self.audio._buffering) + + def test_change_to_paused_while_buffering(self): + playbin = self.audio._playbin + self.audio.start_playback() + playbin.set_state.assert_called_with(gst.STATE_PLAYING) + playbin.set_state.reset_mock() + + self.audio._on_buffering(0) + playbin.set_state.assert_called_with(gst.STATE_PAUSED) + self.audio.pause_playback() + playbin.set_state.reset_mock() + + self.audio._on_buffering(100) + self.assertEqual(playbin.set_state.call_count, 0) + self.assertFalse(self.audio._buffering) + + def test_change_to_stopped_while_buffering(self): + playbin = self.audio._playbin + self.audio.start_playback() + playbin.set_state.assert_called_with(gst.STATE_PLAYING) + playbin.set_state.reset_mock() + + self.audio._on_buffering(0) + playbin.set_state.assert_called_with(gst.STATE_PAUSED) + playbin.set_state.reset_mock() + + self.audio.stop_playback() + playbin.set_state.assert_called_with(gst.STATE_NULL) + self.assertFalse(self.audio._buffering) From 4c666e47eefee0596b72486d6bbcab888136207c Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Tue, 15 Jul 2014 00:31:13 +0200 Subject: [PATCH 6/6] core: Ignore buffering for now --- mopidy/core/actor.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/mopidy/core/actor.py b/mopidy/core/actor.py index 9d80d04c..429dd2ef 100644 --- a/mopidy/core/actor.py +++ b/mopidy/core/actor.py @@ -72,7 +72,10 @@ class Core(pykka.ThreadingActor, audio.AudioListener, backend.BackendListener): # playback, but mopidy.core doesn't know this, so we need to update # mopidy.core's state to match the actual state in mopidy.audio. If we # don't do this, clients will think that we're still playing. - if (new_state == PlaybackState.PAUSED + + # We ignore cases when target state is set as this is buffering + # updates (at least for now) and we need to get #234 fixed... + if (new_state == PlaybackState.PAUSED and not target_state and self.playback.state != PlaybackState.PAUSED): self.playback.state = new_state self.playback._trigger_track_playback_paused()