New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make sure the stream backend playback/library handle URIs the same way #1447
Merged
jodal
merged 7 commits into
mopidy:develop
from
adamcik:fix/1445-unify-stream-uri-handling
Feb 15, 2016
Merged
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
9296ddd
stream: Update playback tests to include backend
adamcik a6495e0
stream: Update library tests to include backend
adamcik 9aa2a8a
stream: Start moving state up to backend
adamcik ce81b36
stream: Add scheme and blacklist check to translate_uri
adamcik 2e5cfba
stream: Make library lookup use stream unwrapping (fixes #1445)
adamcik f53a0d2
stream: Address review comments for PR#1447
adamcik 3f0d7b9
docs: Add stream backend to changelog
adamcik File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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,51 +52,48 @@ 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) | ||
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] | ||
|
||
|
||
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): | ||
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( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wouldn't it be nicer with |
||
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)[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,29 +136,29 @@ 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) | ||
|
||
if content is None: | ||
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( | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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,43 +12,52 @@ | |
|
||
|
||
@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 | ||
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) | ||
backend.library.lookup('http://example.com') == [] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing |
||
|
||
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)] |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't it be nicer with
_, scan_result = ...
?