From 9296ddd75b0e287a93a3054089c75ae5bfcbe4f6 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Sun, 14 Feb 2016 23:47:51 +0100 Subject: [PATCH 1/7] stream: Update playback tests to include backend --- tests/stream/test_playback.py | 44 +++++++++++++++++++++++------------ 1 file changed, 29 insertions(+), 15 deletions(-) diff --git a/tests/stream/test_playback.py b/tests/stream/test_playback.py index ef7da0bf..4ff684ca 100644 --- a/tests/stream/test_playback.py +++ b/tests/stream/test_playback.py @@ -4,6 +4,8 @@ import mock import pytest +import requests.exceptions + import responses from mopidy import exceptions @@ -27,6 +29,11 @@ def config(): 'proxy': {}, 'stream': { 'timeout': TIMEOUT, + 'metadata_blacklist': [], + 'protocols': ['file'], + }, + 'file': { + 'enabled': False }, } @@ -36,24 +43,21 @@ def audio(): return mock.Mock() -@pytest.fixture +@pytest.yield_fixture def scanner(): - scan_mock = mock.Mock(spec=scan.Scanner) - scan_mock.scan.return_value = None - return scan_mock + patcher = mock.patch.object(scan, 'Scanner') + yield patcher.start()() + patcher.stop() @pytest.fixture -def backend(scanner): - backend = mock.Mock() - backend.uri_schemes = ['file'] - backend._scanner = scanner - return backend +def backend(audio, config, scanner): + return actor.StreamBackend(audio=audio, config=config) @pytest.fixture -def provider(audio, backend, config): - return actor.StreamPlaybackProvider(audio, backend, config) +def provider(backend): + return backend.playback class TestTranslateURI(object): @@ -184,14 +188,24 @@ class TestTranslateURI(object): % STREAM_URI in caplog.text()) assert result == STREAM_URI - def test_failed_download_returns_none(self, provider, caplog): - with mock.patch.object(actor, 'http') as http_mock: - http_mock.download.return_value = None + @responses.activate + def test_failed_download_returns_none(self, scanner, provider, caplog): + scanner.scan.side_effect = [ + mock.Mock(mime='text/foo', playable=False) + ] - result = provider.translate_uri(PLAYLIST_URI) + responses.add( + responses.GET, PLAYLIST_URI, + body=requests.exceptions.HTTPError('Kaboom')) + + result = provider.translate_uri(PLAYLIST_URI) assert result is None + assert ( + 'Unwrapping stream from URI (%s) failed: ' + 'error downloading URI' % PLAYLIST_URI) in caplog.text() + @responses.activate def test_playlist_references_itself(self, scanner, provider, caplog): scanner.scan.side_effect = [ From a6495e0ecdb0187c3ec0bed038b01a2fe55227d2 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Sun, 14 Feb 2016 23:49:05 +0100 Subject: [PATCH 2/7] stream: Update library tests to include backend --- tests/stream/test_library.py | 52 +++++++++++++++++++++--------------- 1 file changed, 30 insertions(+), 22 deletions(-) diff --git a/tests/stream/test_library.py b/tests/stream/test_library.py index 67053924..682c98ab 100644 --- a/tests/stream/test_library.py +++ b/tests/stream/test_library.py @@ -4,7 +4,6 @@ import mock import pytest -from mopidy.audio import scan from mopidy.internal import path from mopidy.models import Track from mopidy.stream import actor @@ -13,16 +12,23 @@ from tests import path_to_data_dir @pytest.fixture -def scanner(): - return scan.Scanner(timeout=100, proxy_config={}) +def config(): + return { + 'proxy': {}, + 'stream': { + 'timeout': 1000, + 'metadata_blacklist': [], + 'protocols': ['file'], + }, + 'file': { + 'enabled': False + }, + } @pytest.fixture -def backend(scanner): - backend = mock.Mock() - backend.uri_schemes = ['file'] - backend._scanner = scanner - return backend +def audio(): + return mock.Mock() @pytest.fixture @@ -30,26 +36,28 @@ def track_uri(): return path.path_to_uri(path_to_data_dir('song1.wav')) -def test_lookup_ignores_unknown_scheme(backend): - library = actor.StreamLibraryProvider(backend, []) - - assert library.lookup('http://example.com') == [] +def test_lookup_ignores_unknown_scheme(audio, config): + backend = actor.StreamBackend(audio=audio, config=config) + backend.library.lookup('http://example.com') == [] -def test_lookup_respects_blacklist(backend, track_uri): - library = actor.StreamLibraryProvider(backend, [track_uri]) +def test_lookup_respects_blacklist(audio, config, track_uri): + config['stream']['metadata_blacklist'].append(track_uri) + backend = actor.StreamBackend(audio=audio, config=config) - assert library.lookup(track_uri) == [Track(uri=track_uri)] + assert backend.library.lookup(track_uri) == [Track(uri=track_uri)] -def test_lookup_respects_blacklist_globbing(backend, track_uri): - blacklist = [path.path_to_uri(path_to_data_dir('')) + '*'] - library = actor.StreamLibraryProvider(backend, blacklist) +def test_lookup_respects_blacklist_globbing(audio, config, track_uri): + blacklist_glob = path.path_to_uri(path_to_data_dir('')) + '*' + config['stream']['metadata_blacklist'].append(blacklist_glob) + backend = actor.StreamBackend(audio=audio, config=config) - assert library.lookup(track_uri) == [Track(uri=track_uri)] + assert backend.library.lookup(track_uri) == [Track(uri=track_uri)] -def test_lookup_converts_uri_metadata_to_track(backend, track_uri): - library = actor.StreamLibraryProvider(backend, []) +def test_lookup_converts_uri_metadata_to_track(audio, config, track_uri): + backend = actor.StreamBackend(audio=audio, config=config) - assert library.lookup(track_uri) == [Track(length=4406, uri=track_uri)] + result = backend.library.lookup(track_uri) + assert result == [Track(length=4406, uri=track_uri)] From 9aa2a8a370bcaf6e1844966db094ab23151fb375 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Sun, 14 Feb 2016 23:50:18 +0100 Subject: [PATCH 3/7] stream: Start moving state up to backend This allows us to start unifying how we handle playlists in the library and playback cases. --- mopidy/stream/actor.py | 43 ++++++++++++++++++------------------------ 1 file changed, 18 insertions(+), 25 deletions(-) diff --git a/mopidy/stream/actor.py b/mopidy/stream/actor.py index c2e39652..b42985d0 100644 --- a/mopidy/stream/actor.py +++ b/mopidy/stream/actor.py @@ -25,10 +25,19 @@ class StreamBackend(pykka.ThreadingActor, backend.Backend): timeout=config['stream']['timeout'], proxy_config=config['proxy']) - self.library = StreamLibraryProvider( - backend=self, blacklist=config['stream']['metadata_blacklist']) - self.playback = StreamPlaybackProvider( - audio=audio, backend=self, config=config) + self._session = http.get_requests_session( + proxy_config=config['proxy'], + user_agent='%s/%s' % ( + stream.Extension.dist_name, stream.Extension.version)) + + blacklist = config['stream']['metadata_blacklist'] + self._blacklist_re = re.compile( + r'^(%s)$' % '|'.join(fnmatch.translate(u) for u in blacklist)) + + self._timeout = config['stream']['timeout'] + + self.library = StreamLibraryProvider(backend=self) + self.playback = StreamPlaybackProvider(audio=audio, backend=self) self.playlists = None self.uri_schemes = audio_lib.supported_uri_schemes( @@ -43,23 +52,16 @@ class StreamBackend(pykka.ThreadingActor, backend.Backend): class StreamLibraryProvider(backend.LibraryProvider): - - def __init__(self, backend, blacklist): - super(StreamLibraryProvider, self).__init__(backend) - self._scanner = backend._scanner - self._blacklist_re = re.compile( - r'^(%s)$' % '|'.join(fnmatch.translate(u) for u in blacklist)) - def lookup(self, uri): if urllib.parse.urlsplit(uri).scheme not in self.backend.uri_schemes: return [] - if self._blacklist_re.match(uri): + if self.backend._blacklist_re.match(uri): logger.debug('URI matched metadata lookup blacklist: %s', uri) return [Track(uri=uri)] try: - result = self._scanner.scan(uri) + result = self.backend._scanner.scan(uri) track = tags.convert_tags_to_track(result.tags).replace( uri=uri, length=result.duration) except exceptions.ScannerError as e: @@ -71,21 +73,12 @@ class StreamLibraryProvider(backend.LibraryProvider): class StreamPlaybackProvider(backend.PlaybackProvider): - def __init__(self, audio, backend, config): - super(StreamPlaybackProvider, self).__init__(audio, backend) - self._config = config - self._scanner = backend._scanner - self._session = http.get_requests_session( - proxy_config=config['proxy'], - user_agent='%s/%s' % ( - stream.Extension.dist_name, stream.Extension.version)) - def translate_uri(self, uri): return _unwrap_stream( uri, - timeout=self._config['stream']['timeout'], - scanner=self._scanner, - requests_session=self._session) + timeout=self.backend._timeout, + scanner=self.backend._scanner, + requests_session=self.backend._session) def _unwrap_stream(uri, timeout, scanner, requests_session): From ce81b362dc338d7c1216b68183d46e59051d7b97 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Mon, 15 Feb 2016 00:00:30 +0100 Subject: [PATCH 4/7] stream: Add scheme and blacklist check to translate_uri We don't bother with this inside the unwrap code as if something redirects us so be it. --- mopidy/stream/actor.py | 7 +++++++ tests/stream/test_playback.py | 2 +- 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/mopidy/stream/actor.py b/mopidy/stream/actor.py index b42985d0..ff99264d 100644 --- a/mopidy/stream/actor.py +++ b/mopidy/stream/actor.py @@ -74,6 +74,13 @@ class StreamLibraryProvider(backend.LibraryProvider): class StreamPlaybackProvider(backend.PlaybackProvider): def translate_uri(self, uri): + if urllib.parse.urlsplit(uri).scheme not in self.backend.uri_schemes: + return None + + if self.backend._blacklist_re.match(uri): + logger.debug('URI matched metadata lookup blacklist: %s', uri) + return uri + return _unwrap_stream( uri, timeout=self.backend._timeout, diff --git a/tests/stream/test_playback.py b/tests/stream/test_playback.py index 4ff684ca..1816f73e 100644 --- a/tests/stream/test_playback.py +++ b/tests/stream/test_playback.py @@ -30,7 +30,7 @@ def config(): 'stream': { 'timeout': TIMEOUT, 'metadata_blacklist': [], - 'protocols': ['file'], + 'protocols': ['http'], }, 'file': { 'enabled': False From 2e5cfba710d1f8c9df39b0c4eaf8b7f66ff59ad4 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Mon, 15 Feb 2016 00:05:01 +0100 Subject: [PATCH 5/7] stream: Make library lookup use stream unwrapping (fixes #1445) --- mopidy/stream/actor.py | 28 +++++++++++++++++----------- 1 file changed, 17 insertions(+), 11 deletions(-) diff --git a/mopidy/stream/actor.py b/mopidy/stream/actor.py index ff99264d..f58b59ec 100644 --- a/mopidy/stream/actor.py +++ b/mopidy/stream/actor.py @@ -60,12 +60,17 @@ class StreamLibraryProvider(backend.LibraryProvider): logger.debug('URI matched metadata lookup blacklist: %s', uri) return [Track(uri=uri)] - try: - result = self.backend._scanner.scan(uri) + result = _unwrap_stream( + uri, + timeout=self.backend._timeout, + scanner=self.backend._scanner, + requests_session=self.backend._session)[1] + + if result: track = tags.convert_tags_to_track(result.tags).replace( uri=uri, length=result.duration) - except exceptions.ScannerError as e: - logger.warning('Problem looking up %s: %s', uri, e) + else: + logger.warning('Problem looking up %s: %s', uri) track = Track(uri=uri) return [track] @@ -85,9 +90,10 @@ class StreamPlaybackProvider(backend.PlaybackProvider): uri, timeout=self.backend._timeout, scanner=self.backend._scanner, - requests_session=self.backend._session) + requests_session=self.backend._session)[0] +# TODO: cleanup the return value of this. def _unwrap_stream(uri, timeout, scanner, requests_session): """ Get a stream URI from a playlist URI, ``uri``. @@ -105,7 +111,7 @@ def _unwrap_stream(uri, timeout, scanner, requests_session): logger.info( 'Unwrapping stream from URI (%s) failed: ' 'playlist referenced itself', uri) - return None + return None, None else: seen_uris.add(uri) @@ -117,7 +123,7 @@ def _unwrap_stream(uri, timeout, scanner, requests_session): logger.info( 'Unwrapping stream from URI (%s) failed: ' 'timed out in %sms', uri, timeout) - return None + return None, None scan_result = scanner.scan(uri, timeout=scan_timeout) except exceptions.ScannerError as exc: logger.debug('GStreamer failed scanning URI (%s): %s', uri, exc) @@ -130,14 +136,14 @@ def _unwrap_stream(uri, timeout, scanner, requests_session): ): logger.debug( 'Unwrapped potential %s stream: %s', scan_result.mime, uri) - return uri + return uri, scan_result download_timeout = deadline - time.time() if download_timeout < 0: logger.info( 'Unwrapping stream from URI (%s) failed: timed out in %sms', uri, timeout) - return None + return None, None content = http.download( requests_session, uri, timeout=download_timeout) @@ -145,14 +151,14 @@ def _unwrap_stream(uri, timeout, scanner, requests_session): logger.info( 'Unwrapping stream from URI (%s) failed: ' 'error downloading URI %s', original_uri, uri) - return None + return None, None uris = playlists.parse(content) if not uris: logger.debug( 'Failed parsing URI (%s) as playlist; found potential stream.', uri) - return uri + return uri, None # TODO Test streams and return first that seems to be playable logger.debug( From f53a0d220051016bc348cb344f61026fb0d1e866 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Mon, 15 Feb 2016 20:46:43 +0100 Subject: [PATCH 6/7] stream: Address review comments for PR#1447 --- mopidy/stream/actor.py | 23 ++++++++++------------- tests/stream/test_library.py | 2 +- 2 files changed, 11 insertions(+), 14 deletions(-) diff --git a/mopidy/stream/actor.py b/mopidy/stream/actor.py index f58b59ec..0861b5b0 100644 --- a/mopidy/stream/actor.py +++ b/mopidy/stream/actor.py @@ -60,15 +60,13 @@ class StreamLibraryProvider(backend.LibraryProvider): logger.debug('URI matched metadata lookup blacklist: %s', uri) return [Track(uri=uri)] - result = _unwrap_stream( - uri, - timeout=self.backend._timeout, - scanner=self.backend._scanner, - requests_session=self.backend._session)[1] + _, scan_result = _unwrap_stream( + uri, timeout=self.backend._timeout, scanner=self.backend._scanner, + requests_session=self.backend._session) - if result: - track = tags.convert_tags_to_track(result.tags).replace( - uri=uri, length=result.duration) + if scan_result: + track = tags.convert_tags_to_track(scan_result.tags).replace( + uri=uri, length=scan_result.duration) else: logger.warning('Problem looking up %s: %s', uri) track = Track(uri=uri) @@ -86,11 +84,10 @@ class StreamPlaybackProvider(backend.PlaybackProvider): logger.debug('URI matched metadata lookup blacklist: %s', uri) return uri - return _unwrap_stream( - uri, - timeout=self.backend._timeout, - scanner=self.backend._scanner, - requests_session=self.backend._session)[0] + unwrapped_uri, _ = _unwrap_stream( + uri, timeout=self.backend._timeout, scanner=self.backend._scanner, + requests_session=self.backend._session) + return unwrapped_uri # TODO: cleanup the return value of this. diff --git a/tests/stream/test_library.py b/tests/stream/test_library.py index 682c98ab..29348a6c 100644 --- a/tests/stream/test_library.py +++ b/tests/stream/test_library.py @@ -38,7 +38,7 @@ def track_uri(): def test_lookup_ignores_unknown_scheme(audio, config): backend = actor.StreamBackend(audio=audio, config=config) - backend.library.lookup('http://example.com') == [] + assert backend.library.lookup('http://example.com') == [] def test_lookup_respects_blacklist(audio, config, track_uri): From 3f0d7b96d09d3210e45baf4d2dbd9600d43f5b89 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Mon, 15 Feb 2016 20:53:05 +0100 Subject: [PATCH 7/7] docs: Add stream backend to changelog --- docs/changelog.rst | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/docs/changelog.rst b/docs/changelog.rst index b7234100..c7836f09 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -110,6 +110,12 @@ M3U backend - Improve reliability of playlist updates using the core playlist API by applying the write-replace pattern for file updates. +Stream backend +-------------- + +- Make sure both lookup and playback correctly handle playlists and our + blacklist support. (Fixes: :issue:`1445`, PR: :issue:`1447`) + MPD frontend ------------