Merge remote-tracking branch 'adamcik/fix/gh-734-cleanup-audio-buffering' into develop

Conflicts:
	mopidy/audio/actor.py

mopidy/audio/actor.py

@@ -21,6 +21,10 @@ logger = logging.getLogger(__name__)
 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
 
@@ -48,13 +52,14 @@ 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, mixer):
         super(Audio, self).__init__()
 
         self._config = config
         self._mixer = mixer
+        self._target_state = gst.STATE_NULL
+        self._buffering = False
 
         self._playbin = None
         self._signal_ids = {}  # {(element, event): signal_id}
@@ -224,31 +229,17 @@ class Audio(pykka.ThreadingActor):
         self._disconnect(bus, 'message')
         bus.remove_signal_watch()
 
-    def _on_message(self, bus, message):
+    def _on_message(self, bus, msg):
-        if (message.type == gst.MESSAGE_STATE_CHANGED
+        if msg.type == gst.MESSAGE_STATE_CHANGED and msg.src == self._playbin:
-                and message.src == self._playbin):
+            self._on_playbin_state_changed(*msg.parse_state_changed())
-            old_state, new_state, pending_state = message.parse_state_changed()
+        elif msg.type == gst.MESSAGE_BUFFERING:
-            self._on_playbin_state_changed(old_state, new_state, pending_state)
+            self._on_buffering(msg.parse_buffering())
-        elif message.type == gst.MESSAGE_BUFFERING:
+        elif msg.type == gst.MESSAGE_EOS:
-            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:
             self._on_end_of_stream()
-        elif message.type == gst.MESSAGE_ERROR:
+        elif msg.type == gst.MESSAGE_ERROR:
-            error, debug = message.parse_error()
+            self._on_error(*msg.parse_error())
-            logger.error(
+        elif msg.type == gst.MESSAGE_WARNING:
-                '%s Debug message: %s',
+            self._on_warning(*msg.parse_warning())
-                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')
 
     def _on_playbin_state_changed(self, old_state, new_state, pending_state):
         if new_state == gst.STATE_READY and pending_state == gst.STATE_NULL:
@@ -264,25 +255,45 @@ 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 = _GST_STATE_MAPPING[new_state]
-            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
 
+        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)',
+            'Triggering event: state_changed(old_state=%s, new_state=%s, '
-            old_state, new_state)
+            'target_state=%s)', old_state, new_state, target_state)
-        AudioListener.send(
+        AudioListener.send('state_changed', old_state=old_state,
-            'state_changed', old_state=old_state, new_state=new_state)
+                           new_state=new_state, target_state=target_state)
+
+    def _on_buffering(self, percent):
+        if percent < 10 and not self._buffering:
+            self._playbin.set_state(gst.STATE_PAUSED)
+            self._buffering = True
+        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)
 
     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.
@@ -404,6 +415,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):

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

mopidy/core/actor.py

@@ -68,14 +68,17 @@ class Core(
     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
         # 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()

tests/audio/test_actor.py

@@ -142,22 +142,15 @@ class AudioBufferingTest(unittest.TestCase):
         self.audio = audio.Audio(config=None, mixer=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)
+        self.assertTrue(self.audio._buffering)
 
     def test_stay_paused_when_buffering_finished(self):
         playbin = self.audio._playbin
@@ -165,5 +158,35 @@ 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)
+        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)

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)