diff --git a/.github/ISSUE_TEMPLATE.md b/.github/ISSUE_TEMPLATE.md new file mode 100644 index 00000000..4fd5e866 --- /dev/null +++ b/.github/ISSUE_TEMPLATE.md @@ -0,0 +1,3 @@ +Please use https://discuss.mopidy.com/ for support questions. + +GitHub Issues should only be used for confirmed problems with Mopidy and well-defined feature requests. diff --git a/docs/changelog.rst b/docs/changelog.rst index 1cac30e3..08f0396b 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -18,7 +18,25 @@ v2.0.1 (UNRELEASED) Bug fix release. -- Nothing yet. +- Audio: Set ``soft-volume`` flag on GStreamer's playbin element. This is the + playbin's default, but we managed to override it when configuring the playbin + to only process audio. This should fix the "Volume/mute is not available" + warning. + +- Audio: Fix buffer conversion. This fixes image extraction. + (Fixes: :issue:`1469`, PR: :issue:`1472`) + +- Audio: Update scan logic to workaround GStreamer issue where tags and + duration might only be available after we start playing. + (Fixes: :issue:`935`, :issue:`1453`, :issue:`1474` and :issue:`1480`, PR: + :issue:`1487`) + +- Core: Avoid endless loop if all tracks in the tracklist are unplayable and + consume mode is off. (Fixes: :issue:`1221`, :issue:`1454`, PR: :issue:`1455`) + +- File: Ensure path comparision is done between bytestrings only. Fixes crash + where a :confval:`file/media_dirs` path contained non-ASCII characters. + (Fixes: :issue:`1345`, PR: :issue:`1493`) v2.0.0 (2016-02-15) diff --git a/mopidy/audio/actor.py b/mopidy/audio/actor.py index 64300ff9..267b228d 100644 --- a/mopidy/audio/actor.py +++ b/mopidy/audio/actor.py @@ -21,6 +21,9 @@ logger = logging.getLogger(__name__) # set_state() on a pipeline. gst_logger = logging.getLogger('mopidy.audio.gst') +_GST_PLAY_FLAGS_AUDIO = 0x02 +_GST_PLAY_FLAGS_SOFT_VOLUME = 0x10 + _GST_STATE_MAPPING = { Gst.State.PLAYING: PlaybackState.PLAYING, Gst.State.PAUSED: PlaybackState.PAUSED, @@ -448,7 +451,8 @@ class Audio(pykka.ThreadingActor): def _setup_playbin(self): playbin = Gst.ElementFactory.make('playbin') - playbin.set_property('flags', 2) # GST_PLAY_FLAG_AUDIO + playbin.set_property( + 'flags', _GST_PLAY_FLAGS_AUDIO | _GST_PLAY_FLAGS_SOFT_VOLUME) # TODO: turn into config values... playbin.set_property('buffer-size', 5 << 20) # 5MB diff --git a/mopidy/audio/scan.py b/mopidy/audio/scan.py index c63405b0..27888638 100644 --- a/mopidy/audio/scan.py +++ b/mopidy/audio/scan.py @@ -61,8 +61,7 @@ class Scanner(object): try: _start_pipeline(pipeline) - tags, mime, have_audio = _process(pipeline, timeout) - duration = _query_duration(pipeline) + tags, mime, have_audio, duration = _process(pipeline, timeout) seekable = _query_seekable(pipeline) finally: signals.clear() @@ -136,30 +135,6 @@ def _start_pipeline(pipeline): pipeline.set_state(Gst.State.PLAYING) -def _query_duration(pipeline, timeout=100): - # 1. Try and get a duration, return if success. - # 2. Some formats need to play some buffers before duration is found. - # 3. Wait for a duration change event. - # 4. Try and get a duration again. - - success, duration = pipeline.query_duration(Gst.Format.TIME) - if success and duration >= 0: - return duration // Gst.MSECOND - - result = pipeline.set_state(Gst.State.PLAYING) - if result == Gst.StateChangeReturn.FAILURE: - return None - - gst_timeout = timeout * Gst.MSECOND - bus = pipeline.get_bus() - bus.timed_pop_filtered(gst_timeout, Gst.MessageType.DURATION_CHANGED) - - success, duration = pipeline.query_duration(Gst.Format.TIME) - if success and duration >= 0: - return duration // Gst.MSECOND - return None - - def _query_seekable(pipeline): query = Gst.Query.new_seeking(Gst.Format.TIME) pipeline.query(query) @@ -172,6 +147,7 @@ def _process(pipeline, timeout_ms): mime = None have_audio = False missing_message = None + duration = None types = ( Gst.MessageType.ELEMENT | @@ -179,11 +155,12 @@ def _process(pipeline, timeout_ms): Gst.MessageType.ERROR | Gst.MessageType.EOS | Gst.MessageType.ASYNC_DONE | + Gst.MessageType.DURATION_CHANGED | Gst.MessageType.TAG ) timeout = timeout_ms - previous = int(time.time() * 1000) + start = int(time.time() * 1000) while timeout > 0: message = bus.timed_pop_filtered(timeout * Gst.MSECOND, types) @@ -197,7 +174,7 @@ def _process(pipeline, timeout_ms): mime = message.get_structure().get_value('caps').get_name() if mime and ( mime.startswith('text/') or mime == 'application/xml'): - return tags, mime, have_audio + return tags, mime, have_audio, duration elif message.get_structure().get_name() == 'have-audio': have_audio = True elif message.type == Gst.MessageType.ERROR: @@ -205,21 +182,43 @@ def _process(pipeline, timeout_ms): if missing_message and not mime: caps = missing_message.get_structure().get_value('detail') mime = caps.get_structure(0).get_name() - return tags, mime, have_audio + return tags, mime, have_audio, duration raise exceptions.ScannerError(error) elif message.type == Gst.MessageType.EOS: - return tags, mime, have_audio + return tags, mime, have_audio, duration elif message.type == Gst.MessageType.ASYNC_DONE: - if message.src == pipeline: - return tags, mime, have_audio + success, duration = pipeline.query_duration(Gst.Format.TIME) + if success: + duration = duration // Gst.MSECOND + else: + duration = None + + if tags and duration is not None: + return tags, mime, have_audio, duration + + # Workaround for upstream bug which causes tags/duration to arrive + # after pre-roll. We get around this by starting to play the track + # and then waiting for a duration change. + # https://bugzilla.gnome.org/show_bug.cgi?id=763553 + result = pipeline.set_state(Gst.State.PLAYING) + if result == Gst.StateChangeReturn.FAILURE: + return tags, mime, have_audio, duration + + elif message.type == Gst.MessageType.DURATION_CHANGED: + # duration will be read after ASYNC_DONE received; for now + # just give it a non-None value to flag that we have a duration: + duration = 0 elif message.type == Gst.MessageType.TAG: taglist = message.parse_tag() # Note that this will only keep the last tag. tags.update(tags_lib.convert_taglist(taglist)) - now = int(time.time() * 1000) - timeout -= now - previous - previous = now + timeout = timeout_ms - (int(time.time() * 1000) - start) + + # workaround for https://bugzilla.gnome.org/show_bug.cgi?id=763553: + # if we got what we want then stop playing (and wait for ASYNC_DONE) + if tags and duration is not None: + pipeline.set_state(Gst.State.PAUSED) raise exceptions.ScannerError('Timeout after %dms' % timeout_ms) diff --git a/mopidy/audio/tags.py b/mopidy/audio/tags.py index 38a0bac9..5ae86468 100644 --- a/mopidy/audio/tags.py +++ b/mopidy/audio/tags.py @@ -53,6 +53,10 @@ gstreamer-GstTagList.html result[tag].append(value.decode('utf-8', 'replace')) elif isinstance(value, (compat.text_type, bool, numbers.Number)): result[tag].append(value) + elif isinstance(value, Gst.Sample): + data = _extract_sample_data(value) + if data: + result[tag].append(data) else: logger.log( log.TRACE_LOG_LEVEL, @@ -62,6 +66,13 @@ gstreamer-GstTagList.html return result +def _extract_sample_data(sample): + buf = sample.get_buffer() + if not buf: + return None + return buf.extract_dup(0, buf.get_size()) + + # TODO: split based on "stream" and "track" based conversion? i.e. handle data # from radios in it's own helper instead? def convert_tags_to_track(tags): diff --git a/mopidy/core/playback.py b/mopidy/core/playback.py index af59b3d3..b569aebf 100644 --- a/mopidy/core/playback.py +++ b/mopidy/core/playback.py @@ -264,22 +264,27 @@ class PlaybackController(object): return pending = self.core.tracklist.eot_track(self._current_tl_track) - while pending: - # TODO: Avoid infinite loops if all tracks are unplayable. - backend = self._get_backend(pending) - if not backend: - continue + # avoid endless loop if 'repeat' is 'true' and no track is playable + # * 2 -> second run to get all playable track in a shuffled playlist + count = self.core.tracklist.get_length() * 2 - try: - if backend.playback.change_track(pending.track).get(): - self._pending_tl_track = pending - break - except Exception: - logger.exception('%s backend caused an exception.', - backend.actor_ref.actor_class.__name__) + while pending: + backend = self._get_backend(pending) + if backend: + try: + if backend.playback.change_track(pending.track).get(): + self._pending_tl_track = pending + break + except Exception: + logger.exception('%s backend caused an exception.', + backend.actor_ref.actor_class.__name__) self.core.tracklist._mark_unplayable(pending) pending = self.core.tracklist.eot_track(pending) + count -= 1 + if not count: + logger.info('No playable track in the list.') + break def _on_tracklist_change(self): """ @@ -302,6 +307,9 @@ class PlaybackController(object): """ state = self.get_state() current = self._pending_tl_track or self._current_tl_track + # avoid endless loop if 'repeat' is 'true' and no track is playable + # * 2 -> second run to get all playable track in a shuffled playlist + count = self.core.tracklist.get_length() * 2 while current: pending = self.core.tracklist.next_track(current) @@ -313,6 +321,10 @@ class PlaybackController(object): # if current == pending: # break current = pending + count -= 1 + if not count: + logger.info('No playable track in the list.') + break # TODO return result? @@ -364,6 +376,9 @@ class PlaybackController(object): current = self._pending_tl_track or self._current_tl_track pending = tl_track or current or self.core.tracklist.next_track(None) + # avoid endless loop if 'repeat' is 'true' and no track is playable + # * 2 -> second run to get all playable track in a shuffled playlist + count = self.core.tracklist.get_length() * 2 while pending: if self._change(pending, PlaybackState.PLAYING): @@ -372,6 +387,10 @@ class PlaybackController(object): self.core.tracklist._mark_unplayable(pending) current = pending pending = self.core.tracklist.next_track(current) + count -= 1 + if not count: + logger.info('No playable track in the list.') + break # TODO return result? @@ -428,6 +447,9 @@ class PlaybackController(object): self._previous = True state = self.get_state() current = self._pending_tl_track or self._current_tl_track + # avoid endless loop if 'repeat' is 'true' and no track is playable + # * 2 -> second run to get all playable track in a shuffled playlist + count = self.core.tracklist.get_length() * 2 while current: pending = self.core.tracklist.previous_track(current) @@ -439,6 +461,10 @@ class PlaybackController(object): # if current == pending: # break current = pending + count -= 1 + if not count: + logger.info('No playable track in the list.') + break # TODO: no return value? diff --git a/mopidy/file/library.py b/mopidy/file/library.py index 09fa2cf1..10182a38 100644 --- a/mopidy/file/library.py +++ b/mopidy/file/library.py @@ -132,5 +132,6 @@ class FileLibraryProvider(backend.LibraryProvider): def _is_in_basedir(self, local_path): return any( - path.is_path_inside_base_dir(local_path, media_dir['path']) + path.is_path_inside_base_dir( + local_path, media_dir['path'].encode('utf-8')) for media_dir in self._media_dirs) diff --git a/mopidy/internal/path.py b/mopidy/internal/path.py index 498b3016..1c10736f 100644 --- a/mopidy/internal/path.py +++ b/mopidy/internal/path.py @@ -196,6 +196,11 @@ def find_mtimes(root, follow=False): def is_path_inside_base_dir(path, base_path): + if not isinstance(path, bytes): + raise ValueError('path is not a bytestring') + if not isinstance(base_path, bytes): + raise ValueError('base_path is not a bytestring') + if path.endswith(os.sep): raise ValueError('Path %s cannot end with a path separator' % path) diff --git a/mopidy/local/translator.py b/mopidy/local/translator.py index 16842f59..b8fcff90 100644 --- a/mopidy/local/translator.py +++ b/mopidy/local/translator.py @@ -46,7 +46,7 @@ def path_to_local_track_uri(relpath): def path_to_local_directory_uri(relpath): - """Convert path relative to :confval:`local/media_dir` directory URI.""" + """Convert path relative to :confval:`local/media_dir` to directory URI.""" if isinstance(relpath, compat.text_type): relpath = relpath.encode('utf-8') return 'local:directory:%s' % urllib.quote(relpath) diff --git a/tests/core/test_playback.py b/tests/core/test_playback.py index 3625893d..7a130bad 100644 --- a/tests/core/test_playback.py +++ b/tests/core/test_playback.py @@ -15,11 +15,42 @@ from tests import dummy_audio class TestPlaybackProvider(backend.PlaybackProvider): + + def __init__(self, audio, backend): + super(TestPlaybackProvider, self).__init__(audio, backend) + self._call_limit = 10 + self._call_count = 0 + self._call_onetime = False + + def reset_call_limit(self): + self._call_count = 0 + self._call_onetime = False + + def is_call_limit_reached(self): + return self._call_count > self._call_limit + + def _translate_uri_call_limit(self, uri): + self._call_count += 1 + if self._call_count > self._call_limit: + # return any url (not 'None') to stop the endless loop + return 'assert: call limit reached' + if 'limit_never' in uri: + # unplayable + return None + elif 'limit_one' in uri: + # one time playable + if self._call_onetime: + return None + self._call_onetime = True + return uri + def translate_uri(self, uri): if 'error' in uri: raise Exception(uri) elif 'unplayable' in uri: return None + elif 'limit' in uri: + return self._translate_uri_call_limit(uri) else: return uri @@ -1182,3 +1213,77 @@ class TestBug1352Regression(BaseTest): self.core.history._add_track.assert_called_once_with(self.tracks[1]) self.core.tracklist._mark_playing.assert_called_once_with(tl_tracks[1]) + + +class TestEndlessLoop(BaseTest): + + tracks_play = [ + Track(uri='dummy:limit_never:a'), + Track(uri='dummy:limit_never:b') + ] + + tracks_other = [ + Track(uri='dummy:limit_never:a'), + Track(uri='dummy:limit_one'), + Track(uri='dummy:limit_never:b') + ] + + def test_play(self): + self.core.tracklist.clear() + self.core.tracklist.add(self.tracks_play) + + self.backend.playback.reset_call_limit().get() + self.core.tracklist.set_repeat(True) + + tl_tracks = self.core.tracklist.get_tl_tracks() + self.core.playback.play(tl_tracks[0]) + self.replay_events() + + self.assertFalse(self.backend.playback.is_call_limit_reached().get()) + + def test_next(self): + self.core.tracklist.clear() + self.core.tracklist.add(self.tracks_other) + + self.backend.playback.reset_call_limit().get() + self.core.tracklist.set_repeat(True) + + tl_tracks = self.core.tracklist.get_tl_tracks() + self.core.playback.play(tl_tracks[1]) + self.replay_events() + + self.core.playback.next() + self.replay_events() + + self.assertFalse(self.backend.playback.is_call_limit_reached().get()) + + def test_previous(self): + self.core.tracklist.clear() + self.core.tracklist.add(self.tracks_other) + + self.backend.playback.reset_call_limit().get() + self.core.tracklist.set_repeat(True) + + tl_tracks = self.core.tracklist.get_tl_tracks() + self.core.playback.play(tl_tracks[1]) + self.replay_events() + + self.core.playback.previous() + self.replay_events() + + self.assertFalse(self.backend.playback.is_call_limit_reached().get()) + + def test_on_about_to_finish(self): + self.core.tracklist.clear() + self.core.tracklist.add(self.tracks_other) + + self.backend.playback.reset_call_limit().get() + self.core.tracklist.set_repeat(True) + + tl_tracks = self.core.tracklist.get_tl_tracks() + self.core.playback.play(tl_tracks[1]) + self.replay_events() + + self.trigger_about_to_finish() + + self.assertFalse(self.backend.playback.is_call_limit_reached().get()) diff --git a/tests/internal/test_path.py b/tests/internal/test_path.py index 9e09c39a..6eebaaa3 100644 --- a/tests/internal/test_path.py +++ b/tests/internal/test_path.py @@ -7,6 +7,8 @@ import shutil import tempfile import unittest +import pytest + from mopidy import compat, exceptions from mopidy.internal import path from mopidy.internal.gi import GLib @@ -392,6 +394,30 @@ class FindMTimesTest(unittest.TestCase): self.assertEqual(errors, {}) +class TestIsPathInsideBaseDir(object): + def test_when_inside(self): + assert path.is_path_inside_base_dir( + '/æ/øå'.encode('utf-8'), + '/æ'.encode('utf-8')) + + def test_when_outside(self): + assert not path.is_path_inside_base_dir( + '/æ/øå'.encode('utf-8'), + '/ø'.encode('utf-8')) + + def test_byte_inside_str_fails(self): + with pytest.raises(ValueError): + path.is_path_inside_base_dir('/æ/øå'.encode('utf-8'), '/æ') + + def test_str_inside_byte_fails(self): + with pytest.raises(ValueError): + path.is_path_inside_base_dir('/æ/øå', '/æ'.encode('utf-8')) + + def test_str_inside_str_fails(self): + with pytest.raises(ValueError): + path.is_path_inside_base_dir('/æ/øå', '/æ') + + # TODO: kill this in favour of just os.path.getmtime + mocks class MtimeTest(unittest.TestCase):