From 42fdaf3ff01440445f134d392f6014365f394968 Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Tue, 13 Nov 2012 14:02:57 +0100 Subject: [PATCH 1/8] audio: Move tests to make room for more audio tests --- tests/audio/__init__.py | 0 tests/{audio_test.py => audio/actor_test.py} | 0 2 files changed, 0 insertions(+), 0 deletions(-) create mode 100644 tests/audio/__init__.py rename tests/{audio_test.py => audio/actor_test.py} (100%) diff --git a/tests/audio/__init__.py b/tests/audio/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/tests/audio_test.py b/tests/audio/actor_test.py similarity index 100% rename from tests/audio_test.py rename to tests/audio/actor_test.py From 76b1fa8e1bc7efa89533d90f2fbf23a8619a512c Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Tue, 13 Nov 2012 20:31:38 +0100 Subject: [PATCH 2/8] audio: Add state_changed(old_state, new_state) event --- mopidy/audio/listener.py | 15 +++++++++++++++ tests/audio/listener_test.py | 16 ++++++++++++++++ 2 files changed, 31 insertions(+) create mode 100644 tests/audio/listener_test.py diff --git a/mopidy/audio/listener.py b/mopidy/audio/listener.py index 42c85e1e..da5f7b39 100644 --- a/mopidy/audio/listener.py +++ b/mopidy/audio/listener.py @@ -28,3 +28,18 @@ class AudioListener(object): *MAY* be implemented by actor. """ pass + + def state_changed(self, old_state, new_state): + """ + Called after the playback state have changed. + + Will be called for both immediate and async state changes in GStreamer. + + *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: string from :class:`mopidy.core.PlaybackState` field + """ + pass diff --git a/tests/audio/listener_test.py b/tests/audio/listener_test.py new file mode 100644 index 00000000..b3274721 --- /dev/null +++ b/tests/audio/listener_test.py @@ -0,0 +1,16 @@ +from __future__ import unicode_literals + +from mopidy import audio + +from tests import unittest + + +class AudioListenerTest(unittest.TestCase): + def setUp(self): + self.listener = audio.AudioListener() + + 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) From f9bd0d00b3ae187dbe1e441a154508cb718f2c95 Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Wed, 14 Nov 2012 00:35:17 +0100 Subject: [PATCH 3/8] audio: Move PlaybackState from core to audio so audio can use it --- mopidy/audio/__init__.py | 1 + mopidy/audio/constants.py | 16 ++++++++++++++++ mopidy/core/playback.py | 17 ++--------------- 3 files changed, 19 insertions(+), 15 deletions(-) create mode 100644 mopidy/audio/constants.py diff --git a/mopidy/audio/__init__.py b/mopidy/audio/__init__.py index c3fbc0c9..7cf1dcee 100644 --- a/mopidy/audio/__init__.py +++ b/mopidy/audio/__init__.py @@ -3,3 +3,4 @@ from __future__ import unicode_literals # flake8: noqa from .actor import Audio from .listener import AudioListener +from .constants import PlaybackState diff --git a/mopidy/audio/constants.py b/mopidy/audio/constants.py new file mode 100644 index 00000000..08ad9768 --- /dev/null +++ b/mopidy/audio/constants.py @@ -0,0 +1,16 @@ +from __future__ import unicode_literals + + +class PlaybackState(object): + """ + Enum of playback states. + """ + + #: Constant representing the paused state. + PAUSED = 'paused' + + #: Constant representing the playing state. + PLAYING = 'playing' + + #: Constant representing the stopped state. + STOPPED = 'stopped' diff --git a/mopidy/core/playback.py b/mopidy/core/playback.py index 54364ec2..273eb68d 100644 --- a/mopidy/core/playback.py +++ b/mopidy/core/playback.py @@ -4,6 +4,8 @@ import logging import random import urlparse +from mopidy.audio import PlaybackState + from . import listener @@ -24,21 +26,6 @@ def option_wrapper(name, default): return property(get_option, set_option) -class PlaybackState(object): - """ - Enum of playback states. - """ - - #: Constant representing the paused state. - PAUSED = 'paused' - - #: Constant representing the playing state. - PLAYING = 'playing' - - #: Constant representing the stopped state. - STOPPED = 'stopped' - - class PlaybackController(object): # pylint: disable = R0902 # Too many instance attributes From 87ce7bbe115aabd107149e4730b7c0a1e83b6b78 Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Wed, 14 Nov 2012 01:49:05 +0100 Subject: [PATCH 4/8] audio: Maintain state and trigger events based on GStreamer state changes --- mopidy/audio/actor.py | 44 +++++++++++++++++++++++++++++++---- tests/audio/actor_test.py | 49 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 88 insertions(+), 5 deletions(-) diff --git a/mopidy/audio/actor.py b/mopidy/audio/actor.py index a7b4e8d8..49794c76 100644 --- a/mopidy/audio/actor.py +++ b/mopidy/audio/actor.py @@ -13,6 +13,7 @@ from mopidy import settings from mopidy.utils import process from . import mixers +from .constants import PlaybackState from .listener import AudioListener logger = logging.getLogger('mopidy.audio') @@ -29,9 +30,11 @@ class Audio(pykka.ThreadingActor): - :attr:`mopidy.settings.OUTPUT` - :attr:`mopidy.settings.MIXER` - :attr:`mopidy.settings.MIXER_TRACK` - """ + #: The GStreamer state mapped to :class:`mopidy.audio.PlaybackState` + state = PlaybackState.STOPPED + def __init__(self): super(Audio, self).__init__() @@ -160,8 +163,12 @@ class Audio(pykka.ThreadingActor): bus.remove_signal_watch() def _on_message(self, bus, message): - if message.type == gst.MESSAGE_EOS: - self._trigger_reached_end_of_stream_event() + 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_EOS: + self._on_end_of_stream() elif message.type == gst.MESSAGE_ERROR: error, debug = message.parse_error() logger.error('%s %s', error, debug) @@ -170,8 +177,35 @@ class Audio(pykka.ThreadingActor): error, debug = message.parse_warning() logger.warning('%s %s', error, debug) - def _trigger_reached_end_of_stream_event(self): - logger.debug('Triggering reached end of stream event') + def _on_playbin_state_changed(self, old_state, new_state, pending_state): + if new_state == gst.STATE_READY and pending_state == gst.STATE_NULL: + # XXX: We're not called on the last state cheng when going down to + # NULL, so we rewrite the second to last call to get the expected + # behavior. + new_state = gst.STATE_NULL + pending_state = gst.STATE_VOID_PENDING + + if pending_state != gst.STATE_VOID_PENDING: + return # Ignore intermediate state changes + + 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 + + old_state, self.state = self.state, new_state + + logger.debug('Triggering state_changed event') + AudioListener.send('state_changed', + old_state=old_state, new_state=new_state) + + def _on_end_of_stream(self): + logger.debug('Triggering reached_end_of_stream event') AudioListener.send('reached_end_of_stream') def set_uri(self, uri): diff --git a/tests/audio/actor_test.py b/tests/audio/actor_test.py index b8b65e83..64666d9d 100644 --- a/tests/audio/actor_test.py +++ b/tests/audio/actor_test.py @@ -1,5 +1,9 @@ from __future__ import unicode_literals +import pygst +pygst.require('0.10') +import gst + from mopidy import audio, settings from mopidy.utils.path import path_to_uri @@ -63,3 +67,48 @@ class AudioTest(unittest.TestCase): @unittest.SkipTest def test_invalid_output_raises_error(self): pass # TODO + + +class AudioStateTest(unittest.TestCase): + def setUp(self): + self.audio = audio.Audio() + + def test_state_starts_as_stopped(self): + self.assertEqual(audio.PlaybackState.STOPPED, self.audio.state) + + def test_state_does_not_change_when_in_gst_ready_state(self): + self.audio._on_playbin_state_changed( + gst.STATE_NULL, gst.STATE_READY, gst.STATE_VOID_PENDING) + + self.assertEqual(audio.PlaybackState.STOPPED, self.audio.state) + + def test_state_changes_from_stopped_to_playing_on_play(self): + self.audio._on_playbin_state_changed( + gst.STATE_NULL, gst.STATE_READY, gst.STATE_PLAYING) + self.audio._on_playbin_state_changed( + gst.STATE_READY, gst.STATE_PAUSED, gst.STATE_PLAYING) + self.audio._on_playbin_state_changed( + gst.STATE_PAUSED, gst.STATE_PLAYING, gst.STATE_VOID_PENDING) + + self.assertEqual(audio.PlaybackState.PLAYING, self.audio.state) + + def test_state_changes_from_playing_to_paused_on_pause(self): + self.audio.state = audio.PlaybackState.PLAYING + + self.audio._on_playbin_state_changed( + gst.STATE_PLAYING, gst.STATE_PAUSED, gst.STATE_VOID_PENDING) + + self.assertEqual(audio.PlaybackState.PAUSED, self.audio.state) + + def test_state_changes_from_playing_to_stopped_on_stop(self): + self.audio.state = audio.PlaybackState.PLAYING + + self.audio._on_playbin_state_changed( + gst.STATE_PLAYING, gst.STATE_PAUSED, gst.STATE_NULL) + self.audio._on_playbin_state_changed( + gst.STATE_PAUSED, gst.STATE_READY, gst.STATE_NULL) + # We never get the following call, so the logic must work without it + #self.audio._on_playbin_state_changed( + # gst.STATE_READY, gst.STATE_NULL, gst.STATE_VOID_PENDING) + + self.assertEqual(audio.PlaybackState.STOPPED, self.audio.state) From 9168982a6173dd9da0c4f441a5a71d5859956f2b Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Wed, 14 Nov 2012 01:56:46 +0100 Subject: [PATCH 5/8] core: Pause playback if audio is paused and playback isn't (fixes #232) --- mopidy/core/actor.py | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/mopidy/core/actor.py b/mopidy/core/actor.py index 731e5309..858eeaf9 100644 --- a/mopidy/core/actor.py +++ b/mopidy/core/actor.py @@ -4,7 +4,7 @@ import itertools import pykka -from mopidy.audio import AudioListener +from mopidy.audio import AudioListener, PlaybackState from .library import LibraryController from .playback import PlaybackController @@ -55,6 +55,14 @@ class Core(pykka.ThreadingActor, AudioListener): def reached_end_of_stream(self): self.playback.on_end_of_track() + def state_changed(self, old_state, new_state): + # XXX: This is a temporary fix for issue #232 while we wait for a more + # permanent solution with the implementation of issue #234. + if (new_state == PlaybackState.PAUSED + and self.playback.state != PlaybackState.PAUSED): + self.playback.state = new_state + self.playback._trigger_track_playback_paused() + class Backends(list): def __init__(self, backends): From f0c8c2287fe0741d60cb8291ae6f21ce8d4160a8 Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Wed, 14 Nov 2012 23:37:27 +0100 Subject: [PATCH 6/8] audio: Fix typo --- mopidy/audio/actor.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mopidy/audio/actor.py b/mopidy/audio/actor.py index 49794c76..1355cfef 100644 --- a/mopidy/audio/actor.py +++ b/mopidy/audio/actor.py @@ -179,7 +179,7 @@ class Audio(pykka.ThreadingActor): def _on_playbin_state_changed(self, old_state, new_state, pending_state): if new_state == gst.STATE_READY and pending_state == gst.STATE_NULL: - # XXX: We're not called on the last state cheng when going down to + # XXX: We're not called on the last state change when going down to # NULL, so we rewrite the second to last call to get the expected # behavior. new_state = gst.STATE_NULL From 36a14bd8d60786bb7b392261ad0f2a7a59fe30d6 Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Wed, 14 Nov 2012 23:40:09 +0100 Subject: [PATCH 7/8] audio: Make log message more useful --- mopidy/audio/actor.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/mopidy/audio/actor.py b/mopidy/audio/actor.py index 1355cfef..f2208a0a 100644 --- a/mopidy/audio/actor.py +++ b/mopidy/audio/actor.py @@ -200,7 +200,9 @@ class Audio(pykka.ThreadingActor): old_state, self.state = self.state, new_state - logger.debug('Triggering state_changed event') + 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) From 326970e5adcb815e3249c0c249bdbfdc1943990d Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Wed, 14 Nov 2012 23:42:33 +0100 Subject: [PATCH 8/8] core: Explain reason behind temporary state syncing hack --- mopidy/core/actor.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/mopidy/core/actor.py b/mopidy/core/actor.py index 858eeaf9..3ebb785b 100644 --- a/mopidy/core/actor.py +++ b/mopidy/core/actor.py @@ -57,7 +57,11 @@ class Core(pykka.ThreadingActor, AudioListener): def state_changed(self, old_state, new_state): # XXX: This is a temporary fix for issue #232 while we wait for a more - # permanent solution with the implementation of issue #234. + # permanent solution with the implementation of issue #234. When the + # Spotify play token is lost, the Spotify backend pauses audio + # 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 and self.playback.state != PlaybackState.PAUSED): self.playback.state = new_state