From 2234a04fc7e1507307f3f70dc825188cca289375 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Tue, 31 Mar 2015 21:27:08 +0200 Subject: [PATCH 01/23] audio: Make outputs helper only handle tee-ing. The queue which is needed for gapless has been moved up to a audio-sink bin which also wraps the outputs. --- mopidy/audio/actor.py | 46 +++++++++++++++++++++++++------------------ 1 file changed, 27 insertions(+), 19 deletions(-) diff --git a/mopidy/audio/actor.py b/mopidy/audio/actor.py index b4c78ecb..3d8c8ef8 100644 --- a/mopidy/audio/actor.py +++ b/mopidy/audio/actor.py @@ -152,26 +152,12 @@ class _Appsrc(object): # TODO: expose this as a property on audio when #790 gets further along. class _Outputs(gst.Bin): def __init__(self): - gst.Bin.__init__(self) + gst.Bin.__init__(self, 'outputs') self._tee = gst.element_factory_make('tee') self.add(self._tee) - # Queue element to buy us time between the about to finish event and - # the actual switch, i.e. about to switch can block for longer thanks - # to this queue. - # TODO: make the min-max values a setting? - # TODO: this does not belong in this class. - queue = gst.element_factory_make('queue') - queue.set_property('max-size-buffers', 0) - queue.set_property('max-size-bytes', 0) - queue.set_property('max-size-time', 5 * gst.SECOND) - queue.set_property('min-threshold-time', 3 * gst.SECOND) - self.add(queue) - - queue.link(self._tee) - - ghost_pad = gst.GhostPad('sink', queue.get_pad('sink')) + ghost_pad = gst.GhostPad('sink', self._tee.get_pad('sink')) self.add_pad(ghost_pad) # Add an always connected fakesink which respects the clock so the tee @@ -451,8 +437,9 @@ class Audio(pykka.ThreadingActor): try: self._setup_preferences() self._setup_playbin() - self._setup_output() + self._setup_outputs() self._setup_mixer() + self._setup_audio_sink() except gobject.GError as ex: logger.exception(ex) process.exit_process() @@ -492,7 +479,7 @@ class Audio(pykka.ThreadingActor): self._signals.disconnect(self._playbin, 'source-setup') self._playbin.set_state(gst.STATE_NULL) - def _setup_output(self): + def _setup_outputs(self): # We don't want to use outputs for regular testing, so just install # an unsynced fakesink when someone asks for a 'testoutput'. if self._config['audio']['output'] == 'testoutput': @@ -505,12 +492,33 @@ class Audio(pykka.ThreadingActor): process.exit_process() # TODO: move this up the chain self._handler.setup_event_handling(self._outputs.get_pad('sink')) - self._playbin.set_property('audio-sink', self._outputs) def _setup_mixer(self): if self.mixer: self.mixer.setup(self._playbin, self.actor_ref.proxy().mixer) + def _setup_audio_sink(self): + audio_sink = gst.Bin('audio-sink') + + # Queue element to buy us time between the about to finish event and + # the actual switch, i.e. about to switch can block for longer thanks + # to this queue. + # TODO: make the min-max values a setting? + queue = gst.element_factory_make('queue') + queue.set_property('max-size-buffers', 0) + queue.set_property('max-size-bytes', 0) + queue.set_property('max-size-time', 5 * gst.SECOND) + queue.set_property('min-threshold-time', 3 * gst.SECOND) + + audio_sink.add(queue) + audio_sink.add(self._outputs) + queue.link(self._outputs) + + ghost_pad = gst.GhostPad('sink', queue.get_pad('sink')) + audio_sink.add_pad(ghost_pad) + + self._playbin.set_property('audio-sink', audio_sink) + def _teardown_mixer(self): if self.mixer: self.mixer.teardown() From 8236417e9d2c4d9c400b60dcbc4714de52b2bcbe Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Thu, 2 Apr 2015 23:26:05 +0200 Subject: [PATCH 02/23] audio: Move software volume into audiosink. This turns off playbin controlled volume, which implies that pulsesink volume can no longer be controlled by Mopidy. This is likely something we have to break, or at least rethink for multiple output support any way. With this change we now have software volume after our large queue, which means volume changes should happen much faster. --- mopidy/audio/actor.py | 34 ++++++++++------------------------ 1 file changed, 10 insertions(+), 24 deletions(-) diff --git a/mopidy/audio/actor.py b/mopidy/audio/actor.py index 3d8c8ef8..66718368 100644 --- a/mopidy/audio/actor.py +++ b/mopidy/audio/actor.py @@ -36,23 +36,6 @@ _GST_STATE_MAPPING = { MB = 1 << 20 -# GST_PLAY_FLAG_VIDEO (1<<0) -# GST_PLAY_FLAG_AUDIO (1<<1) -# GST_PLAY_FLAG_TEXT (1<<2) -# GST_PLAY_FLAG_VIS (1<<3) -# GST_PLAY_FLAG_SOFT_VOLUME (1<<4) -# GST_PLAY_FLAG_NATIVE_AUDIO (1<<5) -# GST_PLAY_FLAG_NATIVE_VIDEO (1<<6) -# GST_PLAY_FLAG_DOWNLOAD (1<<7) -# GST_PLAY_FLAG_BUFFERING (1<<8) -# GST_PLAY_FLAG_DEINTERLACE (1<<9) -# GST_PLAY_FLAG_SOFT_COLORBALANCE (1<<10) - -# Default flags to use for playbin: AUDIO, SOFT_VOLUME -# TODO: consider removing soft volume when we do multi outputs and handling it -# ourselves. -PLAYBIN_FLAGS = (1 << 1) | (1 << 4) - class _Signals(object): """Helper for tracking gobject signal registrations""" @@ -438,7 +421,6 @@ class Audio(pykka.ThreadingActor): self._setup_preferences() self._setup_playbin() self._setup_outputs() - self._setup_mixer() self._setup_audio_sink() except gobject.GError as ex: logger.exception(ex) @@ -459,7 +441,7 @@ class Audio(pykka.ThreadingActor): def _setup_playbin(self): playbin = gst.element_factory_make('playbin2') - playbin.set_property('flags', PLAYBIN_FLAGS) + playbin.set_property('flags', 2) # GST_PLAY_FLAG_AUDIO # TODO: turn into config values... playbin.set_property('buffer-size', 2 * 1024 * 1024) @@ -493,10 +475,6 @@ class Audio(pykka.ThreadingActor): self._handler.setup_event_handling(self._outputs.get_pad('sink')) - def _setup_mixer(self): - if self.mixer: - self.mixer.setup(self._playbin, self.actor_ref.proxy().mixer) - def _setup_audio_sink(self): audio_sink = gst.Bin('audio-sink') @@ -512,7 +490,15 @@ class Audio(pykka.ThreadingActor): audio_sink.add(queue) audio_sink.add(self._outputs) - queue.link(self._outputs) + + if self.mixer: + volume = gst.element_factory_make('volume') + audio_sink.add(volume) + queue.link(volume) + volume.link(self._outputs) + self.mixer.setup(volume, self.actor_ref.proxy().mixer) + else: + queue.link(self._outputs) ghost_pad = gst.GhostPad('sink', queue.get_pad('sink')) audio_sink.add_pad(ghost_pad) From e76c3c90120df63a3cb06dd42a222f44f2e23e78 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Thu, 2 Apr 2015 23:35:15 +0200 Subject: [PATCH 03/23] audio: Remove notify::mute/volume from software mixer These will never be triggered externally when using plain software volume. --- mopidy/audio/actor.py | 22 +++++----------------- 1 file changed, 5 insertions(+), 17 deletions(-) diff --git a/mopidy/audio/actor.py b/mopidy/audio/actor.py index 66718368..77b64d58 100644 --- a/mopidy/audio/actor.py +++ b/mopidy/audio/actor.py @@ -183,10 +183,6 @@ class SoftwareMixer(object): def setup(self, element, mixer_ref): self._element = element - - self._signals.connect(element, 'notify::volume', self._volume_changed) - self._signals.connect(element, 'notify::mute', self._mute_changed) - self._mixer.setup(mixer_ref) def teardown(self): @@ -198,24 +194,16 @@ class SoftwareMixer(object): def set_volume(self, volume): self._element.set_property('volume', volume / 100.0) + self._mixer.trigger_volume_changed(volume) def get_mute(self): return self._element.get_property('mute') def set_mute(self, mute): - return self._element.set_property('mute', bool(mute)) - - def _volume_changed(self, element, property_): - old_volume, self._last_volume = self._last_volume, self.get_volume() - if old_volume != self._last_volume: - gst_logger.debug('Notify volume: %s', self._last_volume / 100.0) - self._mixer.trigger_volume_changed(self._last_volume) - - def _mute_changed(self, element, property_): - old_mute, self._last_mute = self._last_mute, self.get_mute() - if old_mute != self._last_mute: - gst_logger.debug('Notify mute: %s', self._last_mute) - self._mixer.trigger_mute_changed(self._last_mute) + result = self._element.set_property('mute', bool(mute)) + if result: + self._mixer.trigger_mute_changed(bool(mute)) + return result class _Handler(object): From 9f90b37aa53748302a3fc9fb3f39b8c9f3438e0d Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Thu, 2 Apr 2015 23:40:58 +0200 Subject: [PATCH 04/23] audio: Limit post tee queue size Not sure how small we can safely make this, but basically with the volume element in front of the tee we "need" this as small as possible so the volume changes fell snappy. Alternative would be one volume element per tee branch. --- mopidy/audio/actor.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/mopidy/audio/actor.py b/mopidy/audio/actor.py index 77b64d58..b4e5ebbc 100644 --- a/mopidy/audio/actor.py +++ b/mopidy/audio/actor.py @@ -164,7 +164,9 @@ class _Outputs(gst.Bin): def _add(self, element): # All tee branches need a queue in front of them. + # But keep the queue short so the volume change isn't to slow: queue = gst.element_factory_make('queue') + queue.set_property('max-size-buffers', 5) self.add(element) self.add(queue) queue.link(element) From db48845e91212d1a0f5577b06f0fdf7fbd390023 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Thu, 2 Apr 2015 23:48:27 +0200 Subject: [PATCH 05/23] audio: Adjust queue sizes. These are mostly just gut feeling guesses. We should really start exposing at least a few of these as settings soon. --- mopidy/audio/actor.py | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/mopidy/audio/actor.py b/mopidy/audio/actor.py index b4e5ebbc..35bd215f 100644 --- a/mopidy/audio/actor.py +++ b/mopidy/audio/actor.py @@ -34,8 +34,6 @@ _GST_STATE_MAPPING = { gst.STATE_PAUSED: PlaybackState.PAUSED, gst.STATE_NULL: PlaybackState.STOPPED} -MB = 1 << 20 - class _Signals(object): """Helper for tracking gobject signal registrations""" @@ -97,7 +95,7 @@ class _Appsrc(object): source.set_property('caps', self._caps) source.set_property('format', b'time') source.set_property('stream-type', b'seekable') - source.set_property('max-bytes', 1 * MB) + source.set_property('max-bytes', 1 << 20) # 1MB source.set_property('min-percent', 50) if self._need_data_callback: @@ -434,8 +432,8 @@ class Audio(pykka.ThreadingActor): playbin.set_property('flags', 2) # GST_PLAY_FLAG_AUDIO # TODO: turn into config values... - playbin.set_property('buffer-size', 2 * 1024 * 1024) - playbin.set_property('buffer-duration', 2 * gst.SECOND) + playbin.set_property('buffer-size', 5 << 20) # 5MB + playbin.set_property('buffer-duration', 5 * gst.SECOND) self._signals.connect(playbin, 'source-setup', self._on_source_setup) self._signals.connect(playbin, 'about-to-finish', @@ -475,8 +473,8 @@ class Audio(pykka.ThreadingActor): queue = gst.element_factory_make('queue') queue.set_property('max-size-buffers', 0) queue.set_property('max-size-bytes', 0) - queue.set_property('max-size-time', 5 * gst.SECOND) - queue.set_property('min-threshold-time', 3 * gst.SECOND) + queue.set_property('max-size-time', 3 * gst.SECOND) + queue.set_property('min-threshold-time', 1 * gst.SECOND) audio_sink.add(queue) audio_sink.add(self._outputs) From bee0a4c4d5fdc0843eeb8a71ac72688af196ea32 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Thu, 2 Apr 2015 23:59:46 +0200 Subject: [PATCH 06/23] docs: Add audio volume changes to changelog --- docs/changelog.rst | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/docs/changelog.rst b/docs/changelog.rst index b976c169..01a5dc6d 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -5,6 +5,19 @@ Changelog This changelog is used to track all major changes to Mopidy. +v1.0.1 (unreleased) +=================== + +Audio +----- + +- Software volume control has been reworked to greatly reduce the delay between + changing the volume and the change taking effect. (Fixes: :issue:`1097`) + +- Software volume is no longer tied to the PulseAudio application volume when + using ``pulsesink``. This behavior was confusing for many users and doesn't + work well with the plans for multiple outputs. + v1.0.0 (2015-03-25) =================== From 5d94a265cd240c1f0eab43286e1dae7848442c48 Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Sun, 5 Apr 2015 02:13:30 +0200 Subject: [PATCH 07/23] docs: Tweak changelog --- docs/changelog.rst | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/docs/changelog.rst b/docs/changelog.rst index 01a5dc6d..ce7be87b 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -5,18 +5,18 @@ Changelog This changelog is used to track all major changes to Mopidy. -v1.0.1 (unreleased) +v1.0.1 (UNRELEASED) =================== -Audio ------ +- Audio: Software volume control has been reworked to greatly reduce the delay + between changing the volume and the change taking effect. (Fixes: + :issue:`1097`) -- Software volume control has been reworked to greatly reduce the delay between - changing the volume and the change taking effect. (Fixes: :issue:`1097`) +- Audio: As a side effect of the previous bug fix, software volume is no longer + tied to the PulseAudio application volume when using ``pulsesink``. This + behavior was confusing for many users and doesn't work well with the plans + for multiple outputs. -- Software volume is no longer tied to the PulseAudio application volume when - using ``pulsesink``. This behavior was confusing for many users and doesn't - work well with the plans for multiple outputs. v1.0.0 (2015-03-25) =================== From c77b63f4c8dcbd495ec88d24709571b7061d1b62 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Fri, 10 Apr 2015 23:28:00 +0200 Subject: [PATCH 08/23] audio: Add main method to scanner for quick testing --- mopidy/audio/scan.py | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/mopidy/audio/scan.py b/mopidy/audio/scan.py index 3880d91a..58793905 100644 --- a/mopidy/audio/scan.py +++ b/mopidy/audio/scan.py @@ -160,3 +160,28 @@ def _process(pipeline, timeout_ms): timeout -= clock.get_time() - start raise exceptions.ScannerError('Timeout after %dms' % timeout_ms) + + +if __name__ == '__main__': + import os + import sys + + import gobject + + from mopidy.utils import path + + gobject.threads_init() + + scanner = Scanner(5000) + for uri in sys.argv[1:]: + if not gst.uri_is_valid(uri): + uri = path.path_to_uri(os.path.abspath(uri)) + try: + result = scanner.scan(uri) + for key in ('uri', 'mime', 'duration', 'seekable'): + print '%-20s %s' % (key, getattr(result, key)) + print 'tags' + for tag, value in result.tags.items(): + print '%-20s %s' % (tag, value) + except exceptions.ScannerError as error: + print '%s: %s' % (uri, error) From 05c4af017ba2891ee2bcfa56951421fbcfaad76a Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Fri, 10 Apr 2015 23:31:05 +0200 Subject: [PATCH 09/23] audio: Create fakesinks on the fly for scanner pads This makes us correctly handle say when someone gives us a movie, or something else that seems to have multiple things that can be encoded internally. --- mopidy/audio/scan.py | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/mopidy/audio/scan.py b/mopidy/audio/scan.py index 58793905..d9e5ae94 100644 --- a/mopidy/audio/scan.py +++ b/mopidy/audio/scan.py @@ -70,17 +70,16 @@ def _setup_pipeline(uri, proxy_config=None): typefind = gst.element_factory_make('typefind') decodebin = gst.element_factory_make('decodebin2') - sink = gst.element_factory_make('fakesink') pipeline = gst.element_factory_make('pipeline') - pipeline.add_many(src, typefind, decodebin, sink) + pipeline.add_many(src, typefind, decodebin) gst.element_link_many(src, typefind, decodebin) if proxy_config: utils.setup_proxy(src, proxy_config) decodebin.set_property('caps', _RAW_AUDIO) - decodebin.connect('pad-added', _pad_added, sink) + decodebin.connect('pad-added', _pad_added, pipeline) typefind.connect('have-type', _have_type, decodebin) return pipeline @@ -92,8 +91,13 @@ def _have_type(element, probability, caps, decodebin): element.get_bus().post(msg) -def _pad_added(element, pad, sink): - return pad.link(sink.get_pad('sink')) +def _pad_added(element, pad, pipeline): + sink = gst.element_factory_make('fakesink') + sink.set_property('sync', False) + + pipeline.add(sink) + sink.sync_state_with_parent() + pad.link(sink.get_pad('sink')) def _start_pipeline(pipeline): From dfaa3f143391c2d856d9b7d9b69e1841cc0e3f71 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Sat, 11 Apr 2015 00:03:20 +0200 Subject: [PATCH 10/23] audio: Have scanner tell us if we found decodeable audio --- mopidy/audio/scan.py | 32 ++++++++++++++++++++------------ 1 file changed, 20 insertions(+), 12 deletions(-) diff --git a/mopidy/audio/scan.py b/mopidy/audio/scan.py index d9e5ae94..822277eb 100644 --- a/mopidy/audio/scan.py +++ b/mopidy/audio/scan.py @@ -14,7 +14,7 @@ from mopidy.utils import encoding _missing_plugin_desc = gst.pbutils.missing_plugin_message_get_description _Result = collections.namedtuple( - 'Result', ('uri', 'tags', 'duration', 'seekable', 'mime')) + 'Result', ('uri', 'tags', 'duration', 'seekable', 'mime', 'playable')) _RAW_AUDIO = gst.Caps(b'audio/x-raw-int; audio/x-raw-float') @@ -51,14 +51,14 @@ class Scanner(object): try: _start_pipeline(pipeline) - tags, mime = _process(pipeline, self._timeout_ms) + tags, mime, have_audio = _process(pipeline, self._timeout_ms) duration = _query_duration(pipeline) seekable = _query_seekable(pipeline) finally: pipeline.set_state(gst.STATE_NULL) del pipeline - return _Result(uri, tags, duration, seekable, mime) + return _Result(uri, tags, duration, seekable, mime, have_audio) # Turns out it's _much_ faster to just create a new pipeline for every as @@ -87,8 +87,9 @@ def _setup_pipeline(uri, proxy_config=None): def _have_type(element, probability, caps, decodebin): decodebin.set_property('sink-caps', caps) - msg = gst.message_new_application(element, caps.get_structure(0)) - element.get_bus().post(msg) + struct = gst.Structure('have-type') + struct['caps'] = caps.get_structure(0) + element.get_bus().post(gst.message_new_application(element, struct)) def _pad_added(element, pad, pipeline): @@ -99,6 +100,10 @@ def _pad_added(element, pad, pipeline): sink.sync_state_with_parent() pad.link(sink.get_pad('sink')) + if pad.get_caps().is_subset(_RAW_AUDIO): + struct = gst.Structure('have-audio') + element.get_bus().post(gst.message_new_application(element, struct)) + def _start_pipeline(pipeline): if pipeline.set_state(gst.STATE_PAUSED) == gst.STATE_CHANGE_NO_PREROLL: @@ -127,7 +132,7 @@ def _process(pipeline, timeout_ms): clock = pipeline.get_clock() bus = pipeline.get_bus() timeout = timeout_ms * gst.MSECOND - tags, mime, missing_description = {}, None, None + tags, mime, have_audio, missing_description = {}, None, False, None types = (gst.MESSAGE_ELEMENT | gst.MESSAGE_APPLICATION | gst.MESSAGE_ERROR | gst.MESSAGE_EOS | gst.MESSAGE_ASYNC_DONE | gst.MESSAGE_TAG) @@ -143,19 +148,22 @@ def _process(pipeline, timeout_ms): missing_description = encoding.locale_decode( _missing_plugin_desc(message)) elif message.type == gst.MESSAGE_APPLICATION: - mime = message.structure.get_name() - if mime.startswith('text/') or mime == 'application/xml': - return tags, mime + if message.structure.get_name() == 'have-type': + mime = message.structure['caps'].get_name() + if mime.startswith('text/') or mime == 'application/xml': + return tags, mime, have_audio + elif message.structure.get_name() == 'have-audio': + have_audio = True elif message.type == gst.MESSAGE_ERROR: error = encoding.locale_decode(message.parse_error()[0]) if missing_description: error = '%s (%s)' % (missing_description, error) raise exceptions.ScannerError(error) elif message.type == gst.MESSAGE_EOS: - return tags, mime + return tags, mime, have_audio elif message.type == gst.MESSAGE_ASYNC_DONE: if message.src == pipeline: - return tags, mime + return tags, mime, have_audio elif message.type == gst.MESSAGE_TAG: taglist = message.parse_tag() # Note that this will only keep the last tag. @@ -182,7 +190,7 @@ if __name__ == '__main__': uri = path.path_to_uri(os.path.abspath(uri)) try: result = scanner.scan(uri) - for key in ('uri', 'mime', 'duration', 'seekable'): + for key in ('uri', 'mime', 'duration', 'playable', 'seekable'): print '%-20s %s' % (key, getattr(result, key)) print 'tags' for tag, value in result.tags.items(): From 9bc4d8b713bcb22e65544e6bff860ab8432dca3a Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Sat, 11 Apr 2015 00:29:07 +0200 Subject: [PATCH 11/23] audio: Make scanner handle all media types. I don't think this makes anything slower, as before we would still decode anything we came across in the hopes that we find raw audio. --- mopidy/audio/scan.py | 3 +-- tests/audio/test_scan.py | 29 +++++++++++++---------------- 2 files changed, 14 insertions(+), 18 deletions(-) diff --git a/mopidy/audio/scan.py b/mopidy/audio/scan.py index 822277eb..9412b231 100644 --- a/mopidy/audio/scan.py +++ b/mopidy/audio/scan.py @@ -78,9 +78,8 @@ def _setup_pipeline(uri, proxy_config=None): if proxy_config: utils.setup_proxy(src, proxy_config) - decodebin.set_property('caps', _RAW_AUDIO) - decodebin.connect('pad-added', _pad_added, pipeline) typefind.connect('have-type', _have_type, decodebin) + decodebin.connect('pad-added', _pad_added, pipeline) return pipeline diff --git a/tests/audio/test_scan.py b/tests/audio/test_scan.py index b2937a3f..f58b2202 100644 --- a/tests/audio/test_scan.py +++ b/tests/audio/test_scan.py @@ -16,8 +16,7 @@ from tests import path_to_data_dir class ScannerTest(unittest.TestCase): def setUp(self): # noqa: N802 self.errors = {} - self.tags = {} - self.durations = {} + self.result = {} def find(self, path): media_dir = path_to_data_dir(path) @@ -31,19 +30,17 @@ class ScannerTest(unittest.TestCase): uri = path_lib.path_to_uri(path) key = uri[len('file://'):] try: - result = scanner.scan(uri) - self.tags[key] = result.tags - self.durations[key] = result.duration + self.result[key] = scanner.scan(uri) except exceptions.ScannerError as error: self.errors[key] = error def check(self, name, key, value): name = path_to_data_dir(name) - self.assertEqual(self.tags[name][key], value) + self.assertEqual(self.result[name].tags[key], value) def test_tags_is_set(self): self.scan(self.find('scanner/simple')) - self.assert_(self.tags) + self.assert_(self.result.values()[0].tags) def test_errors_is_not_set(self): self.scan(self.find('scanner/simple')) @@ -52,10 +49,10 @@ class ScannerTest(unittest.TestCase): def test_duration_is_set(self): self.scan(self.find('scanner/simple')) - self.assertEqual( - self.durations[path_to_data_dir('scanner/simple/song1.mp3')], 4680) - self.assertEqual( - self.durations[path_to_data_dir('scanner/simple/song1.ogg')], 4680) + ogg = path_to_data_dir('scanner/simple/song1.ogg') + mp3 = path_to_data_dir('scanner/simple/song1.mp3') + self.assertEqual(self.result[mp3].duration, 4680) + self.assertEqual(self.result[ogg].duration, 4680) def test_artist_is_set(self): self.scan(self.find('scanner/simple')) @@ -78,17 +75,17 @@ class ScannerTest(unittest.TestCase): def test_other_media_is_ignored(self): self.scan(self.find('scanner/image')) - self.assert_(self.errors) + self.assertFalse(self.result.values()[0].playable) def test_log_file_that_gst_thinks_is_mpeg_1_is_ignored(self): self.scan([path_to_data_dir('scanner/example.log')]) - self.assertLess( - self.durations[path_to_data_dir('scanner/example.log')], 100) + log = path_to_data_dir('scanner/example.log') + self.assertLess(self.result[log].duration, 100) def test_empty_wav_file(self): self.scan([path_to_data_dir('scanner/empty.wav')]) - self.assertEqual( - self.durations[path_to_data_dir('scanner/empty.wav')], 0) + wav = path_to_data_dir('scanner/empty.wav') + self.assertEqual(self.result[wav].duration, 0) @unittest.SkipTest def test_song_without_time_is_handeled(self): From 48a461991a764b0bea368f7db3b3640ca80aa816 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Sat, 11 Apr 2015 00:38:46 +0200 Subject: [PATCH 12/23] local: Skip unplayable tracks --- mopidy/local/commands.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/mopidy/local/commands.py b/mopidy/local/commands.py index af8b0025..4383decb 100644 --- a/mopidy/local/commands.py +++ b/mopidy/local/commands.py @@ -135,7 +135,9 @@ class ScanCommand(commands.Command): file_uri = path.path_to_uri(os.path.join(media_dir, relpath)) result = scanner.scan(file_uri) tags, duration = result.tags, result.duration - if duration < MIN_DURATION_MS: + if not result.playable: + logger.warning('Failed %s: No audio found in file.', uri) + elif duration < MIN_DURATION_MS: logger.warning('Failed %s: Track shorter than %dms', uri, MIN_DURATION_MS) else: From 1a1a0753a47ade4faa01248136acf6062ded5365 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Sun, 12 Apr 2015 14:16:35 +0200 Subject: [PATCH 13/23] audio: Use print function in scanner --- mopidy/audio/scan.py | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/mopidy/audio/scan.py b/mopidy/audio/scan.py index 9412b231..f2a93620 100644 --- a/mopidy/audio/scan.py +++ b/mopidy/audio/scan.py @@ -1,4 +1,5 @@ -from __future__ import absolute_import, division, unicode_literals +from __future__ import ( + absolute_import, division, print_function, unicode_literals) import collections @@ -190,9 +191,9 @@ if __name__ == '__main__': try: result = scanner.scan(uri) for key in ('uri', 'mime', 'duration', 'playable', 'seekable'): - print '%-20s %s' % (key, getattr(result, key)) - print 'tags' + print('%-20s %s' % (key, getattr(result, key))) + print('tags') for tag, value in result.tags.items(): - print '%-20s %s' % (tag, value) + print('%-20s %s' % (tag, value)) except exceptions.ScannerError as error: - print '%s: %s' % (uri, error) + print('%s: %s' % (uri, error)) From 68c2758009b58ba81419b25baacb3ec6570cc174 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Sun, 12 Apr 2015 14:24:28 +0200 Subject: [PATCH 14/23] docs: Add scanner improvements to changelog --- docs/changelog.rst | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/docs/changelog.rst b/docs/changelog.rst index ce7be87b..b3a7a459 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -17,6 +17,10 @@ v1.0.1 (UNRELEASED) behavior was confusing for many users and doesn't work well with the plans for multiple outputs. +- Audio: Update scanner to decode all media it finds. This should fix cases + where the scanner hangs on non-audio files like video. The scanner will now + also let us know if we found any decodeable audio. (Fixes: :issue:`726`) + v1.0.0 (2015-03-25) =================== From f85ea2a39d9d87e5da491847a4bd3ad165fbd5ca Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Sun, 12 Apr 2015 23:03:46 +0200 Subject: [PATCH 15/23] flake8: Fix new import order warnings (cherry picked from commit 71ab9733c760b68a49fa80f21793e1dbcdc3d86f) --- mopidy/audio/actor.py | 2 +- mopidy/audio/scan.py | 2 +- mopidy/utils/deps.py | 4 ++-- tests/audio/test_actor.py | 4 ++-- tests/stream/test_library.py | 4 ++-- tests/utils/test_deps.py | 4 ++-- 6 files changed, 10 insertions(+), 10 deletions(-) diff --git a/mopidy/audio/actor.py b/mopidy/audio/actor.py index 35bd215f..e0a7892a 100644 --- a/mopidy/audio/actor.py +++ b/mopidy/audio/actor.py @@ -8,7 +8,7 @@ import gobject import pygst pygst.require('0.10') import gst # noqa -import gst.pbutils +import gst.pbutils # noqa import pykka diff --git a/mopidy/audio/scan.py b/mopidy/audio/scan.py index f2a93620..d1e83407 100644 --- a/mopidy/audio/scan.py +++ b/mopidy/audio/scan.py @@ -6,7 +6,7 @@ import collections import pygst pygst.require('0.10') import gst # noqa -import gst.pbutils +import gst.pbutils # noqa from mopidy import exceptions from mopidy.audio import utils diff --git a/mopidy/utils/deps.py b/mopidy/utils/deps.py index bc9f7c2f..aafede9d 100644 --- a/mopidy/utils/deps.py +++ b/mopidy/utils/deps.py @@ -5,12 +5,12 @@ import os import platform import sys +import pkg_resources + import pygst pygst.require('0.10') import gst # noqa -import pkg_resources - from mopidy.utils import formatting diff --git a/tests/audio/test_actor.py b/tests/audio/test_actor.py index fbc440de..b00646bc 100644 --- a/tests/audio/test_actor.py +++ b/tests/audio/test_actor.py @@ -6,12 +6,12 @@ import unittest import gobject gobject.threads_init() +import mock + import pygst pygst.require('0.10') import gst # noqa -import mock - import pykka from mopidy import audio diff --git a/tests/stream/test_library.py b/tests/stream/test_library.py index 93292376..65c59cb6 100644 --- a/tests/stream/test_library.py +++ b/tests/stream/test_library.py @@ -5,12 +5,12 @@ import unittest import gobject gobject.threads_init() +import mock + import pygst pygst.require('0.10') import gst # noqa: pygst magic is needed to import correct gst -import mock - from mopidy.models import Track from mopidy.stream import actor from mopidy.utils.path import path_to_uri diff --git a/tests/utils/test_deps.py b/tests/utils/test_deps.py index 95f5b982..3b06973f 100644 --- a/tests/utils/test_deps.py +++ b/tests/utils/test_deps.py @@ -6,12 +6,12 @@ import unittest import mock +import pkg_resources + import pygst pygst.require('0.10') import gst # noqa -import pkg_resources - from mopidy.utils import deps From c8b348a61deae2c8de2d504df1643dd90ec2de60 Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Mon, 13 Apr 2015 08:16:54 +0200 Subject: [PATCH 16/23] docs: Tweak changelog --- docs/changelog.rst | 2 ++ 1 file changed, 2 insertions(+) diff --git a/docs/changelog.rst b/docs/changelog.rst index b3a7a459..6cf8ca15 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -8,6 +8,8 @@ This changelog is used to track all major changes to Mopidy. v1.0.1 (UNRELEASED) =================== +Bug fix release. + - Audio: Software volume control has been reworked to greatly reduce the delay between changing the volume and the change taking effect. (Fixes: :issue:`1097`) From d545fd198e5966e12640ef27a8b975e29e58a51b Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Tue, 14 Apr 2015 21:55:26 +0200 Subject: [PATCH 17/23] http: Fix threading issue causing duplicate WS messages The problem presents itself when a JSON-RPC call triggers some event in core. When this happens we have a thread outside of Tornado call `write_message` interleaved with a potentially ongoing write if the JSON-RPC response. To avoid this we now follow Tornado best practices and add callbacks to the IOLoop to ensure that there isn't any interleaved access of Tornado state. --- docs/changelog.rst | 2 ++ mopidy/http/handlers.py | 29 +++++++++++++++++++---------- 2 files changed, 21 insertions(+), 10 deletions(-) diff --git a/docs/changelog.rst b/docs/changelog.rst index 6cf8ca15..ebc42c44 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -23,6 +23,8 @@ Bug fix release. where the scanner hangs on non-audio files like video. The scanner will now also let us know if we found any decodeable audio. (Fixes: :issue:`726`) +- HTTP: Fix threading bug that would cause duplicate delivery of WS messages. + v1.0.0 (2015-03-25) =================== diff --git a/mopidy/http/handlers.py b/mopidy/http/handlers.py index a5baf992..4f4b5988 100644 --- a/mopidy/http/handlers.py +++ b/mopidy/http/handlers.py @@ -5,6 +5,7 @@ import os import socket import tornado.escape +import tornado.ioloop import tornado.web import tornado.websocket @@ -65,6 +66,19 @@ def make_jsonrpc_wrapper(core_actor): ) +def _send_broadcast(client, msg): + # We could check for client.ws_connection, but we don't really + # care why the broadcast failed, we just want the rest of them + # to succeed, so catch everything. + try: + client.write_message(msg) + except Exception as e: + error_msg = encoding.locale_decode(e) + logger.debug('Broadcast of WebSocket message to %s failed: %s', + client.request.remote_ip, error_msg) + # TODO: should this do the same cleanup as the on_message code? + + class WebSocketHandler(tornado.websocket.WebSocketHandler): # XXX This set is shared by all WebSocketHandler objects. This isn't @@ -74,17 +88,12 @@ class WebSocketHandler(tornado.websocket.WebSocketHandler): @classmethod def broadcast(cls, msg): + # This can be called from outside the Tornado ioloop, so we need to + # safely cross the thread boundary by adding a callback to the loop. + loop = tornado.ioloop.IOLoop.current() for client in cls.clients: - # We could check for client.ws_connection, but we don't really - # care why the broadcast failed, we just want the rest of them - # to succeed, so catch everything. - try: - client.write_message(msg) - except Exception as e: - error_msg = encoding.locale_decode(e) - logger.debug('Broadcast of WebSocket message to %s failed: %s', - client.request.remote_ip, error_msg) - # TODO: should this do the same cleanup as the on_message code? + # One callback per client to keep time we hold up the loop short + loop.add_callback(_send_broadcast, client, msg) def initialize(self, core): self.jsonrpc = make_jsonrpc_wrapper(core) From 9bb278f00e6c86bc55f32f83437b534152788d75 Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Wed, 22 Apr 2015 22:48:43 +0200 Subject: [PATCH 18/23] core: Make history controller traversable Fixes mopidy/mopidy.js#6 --- docs/changelog.rst | 2 ++ mopidy/core/history.py | 1 + 2 files changed, 3 insertions(+) diff --git a/docs/changelog.rst b/docs/changelog.rst index ebc42c44..0c470386 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -10,6 +10,8 @@ v1.0.1 (UNRELEASED) Bug fix release. +- Core: Make the new history controller available for use. (Fixes: :js:`6`) + - Audio: Software volume control has been reworked to greatly reduce the delay between changing the volume and the change taking effect. (Fixes: :issue:`1097`) diff --git a/mopidy/core/history.py b/mopidy/core/history.py index f0d5e9d4..ae697e8e 100644 --- a/mopidy/core/history.py +++ b/mopidy/core/history.py @@ -11,6 +11,7 @@ logger = logging.getLogger(__name__) class HistoryController(object): + pykka_traversable = True def __init__(self): self._history = [] From d4c695ac75018ba8845fe7b2ccc8867ef4ee355b Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Thu, 23 Apr 2015 23:08:25 +0200 Subject: [PATCH 19/23] mpd: Split browse and playlist name to uri caching --- docs/changelog.rst | 3 +++ mopidy/mpd/uri_mapper.py | 20 ++++++++++++----- tests/mpd/protocol/test_regression.py | 32 ++++++++++++++++++++++++++- 3 files changed, 48 insertions(+), 7 deletions(-) diff --git a/docs/changelog.rst b/docs/changelog.rst index 0c470386..647ca651 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -27,6 +27,9 @@ Bug fix release. - HTTP: Fix threading bug that would cause duplicate delivery of WS messages. +- MPD: Fix case where a playlist that is present in both browse and as a listed + playlist breaks the MPD frontend protocol output. (Fixes :issue:`1120`) + v1.0.0 (2015-03-25) =================== diff --git a/mopidy/mpd/uri_mapper.py b/mopidy/mpd/uri_mapper.py index 08c7f689..89dfd619 100644 --- a/mopidy/mpd/uri_mapper.py +++ b/mopidy/mpd/uri_mapper.py @@ -2,8 +2,12 @@ from __future__ import absolute_import, unicode_literals import re +# TOOD: refactor this into a generic mapper that does not know about browse +# or playlists and then use one instance for each case? + class MpdUriMapper(object): + """ Maintains the mappings between uniquified MPD names and URIs. """ @@ -17,7 +21,8 @@ class MpdUriMapper(object): def __init__(self, core=None): self.core = core self._uri_from_name = {} - self._name_from_uri = {} + self._browse_name_from_uri = {} + self._playlist_name_from_uri = {} def _create_unique_name(self, name, uri): stripped_name = self._invalid_browse_chars.sub(' ', name) @@ -30,13 +35,16 @@ class MpdUriMapper(object): i += 1 return name - def insert(self, name, uri): + def insert(self, name, uri, playlist=False): """ Create a unique and MPD compatible name that maps to the given URI. """ name = self._create_unique_name(name, uri) self._uri_from_name[name] = uri - self._name_from_uri[uri] = name + if playlist: + self._playlist_name_from_uri[uri] = name + else: + self._browse_name_from_uri[uri] = name return name def uri_from_name(self, name): @@ -56,7 +64,7 @@ class MpdUriMapper(object): if not playlist_ref.name: continue name = self._invalid_playlist_chars.sub('|', playlist_ref.name) - self.insert(name, playlist_ref.uri) + self.insert(name, playlist_ref.uri, playlist=True) def playlist_uri_from_name(self, name): """ @@ -70,6 +78,6 @@ class MpdUriMapper(object): """ Helper function to retrieve the unique MPD playlist name from its URI. """ - if uri not in self._name_from_uri: + if uri not in self._playlist_name_from_uri: self.refresh_playlists_mapping() - return self._name_from_uri[uri] + return self._playlist_name_from_uri[uri] diff --git a/tests/mpd/protocol/test_regression.py b/tests/mpd/protocol/test_regression.py index 6fb59afd..60a71ee8 100644 --- a/tests/mpd/protocol/test_regression.py +++ b/tests/mpd/protocol/test_regression.py @@ -2,7 +2,7 @@ from __future__ import absolute_import, unicode_literals import random -from mopidy.models import Track +from mopidy.models import Playlist, Ref, Track from tests.mpd import protocol @@ -175,3 +175,33 @@ class IssueGH137RegressionTest(protocol.BaseTestCase): u'Album "This Is Remixed Hits - Mashups & Rare 12" Mixes"') self.assertInResponse('ACK [2@0] {list} Invalid unquoted character') + + +class IssueGH1120RegressionTest(protocol.BaseTestCase): + """ + The issue: https://github.com/mopidy/mopidy/issues/1120 + + How to reproduce: + + - A playlist must be in both browse results and playlists + - Call for instance ``lsinfo "/"`` to populate the cache with the + playlist name from the playlist backend. + - Call ``lsinfo "/dummy"`` to override the playlist name with the browse + name. + - Call ``lsinfo "/"`` and we now have an invalid name with ``/`` in it. + + """ + + def test(self): + self.backend.library.dummy_browse_result = { + 'dummy:/': [Ref.playlist(name='Top 100 tracks', uri='dummy:/1')], + } + self.backend.playlists.set_dummy_playlists([ + Playlist(name='Top 100 tracks', uri='dummy:/1'), + ]) + + response1 = self.send_request('lsinfo "/"') + self.send_request('lsinfo "/dummy"') + + response2 = self.send_request('lsinfo "/"') + self.assertEqual(response1, response2) From 00cff144a4cfff9a54994b8cbb9f89bbcb4f7c6b Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Thu, 23 Apr 2015 23:09:32 +0200 Subject: [PATCH 20/23] mpd: Minor code cleanups in URI mapper helper --- mopidy/mpd/uri_mapper.py | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/mopidy/mpd/uri_mapper.py b/mopidy/mpd/uri_mapper.py index 89dfd619..9e7ec2dd 100644 --- a/mopidy/mpd/uri_mapper.py +++ b/mopidy/mpd/uri_mapper.py @@ -51,20 +51,21 @@ class MpdUriMapper(object): """ Return the uri for the given MPD name. """ - if name in self._uri_from_name: - return self._uri_from_name[name] + return self._uri_from_name.get(name) def refresh_playlists_mapping(self): """ Maintain map between playlists and unique playlist names to be used by MPD. """ - if self.core is not None: - for playlist_ref in self.core.playlists.as_list().get(): - if not playlist_ref.name: - continue - name = self._invalid_playlist_chars.sub('|', playlist_ref.name) - self.insert(name, playlist_ref.uri, playlist=True) + if self.core is None: + return + + for playlist_ref in self.core.playlists.as_list().get(): + if not playlist_ref.name: + continue + name = self._invalid_playlist_chars.sub('|', playlist_ref.name) + self.insert(name, playlist_ref.uri, playlist=True) def playlist_uri_from_name(self, name): """ From f4dcd598ac55885c280ab29632e0484211a4b487 Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Thu, 23 Apr 2015 23:25:41 +0200 Subject: [PATCH 21/23] docs: Add references to PRs --- docs/changelog.rst | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/docs/changelog.rst b/docs/changelog.rst index 647ca651..0bf39e7c 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -14,7 +14,7 @@ Bug fix release. - Audio: Software volume control has been reworked to greatly reduce the delay between changing the volume and the change taking effect. (Fixes: - :issue:`1097`) + :issue:`1097`, PR: :issue:`1101`) - Audio: As a side effect of the previous bug fix, software volume is no longer tied to the PulseAudio application volume when using ``pulsesink``. This @@ -23,12 +23,15 @@ Bug fix release. - Audio: Update scanner to decode all media it finds. This should fix cases where the scanner hangs on non-audio files like video. The scanner will now - also let us know if we found any decodeable audio. (Fixes: :issue:`726`) + also let us know if we found any decodeable audio. (Fixes: :issue:`726`, PR: + issue:`1124`) - HTTP: Fix threading bug that would cause duplicate delivery of WS messages. + (PR: :issue:`1127`) - MPD: Fix case where a playlist that is present in both browse and as a listed - playlist breaks the MPD frontend protocol output. (Fixes :issue:`1120`) + playlist breaks the MPD frontend protocol output. (Fixes :issue:`1120`, PR: + :issue:`1142`) v1.0.0 (2015-03-25) From 310e9109c17a429ab247a7d5d38481b5ffa2f4b9 Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Thu, 23 Apr 2015 23:34:09 +0200 Subject: [PATCH 22/23] docs: Add release date for v1.0.1 --- docs/changelog.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/changelog.rst b/docs/changelog.rst index 0bf39e7c..3b0336a0 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -5,7 +5,7 @@ Changelog This changelog is used to track all major changes to Mopidy. -v1.0.1 (UNRELEASED) +v1.0.1 (2015-04-23) =================== Bug fix release. From 9c793a38ff1e86bc397627ab895194ca53a0a885 Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Thu, 23 Apr 2015 23:35:45 +0200 Subject: [PATCH 23/23] Bump version to 1.0.1 --- mopidy/__init__.py | 2 +- tests/test_version.py | 5 +++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/mopidy/__init__.py b/mopidy/__init__.py index 388bb9f0..8dff0012 100644 --- a/mopidy/__init__.py +++ b/mopidy/__init__.py @@ -30,4 +30,4 @@ except ImportError: warnings.filterwarnings('ignore', 'could not open display') -__version__ = '1.0.0' +__version__ = '1.0.1' diff --git a/tests/test_version.py b/tests/test_version.py index 932cc639..3d284121 100644 --- a/tests/test_version.py +++ b/tests/test_version.py @@ -55,5 +55,6 @@ class VersionTest(unittest.TestCase): self.assertVersionLess('0.19.2', '0.19.3') self.assertVersionLess('0.19.3', '0.19.4') self.assertVersionLess('0.19.4', '0.19.5') - self.assertVersionLess('0.19.5', __version__) - self.assertVersionLess(__version__, '1.0.1') + self.assertVersionLess('0.19.5', '1.0.0') + self.assertVersionLess('1.0.0', __version__) + self.assertVersionLess(__version__, '1.0.2')