From 1e8bef25d36e1c257a369ff0ce5729c2939f384e Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Mon, 22 Feb 2016 12:42:56 +0100 Subject: [PATCH 01/24] audio: Set soft-volume flag on playbin --- docs/changelog.rst | 5 ++++- mopidy/audio/actor.py | 6 +++++- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/docs/changelog.rst b/docs/changelog.rst index 73dad36d..dad0c741 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -10,7 +10,10 @@ 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. 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 From 7fd989ac9bed3490cdf3a7150065242489036a8c Mon Sep 17 00:00:00 2001 From: Lina He Date: Fri, 11 Mar 2016 15:19:18 +0100 Subject: [PATCH 02/24] Modify a mistake in annotations I found this mistake when reading the documentation of Mopidy. Just add a "to" to the annotation. --- mopidy/local/translator.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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) From a372793cbe634ed4bd2441ea1b3042d125a0e2df Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Wed, 23 Mar 2016 08:57:43 +0100 Subject: [PATCH 03/24] meta: Create placeholder ISSUE_TEMPLATE We probably want more here, but it's a start. --- .github/ISSUE_TEMPLATE.md | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 .github/ISSUE_TEMPLATE.md diff --git a/.github/ISSUE_TEMPLATE.md b/.github/ISSUE_TEMPLATE.md new file mode 100644 index 00000000..c7cfc136 --- /dev/null +++ b/.github/ISSUE_TEMPLATE.md @@ -0,0 +1,2 @@ +Please use https://discuss.mopidy.com for support questions. +Bugs should only be used for confirmed problems with Mopidy and feature requests. From 86b032736973c0050f79905e2647ff398bb92296 Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Wed, 23 Mar 2016 19:45:59 +0100 Subject: [PATCH 04/24] github: Tweak issues template --- .github/ISSUE_TEMPLATE.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/.github/ISSUE_TEMPLATE.md b/.github/ISSUE_TEMPLATE.md index c7cfc136..f2b1e2e5 100644 --- a/.github/ISSUE_TEMPLATE.md +++ b/.github/ISSUE_TEMPLATE.md @@ -1,2 +1,3 @@ -Please use https://discuss.mopidy.com for support questions. -Bugs should only be used for confirmed problems with Mopidy and feature requests. +Please use [discuss.mopidy.com](https://discuss.mopidy.com) for support questions. + +GtHub Issues should only be used for confirmed problems with Mopidy and well-defined feature requests. From a1057eca543593756f7bfbcb0f3a30718be3d701 Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Wed, 23 Mar 2016 19:47:37 +0100 Subject: [PATCH 05/24] github: Remove Markdown link --- .github/ISSUE_TEMPLATE.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/ISSUE_TEMPLATE.md b/.github/ISSUE_TEMPLATE.md index f2b1e2e5..a04dac51 100644 --- a/.github/ISSUE_TEMPLATE.md +++ b/.github/ISSUE_TEMPLATE.md @@ -1,3 +1,3 @@ -Please use [discuss.mopidy.com](https://discuss.mopidy.com) for support questions. +Please use https://discuss.mopidy.com/ for support questions. GtHub Issues should only be used for confirmed problems with Mopidy and well-defined feature requests. From 77358806dec8a73b0530d5d32fc2a67e22290b3c Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Fri, 25 Mar 2016 12:44:39 +0100 Subject: [PATCH 06/24] github: Fix typo --- .github/ISSUE_TEMPLATE.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/ISSUE_TEMPLATE.md b/.github/ISSUE_TEMPLATE.md index a04dac51..4fd5e866 100644 --- a/.github/ISSUE_TEMPLATE.md +++ b/.github/ISSUE_TEMPLATE.md @@ -1,3 +1,3 @@ Please use https://discuss.mopidy.com/ for support questions. -GtHub Issues should only be used for confirmed problems with Mopidy and well-defined feature requests. +GitHub Issues should only be used for confirmed problems with Mopidy and well-defined feature requests. From f0788515cd4b207b127142452004389f0211a80b Mon Sep 17 00:00:00 2001 From: Jens Luetjen Date: Sat, 27 Feb 2016 11:24:04 +0100 Subject: [PATCH 07/24] Find images in audio files Handle Gst.Sample as image in audio file tags (scaned with Gst1.0). --- mopidy/audio/tags.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/mopidy/audio/tags.py b/mopidy/audio/tags.py index 38a0bac9..a5714567 100644 --- a/mopidy/audio/tags.py +++ b/mopidy/audio/tags.py @@ -53,6 +53,12 @@ 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): + buf = value.get_buffer() + (found, mapinfo) = buf.map(Gst.MapFlags.READ) + if found: + result[tag].append(bytes(mapinfo.data)) + buf.unmap(mapinfo) else: logger.log( log.TRACE_LOG_LEVEL, From 6faa19dbf3e444ed3a2372de399e069d8d30b5bb Mon Sep 17 00:00:00 2001 From: Jens Luetjen Date: Mon, 29 Feb 2016 19:40:49 +0100 Subject: [PATCH 08/24] Always unmap mapped buffer. Always unmap the mapped memory, even in case of exception. --- mopidy/audio/tags.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/mopidy/audio/tags.py b/mopidy/audio/tags.py index a5714567..1625ccc4 100644 --- a/mopidy/audio/tags.py +++ b/mopidy/audio/tags.py @@ -57,8 +57,10 @@ gstreamer-GstTagList.html buf = value.get_buffer() (found, mapinfo) = buf.map(Gst.MapFlags.READ) if found: - result[tag].append(bytes(mapinfo.data)) - buf.unmap(mapinfo) + try: + result[tag].append(bytes(mapinfo.data)) + finally: + buf.unmap(mapinfo) else: logger.log( log.TRACE_LOG_LEVEL, From 64a58e0662898c6e3d408fdfd9882ed61c98f21b Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Fri, 25 Mar 2016 13:09:44 +0100 Subject: [PATCH 09/24] audio: Move sample to data conversion to a helper. Also add check for None buffer being returned. --- mopidy/audio/tags.py | 23 ++++++++++++++++------- 1 file changed, 16 insertions(+), 7 deletions(-) diff --git a/mopidy/audio/tags.py b/mopidy/audio/tags.py index 1625ccc4..e29f37d8 100644 --- a/mopidy/audio/tags.py +++ b/mopidy/audio/tags.py @@ -54,13 +54,9 @@ gstreamer-GstTagList.html elif isinstance(value, (compat.text_type, bool, numbers.Number)): result[tag].append(value) elif isinstance(value, Gst.Sample): - buf = value.get_buffer() - (found, mapinfo) = buf.map(Gst.MapFlags.READ) - if found: - try: - result[tag].append(bytes(mapinfo.data)) - finally: - buf.unmap(mapinfo) + data = _extract_sample_data(value) + if data: + result[tag].append(data) else: logger.log( log.TRACE_LOG_LEVEL, @@ -70,6 +66,19 @@ gstreamer-GstTagList.html return result +def _extract_sample_data(sample): + buf = sample.get_buffer() + if not buf: + return None + found, mapinfo = buf.map(Gst.MapFlags.READ) + if not found: + return None + try: + return bytes(mapinfo.data) + finally: + buf.unmap(mapinfo) + + # 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): From 793e42531476667db6b670d799b5baf3ebc8365f Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Fri, 25 Mar 2016 13:11:07 +0100 Subject: [PATCH 10/24] audio: Make buffer conversion work on older GStreamer installs --- mopidy/audio/tags.py | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/mopidy/audio/tags.py b/mopidy/audio/tags.py index e29f37d8..5ae86468 100644 --- a/mopidy/audio/tags.py +++ b/mopidy/audio/tags.py @@ -70,13 +70,7 @@ def _extract_sample_data(sample): buf = sample.get_buffer() if not buf: return None - found, mapinfo = buf.map(Gst.MapFlags.READ) - if not found: - return None - try: - return bytes(mapinfo.data) - finally: - buf.unmap(mapinfo) + return buf.extract_dup(0, buf.get_size()) # TODO: split based on "stream" and "track" based conversion? i.e. handle data From 3c535409adadb51eacf7e2f1df9196d493413bfe Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Fri, 25 Mar 2016 14:34:15 +0100 Subject: [PATCH 11/24] docs: Add buffer fix to changelog --- docs/changelog.rst | 3 +++ 1 file changed, 3 insertions(+) diff --git a/docs/changelog.rst b/docs/changelog.rst index dad0c741..f8f7e43b 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -15,6 +15,9 @@ Bug fix release. to only process audio. This should fix the "Volume/mute is not available" warning. +- Audio: Fix buffer conversion. This fixes image extraction. + (PR: :issue:`1472`) + v2.0.0 (2016-02-15) =================== From 3183f43b18955a55b41b953502e49b67229ecf0f Mon Sep 17 00:00:00 2001 From: SeeSpotRun Date: Tue, 15 Mar 2016 14:42:29 +1000 Subject: [PATCH 12/24] scanner: workaround for gstreamer not pushing tags before PREROLL refer https://bugzilla.gnome.org/show_bug.cgi?id=763553 --- mopidy/audio/scan.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/mopidy/audio/scan.py b/mopidy/audio/scan.py index c63405b0..1a199e58 100644 --- a/mopidy/audio/scan.py +++ b/mopidy/audio/scan.py @@ -210,8 +210,11 @@ def _process(pipeline, timeout_ms): elif message.type == Gst.MessageType.EOS: return tags, mime, have_audio elif message.type == Gst.MessageType.ASYNC_DONE: - if message.src == pipeline: + if tags: return tags, mime, have_audio + else: + # workaround for https://bugzilla.gnome.org/show_bug.cgi?id=763553: + pipeline.set_state(Gst.State.PLAYING) elif message.type == Gst.MessageType.TAG: taglist = message.parse_tag() # Note that this will only keep the last tag. @@ -220,6 +223,9 @@ def _process(pipeline, timeout_ms): now = int(time.time() * 1000) timeout -= now - previous previous = now + # workaround for https://bugzilla.gnome.org/show_bug.cgi?id=763553: + if tags: + pipeline.set_state(Gst.State.PAUSED) raise exceptions.ScannerError('Timeout after %dms' % timeout_ms) From 57518861ea55546d8f491448ff65453d2cf0d184 Mon Sep 17 00:00:00 2001 From: SeeSpotRun Date: Tue, 15 Mar 2016 18:37:23 +1000 Subject: [PATCH 13/24] scanner: move _query_duration() into _process() --- mopidy/audio/scan.py | 59 +++++++++++++++++++------------------------- 1 file changed, 25 insertions(+), 34 deletions(-) diff --git a/mopidy/audio/scan.py b/mopidy/audio/scan.py index 1a199e58..4642c3f2 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,6 +155,7 @@ def _process(pipeline, timeout_ms): Gst.MessageType.ERROR | Gst.MessageType.EOS | Gst.MessageType.ASYNC_DONE | + Gst.MessageType.DURATION_CHANGED | Gst.MessageType.TAG ) @@ -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,16 +182,28 @@ 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 tags: - return tags, mime, have_audio + success, duration = pipeline.query_duration(Gst.Format.TIME) + if success: + duration = duration // Gst.MSECOND else: - # workaround for https://bugzilla.gnome.org/show_bug.cgi?id=763553: - pipeline.set_state(Gst.State.PLAYING) + duration = None + if (tags and duration + # workaround for + # https://bugzilla.gnome.org/show_bug.cgi?id=763553: + # try to start pipeline playing; if it doesn't then + # give up: + ) or ( pipeline.set_state(Gst.State.PLAYING) + == 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. @@ -223,8 +212,10 @@ def _process(pipeline, timeout_ms): now = int(time.time() * 1000) timeout -= now - previous previous = now + # workaround for https://bugzilla.gnome.org/show_bug.cgi?id=763553: - if tags: + # if we got what we want then stop playing (and wait for ASYNC_DONE) + if tags and duration: pipeline.set_state(Gst.State.PAUSED) raise exceptions.ScannerError('Timeout after %dms' % timeout_ms) From 6379f768890e6e412ade1889e0fc2cc25e4425f2 Mon Sep 17 00:00:00 2001 From: SeeSpotRun Date: Tue, 15 Mar 2016 18:40:25 +1000 Subject: [PATCH 14/24] scanner: minor simplification of timeout calculations --- mopidy/audio/scan.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/mopidy/audio/scan.py b/mopidy/audio/scan.py index 4642c3f2..162d1916 100644 --- a/mopidy/audio/scan.py +++ b/mopidy/audio/scan.py @@ -160,7 +160,7 @@ def _process(pipeline, timeout_ms): ) 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) @@ -209,9 +209,7 @@ def _process(pipeline, timeout_ms): # 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) From 2d6f00b9124adb8a38ea32f3d3e8669bb9c4ecf6 Mon Sep 17 00:00:00 2001 From: SeeSpotRun Date: Tue, 15 Mar 2016 18:59:06 +1000 Subject: [PATCH 15/24] scanner: formatting to make travis / tox / flake8 happy --- mopidy/audio/scan.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/mopidy/audio/scan.py b/mopidy/audio/scan.py index 162d1916..99b14c82 100644 --- a/mopidy/audio/scan.py +++ b/mopidy/audio/scan.py @@ -197,8 +197,8 @@ def _process(pipeline, timeout_ms): # https://bugzilla.gnome.org/show_bug.cgi?id=763553: # try to start pipeline playing; if it doesn't then # give up: - ) or ( pipeline.set_state(Gst.State.PLAYING) - == Gst.StateChangeReturn.FAILURE): + ) or (pipeline.set_state(Gst.State.PLAYING) == + 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 @@ -209,7 +209,7 @@ def _process(pipeline, timeout_ms): # Note that this will only keep the last tag. tags.update(tags_lib.convert_taglist(taglist)) - timeout = timeout_ms - ( int(time.time() * 1000) - start ) + 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) From c972ecd1f26431adfa01a4ac32d77223d01fed52 Mon Sep 17 00:00:00 2001 From: SeeSpotRun Date: Wed, 16 Mar 2016 08:42:34 +1000 Subject: [PATCH 16/24] scanner: fix newby logic error --- mopidy/audio/scan.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/mopidy/audio/scan.py b/mopidy/audio/scan.py index 99b14c82..ff96b641 100644 --- a/mopidy/audio/scan.py +++ b/mopidy/audio/scan.py @@ -192,7 +192,7 @@ def _process(pipeline, timeout_ms): duration = duration // Gst.MSECOND else: duration = None - if (tags and duration + if (tags and duration is not None # workaround for # https://bugzilla.gnome.org/show_bug.cgi?id=763553: # try to start pipeline playing; if it doesn't then @@ -213,7 +213,7 @@ def _process(pipeline, timeout_ms): # 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: + if tags and duration is not None: pipeline.set_state(Gst.State.PAUSED) raise exceptions.ScannerError('Timeout after %dms' % timeout_ms) From e9137e132a04baf9d36ee3f34bf682c8ce3335d8 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Fri, 25 Mar 2016 15:17:02 +0100 Subject: [PATCH 17/24] audio: Try and simplify logic for going to playing in scanner --- mopidy/audio/scan.py | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/mopidy/audio/scan.py b/mopidy/audio/scan.py index ff96b641..27888638 100644 --- a/mopidy/audio/scan.py +++ b/mopidy/audio/scan.py @@ -192,14 +192,18 @@ def _process(pipeline, timeout_ms): duration = duration // Gst.MSECOND else: duration = None - if (tags and duration is not None - # workaround for - # https://bugzilla.gnome.org/show_bug.cgi?id=763553: - # try to start pipeline playing; if it doesn't then - # give up: - ) or (pipeline.set_state(Gst.State.PLAYING) == - Gst.StateChangeReturn.FAILURE): + + 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: From accd6bd1a9957ec89d591c599b7902b6d96deb5e Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Fri, 25 Mar 2016 15:18:13 +0100 Subject: [PATCH 18/24] docs: Add duration fix to changelog. --- docs/changelog.rst | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/docs/changelog.rst b/docs/changelog.rst index f8f7e43b..2e9ca56a 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -18,6 +18,11 @@ Bug fix release. - Audio: Fix buffer conversion. This fixes image extraction. (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`) + v2.0.0 (2016-02-15) =================== From a2f0d6960ff0e1f02d15f201cb37133b7aadc83a Mon Sep 17 00:00:00 2001 From: Jens Luetjen Date: Wed, 17 Feb 2016 22:54:22 +0100 Subject: [PATCH 19/24] Avoid endless loop if all tracks are unplayable. Limit the number of tries for changing to the nest track. The limit is 2 * tracklist length get all tracks in a shuffled playlist. --- mopidy/core/playback.py | 36 ++++++++++++++++++++++++------------ 1 file changed, 24 insertions(+), 12 deletions(-) diff --git a/mopidy/core/playback.py b/mopidy/core/playback.py index d6c470f2..0d945b1c 100644 --- a/mopidy/core/playback.py +++ b/mopidy/core/playback.py @@ -252,22 +252,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): """ @@ -352,6 +357,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): @@ -360,6 +368,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? From 6116705c1be9c3d39281991f39b3bfb417d9ed36 Mon Sep 17 00:00:00 2001 From: Jens Luetjen Date: Wed, 17 Feb 2016 23:41:56 +0100 Subject: [PATCH 20/24] Avoid endless loop in 'next' and 'previous'. --- mopidy/core/playback.py | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/mopidy/core/playback.py b/mopidy/core/playback.py index 0d945b1c..ab96171e 100644 --- a/mopidy/core/playback.py +++ b/mopidy/core/playback.py @@ -295,6 +295,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) @@ -306,6 +309,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? @@ -428,6 +435,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 +449,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? From bf4da7a62700d623757a6e9de94b28cc2ed6c7cc Mon Sep 17 00:00:00 2001 From: Jens Luetjen Date: Thu, 18 Feb 2016 20:30:28 +0100 Subject: [PATCH 21/24] Add tests to ensure that play, next will not busy-loop Test PlaybackController functions play(), next(), previous() and _on_about_to_finish() that they will not loop forever if all tracks are unplayable. --- tests/core/test_playback.py | 105 ++++++++++++++++++++++++++++++++++++ 1 file changed, 105 insertions(+) diff --git a/tests/core/test_playback.py b/tests/core/test_playback.py index cfd58793..3572800c 100644 --- a/tests/core/test_playback.py +++ b/tests/core/test_playback.py @@ -14,11 +14,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 @@ -1125,3 +1156,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()) From aafcf8559d0a56db9f9a3d384535c6ba1272f20e Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Sat, 26 Mar 2016 09:51:14 +0100 Subject: [PATCH 22/24] docs: Add PR #1455 to changelog --- docs/changelog.rst | 3 +++ 1 file changed, 3 insertions(+) diff --git a/docs/changelog.rst b/docs/changelog.rst index 2e9ca56a..ee2ddf8c 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -23,6 +23,9 @@ Bug fix release. (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`) + v2.0.0 (2016-02-15) =================== From 8820a88e0c36e2f813c39c448428a4800019d9d6 Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Sat, 26 Mar 2016 11:27:57 +0100 Subject: [PATCH 23/24] file: Fix crash when media_dirs contains non-ASCII chars Fixes #1345 --- docs/changelog.rst | 4 ++++ mopidy/file/library.py | 3 ++- mopidy/internal/path.py | 5 +++++ tests/internal/test_path.py | 26 ++++++++++++++++++++++++++ 4 files changed, 37 insertions(+), 1 deletion(-) diff --git a/docs/changelog.rst b/docs/changelog.rst index ee2ddf8c..067e59fb 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -26,6 +26,10 @@ Bug fix release. - 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/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/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): From 095329ce8224cf19f8c652d9d9abdd2b447357de Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Sat, 26 Mar 2016 11:55:11 +0100 Subject: [PATCH 24/24] docs: PR #1472 fixes #1469 --- docs/changelog.rst | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/changelog.rst b/docs/changelog.rst index ee2ddf8c..acb8f0c7 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -16,12 +16,12 @@ Bug fix release. warning. - Audio: Fix buffer conversion. This fixes image extraction. - (PR: :issue:`1472`) + (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`) + (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`)