diff --git a/docs/changelog.rst b/docs/changelog.rst index b7234100c0..c7836f0977 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 ------------ diff --git a/mopidy/stream/actor.py b/mopidy/stream/actor.py index c2e396527b..0861b5b062 100644 --- a/mopidy/stream/actor.py +++ b/mopidy/stream/actor.py @@ -25,10 +25,19 @@ def __init__(self, config, audio): 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,27 +52,23 @@ def __init__(self, config, audio): 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) - 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) + _, scan_result = _unwrap_stream( + uri, timeout=self.backend._timeout, scanner=self.backend._scanner, + requests_session=self.backend._session) + + 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) return [track] @@ -71,23 +76,21 @@ def lookup(self, uri): 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) + 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 + + 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. def _unwrap_stream(uri, timeout, scanner, requests_session): """ Get a stream URI from a playlist URI, ``uri``. @@ -105,7 +108,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 +120,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 +133,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 +148,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( diff --git a/tests/stream/test_library.py b/tests/stream/test_library.py index 6705392425..29348a6ce9 100644 --- a/tests/stream/test_library.py +++ b/tests/stream/test_library.py @@ -4,7 +4,6 @@ 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 @@ @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, []) +def test_lookup_ignores_unknown_scheme(audio, config): + backend = actor.StreamBackend(audio=audio, config=config) + assert backend.library.lookup('http://example.com') == [] - assert library.lookup('http://example.com') == [] +def test_lookup_respects_blacklist(audio, config, track_uri): + config['stream']['metadata_blacklist'].append(track_uri) + backend = actor.StreamBackend(audio=audio, config=config) -def test_lookup_respects_blacklist(backend, track_uri): - library = actor.StreamLibraryProvider(backend, [track_uri]) + assert backend.library.lookup(track_uri) == [Track(uri=track_uri)] - assert library.lookup(track_uri) == [Track(uri=track_uri)] +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) -def test_lookup_respects_blacklist_globbing(backend, track_uri): - blacklist = [path.path_to_uri(path_to_data_dir('')) + '*'] - library = actor.StreamLibraryProvider(backend, blacklist) + assert backend.library.lookup(track_uri) == [Track(uri=track_uri)] - assert library.lookup(track_uri) == [Track(uri=track_uri)] +def test_lookup_converts_uri_metadata_to_track(audio, config, track_uri): + backend = actor.StreamBackend(audio=audio, config=config) -def test_lookup_converts_uri_metadata_to_track(backend, track_uri): - library = actor.StreamLibraryProvider(backend, []) - - 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)] diff --git a/tests/stream/test_playback.py b/tests/stream/test_playback.py index ef7da0bf39..1816f73efd 100644 --- a/tests/stream/test_playback.py +++ b/tests/stream/test_playback.py @@ -4,6 +4,8 @@ import pytest +import requests.exceptions + import responses from mopidy import exceptions @@ -27,6 +29,11 @@ def config(): 'proxy': {}, 'stream': { 'timeout': TIMEOUT, + 'metadata_blacklist': [], + 'protocols': ['http'], + }, + '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 @@ def test_scan_fails_and_playlist_parsing_fails( % 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) + ] + + responses.add( + responses.GET, PLAYLIST_URI, + body=requests.exceptions.HTTPError('Kaboom')) - result = provider.translate_uri(PLAYLIST_URI) + 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 = [