From fab020f2d0b202634c4f4ddfc4296eafc669e071 Mon Sep 17 00:00:00 2001 From: sandos Date: Mon, 12 Dec 2011 22:22:03 +0100 Subject: [PATCH 01/14] performance of playlistinfo and status not dependent on playlist length --- mopidy/backends/base/current_playlist.py | 10 ++++++++++ mopidy/frontends/mpd/protocol/current_playlist.py | 6 ++++++ mopidy/frontends/mpd/protocol/status.py | 4 ++-- 3 files changed, 18 insertions(+), 2 deletions(-) diff --git a/mopidy/backends/base/current_playlist.py b/mopidy/backends/base/current_playlist.py index 17125ac0..367f9c5d 100644 --- a/mopidy/backends/base/current_playlist.py +++ b/mopidy/backends/base/current_playlist.py @@ -1,6 +1,7 @@ from copy import copy import logging import random +import pprint from mopidy.listeners import BackendListener from mopidy.models import CpTrack @@ -28,6 +29,7 @@ class CurrentPlaylistController(object): Read-only. """ + logger.debug(u'current_playlist.cp_tracks') return [copy(ct) for ct in self._cp_tracks] @property @@ -37,8 +39,16 @@ class CurrentPlaylistController(object): Read-only. """ + logger.debug(u'current_playlist.tracks()') return [ct[1] for ct in self._cp_tracks] + @property + def tracks_len(self): + """ + Length of current playlist + """ + return len(self._cp_tracks) + @property def version(self): """ diff --git a/mopidy/frontends/mpd/protocol/current_playlist.py b/mopidy/frontends/mpd/protocol/current_playlist.py index c7136804..c566bf7e 100644 --- a/mopidy/frontends/mpd/protocol/current_playlist.py +++ b/mopidy/frontends/mpd/protocol/current_playlist.py @@ -254,6 +254,12 @@ def playlistinfo(context, songpos=None, end = songpos + 1 if start == -1: end = None + else: + #Fetch one single track, hot code path (avoid deep-copying the entire playlist) + res = context.backend.current_playlist.get(cpid=songpos).get() + cpids = [res.cpid] + l = [res.track] + return tracks_to_mpd_format(l, 0, 1, cpids=cpids) cpids = [ct[0] for ct in context.backend.current_playlist.cp_tracks.get()] return tracks_to_mpd_format( diff --git a/mopidy/frontends/mpd/protocol/status.py b/mopidy/frontends/mpd/protocol/status.py index 20a66775..b0bc7ad7 100644 --- a/mopidy/frontends/mpd/protocol/status.py +++ b/mopidy/frontends/mpd/protocol/status.py @@ -166,7 +166,7 @@ def status(context): decimal places for millisecond precision. """ futures = { - 'current_playlist.tracks': context.backend.current_playlist.tracks, + 'current_playlist.tracks_len': context.backend.current_playlist.tracks_len, 'current_playlist.version': context.backend.current_playlist.version, 'mixer.volume': context.mixer.volume, 'playback.consume': context.backend.playback.consume, @@ -213,7 +213,7 @@ def _status_consume(futures): return 0 def _status_playlist_length(futures): - return len(futures['current_playlist.tracks'].get()) + return futures['current_playlist.tracks_len'].get() def _status_playlist_version(futures): return futures['current_playlist.version'].get() From 1414c2394b1f46ce6c0f2c5825353cbb9e099d66 Mon Sep 17 00:00:00 2001 From: sandos Date: Mon, 12 Dec 2011 22:23:28 +0100 Subject: [PATCH 02/14] Remove unused import --- mopidy/backends/base/current_playlist.py | 1 - 1 file changed, 1 deletion(-) diff --git a/mopidy/backends/base/current_playlist.py b/mopidy/backends/base/current_playlist.py index 367f9c5d..85c4e135 100644 --- a/mopidy/backends/base/current_playlist.py +++ b/mopidy/backends/base/current_playlist.py @@ -1,7 +1,6 @@ from copy import copy import logging import random -import pprint from mopidy.listeners import BackendListener from mopidy.models import CpTrack From 5b1d77e79f27acb77c3339e1f1ae28b9071cfbcd Mon Sep 17 00:00:00 2001 From: sandos Date: Mon, 12 Dec 2011 22:26:33 +0100 Subject: [PATCH 03/14] Remove some logging --- mopidy/backends/base/current_playlist.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/mopidy/backends/base/current_playlist.py b/mopidy/backends/base/current_playlist.py index 85c4e135..1134984b 100644 --- a/mopidy/backends/base/current_playlist.py +++ b/mopidy/backends/base/current_playlist.py @@ -28,7 +28,6 @@ class CurrentPlaylistController(object): Read-only. """ - logger.debug(u'current_playlist.cp_tracks') return [copy(ct) for ct in self._cp_tracks] @property @@ -38,7 +37,6 @@ class CurrentPlaylistController(object): Read-only. """ - logger.debug(u'current_playlist.tracks()') return [ct[1] for ct in self._cp_tracks] @property From c5a4bb0e229a070192d40d068b426aa8e9e7eddb Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Tue, 27 Dec 2011 22:22:30 +0100 Subject: [PATCH 04/14] Rename tracks_len to length, and add test --- mopidy/backends/base/current_playlist.py | 4 ++-- mopidy/frontends/mpd/protocol/status.py | 4 ++-- tests/backends/base/current_playlist.py | 7 +++++++ 3 files changed, 11 insertions(+), 4 deletions(-) diff --git a/mopidy/backends/base/current_playlist.py b/mopidy/backends/base/current_playlist.py index 1134984b..966456a4 100644 --- a/mopidy/backends/base/current_playlist.py +++ b/mopidy/backends/base/current_playlist.py @@ -40,9 +40,9 @@ class CurrentPlaylistController(object): return [ct[1] for ct in self._cp_tracks] @property - def tracks_len(self): + def length(self): """ - Length of current playlist + Length of the current playlist. """ return len(self._cp_tracks) diff --git a/mopidy/frontends/mpd/protocol/status.py b/mopidy/frontends/mpd/protocol/status.py index b0bc7ad7..f4d66c56 100644 --- a/mopidy/frontends/mpd/protocol/status.py +++ b/mopidy/frontends/mpd/protocol/status.py @@ -166,7 +166,7 @@ def status(context): decimal places for millisecond precision. """ futures = { - 'current_playlist.tracks_len': context.backend.current_playlist.tracks_len, + 'current_playlist.length': context.backend.current_playlist.length, 'current_playlist.version': context.backend.current_playlist.version, 'mixer.volume': context.mixer.volume, 'playback.consume': context.backend.playback.consume, @@ -213,7 +213,7 @@ def _status_consume(futures): return 0 def _status_playlist_length(futures): - return futures['current_playlist.tracks_len'].get() + return futures['current_playlist.length'].get() def _status_playlist_version(futures): return futures['current_playlist.version'].get() diff --git a/tests/backends/base/current_playlist.py b/tests/backends/base/current_playlist.py index c81f4a0d..6d4854a7 100644 --- a/tests/backends/base/current_playlist.py +++ b/tests/backends/base/current_playlist.py @@ -18,6 +18,13 @@ class CurrentPlaylistControllerTest(object): assert len(self.tracks) == 3, 'Need three tracks to run tests.' + def test_length(self): + self.assertEqual(0, len(self.controller.cp_tracks)) + self.assertEqual(0, self.controller.length) + self.controller.append(self.tracks) + self.assertEqual(3, len(self.controller.cp_tracks)) + self.assertEqual(3, self.controller.length) + def test_add(self): for track in self.tracks: cp_track = self.controller.add(track) From aeee5518ac9c96c2728dd24291142da21c03a307 Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Tue, 27 Dec 2011 23:57:03 +0100 Subject: [PATCH 05/14] Improved and simplified the 'playlistinfo' command handler Cleaning up the rest of the code, it became obvious that sandos' performance patch did not alter the semantics of 'playlistinfo'. --- .../mpd/protocol/current_playlist.py | 30 ++++++++----------- 1 file changed, 12 insertions(+), 18 deletions(-) diff --git a/mopidy/frontends/mpd/protocol/current_playlist.py b/mopidy/frontends/mpd/protocol/current_playlist.py index c566bf7e..14816024 100644 --- a/mopidy/frontends/mpd/protocol/current_playlist.py +++ b/mopidy/frontends/mpd/protocol/current_playlist.py @@ -255,32 +255,26 @@ def playlistinfo(context, songpos=None, if start == -1: end = None else: - #Fetch one single track, hot code path (avoid deep-copying the entire playlist) - res = context.backend.current_playlist.get(cpid=songpos).get() - cpids = [res.cpid] - l = [res.track] - return tracks_to_mpd_format(l, 0, 1, cpids=cpids) - cpids = [ct[0] for ct in - context.backend.current_playlist.cp_tracks.get()] - return tracks_to_mpd_format( - context.backend.current_playlist.tracks.get(), - start, end, cpids=cpids) + # Hot code path: Fetch a single track, while avoiding deep-copying + # the entire playlist + cp_track = context.backend.current_playlist.get(cpid=songpos).get() + cpids = [cp_track.cpid] + tracks = [cp_track.track] + return tracks_to_mpd_format(tracks, 0, 1, cpids=cpids) else: if start is None: start = 0 start = int(start) - if not (0 <= start <= len( - context.backend.current_playlist.tracks.get())): + if not (0 <= start <= context.backend.current_playlist.length.get()): raise MpdArgError(u'Bad song index', command=u'playlistinfo') if end is not None: end = int(end) - if end > len(context.backend.current_playlist.tracks.get()): + if end > context.backend.current_playlist.length.get(): end = None - cpids = [ct[0] for ct in - context.backend.current_playlist.cp_tracks.get()] - return tracks_to_mpd_format( - context.backend.current_playlist.tracks.get(), - start, end, cpids=cpids) + cp_tracks = context.backend.current_playlist.cp_tracks.get() + cpids = [cp_track.cpid for cp_track in cp_tracks] + tracks = [cp_track.track for cp_track in cp_tracks] + return tracks_to_mpd_format(tracks, start, end, cpids=cpids) @handle_request(r'^playlistsearch "(?P[^"]+)" "(?P[^"]+)"$') @handle_request(r'^playlistsearch (?P\S+) "(?P[^"]+)"$') From e0e3a1c518742eca42a47040bd0cbae1f1ed1ee1 Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Wed, 28 Dec 2011 00:05:28 +0100 Subject: [PATCH 06/14] Update changelog --- docs/changes.rst | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/docs/changes.rst b/docs/changes.rst index d39d6fc2..ac2c21bf 100644 --- a/docs/changes.rst +++ b/docs/changes.rst @@ -18,6 +18,13 @@ v0.7.0 (in development) if you use playlist folders, you will no longer get lots of log messages about bad playlists. +- Added the method + :meth:`mopidy.backends.base.CurrentPlaylistController.length()` to reduce the + need for copying the entire current playlist from one thread to another. + Thanks to John Bäckstrand. + +- The MPD command ``playlistinfo`` is now faster, thanks to John Bäckstrand. + v0.6.0 (2011-10-09) =================== From 6f6e2c7fd7ae7807591af88d17d2c2a502b631d8 Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Wed, 28 Dec 2011 00:30:38 +0100 Subject: [PATCH 07/14] Let track{,s}_to_mpd_format() understand CpTrack objects Thus the cpid and cpids kwargs can be removed, and lots of code doing formatting of MPD responses can be simplified. This also reduces the need for making full copies of the current playlist, which improves performance. --- .../mpd/protocol/current_playlist.py | 24 ++++++----------- mopidy/frontends/mpd/protocol/status.py | 8 +++--- mopidy/frontends/mpd/translator.py | 26 ++++++++++--------- tests/frontends/mpd/serializer_test.py | 8 +++--- 4 files changed, 30 insertions(+), 36 deletions(-) diff --git a/mopidy/frontends/mpd/protocol/current_playlist.py b/mopidy/frontends/mpd/protocol/current_playlist.py index 14816024..b04c86d0 100644 --- a/mopidy/frontends/mpd/protocol/current_playlist.py +++ b/mopidy/frontends/mpd/protocol/current_playlist.py @@ -1,7 +1,8 @@ from mopidy.frontends.mpd.exceptions import (MpdArgError, MpdNoExistError, MpdNotImplemented) from mopidy.frontends.mpd.protocol import handle_request -from mopidy.frontends.mpd.translator import tracks_to_mpd_format +from mopidy.frontends.mpd.translator import (track_to_mpd_format, + tracks_to_mpd_format) @handle_request(r'^add "(?P[^"]*)"$') def add(context, uri): @@ -193,10 +194,9 @@ def playlistfind(context, tag, needle): if tag == 'filename': try: cp_track = context.backend.current_playlist.get(uri=needle).get() - (cpid, track) = cp_track position = context.backend.current_playlist.cp_tracks.get().index( cp_track) - return track.mpd_format(cpid=cpid, position=position) + return track_to_mpd_format(cp_track, position=position) except LookupError: return None raise MpdNotImplemented # TODO @@ -217,14 +217,12 @@ def playlistid(context, cpid=None): cp_track = context.backend.current_playlist.get(cpid=cpid).get() position = context.backend.current_playlist.cp_tracks.get().index( cp_track) - return cp_track.track.mpd_format(position=position, cpid=cpid) + return track_to_mpd_format(cp_track, position=position) except LookupError: raise MpdNoExistError(u'No such song', command=u'playlistid') else: - cpids = [ct[0] for ct in - context.backend.current_playlist.cp_tracks.get()] return tracks_to_mpd_format( - context.backend.current_playlist.tracks.get(), cpids=cpids) + context.backend.current_playlist.cp_tracks.get()) @handle_request(r'^playlistinfo$') @handle_request(r'^playlistinfo "(?P-?\d+)"$') @@ -258,9 +256,7 @@ def playlistinfo(context, songpos=None, # Hot code path: Fetch a single track, while avoiding deep-copying # the entire playlist cp_track = context.backend.current_playlist.get(cpid=songpos).get() - cpids = [cp_track.cpid] - tracks = [cp_track.track] - return tracks_to_mpd_format(tracks, 0, 1, cpids=cpids) + return tracks_to_mpd_format([cp_track], 0, 1) else: if start is None: start = 0 @@ -272,9 +268,7 @@ def playlistinfo(context, songpos=None, if end > context.backend.current_playlist.length.get(): end = None cp_tracks = context.backend.current_playlist.cp_tracks.get() - cpids = [cp_track.cpid for cp_track in cp_tracks] - tracks = [cp_track.track for cp_track in cp_tracks] - return tracks_to_mpd_format(tracks, start, end, cpids=cpids) + return tracks_to_mpd_format(cp_tracks, start, end) @handle_request(r'^playlistsearch "(?P[^"]+)" "(?P[^"]+)"$') @handle_request(r'^playlistsearch (?P\S+) "(?P[^"]+)"$') @@ -313,10 +307,8 @@ def plchanges(context, version): """ # XXX Naive implementation that returns all tracks as changed if int(version) < context.backend.current_playlist.version: - cpids = [ct[0] for ct in - context.backend.current_playlist.cp_tracks.get()] return tracks_to_mpd_format( - context.backend.current_playlist.tracks.get(), cpids=cpids) + context.backend.current_playlist.cp_tracks.get()) @handle_request(r'^plchangesposid "(?P\d+)"$') def plchangesposid(context, version): diff --git a/mopidy/frontends/mpd/protocol/status.py b/mopidy/frontends/mpd/protocol/status.py index f4d66c56..f32c46c8 100644 --- a/mopidy/frontends/mpd/protocol/status.py +++ b/mopidy/frontends/mpd/protocol/status.py @@ -1,8 +1,9 @@ import pykka.future from mopidy.backends.base import PlaybackController -from mopidy.frontends.mpd.protocol import handle_request from mopidy.frontends.mpd.exceptions import MpdNotImplemented +from mopidy.frontends.mpd.protocol import handle_request +from mopidy.frontends.mpd.translator import track_to_mpd_format #: Subsystems that can be registered with idle command. SUBSYSTEMS = ['database', 'mixer', 'options', 'output', @@ -32,9 +33,8 @@ def currentsong(context): """ current_cp_track = context.backend.playback.current_cp_track.get() if current_cp_track is not None: - return current_cp_track.track.mpd_format( - position=context.backend.playback.current_playlist_position.get(), - cpid=current_cp_track.cpid) + position = context.backend.playback.current_playlist_position.get() + return track_to_mpd_format(current_cp_track, position=position) @handle_request(r'^idle$') @handle_request(r'^idle (?P.+)$') diff --git a/mopidy/frontends/mpd/translator.py b/mopidy/frontends/mpd/translator.py index 562b2d2d..6ae32c9e 100644 --- a/mopidy/frontends/mpd/translator.py +++ b/mopidy/frontends/mpd/translator.py @@ -2,26 +2,28 @@ import os import re from mopidy import settings -from mopidy.utils.path import mtime as get_mtime from mopidy.frontends.mpd import protocol -from mopidy.utils.path import uri_to_path, split_path +from mopidy.models import CpTrack +from mopidy.utils.path import mtime as get_mtime, uri_to_path, split_path -def track_to_mpd_format(track, position=None, cpid=None): +def track_to_mpd_format(track, position=None): """ Format track for output to MPD client. :param track: the track - :type track: :class:`mopidy.models.Track` + :type track: :class:`mopidy.models.Track` or :class:`mopidy.models.CpTrack` :param position: track's position in playlist :type position: integer - :param cpid: track's CPID (current playlist ID) - :type cpid: integer :param key: if we should set key :type key: boolean :param mtime: if we should set mtime :type mtime: boolean :rtype: list of two-tuples """ + if isinstance(track, CpTrack): + (cpid, track) = track + else: + (cpid, track) = (None, track) result = [ ('file', track.uri or ''), ('Time', track.length and (track.length // 1000) or 0), @@ -88,14 +90,15 @@ def artists_to_mpd_format(artists): artists.sort(key=lambda a: a.name) return u', '.join([a.name for a in artists if a.name]) -def tracks_to_mpd_format(tracks, start=0, end=None, cpids=None): +def tracks_to_mpd_format(tracks, start=0, end=None): """ Format list of tracks for output to MPD client. Optionally limit output to the slice ``[start:end]`` of the list. :param tracks: the tracks - :type tracks: list of :class:`mopidy.models.Track` + :type tracks: list of :class:`mopidy.models.Track` or + :class:`mopidy.models.CpTrack` :param start: position of first track to include in output :type start: int (positive or negative) :param end: position after last track to include in output @@ -106,11 +109,10 @@ def tracks_to_mpd_format(tracks, start=0, end=None, cpids=None): end = len(tracks) tracks = tracks[start:end] positions = range(start, end) - cpids = cpids and cpids[start:end] or [None for _ in tracks] - assert len(tracks) == len(positions) == len(cpids) + assert len(tracks) == len(positions) result = [] - for track, position, cpid in zip(tracks, positions, cpids): - result.append(track_to_mpd_format(track, position, cpid)) + for track, position in zip(tracks, positions): + result.append(track_to_mpd_format(track, position)) return result def playlist_to_mpd_format(playlist, *args, **kwargs): diff --git a/tests/frontends/mpd/serializer_test.py b/tests/frontends/mpd/serializer_test.py index 681ab20f..a20abaed 100644 --- a/tests/frontends/mpd/serializer_test.py +++ b/tests/frontends/mpd/serializer_test.py @@ -4,7 +4,7 @@ import os from mopidy import settings from mopidy.utils.path import mtime, uri_to_path from mopidy.frontends.mpd import translator, protocol -from mopidy.models import Album, Artist, Playlist, Track +from mopidy.models import Album, Artist, CpTrack, Playlist, Track from tests import unittest @@ -45,17 +45,17 @@ class TrackMpdFormatTest(unittest.TestCase): self.assert_(('Pos', 1) not in result) def test_track_to_mpd_format_with_cpid(self): - result = translator.track_to_mpd_format(Track(), cpid=1) + result = translator.track_to_mpd_format(CpTrack(1, Track())) self.assert_(('Id', 1) not in result) def test_track_to_mpd_format_with_position_and_cpid(self): - result = translator.track_to_mpd_format(Track(), position=1, cpid=2) + result = translator.track_to_mpd_format(CpTrack(2, Track()), position=1) self.assert_(('Pos', 1) in result) self.assert_(('Id', 2) in result) def test_track_to_mpd_format_for_nonempty_track(self): result = translator.track_to_mpd_format( - self.track, position=9, cpid=122) + CpTrack(122, self.track), position=9) self.assert_(('file', 'a uri') in result) self.assert_(('Time', 137) in result) self.assert_(('Artist', 'an artist') in result) From ab4f21b38935b3c0e17592341c2a0cbb54e71ec7 Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Wed, 28 Dec 2011 00:33:42 +0100 Subject: [PATCH 08/14] Remove Track.mpd_format() which is no longer in use --- mopidy/models.py | 4 ---- 1 file changed, 4 deletions(-) diff --git a/mopidy/models.py b/mopidy/models.py index ed323b71..24c45ff1 100644 --- a/mopidy/models.py +++ b/mopidy/models.py @@ -185,10 +185,6 @@ class Track(ImmutableObject): self.__dict__['artists'] = frozenset(kwargs.pop('artists', [])) super(Track, self).__init__(*args, **kwargs) - def mpd_format(self, *args, **kwargs): - from mopidy.frontends.mpd import translator - return translator.track_to_mpd_format(self, *args, **kwargs) - class Playlist(ImmutableObject): """ From 716c5b03e24ecd5cb2e7e7581e392d0964dcade2 Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Wed, 28 Dec 2011 00:36:51 +0100 Subject: [PATCH 09/14] Remove Playlist.mpd_format() and its usage --- mopidy/frontends/mpd/protocol/music_db.py | 9 ++++++--- mopidy/frontends/mpd/protocol/stored_playlists.py | 5 +++-- mopidy/models.py | 4 ---- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/mopidy/frontends/mpd/protocol/music_db.py b/mopidy/frontends/mpd/protocol/music_db.py index 0343b3ab..299fce97 100644 --- a/mopidy/frontends/mpd/protocol/music_db.py +++ b/mopidy/frontends/mpd/protocol/music_db.py @@ -1,8 +1,9 @@ import re import shlex -from mopidy.frontends.mpd.protocol import handle_request, stored_playlists from mopidy.frontends.mpd.exceptions import MpdArgError, MpdNotImplemented +from mopidy.frontends.mpd.protocol import handle_request, stored_playlists +from mopidy.frontends.mpd.translator import playlist_to_mpd_format def _build_query(mpd_query): """ @@ -68,7 +69,8 @@ def find(context, mpd_query): - also uses the search type "date". """ query = _build_query(mpd_query) - return context.backend.library.find_exact(**query).get().mpd_format() + return playlist_to_mpd_format( + context.backend.library.find_exact(**query).get()) @handle_request(r'^findadd ' r'(?P("?([Aa]lbum|[Aa]rtist|[Ff]ilename|[Tt]itle|[Aa]ny)"? ' @@ -324,7 +326,8 @@ def search(context, mpd_query): - also uses the search type "date". """ query = _build_query(mpd_query) - return context.backend.library.search(**query).get().mpd_format() + return playlist_to_mpd_format( + context.backend.library.search(**query).get()) @handle_request(r'^update( "(?P[^"]+)")*$') def update(context, uri=None, rescan_unmodified_files=False): diff --git a/mopidy/frontends/mpd/protocol/stored_playlists.py b/mopidy/frontends/mpd/protocol/stored_playlists.py index 0a157f66..bb39d328 100644 --- a/mopidy/frontends/mpd/protocol/stored_playlists.py +++ b/mopidy/frontends/mpd/protocol/stored_playlists.py @@ -1,7 +1,8 @@ import datetime as dt -from mopidy.frontends.mpd.protocol import handle_request from mopidy.frontends.mpd.exceptions import MpdNoExistError, MpdNotImplemented +from mopidy.frontends.mpd.protocol import handle_request +from mopidy.frontends.mpd.translator import playlist_to_mpd_format @handle_request(r'^listplaylist "(?P[^"]+)"$') def listplaylist(context, name): @@ -40,7 +41,7 @@ def listplaylistinfo(context, name): """ try: playlist = context.backend.stored_playlists.get(name=name).get() - return playlist.mpd_format() + return playlist_to_mpd_format(playlist) except LookupError: raise MpdNoExistError( u'No such playlist', command=u'listplaylistinfo') diff --git a/mopidy/models.py b/mopidy/models.py index 24c45ff1..9a508ba7 100644 --- a/mopidy/models.py +++ b/mopidy/models.py @@ -220,7 +220,3 @@ class Playlist(ImmutableObject): def length(self): """The number of tracks in the playlist. Read-only.""" return len(self.tracks) - - def mpd_format(self, *args, **kwargs): - from mopidy.frontends.mpd import translator - return translator.playlist_to_mpd_format(self, *args, **kwargs) From 7b0954bef8fd672f45dcf7dfd343f532fcc93368 Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Wed, 28 Dec 2011 00:57:43 +0100 Subject: [PATCH 10/14] Simplify 'playlistinfo' implementation further, guided by new test asserts --- .../mpd/protocol/current_playlist.py | 19 +++++-------------- .../mpd/protocol/current_playlist_test.py | 18 ++++++++++++++++++ 2 files changed, 23 insertions(+), 14 deletions(-) diff --git a/mopidy/frontends/mpd/protocol/current_playlist.py b/mopidy/frontends/mpd/protocol/current_playlist.py index b04c86d0..ca0d1156 100644 --- a/mopidy/frontends/mpd/protocol/current_playlist.py +++ b/mopidy/frontends/mpd/protocol/current_playlist.py @@ -225,6 +225,7 @@ def playlistid(context, cpid=None): context.backend.current_playlist.cp_tracks.get()) @handle_request(r'^playlistinfo$') +@handle_request(r'^playlistinfo "-1"$') @handle_request(r'^playlistinfo "(?P-?\d+)"$') @handle_request(r'^playlistinfo "(?P\d+):(?P\d+)*"$') def playlistinfo(context, songpos=None, @@ -243,20 +244,10 @@ def playlistinfo(context, songpos=None, - uses negative indexes, like ``playlistinfo "-1"``, to request the entire playlist """ - if songpos == "-1": - songpos = None - if songpos is not None: songpos = int(songpos) - start = songpos - end = songpos + 1 - if start == -1: - end = None - else: - # Hot code path: Fetch a single track, while avoiding deep-copying - # the entire playlist - cp_track = context.backend.current_playlist.get(cpid=songpos).get() - return tracks_to_mpd_format([cp_track], 0, 1) + cp_track = context.backend.current_playlist.get(cpid=songpos).get() + return track_to_mpd_format(cp_track, position=songpos) else: if start is None: start = 0 @@ -267,8 +258,8 @@ def playlistinfo(context, songpos=None, end = int(end) if end > context.backend.current_playlist.length.get(): end = None - cp_tracks = context.backend.current_playlist.cp_tracks.get() - return tracks_to_mpd_format(cp_tracks, start, end) + cp_tracks = context.backend.current_playlist.cp_tracks.get() + return tracks_to_mpd_format(cp_tracks, start, end) @handle_request(r'^playlistsearch "(?P[^"]+)" "(?P[^"]+)"$') @handle_request(r'^playlistsearch (?P\S+) "(?P[^"]+)"$') diff --git a/tests/frontends/mpd/protocol/current_playlist_test.py b/tests/frontends/mpd/protocol/current_playlist_test.py index 343b230b..321fc6ee 100644 --- a/tests/frontends/mpd/protocol/current_playlist_test.py +++ b/tests/frontends/mpd/protocol/current_playlist_test.py @@ -271,11 +271,17 @@ class CurrentPlaylistHandlerTest(protocol.BaseTestCase): self.sendRequest(u'playlistinfo') self.assertInResponse(u'Title: a') + self.assertInResponse(u'Pos: 0') self.assertInResponse(u'Title: b') + self.assertInResponse(u'Pos: 1') self.assertInResponse(u'Title: c') + self.assertInResponse(u'Pos: 2') self.assertInResponse(u'Title: d') + self.assertInResponse(u'Pos: 3') self.assertInResponse(u'Title: e') + self.assertInResponse(u'Pos: 4') self.assertInResponse(u'Title: f') + self.assertInResponse(u'Pos: 5') self.assertInResponse(u'OK') def test_playlistinfo_with_songpos(self): @@ -286,11 +292,17 @@ class CurrentPlaylistHandlerTest(protocol.BaseTestCase): self.sendRequest(u'playlistinfo "4"') self.assertNotInResponse(u'Title: a') + self.assertNotInResponse(u'Pos: 0') self.assertNotInResponse(u'Title: b') + self.assertNotInResponse(u'Pos: 1') self.assertNotInResponse(u'Title: c') + self.assertNotInResponse(u'Pos: 2') self.assertNotInResponse(u'Title: d') + self.assertNotInResponse(u'Pos: 3') self.assertInResponse(u'Title: e') + self.assertInResponse(u'Pos: 4') self.assertNotInResponse(u'Title: f') + self.assertNotInResponse(u'Pos: 5') self.assertInResponse(u'OK') def test_playlistinfo_with_negative_songpos_same_as_playlistinfo(self): @@ -306,11 +318,17 @@ class CurrentPlaylistHandlerTest(protocol.BaseTestCase): self.sendRequest(u'playlistinfo "2:"') self.assertNotInResponse(u'Title: a') + self.assertNotInResponse(u'Pos: 0') self.assertNotInResponse(u'Title: b') + self.assertNotInResponse(u'Pos: 1') self.assertInResponse(u'Title: c') + self.assertInResponse(u'Pos: 2') self.assertInResponse(u'Title: d') + self.assertInResponse(u'Pos: 3') self.assertInResponse(u'Title: e') + self.assertInResponse(u'Pos: 4') self.assertInResponse(u'Title: f') + self.assertInResponse(u'Pos: 5') self.assertInResponse(u'OK') def test_playlistinfo_with_closed_range(self): From cda2fbbe965765a1cdf4224fb125b2c70bbe4d66 Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Wed, 28 Dec 2011 01:55:02 +0100 Subject: [PATCH 11/14] Add index() method to CurrentPlaylistController to reduce copying of the playlist --- docs/changes.rst | 7 ++++--- mopidy/backends/base/current_playlist.py | 14 +++++++++++++- mopidy/frontends/mpd/protocol/current_playlist.py | 14 +++++--------- tests/backends/base/current_playlist.py | 14 +++++++++++++- 4 files changed, 35 insertions(+), 14 deletions(-) diff --git a/docs/changes.rst b/docs/changes.rst index ac2c21bf..5bebd49e 100644 --- a/docs/changes.rst +++ b/docs/changes.rst @@ -18,13 +18,14 @@ v0.7.0 (in development) if you use playlist folders, you will no longer get lots of log messages about bad playlists. +- The MPD command ``playlistinfo`` is now faster, thanks to John Bäckstrand. + - Added the method - :meth:`mopidy.backends.base.CurrentPlaylistController.length()` to reduce the + :meth:`mopidy.backends.base.CurrentPlaylistController.length()` and + :meth:`mopidy.backends.base.CurrentPlaylistController.index()` to reduce the need for copying the entire current playlist from one thread to another. Thanks to John Bäckstrand. -- The MPD command ``playlistinfo`` is now faster, thanks to John Bäckstrand. - v0.6.0 (2011-10-09) =================== diff --git a/mopidy/backends/base/current_playlist.py b/mopidy/backends/base/current_playlist.py index 966456a4..9764ed6a 100644 --- a/mopidy/backends/base/current_playlist.py +++ b/mopidy/backends/base/current_playlist.py @@ -136,6 +136,19 @@ class CurrentPlaylistController(object): else: raise LookupError(u'"%s" match multiple tracks' % criteria_string) + def index(self, cp_track): + """ + Get index of the given (CPID integer, :class:`mopidy.models.Track`) + two-tuple in the current playlist. + + Raises :exc:`ValueError` if not found. + + :param cp_track: track to find the index of + :type cp_track: two-tuple (CPID integer, :class:`mopidy.models.Track`) + :rtype: int + """ + return self._cp_tracks.index(cp_track) + def move(self, start, end, to_position): """ Move the tracks in the slice ``[start:end]`` to ``to_position``. @@ -175,7 +188,6 @@ class CurrentPlaylistController(object): :param criteria: on or more criteria to match by :type criteria: dict - :type track: :class:`mopidy.models.Track` """ cp_track = self.get(**criteria) position = self._cp_tracks.index(cp_track) diff --git a/mopidy/frontends/mpd/protocol/current_playlist.py b/mopidy/frontends/mpd/protocol/current_playlist.py index ca0d1156..9d8c73b2 100644 --- a/mopidy/frontends/mpd/protocol/current_playlist.py +++ b/mopidy/frontends/mpd/protocol/current_playlist.py @@ -158,8 +158,7 @@ def moveid(context, cpid, to): cpid = int(cpid) to = int(to) cp_track = context.backend.current_playlist.get(cpid=cpid).get() - position = context.backend.current_playlist.cp_tracks.get().index( - cp_track) + position = context.backend.current_playlist.index(cp_track).get() context.backend.current_playlist.move(position, position + 1, to) @handle_request(r'^playlist$') @@ -194,8 +193,7 @@ def playlistfind(context, tag, needle): if tag == 'filename': try: cp_track = context.backend.current_playlist.get(uri=needle).get() - position = context.backend.current_playlist.cp_tracks.get().index( - cp_track) + position = context.backend.current_playlist.index(cp_track).get() return track_to_mpd_format(cp_track, position=position) except LookupError: return None @@ -215,8 +213,7 @@ def playlistid(context, cpid=None): try: cpid = int(cpid) cp_track = context.backend.current_playlist.get(cpid=cpid).get() - position = context.backend.current_playlist.cp_tracks.get().index( - cp_track) + position = context.backend.current_playlist.index(cp_track).get() return track_to_mpd_format(cp_track, position=position) except LookupError: raise MpdNoExistError(u'No such song', command=u'playlistid') @@ -375,7 +372,6 @@ def swapid(context, cpid1, cpid2): cpid2 = int(cpid2) cp_track1 = context.backend.current_playlist.get(cpid=cpid1).get() cp_track2 = context.backend.current_playlist.get(cpid=cpid2).get() - cp_tracks = context.backend.current_playlist.cp_tracks.get() - position1 = cp_tracks.index(cp_track1) - position2 = cp_tracks.index(cp_track2) + position1 = context.backend.current_playlist.index(cp_track1).get() + position2 = context.backend.current_playlist.index(cp_track2).get() swap(context, position1, position2) diff --git a/tests/backends/base/current_playlist.py b/tests/backends/base/current_playlist.py index 6d4854a7..2dab625e 100644 --- a/tests/backends/base/current_playlist.py +++ b/tests/backends/base/current_playlist.py @@ -1,7 +1,7 @@ import mock import random -from mopidy.models import Playlist, Track +from mopidy.models import CpTrack, Playlist, Track from mopidy.gstreamer import GStreamer from tests.backends.base import populate_playlist @@ -143,6 +143,18 @@ class CurrentPlaylistControllerTest(object): self.assertEqual(self.playback.state, self.playback.STOPPED) self.assertEqual(self.playback.current_track, None) + def test_index_returns_index_of_track(self): + cp_tracks = [] + for track in self.tracks: + cp_tracks.append(self.controller.add(track)) + self.assertEquals(0, self.controller.index(cp_tracks[0])) + self.assertEquals(1, self.controller.index(cp_tracks[1])) + self.assertEquals(2, self.controller.index(cp_tracks[2])) + + def test_index_raises_value_error_if_item_not_found(self): + test = lambda: self.controller.index(CpTrack(0, Track())) + self.assertRaises(ValueError, test) + @populate_playlist def test_move_single(self): self.controller.move(0, 0, 2) From 5573c33a7059f156b076b877f92e17292ae30075 Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Wed, 28 Dec 2011 01:57:47 +0100 Subject: [PATCH 12/14] Replace CpTrack indexes with namedtuple field names --- mopidy/backends/base/current_playlist.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/mopidy/backends/base/current_playlist.py b/mopidy/backends/base/current_playlist.py index 9764ed6a..4e439931 100644 --- a/mopidy/backends/base/current_playlist.py +++ b/mopidy/backends/base/current_playlist.py @@ -28,7 +28,7 @@ class CurrentPlaylistController(object): Read-only. """ - return [copy(ct) for ct in self._cp_tracks] + return [copy(cp_track) for cp_track in self._cp_tracks] @property def tracks(self): @@ -37,7 +37,7 @@ class CurrentPlaylistController(object): Read-only. """ - return [ct[1] for ct in self._cp_tracks] + return [cp_track.track for cp_track in self._cp_tracks] @property def length(self): @@ -123,9 +123,9 @@ class CurrentPlaylistController(object): matches = self._cp_tracks for (key, value) in criteria.iteritems(): if key == 'cpid': - matches = filter(lambda ct: ct[0] == value, matches) + matches = filter(lambda ct: ct.cpid == value, matches) else: - matches = filter(lambda ct: getattr(ct[1], key) == value, + matches = filter(lambda ct: getattr(ct.track, key) == value, matches) if len(matches) == 1: return matches[0] From 4f8fbac44c1636e54c08060e51980a1b138857f8 Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Wed, 28 Dec 2011 02:03:19 +0100 Subject: [PATCH 13/14] Use CurrentPlaylistController.length once more --- mopidy/frontends/mpd/protocol/current_playlist.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mopidy/frontends/mpd/protocol/current_playlist.py b/mopidy/frontends/mpd/protocol/current_playlist.py index 9d8c73b2..91ef7811 100644 --- a/mopidy/frontends/mpd/protocol/current_playlist.py +++ b/mopidy/frontends/mpd/protocol/current_playlist.py @@ -75,7 +75,7 @@ def delete_range(context, start, end=None): if end is not None: end = int(end) else: - end = len(context.backend.current_playlist.tracks.get()) + end = context.backend.current_playlist.length.get() cp_tracks = context.backend.current_playlist.cp_tracks.get()[start:end] if not cp_tracks: raise MpdArgError(u'Bad song index', command=u'delete') From b0698d2e0ad306bbf1bd5259d9b8abd547d7eb29 Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Wed, 28 Dec 2011 02:38:07 +0100 Subject: [PATCH 14/14] Add slice() method to CurrentPlaylistController to reduce copying of the playlist --- docs/changes.rst | 7 ++++--- mopidy/backends/base/current_playlist.py | 13 +++++++++++++ mopidy/frontends/mpd/protocol/current_playlist.py | 5 +++-- mopidy/frontends/mpd/protocol/playback.py | 7 ++++--- tests/backends/base/current_playlist.py | 12 ++++++++++++ 5 files changed, 36 insertions(+), 8 deletions(-) diff --git a/docs/changes.rst b/docs/changes.rst index 5bebd49e..95541797 100644 --- a/docs/changes.rst +++ b/docs/changes.rst @@ -21,10 +21,11 @@ v0.7.0 (in development) - The MPD command ``playlistinfo`` is now faster, thanks to John Bäckstrand. - Added the method - :meth:`mopidy.backends.base.CurrentPlaylistController.length()` and - :meth:`mopidy.backends.base.CurrentPlaylistController.index()` to reduce the + :meth:`mopidy.backends.base.CurrentPlaylistController.length()`, + :meth:`mopidy.backends.base.CurrentPlaylistController.index()`, and + :meth:`mopidy.backends.base.CurrentPlaylistController.slice()` to reduce the need for copying the entire current playlist from one thread to another. - Thanks to John Bäckstrand. + Thanks to John Bäckstrand for pinpointing the issue. v0.6.0 (2011-10-09) diff --git a/mopidy/backends/base/current_playlist.py b/mopidy/backends/base/current_playlist.py index 4e439931..d7e6c331 100644 --- a/mopidy/backends/base/current_playlist.py +++ b/mopidy/backends/base/current_playlist.py @@ -223,6 +223,19 @@ class CurrentPlaylistController(object): self._cp_tracks = before + shuffled + after self.version += 1 + def slice(self, start, end): + """ + Returns a slice of the current playlist, limited by the given + start and end positions. + + :param start: position of first track to include in slice + :type start: int + :param end: position after last track to include in slice + :type end: int + :rtype: two-tuple of (CPID integer, :class:`mopidy.models.Track`) + """ + return [copy(cp_track) for cp_track in self._cp_tracks[start:end]] + def _trigger_playlist_changed(self): logger.debug(u'Triggering playlist changed event') BackendListener.send('playlist_changed') diff --git a/mopidy/frontends/mpd/protocol/current_playlist.py b/mopidy/frontends/mpd/protocol/current_playlist.py index 91ef7811..0d61c887 100644 --- a/mopidy/frontends/mpd/protocol/current_playlist.py +++ b/mopidy/frontends/mpd/protocol/current_playlist.py @@ -76,7 +76,7 @@ def delete_range(context, start, end=None): end = int(end) else: end = context.backend.current_playlist.length.get() - cp_tracks = context.backend.current_playlist.cp_tracks.get()[start:end] + cp_tracks = context.backend.current_playlist.slice(start, end).get() if not cp_tracks: raise MpdArgError(u'Bad song index', command=u'delete') for (cpid, _) in cp_tracks: @@ -87,7 +87,8 @@ def delete_songpos(context, songpos): """See :meth:`delete_range`""" try: songpos = int(songpos) - (cpid, _) = context.backend.current_playlist.cp_tracks.get()[songpos] + (cpid, _) = context.backend.current_playlist.slice( + songpos, songpos + 1).get()[0] context.backend.current_playlist.remove(cpid=cpid) except IndexError: raise MpdArgError(u'Bad song index', command=u'delete') diff --git a/mopidy/frontends/mpd/protocol/playback.py b/mopidy/frontends/mpd/protocol/playback.py index 63cfe649..948083a8 100644 --- a/mopidy/frontends/mpd/protocol/playback.py +++ b/mopidy/frontends/mpd/protocol/playback.py @@ -178,7 +178,8 @@ def playpos(context, songpos): if songpos == -1: return _play_minus_one(context) try: - cp_track = context.backend.current_playlist.cp_tracks.get()[songpos] + cp_track = context.backend.current_playlist.slice( + songpos, songpos + 1).get()[0] return context.backend.playback.play(cp_track).get() except IndexError: raise MpdArgError(u'Bad song index', command=u'play') @@ -191,8 +192,8 @@ def _play_minus_one(context): elif context.backend.playback.current_cp_track.get() is not None: cp_track = context.backend.playback.current_cp_track.get() return context.backend.playback.play(cp_track).get() - elif context.backend.current_playlist.cp_tracks.get(): - cp_track = context.backend.current_playlist.cp_tracks.get()[0] + elif context.backend.current_playlist.slice(0, 1).get(): + cp_track = context.backend.current_playlist.slice(0, 1).get()[0] return context.backend.playback.play(cp_track).get() else: return # Fail silently diff --git a/tests/backends/base/current_playlist.py b/tests/backends/base/current_playlist.py index 2dab625e..e99cd56c 100644 --- a/tests/backends/base/current_playlist.py +++ b/tests/backends/base/current_playlist.py @@ -260,6 +260,18 @@ class CurrentPlaylistControllerTest(object): self.assertEqual(self.tracks[0], shuffled_tracks[0]) self.assertEqual(set(self.tracks), set(shuffled_tracks)) + @populate_playlist + def test_slice_returns_a_subset_of_tracks(self): + track_slice = self.controller.slice(1, 3) + self.assertEqual(2, len(track_slice)) + self.assertEqual(self.tracks[1], track_slice[0].track) + self.assertEqual(self.tracks[2], track_slice[1].track) + + @populate_playlist + def test_slice_returns_empty_list_if_indexes_outside_tracks_list(self): + self.assertEqual(0, len(self.controller.slice(7, 8))) + self.assertEqual(0, len(self.controller.slice(-1, 1))) + def test_version_does_not_change_when_appending_nothing(self): version = self.controller.version self.controller.append([])