From 138738075b95d3d8eae3468daa542c0270965299 Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Fri, 8 Jul 2022 10:01:11 +0200 Subject: [PATCH 01/38] libfmt 9 support libfmt version 9 broke the API by removing fmt::make_args_checked(). Fixes https://bugs.debian.org/1014543 --- NEWS | 1 + src/Log.hxx | 5 ++++- src/client/Response.hxx | 10 ++++++++-- 3 files changed, 13 insertions(+), 3 deletions(-) diff --git a/NEWS b/NEWS index deca41469..1144fa406 100644 --- a/NEWS +++ b/NEWS @@ -3,6 +3,7 @@ ver 0.23.8 (not yet released) - curl: fix crash if web server does not understand WebDAV * output - pipewire: fix crash with PipeWire 0.3.53 +* support libfmt 9 ver 0.23.7 (2022/05/09) * database diff --git a/src/Log.hxx b/src/Log.hxx index 186131087..3ca8b95b2 100644 --- a/src/Log.hxx +++ b/src/Log.hxx @@ -45,7 +45,10 @@ void LogFmt(LogLevel level, const Domain &domain, const S &format_str, Args&&... args) noexcept { -#if FMT_VERSION >= 70000 +#if FMT_VERSION >= 90000 + return LogVFmt(level, domain, format_str, + fmt::make_format_args(args...)); +#elif FMT_VERSION >= 70000 return LogVFmt(level, domain, fmt::to_string_view(format_str), fmt::make_args_checked(format_str, args...)); diff --git a/src/client/Response.hxx b/src/client/Response.hxx index 7955d07c1..6f9e7da33 100644 --- a/src/client/Response.hxx +++ b/src/client/Response.hxx @@ -82,7 +82,10 @@ public: template bool Fmt(const S &format_str, Args&&... args) noexcept { -#if FMT_VERSION >= 70000 +#if FMT_VERSION >= 90000 + return VFmt(format_str, + fmt::make_format_args(args...)); +#elif FMT_VERSION >= 70000 return VFmt(fmt::to_string_view(format_str), fmt::make_args_checked(format_str, args...)); @@ -109,7 +112,10 @@ public: template void FmtError(enum ack code, const S &format_str, Args&&... args) noexcept { -#if FMT_VERSION >= 70000 +#if FMT_VERSION >= 90000 + return VFmtError(code, format_str, + fmt::make_format_args(args...)); +#elif FMT_VERSION >= 70000 return VFmtError(code, fmt::to_string_view(format_str), fmt::make_args_checked(format_str, args...)); From cd933aa35f9646f90634d343b1a59088387a64da Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Fri, 8 Jul 2022 10:08:25 +0200 Subject: [PATCH 02/38] subprojects: update fmt and vorbis --- subprojects/fmt.wrap | 8 ++++---- subprojects/vorbis.wrap | 7 ++++--- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/subprojects/fmt.wrap b/subprojects/fmt.wrap index 63869be17..366e7bb85 100644 --- a/subprojects/fmt.wrap +++ b/subprojects/fmt.wrap @@ -3,10 +3,10 @@ directory = fmt-8.1.1 source_url = https://github.com/fmtlib/fmt/archive/8.1.1.tar.gz source_filename = fmt-8.1.1.tar.gz source_hash = 3d794d3cf67633b34b2771eb9f073bde87e846e0d395d254df7b211ef1ec7346 -patch_filename = fmt_8.1.1-1_patch.zip -patch_url = https://wrapdb.mesonbuild.com/v2/fmt_8.1.1-1/get_patch -patch_hash = 6035a67c7a8c90bed74c293c7265c769f47a69816125f7566bccb8e2543cee5e +patch_filename = fmt_8.1.1-2_patch.zip +patch_url = https://wrapdb.mesonbuild.com/v2/fmt_8.1.1-2/get_patch +patch_hash = cd001046281330a8862591780a9ea71a1fa594edd0d015deb24e44680c9ea33b +wrapdb_version = 8.1.1-2 [provide] fmt = fmt_dep - diff --git a/subprojects/vorbis.wrap b/subprojects/vorbis.wrap index dfca897c9..6c1a9a94d 100644 --- a/subprojects/vorbis.wrap +++ b/subprojects/vorbis.wrap @@ -3,9 +3,10 @@ directory = libvorbis-1.3.7 source_url = https://downloads.xiph.org/releases/vorbis/libvorbis-1.3.7.tar.xz source_filename = libvorbis-1.3.7.tar.xz source_hash = b33cc4934322bcbf6efcbacf49e3ca01aadbea4114ec9589d1b1e9d20f72954b -patch_filename = vorbis_1.3.7-2_patch.zip -patch_url = https://wrapdb.mesonbuild.com/v2/vorbis_1.3.7-2/get_patch -patch_hash = fe302576cbf8408754b332b539ea1b83f0f96fa9aae50a5d1fea911713d5f21c +patch_filename = vorbis_1.3.7-3_patch.zip +patch_url = https://wrapdb.mesonbuild.com/v2/vorbis_1.3.7-3/get_patch +patch_hash = 6cb90a61ede8c64d3e8e379b96dcc800c9dd69e925122b3d73d8f59a563c3afa +wrapdb_version = 1.3.7-3 [provide] vorbis = vorbis_dep From 2f6ceb49493f48aad2d3a8c576bd73875bf2c503 Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Fri, 8 Jul 2022 10:10:41 +0200 Subject: [PATCH 03/38] python/build/libs.py: update OpenSSL to 3.0.5 --- python/build/libs.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/python/build/libs.py b/python/build/libs.py index afad33c06..061557c26 100644 --- a/python/build/libs.py +++ b/python/build/libs.py @@ -382,8 +382,8 @@ ffmpeg = FfmpegProject( ) openssl = OpenSSLProject( - 'https://www.openssl.org/source/openssl-3.0.3.tar.gz', - 'ee0078adcef1de5f003c62c80cc96527721609c6f3bb42b7795df31f8b558c0b', + 'https://www.openssl.org/source/openssl-3.0.5.tar.gz', + 'aa7d8d9bef71ad6525c55ba11e5f4397889ce49c2c9349dcea6d3e4f0b024a7a', 'include/openssl/ossl_typ.h', ) From 9bdc75524b1276f1c79d79815d44a33bdde3d6df Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Fri, 8 Jul 2022 10:11:20 +0200 Subject: [PATCH 04/38] python/build/libs.py: update CURL to 7.84.0 --- python/build/libs.py | 4 ++-- .../no_CMAKE_C_IMPLICIT_LINK_LIBRARIES.patch | 10 ++++++---- src/lib/curl/patches/no_netrc.patch | 18 +++++++++--------- 3 files changed, 17 insertions(+), 15 deletions(-) diff --git a/python/build/libs.py b/python/build/libs.py index 061557c26..0fca3daca 100644 --- a/python/build/libs.py +++ b/python/build/libs.py @@ -388,8 +388,8 @@ openssl = OpenSSLProject( ) curl = CmakeProject( - 'https://curl.se/download/curl-7.83.1.tar.xz', - '2cb9c2356e7263a1272fd1435ef7cdebf2cd21400ec287b068396deb705c22c4', + 'https://curl.se/download/curl-7.84.0.tar.xz', + '2d118b43f547bfe5bae806d8d47b4e596ea5b25a6c1f080aef49fbcd817c5db8', 'lib/libcurl.a', [ '-DBUILD_CURL_EXE=OFF', diff --git a/src/lib/curl/patches/no_CMAKE_C_IMPLICIT_LINK_LIBRARIES.patch b/src/lib/curl/patches/no_CMAKE_C_IMPLICIT_LINK_LIBRARIES.patch index d7d0ed678..dfa126374 100644 --- a/src/lib/curl/patches/no_CMAKE_C_IMPLICIT_LINK_LIBRARIES.patch +++ b/src/lib/curl/patches/no_CMAKE_C_IMPLICIT_LINK_LIBRARIES.patch @@ -1,6 +1,8 @@ ---- curl-7.75.0.orig/CMakeLists.txt 2021-02-02 09:26:24.000000000 +0100 -+++ curl-7.75.0/CMakeLists.txt 2021-03-25 20:17:25.445684029 +0100 -@@ -1453,7 +1453,7 @@ +Index: curl-7.84.0/CMakeLists.txt +=================================================================== +--- curl-7.84.0.orig/CMakeLists.txt ++++ curl-7.84.0/CMakeLists.txt +@@ -1536,7 +1536,7 @@ set(includedir "\${prefix}/ set(LDFLAGS "${CMAKE_SHARED_LINKER_FLAGS}") set(LIBCURL_LIBS "") set(libdir "${CMAKE_INSTALL_PREFIX}/lib") @@ -8,4 +10,4 @@ +foreach(_lib ${CURL_LIBS}) if(TARGET "${_lib}") set(_libname "${_lib}") - get_target_property(_libtype "${_libname}" TYPE) + get_target_property(_imported "${_libname}" IMPORTED) diff --git a/src/lib/curl/patches/no_netrc.patch b/src/lib/curl/patches/no_netrc.patch index c825ea36a..35b83b621 100644 --- a/src/lib/curl/patches/no_netrc.patch +++ b/src/lib/curl/patches/no_netrc.patch @@ -1,20 +1,20 @@ -Index: curl-7.71.1/lib/url.c +Index: curl-7.84.0/lib/url.c =================================================================== ---- curl-7.71.1.orig/lib/url.c -+++ curl-7.71.1/lib/url.c -@@ -2871,6 +2871,7 @@ - } +--- curl-7.84.0.orig/lib/url.c ++++ curl-7.84.0/lib/url.c +@@ -3003,6 +3003,7 @@ static CURLcode override_login(struct Cu + #ifndef CURL_DISABLE_NETRC conn->bits.netrc = FALSE; +#ifndef __BIONIC__ if(data->set.use_netrc && !data->set.str[STRING_USERNAME]) { bool netrc_user_changed = FALSE; bool netrc_passwd_changed = FALSE; -@@ -2895,6 +2896,7 @@ - conn->bits.user_passwd = TRUE; /* enable user+password */ +@@ -3079,6 +3080,7 @@ static CURLcode override_login(struct Cu + return CURLE_OUT_OF_MEMORY; } } +#endif - /* for updated strings, we update them in the URL */ - if(*userp) { + return CURLE_OK; + } From 2d7181105dddd0a73835e47f14513f5ce363458f Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Fri, 8 Jul 2022 10:46:15 +0200 Subject: [PATCH 05/38] output/MultipleOutputs: SetVolume() throws on error This reveals more about the nature of an error instead of just returning "problems setting volume". --- NEWS | 2 ++ src/command/OtherCommands.cxx | 15 +++------ src/mixer/MixerAll.cxx | 58 ++++++++++++++++++++++++++-------- src/mixer/Volume.cxx | 8 ++--- src/mixer/Volume.hxx | 5 ++- src/output/MultipleOutputs.hxx | 5 +-- 6 files changed, 62 insertions(+), 31 deletions(-) diff --git a/NEWS b/NEWS index 1144fa406..c9f55947e 100644 --- a/NEWS +++ b/NEWS @@ -3,6 +3,8 @@ ver 0.23.8 (not yet released) - curl: fix crash if web server does not understand WebDAV * output - pipewire: fix crash with PipeWire 0.3.53 +* mixer + - better error messages * support libfmt 9 ver 0.23.7 (2022/05/09) diff --git a/src/command/OtherCommands.cxx b/src/command/OtherCommands.cxx index 9cac216f0..43a53f7c8 100644 --- a/src/command/OtherCommands.cxx +++ b/src/command/OtherCommands.cxx @@ -333,15 +333,11 @@ handle_getvol(Client &client, Request, Response &r) } CommandResult -handle_setvol(Client &client, Request args, Response &r) +handle_setvol(Client &client, Request args, Response &) { unsigned level = args.ParseUnsigned(0, 100); - if (!volume_level_change(client.GetPartition().outputs, level)) { - r.Error(ACK_ERROR_SYSTEM, "problems setting volume"); - return CommandResult::ERROR; - } - + volume_level_change(client.GetPartition().outputs, level); return CommandResult::OK; } @@ -364,11 +360,8 @@ handle_volume(Client &client, Request args, Response &r) else if (new_volume > 100) new_volume = 100; - if (new_volume != old_volume && - !volume_level_change(outputs, new_volume)) { - r.Error(ACK_ERROR_SYSTEM, "problems setting volume"); - return CommandResult::ERROR; - } + if (new_volume != old_volume) + volume_level_change(outputs, new_volume); return CommandResult::OK; } diff --git a/src/mixer/MixerAll.cxx b/src/mixer/MixerAll.cxx index 2d9c9080e..96830e7cc 100644 --- a/src/mixer/MixerAll.cxx +++ b/src/mixer/MixerAll.cxx @@ -73,42 +73,74 @@ MultipleOutputs::GetVolume() const noexcept return total / ok; } -static bool -output_mixer_set_volume(AudioOutputControl &ao, unsigned volume) noexcept +enum class SetVolumeResult { + NO_MIXER, + DISABLED, + ERROR, + OK, +}; + +static SetVolumeResult +output_mixer_set_volume(AudioOutputControl &ao, unsigned volume) { assert(volume <= 100); auto *mixer = ao.GetMixer(); if (mixer == nullptr) - return false; + return SetVolumeResult::NO_MIXER; /* software mixers are always updated, even if they are disabled */ if (!ao.IsReallyEnabled() && !mixer->IsPlugin(software_mixer_plugin)) - return false; + return SetVolumeResult::DISABLED; try { mixer_set_volume(mixer, volume); - return true; + return SetVolumeResult::OK; } catch (...) { FmtError(mixer_domain, "Failed to set mixer for '{}': {}", ao.GetName(), std::current_exception()); - return false; + std::throw_with_nested(std::runtime_error(fmt::format("Failed to set mixer for '{}'", + ao.GetName()))); } } -bool -MultipleOutputs::SetVolume(unsigned volume) noexcept +void +MultipleOutputs::SetVolume(unsigned volume) { assert(volume <= 100); - bool success = false; - for (const auto &ao : outputs) - success = output_mixer_set_volume(*ao, volume) - || success; + SetVolumeResult result = SetVolumeResult::NO_MIXER; + std::exception_ptr error; - return success; + for (const auto &ao : outputs) { + try { + auto r = output_mixer_set_volume(*ao, volume); + if (r > result) + result = r; + } catch (...) { + /* remember the first error */ + if (!error) { + error = std::current_exception(); + result = SetVolumeResult::ERROR; + } + } + } + + switch (result) { + case SetVolumeResult::NO_MIXER: + throw std::runtime_error{"No mixer"}; + + case SetVolumeResult::DISABLED: + throw std::runtime_error{"All outputs are disabled"}; + + case SetVolumeResult::ERROR: + std::rethrow_exception(error); + + case SetVolumeResult::OK: + break; + } } static int diff --git a/src/mixer/Volume.cxx b/src/mixer/Volume.cxx index 5f37e48fc..4afb97e5d 100644 --- a/src/mixer/Volume.cxx +++ b/src/mixer/Volume.cxx @@ -71,16 +71,16 @@ software_volume_change(MultipleOutputs &outputs, unsigned volume) return true; } -static bool +static void hardware_volume_change(MultipleOutputs &outputs, unsigned volume) { /* reset the cache */ last_hardware_volume = -1; - return outputs.SetVolume(volume); + outputs.SetVolume(volume); } -bool +void volume_level_change(MultipleOutputs &outputs, unsigned volume) { assert(volume <= 100); @@ -89,7 +89,7 @@ volume_level_change(MultipleOutputs &outputs, unsigned volume) idle_add(IDLE_MIXER); - return hardware_volume_change(outputs, volume); + hardware_volume_change(outputs, volume); } bool diff --git a/src/mixer/Volume.hxx b/src/mixer/Volume.hxx index ae38ce6f0..b245fe7fe 100644 --- a/src/mixer/Volume.hxx +++ b/src/mixer/Volume.hxx @@ -30,7 +30,10 @@ InvalidateHardwareVolume() noexcept; int volume_level_get(const MultipleOutputs &outputs) noexcept; -bool +/** + * Throws on error. + */ +void volume_level_change(MultipleOutputs &outputs, unsigned volume); bool diff --git a/src/output/MultipleOutputs.hxx b/src/output/MultipleOutputs.hxx index 761111ae3..d32c2c7e9 100644 --- a/src/output/MultipleOutputs.hxx +++ b/src/output/MultipleOutputs.hxx @@ -141,10 +141,11 @@ public: /** * Sets the volume on all available mixers. * + * Throws on error. + * * @param volume the volume (range 0..100) - * @return true on success, false on failure */ - bool SetVolume(unsigned volume) noexcept; + void SetVolume(unsigned volume); /** * Similar to GetVolume(), but gets the volume only for From 47680f936be878281e65bc11834ec90c63bac17d Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Fri, 8 Jul 2022 10:35:09 +0200 Subject: [PATCH 06/38] mixer/All: auto-open "global" mixers If a mixer is "global", it is available even if the output isn't open. However, since the check was changed from IsEnabled() to IsReallyEnabled(), enabled outputs have not yet been used have not been "really" enabled yet, preventing using the mixer. Fixes a regression by commit 35dbc1a90c5110fbf6ab18e98c43463d1b4ea4f3 (part of https://github.com/MusicPlayerDaemon/MPD/pull/1480). Closes https://github.com/MusicPlayerDaemon/MPD/issues/1563 --- NEWS | 1 + src/mixer/MixerAll.cxx | 5 ++++- src/mixer/MixerInternal.hxx | 4 ++++ 3 files changed, 9 insertions(+), 1 deletion(-) diff --git a/NEWS b/NEWS index c9f55947e..cb25c1a0e 100644 --- a/NEWS +++ b/NEWS @@ -5,6 +5,7 @@ ver 0.23.8 (not yet released) - pipewire: fix crash with PipeWire 0.3.53 * mixer - better error messages + - alsa: fix setting volume before playback starts * support libfmt 9 ver 0.23.7 (2022/05/09) diff --git a/src/mixer/MixerAll.cxx b/src/mixer/MixerAll.cxx index 96830e7cc..302a06160 100644 --- a/src/mixer/MixerAll.cxx +++ b/src/mixer/MixerAll.cxx @@ -91,7 +91,10 @@ output_mixer_set_volume(AudioOutputControl &ao, unsigned volume) /* software mixers are always updated, even if they are disabled */ - if (!ao.IsReallyEnabled() && !mixer->IsPlugin(software_mixer_plugin)) + if (!mixer->IsPlugin(software_mixer_plugin) && + /* "global" mixers can be used even if the output hasn't + been used yet */ + !(mixer->IsGlobal() ? ao.IsEnabled() : ao.IsReallyEnabled())) return SetVolumeResult::DISABLED; try { diff --git a/src/mixer/MixerInternal.hxx b/src/mixer/MixerInternal.hxx index 9fc92d2bd..68b89df4f 100644 --- a/src/mixer/MixerInternal.hxx +++ b/src/mixer/MixerInternal.hxx @@ -63,6 +63,10 @@ public: return &plugin == &other; } + bool IsGlobal() const noexcept { + return plugin.global; + } + /** * Open mixer device * From ecee6f415ba5b7314b7c932c8438eeba1fc84903 Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Fri, 8 Jul 2022 11:10:14 +0200 Subject: [PATCH 07/38] mixer/MixerInternal: remember error details If a mixer is not open, rethrow the original exception each time setting the volume is requested. This further improves error messages sent to MPD clients. --- src/mixer/MixerControl.cxx | 28 +++++++++------------------- src/mixer/MixerInternal.hxx | 14 ++++++++------ 2 files changed, 17 insertions(+), 25 deletions(-) diff --git a/src/mixer/MixerControl.cxx b/src/mixer/MixerControl.cxx index 8ed050482..51ea759c1 100644 --- a/src/mixer/MixerControl.cxx +++ b/src/mixer/MixerControl.cxx @@ -60,9 +60,9 @@ mixer_open(Mixer *mixer) try { mixer->Open(); mixer->open = true; - mixer->failed = false; + mixer->failure = {}; } catch (...) { - mixer->failed = true; + mixer->failure = std::current_exception(); throw; } } @@ -75,6 +75,7 @@ mixer_close_internal(Mixer *mixer) mixer->Close(); mixer->open = false; + mixer->failure = {}; } void @@ -95,20 +96,6 @@ mixer_auto_close(Mixer *mixer) mixer_close(mixer); } -/* - * Close the mixer due to failure. The mutex must be locked before - * calling this function. - */ -static void -mixer_failed(Mixer *mixer) -{ - assert(mixer->open); - - mixer_close_internal(mixer); - - mixer->failed = true; -} - int mixer_get_volume(Mixer *mixer) { @@ -116,7 +103,7 @@ mixer_get_volume(Mixer *mixer) assert(mixer != nullptr); - if (mixer->plugin.global && !mixer->failed) + if (mixer->plugin.global && !mixer->failure) mixer_open(mixer); const std::scoped_lock protect(mixer->mutex); @@ -125,7 +112,8 @@ mixer_get_volume(Mixer *mixer) try { volume = mixer->GetVolume(); } catch (...) { - mixer_failed(mixer); + mixer_close_internal(mixer); + mixer->failure = std::current_exception(); throw; } } else @@ -140,11 +128,13 @@ mixer_set_volume(Mixer *mixer, unsigned volume) assert(mixer != nullptr); assert(volume <= 100); - if (mixer->plugin.global && !mixer->failed) + if (mixer->plugin.global && !mixer->failure) mixer_open(mixer); const std::scoped_lock protect(mixer->mutex); if (mixer->open) mixer->SetVolume(volume); + else if (mixer->failure) + std::rethrow_exception(mixer->failure); } diff --git a/src/mixer/MixerInternal.hxx b/src/mixer/MixerInternal.hxx index 68b89df4f..fd075000f 100644 --- a/src/mixer/MixerInternal.hxx +++ b/src/mixer/MixerInternal.hxx @@ -25,6 +25,8 @@ #include "thread/Mutex.hxx" #include "util/Compiler.h" +#include + class MixerListener; class Mixer { @@ -39,17 +41,17 @@ public: */ Mutex mutex; + /** + * Contains error details if this mixer has failed. If set, + * it should not be reopened automatically. + */ + std::exception_ptr failure; + /** * Is the mixer device currently open? */ bool open = false; - /** - * Has this mixer failed, and should not be reopened - * automatically? - */ - bool failed = false; - public: explicit Mixer(const MixerPlugin &_plugin, MixerListener &_listener) noexcept From 4d6ae6ffdde6132551107be63e47549ef595ad02 Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Fri, 8 Jul 2022 11:27:17 +0200 Subject: [PATCH 08/38] output/PipeWire: add nullptr check to SetVolume() If the PipeWire output has not yet been enabled and no thread_loop has been created yet, a nullptr dereference in SetVolume() was possible because nullptr was passed to pw_thread_loop_lock(). --- NEWS | 1 + src/output/plugins/PipeWireOutputPlugin.cxx | 8 ++++++++ 2 files changed, 9 insertions(+) diff --git a/NEWS b/NEWS index cb25c1a0e..c221a6669 100644 --- a/NEWS +++ b/NEWS @@ -6,6 +6,7 @@ ver 0.23.8 (not yet released) * mixer - better error messages - alsa: fix setting volume before playback starts + - pipewire: fix crash bug * support libfmt 9 ver 0.23.7 (2022/05/09) diff --git a/src/output/plugins/PipeWireOutputPlugin.cxx b/src/output/plugins/PipeWireOutputPlugin.cxx index c752820d0..c407dd3c1 100644 --- a/src/output/plugins/PipeWireOutputPlugin.cxx +++ b/src/output/plugins/PipeWireOutputPlugin.cxx @@ -312,6 +312,14 @@ PipeWireOutput::PipeWireOutput(const ConfigBlock &block) void PipeWireOutput::SetVolume(float _volume) { + if (thread_loop == nullptr) { + /* the mixer is open (because it is a "global" mixer), + but Enable() on this output has not yet been + called */ + volume = _volume; + return; + } + const PipeWire::ThreadLoopLock lock(thread_loop); float newvol = _volume*_volume*_volume; From be0360d5e84bd3c8abc1d91bb464e021263dea9c Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Fri, 8 Jul 2022 11:44:08 +0200 Subject: [PATCH 09/38] doc/user.rst: clarify .mpdignore documentation Closes https://github.com/MusicPlayerDaemon/MPD/issues/1532 --- doc/user.rst | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/doc/user.rst b/doc/user.rst index 06d8e2924..85b8ba696 100644 --- a/doc/user.rst +++ b/doc/user.rst @@ -1063,7 +1063,19 @@ The "music directory" is where you store your music files. :program:`MPD` stores Depending on the size of your music collection and the speed of the storage, this can take a while. -To exclude a file from the update, create a file called :file:`.mpdignore` in its parent directory. Each line of that file may contain a list of shell wildcards. Matching files in the current directory and all subdirectories are excluded. +To exclude a file from the update, create a file called +:file:`.mpdignore` in its parent directory. Each line of that file +may contain a list of shell wildcards. Matching files (or +directories) in the current directory and all subdirectories are +excluded. Example:: + + *.opus + 99* + +Subject to pattern matching is the file/directory name. It is (not +yet) possible to match nested path names, e.g. something like +``foo/*.flac`` is not possible. + Mounting other storages into the music directory ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ From 915c5442d10fec04cfdcea639f1e9086a2461167 Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Fri, 8 Jul 2022 12:03:26 +0200 Subject: [PATCH 10/38] input/CdioParanoia: use AtScopeExit() for cdio_free_device_list() --- src/input/plugins/CdioParanoiaInputPlugin.cxx | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/input/plugins/CdioParanoiaInputPlugin.cxx b/src/input/plugins/CdioParanoiaInputPlugin.cxx index 0cbaca248..55cc2a5a4 100644 --- a/src/input/plugins/CdioParanoiaInputPlugin.cxx +++ b/src/input/plugins/CdioParanoiaInputPlugin.cxx @@ -30,6 +30,7 @@ #include "util/RuntimeError.hxx" #include "util/Domain.hxx" #include "util/ByteOrder.hxx" +#include "util/ScopeExit.hxx" #include "fs/AllocatedPath.hxx" #include "Log.hxx" #include "config/Block.hxx" @@ -173,9 +174,9 @@ cdio_detect_device() if (devices == nullptr) return nullptr; - AllocatedPath path = AllocatedPath::FromFS(devices[0]); - cdio_free_device_list(devices); - return path; + AtScopeExit(devices) { cdio_free_device_list(devices); }; + + return AllocatedPath::FromFS(devices[0]); } static InputStreamPtr From 8eb3164878ca159016db6afa5606c4ecc6cebc54 Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Fri, 8 Jul 2022 12:04:41 +0200 Subject: [PATCH 11/38] input/CdioParanoia: fix crash if no drive was found cdio_get_devices_with_cap() can return nullptr if no drive was found, or it can instead return an empty list. The latter caused MPD to crash. --- NEWS | 2 ++ src/input/plugins/CdioParanoiaInputPlugin.cxx | 3 +++ 2 files changed, 5 insertions(+) diff --git a/NEWS b/NEWS index c221a6669..5a9270420 100644 --- a/NEWS +++ b/NEWS @@ -1,6 +1,8 @@ ver 0.23.8 (not yet released) * storage - curl: fix crash if web server does not understand WebDAV +* input + - cdio_paranoia: fix crash if no drive was found * output - pipewire: fix crash with PipeWire 0.3.53 * mixer diff --git a/src/input/plugins/CdioParanoiaInputPlugin.cxx b/src/input/plugins/CdioParanoiaInputPlugin.cxx index 55cc2a5a4..29ab784a8 100644 --- a/src/input/plugins/CdioParanoiaInputPlugin.cxx +++ b/src/input/plugins/CdioParanoiaInputPlugin.cxx @@ -176,6 +176,9 @@ cdio_detect_device() AtScopeExit(devices) { cdio_free_device_list(devices); }; + if (devices[0] == nullptr) + return nullptr; + return AllocatedPath::FromFS(devices[0]); } From 1080c917bea8ab3164495f23b58cd64c79eae449 Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Fri, 8 Jul 2022 12:35:55 +0200 Subject: [PATCH 12/38] input/CdioParanoia: use std::min() --- src/input/plugins/CdioParanoiaInputPlugin.cxx | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/input/plugins/CdioParanoiaInputPlugin.cxx b/src/input/plugins/CdioParanoiaInputPlugin.cxx index 29ab784a8..6cdccf8c6 100644 --- a/src/input/plugins/CdioParanoiaInputPlugin.cxx +++ b/src/input/plugins/CdioParanoiaInputPlugin.cxx @@ -35,6 +35,7 @@ #include "Log.hxx" #include "config/Block.hxx" +#include #include #include @@ -328,7 +329,7 @@ CdioParanoiaInputStream::Read(std::unique_lock &, assert(diff >= 0 && diff < CDIO_CD_FRAMESIZE_RAW); const size_t maxwrite = CDIO_CD_FRAMESIZE_RAW - diff; //# of bytes pending in current buffer - const size_t len = (length < maxwrite? length : maxwrite); + const size_t len = std::min(length, maxwrite); //skip diff bytes from this lsn memcpy(wptr, ((const char *)rbuf) + diff, len); From 1714cf3417cefc24edd95cab15f00cf994552ce8 Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Fri, 8 Jul 2022 12:42:19 +0200 Subject: [PATCH 13/38] input/CdioParanoia: use IsEof() in Read() --- src/input/plugins/CdioParanoiaInputPlugin.cxx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/input/plugins/CdioParanoiaInputPlugin.cxx b/src/input/plugins/CdioParanoiaInputPlugin.cxx index 6cdccf8c6..b520c6024 100644 --- a/src/input/plugins/CdioParanoiaInputPlugin.cxx +++ b/src/input/plugins/CdioParanoiaInputPlugin.cxx @@ -294,7 +294,7 @@ CdioParanoiaInputStream::Read(std::unique_lock &, while (length > 0) { /* end of track ? */ - if (lsn_from + lsn_relofs > lsn_to) + if (IsEOF()) break; //current sector was changed ? From d62426f168b31e6550ead492d7edcca62e86d79a Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Fri, 8 Jul 2022 12:41:18 +0200 Subject: [PATCH 14/38] input/CdioParanoia: eliminate redundant field "lsn_to" Use "size" instead. --- src/input/plugins/CdioParanoiaInputPlugin.cxx | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/input/plugins/CdioParanoiaInputPlugin.cxx b/src/input/plugins/CdioParanoiaInputPlugin.cxx index b520c6024..22fcfd905 100644 --- a/src/input/plugins/CdioParanoiaInputPlugin.cxx +++ b/src/input/plugins/CdioParanoiaInputPlugin.cxx @@ -50,7 +50,7 @@ class CdioParanoiaInputStream final : public InputStream { CdIo_t *const cdio; CdromParanoia para; - const lsn_t lsn_from, lsn_to; + const lsn_t lsn_from; int lsn_relofs; char buffer[CDIO_CD_FRAMESIZE_RAW]; @@ -60,10 +60,10 @@ class CdioParanoiaInputStream final : public InputStream { CdioParanoiaInputStream(const char *_uri, Mutex &_mutex, cdrom_drive_t *_drv, CdIo_t *_cdio, bool reverse_endian, - lsn_t _lsn_from, lsn_t _lsn_to) + lsn_t _lsn_from, lsn_t lsn_to) :InputStream(_uri, _mutex), drv(_drv), cdio(_cdio), para(drv), - lsn_from(_lsn_from), lsn_to(_lsn_to), + lsn_from(_lsn_from), lsn_relofs(0), buffer_lsn(-1) { @@ -350,7 +350,7 @@ CdioParanoiaInputStream::Read(std::unique_lock &, bool CdioParanoiaInputStream::IsEOF() const noexcept { - return lsn_from + lsn_relofs > lsn_to; + return offset >= size; } static constexpr const char *cdio_paranoia_prefixes[] = { From df7242de91a08da4a9260141bb4fefaf36e60b42 Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Fri, 8 Jul 2022 12:37:53 +0200 Subject: [PATCH 15/38] input/CdioParanoia: eliminate redundant field "lsn_relofs" --- src/input/plugins/CdioParanoiaInputPlugin.cxx | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/input/plugins/CdioParanoiaInputPlugin.cxx b/src/input/plugins/CdioParanoiaInputPlugin.cxx index 22fcfd905..ee4b82ff4 100644 --- a/src/input/plugins/CdioParanoiaInputPlugin.cxx +++ b/src/input/plugins/CdioParanoiaInputPlugin.cxx @@ -51,7 +51,6 @@ class CdioParanoiaInputStream final : public InputStream { CdromParanoia para; const lsn_t lsn_from; - int lsn_relofs; char buffer[CDIO_CD_FRAMESIZE_RAW]; int buffer_lsn; @@ -64,7 +63,6 @@ class CdioParanoiaInputStream final : public InputStream { :InputStream(_uri, _mutex), drv(_drv), cdio(_cdio), para(drv), lsn_from(_lsn_from), - lsn_relofs(0), buffer_lsn(-1) { /* Set reading mode for full paranoia, but allow @@ -276,7 +274,7 @@ CdioParanoiaInputStream::Seek(std::unique_lock &, return; /* calculate current LSN */ - lsn_relofs = new_offset / CDIO_CD_FRAMESIZE_RAW; + const int32_t lsn_relofs = new_offset / CDIO_CD_FRAMESIZE_RAW; offset = new_offset; { @@ -299,6 +297,8 @@ CdioParanoiaInputStream::Read(std::unique_lock &, //current sector was changed ? const int16_t *rbuf; + + const int32_t lsn_relofs = offset / CDIO_CD_FRAMESIZE_RAW; if (lsn_relofs != buffer_lsn) { const ScopeUnlock unlock(mutex); @@ -339,7 +339,6 @@ CdioParanoiaInputStream::Read(std::unique_lock &, //update offset offset += len; - lsn_relofs = offset / CDIO_CD_FRAMESIZE_RAW; //update length length -= len; } From 807a19889fe3489b89b8310a7ce34f434bdfa85c Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Fri, 8 Jul 2022 13:37:50 +0200 Subject: [PATCH 16/38] input/CdioParanoia: update offset only after successful seek If seeking fails, don't leave the class with a wrong offset. --- src/input/plugins/CdioParanoiaInputPlugin.cxx | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/input/plugins/CdioParanoiaInputPlugin.cxx b/src/input/plugins/CdioParanoiaInputPlugin.cxx index ee4b82ff4..8784273b4 100644 --- a/src/input/plugins/CdioParanoiaInputPlugin.cxx +++ b/src/input/plugins/CdioParanoiaInputPlugin.cxx @@ -275,12 +275,13 @@ CdioParanoiaInputStream::Seek(std::unique_lock &, /* calculate current LSN */ const int32_t lsn_relofs = new_offset / CDIO_CD_FRAMESIZE_RAW; - offset = new_offset; { const ScopeUnlock unlock(mutex); para.Seek(lsn_from + lsn_relofs); } + + offset = new_offset; } size_t From 5573e7836471a3b489d2bb699520285004dd0e57 Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Fri, 8 Jul 2022 13:46:55 +0200 Subject: [PATCH 17/38] input/CdioParanoia: skip seek if seeking within the buffer --- src/input/plugins/CdioParanoiaInputPlugin.cxx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/input/plugins/CdioParanoiaInputPlugin.cxx b/src/input/plugins/CdioParanoiaInputPlugin.cxx index 8784273b4..ed6aaed91 100644 --- a/src/input/plugins/CdioParanoiaInputPlugin.cxx +++ b/src/input/plugins/CdioParanoiaInputPlugin.cxx @@ -276,7 +276,7 @@ CdioParanoiaInputStream::Seek(std::unique_lock &, /* calculate current LSN */ const int32_t lsn_relofs = new_offset / CDIO_CD_FRAMESIZE_RAW; - { + if (lsn_relofs != buffer_lsn) { const ScopeUnlock unlock(mutex); para.Seek(lsn_from + lsn_relofs); } From c32dceb4d45eb9cb3c13b9b3088f895bcf0f9ce5 Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Fri, 8 Jul 2022 12:26:52 +0200 Subject: [PATCH 18/38] input/CdioParanoia: remove loop from Read() The Read() method is not required to fill the whole buffer. By returning as soon as at least one byte was read, we allow faster cancellation. --- NEWS | 1 + src/input/plugins/CdioParanoiaInputPlugin.cxx | 84 ++++++++----------- 2 files changed, 38 insertions(+), 47 deletions(-) diff --git a/NEWS b/NEWS index 5a9270420..89bda0ed6 100644 --- a/NEWS +++ b/NEWS @@ -3,6 +3,7 @@ ver 0.23.8 (not yet released) - curl: fix crash if web server does not understand WebDAV * input - cdio_paranoia: fix crash if no drive was found + - cdio_paranoia: faster cancellation * output - pipewire: fix crash with PipeWire 0.3.53 * mixer diff --git a/src/input/plugins/CdioParanoiaInputPlugin.cxx b/src/input/plugins/CdioParanoiaInputPlugin.cxx index ed6aaed91..10a70850f 100644 --- a/src/input/plugins/CdioParanoiaInputPlugin.cxx +++ b/src/input/plugins/CdioParanoiaInputPlugin.cxx @@ -288,62 +288,52 @@ size_t CdioParanoiaInputStream::Read(std::unique_lock &, void *ptr, size_t length) { - size_t nbytes = 0; - char *wptr = (char *) ptr; + /* end of track ? */ + if (IsEOF()) + return 0; - while (length > 0) { - /* end of track ? */ - if (IsEOF()) - break; + //current sector was changed ? + const int16_t *rbuf; - //current sector was changed ? - const int16_t *rbuf; + const int32_t lsn_relofs = offset / CDIO_CD_FRAMESIZE_RAW; + if (lsn_relofs != buffer_lsn) { + const ScopeUnlock unlock(mutex); - const int32_t lsn_relofs = offset / CDIO_CD_FRAMESIZE_RAW; - if (lsn_relofs != buffer_lsn) { - const ScopeUnlock unlock(mutex); - - try { - rbuf = para.Read().data; - } catch (...) { - char *s_err = cdio_cddap_errors(drv); - if (s_err) { - FmtError(cdio_domain, - "paranoia_read: {}", s_err); - cdio_cddap_free_messages(s_err); - } - - throw; + try { + rbuf = para.Read().data; + } catch (...) { + char *s_err = cdio_cddap_errors(drv); + if (s_err) { + FmtError(cdio_domain, + "paranoia_read: {}", s_err); + cdio_cddap_free_messages(s_err); } - //store current buffer - memcpy(buffer, rbuf, CDIO_CD_FRAMESIZE_RAW); - buffer_lsn = lsn_relofs; - } else { - //use cached sector - rbuf = (const int16_t *)buffer; + throw; } - //correct offset - const int diff = offset - lsn_relofs * CDIO_CD_FRAMESIZE_RAW; - - assert(diff >= 0 && diff < CDIO_CD_FRAMESIZE_RAW); - - const size_t maxwrite = CDIO_CD_FRAMESIZE_RAW - diff; //# of bytes pending in current buffer - const size_t len = std::min(length, maxwrite); - - //skip diff bytes from this lsn - memcpy(wptr, ((const char *)rbuf) + diff, len); - //update pointer - wptr += len; - nbytes += len; - - //update offset - offset += len; - //update length - length -= len; + //store current buffer + memcpy(buffer, rbuf, CDIO_CD_FRAMESIZE_RAW); + buffer_lsn = lsn_relofs; + } else { + //use cached sector + rbuf = (const int16_t *)buffer; } + //correct offset + const int diff = offset - lsn_relofs * CDIO_CD_FRAMESIZE_RAW; + + assert(diff >= 0 && diff < CDIO_CD_FRAMESIZE_RAW); + + const size_t maxwrite = CDIO_CD_FRAMESIZE_RAW - diff; //# of bytes pending in current buffer + const std::size_t nbytes = std::min(length, maxwrite); + + //skip diff bytes from this lsn + memcpy(ptr, ((const char *)rbuf) + diff, nbytes); + + //update offset + offset += nbytes; + return nbytes; } From 3613407ac5ec1a85b6b76476752c7865d244267a Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Fri, 8 Jul 2022 16:02:25 +0200 Subject: [PATCH 19/38] input/CdioParanoia: use typedef lsn_t --- src/input/plugins/CdioParanoiaInputPlugin.cxx | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/input/plugins/CdioParanoiaInputPlugin.cxx b/src/input/plugins/CdioParanoiaInputPlugin.cxx index 10a70850f..5ddcca353 100644 --- a/src/input/plugins/CdioParanoiaInputPlugin.cxx +++ b/src/input/plugins/CdioParanoiaInputPlugin.cxx @@ -53,7 +53,7 @@ class CdioParanoiaInputStream final : public InputStream { const lsn_t lsn_from; char buffer[CDIO_CD_FRAMESIZE_RAW]; - int buffer_lsn; + lsn_t buffer_lsn; public: CdioParanoiaInputStream(const char *_uri, Mutex &_mutex, @@ -274,7 +274,7 @@ CdioParanoiaInputStream::Seek(std::unique_lock &, return; /* calculate current LSN */ - const int32_t lsn_relofs = new_offset / CDIO_CD_FRAMESIZE_RAW; + const lsn_t lsn_relofs = new_offset / CDIO_CD_FRAMESIZE_RAW; if (lsn_relofs != buffer_lsn) { const ScopeUnlock unlock(mutex); @@ -295,7 +295,7 @@ CdioParanoiaInputStream::Read(std::unique_lock &, //current sector was changed ? const int16_t *rbuf; - const int32_t lsn_relofs = offset / CDIO_CD_FRAMESIZE_RAW; + const lsn_t lsn_relofs = offset / CDIO_CD_FRAMESIZE_RAW; if (lsn_relofs != buffer_lsn) { const ScopeUnlock unlock(mutex); From 666e5d790409f4614a1da45a71d7b43b365389b4 Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Fri, 8 Jul 2022 16:03:32 +0200 Subject: [PATCH 20/38] input/CdioParanoia: use integer modulo to calculate "diff" --- src/input/plugins/CdioParanoiaInputPlugin.cxx | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/input/plugins/CdioParanoiaInputPlugin.cxx b/src/input/plugins/CdioParanoiaInputPlugin.cxx index 5ddcca353..a2e9c8211 100644 --- a/src/input/plugins/CdioParanoiaInputPlugin.cxx +++ b/src/input/plugins/CdioParanoiaInputPlugin.cxx @@ -296,6 +296,8 @@ CdioParanoiaInputStream::Read(std::unique_lock &, const int16_t *rbuf; const lsn_t lsn_relofs = offset / CDIO_CD_FRAMESIZE_RAW; + const std::size_t diff = offset % CDIO_CD_FRAMESIZE_RAW; + if (lsn_relofs != buffer_lsn) { const ScopeUnlock unlock(mutex); @@ -320,11 +322,6 @@ CdioParanoiaInputStream::Read(std::unique_lock &, rbuf = (const int16_t *)buffer; } - //correct offset - const int diff = offset - lsn_relofs * CDIO_CD_FRAMESIZE_RAW; - - assert(diff >= 0 && diff < CDIO_CD_FRAMESIZE_RAW); - const size_t maxwrite = CDIO_CD_FRAMESIZE_RAW - diff; //# of bytes pending in current buffer const std::size_t nbytes = std::min(length, maxwrite); From c0d5bd204894d97802c1258f163bff9064d66e4e Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Fri, 8 Jul 2022 16:19:40 +0200 Subject: [PATCH 21/38] decoder/Thread: move code to DecoderControl::LockIsReplayGainEnabled() --- src/decoder/Control.hxx | 6 ++++++ src/decoder/Thread.cxx | 9 +++------ 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/src/decoder/Control.hxx b/src/decoder/Control.hxx index 27d302875..1aa257f09 100644 --- a/src/decoder/Control.hxx +++ b/src/decoder/Control.hxx @@ -257,6 +257,12 @@ public: return HasFailed(); } + [[gnu::pure]] + bool LockIsReplayGainEnabled() const noexcept { + const std::scoped_lock protect(mutex); + return replay_gain_mode != ReplayGainMode::OFF; + } + /** * Transition this obejct from DecoderState::START to * DecoderState::DECODE. diff --git a/src/decoder/Thread.cxx b/src/decoder/Thread.cxx index b302dab72..4432c6605 100644 --- a/src/decoder/Thread.cxx +++ b/src/decoder/Thread.cxx @@ -261,12 +261,9 @@ LoadReplayGain(DecoderClient &client, InputStream &is) static void MaybeLoadReplayGain(DecoderBridge &bridge, InputStream &is) { - { - const std::scoped_lock protect(bridge.dc.mutex); - if (bridge.dc.replay_gain_mode == ReplayGainMode::OFF) - /* ReplayGain is disabled */ - return; - } + if (!bridge.dc.LockIsReplayGainEnabled()) + /* ReplayGain is disabled */ + return; LoadReplayGain(bridge, is); } From 6857286b421aafcec79bbd9c0321e8d988ab09e8 Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Fri, 8 Jul 2022 16:18:31 +0200 Subject: [PATCH 22/38] decoder/Thread: don't scan for replay gain tags in PCM streams This disables a long delay for playing songs from the cdio_paranoia input plugin if ReplayGain is enabled. --- NEWS | 1 + src/decoder/Thread.cxx | 8 ++++++++ 2 files changed, 9 insertions(+) diff --git a/NEWS b/NEWS index 89bda0ed6..c9f5e3919 100644 --- a/NEWS +++ b/NEWS @@ -4,6 +4,7 @@ ver 0.23.8 (not yet released) * input - cdio_paranoia: fix crash if no drive was found - cdio_paranoia: faster cancellation + - cdio_paranoia: don't scan for replay gain tags * output - pipewire: fix crash with PipeWire 0.3.53 * mixer diff --git a/src/decoder/Thread.cxx b/src/decoder/Thread.cxx index 4432c6605..c9635e70b 100644 --- a/src/decoder/Thread.cxx +++ b/src/decoder/Thread.cxx @@ -36,6 +36,7 @@ #include "util/RuntimeError.hxx" #include "util/Domain.hxx" #include "util/ScopeExit.hxx" +#include "util/StringCompare.hxx" #include "thread/Name.hxx" #include "tag/ApeReplayGain.hxx" #include "Log.hxx" @@ -265,6 +266,13 @@ MaybeLoadReplayGain(DecoderBridge &bridge, InputStream &is) /* ReplayGain is disabled */ return; + if (is.HasMimeType() && + StringStartsWith(is.GetMimeType(), "audio/x-mpd-")) + /* skip for (virtual) files (e.g. from the + cdio_paranoia input plugin) which cannot possibly + contain tags */ + return; + LoadReplayGain(bridge, is); } From f55bc6682fb9a4d46e2161311a1856fd366c68d8 Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Fri, 8 Jul 2022 17:16:34 +0200 Subject: [PATCH 23/38] output/PipeWire: move code to ::SetVolume() --- src/output/plugins/PipeWireOutputPlugin.cxx | 30 +++++++++++++-------- 1 file changed, 19 insertions(+), 11 deletions(-) diff --git a/src/output/plugins/PipeWireOutputPlugin.cxx b/src/output/plugins/PipeWireOutputPlugin.cxx index c407dd3c1..79a914ad3 100644 --- a/src/output/plugins/PipeWireOutputPlugin.cxx +++ b/src/output/plugins/PipeWireOutputPlugin.cxx @@ -309,6 +309,23 @@ PipeWireOutput::PipeWireOutput(const ConfigBlock &block) } } +/** + * Throws on error. + * + * @param volume a volume level between 0.0 and 1.0 + */ +static void +SetVolume(struct pw_stream &stream, unsigned channels, float volume) +{ + float value[MAX_CHANNELS]; + std::fill_n(value, channels, volume * volume * volume); + + if (pw_stream_set_control(&stream, + SPA_PROP_channelVolumes, channels, value, + 0) != 0) + throw std::runtime_error("pw_stream_set_control() failed"); +} + void PipeWireOutput::SetVolume(float _volume) { @@ -322,17 +339,8 @@ PipeWireOutput::SetVolume(float _volume) const PipeWire::ThreadLoopLock lock(thread_loop); - float newvol = _volume*_volume*_volume; - - if (stream != nullptr && !restore_volume) { - float vol[MAX_CHANNELS]; - std::fill_n(vol, channels, newvol); - - if (pw_stream_set_control(stream, - SPA_PROP_channelVolumes, channels, vol, - 0) != 0) - throw std::runtime_error("pw_stream_set_control() failed"); - } + if (stream != nullptr && !restore_volume) + ::SetVolume(*stream, channels, _volume); volume = _volume; } From d2fb229685c4bc30498b6a57d3b996da2842deac Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Fri, 8 Jul 2022 17:20:33 +0200 Subject: [PATCH 24/38] output/PipeWire: call ::SetVolume() in ParamChanged() This is a lower-level function without some of the clutter of PipeWireOutput::SetVolume() which is not needed in that case. --- src/output/plugins/PipeWireOutputPlugin.cxx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/output/plugins/PipeWireOutputPlugin.cxx b/src/output/plugins/PipeWireOutputPlugin.cxx index 79a914ad3..78ac228ba 100644 --- a/src/output/plugins/PipeWireOutputPlugin.cxx +++ b/src/output/plugins/PipeWireOutputPlugin.cxx @@ -656,7 +656,7 @@ PipeWireOutput::ParamChanged([[maybe_unused]] uint32_t id, { if (restore_volume) { restore_volume = false; - SetVolume(volume); + ::SetVolume(*stream, channels, volume); } #if defined(ENABLE_DSD) && defined(SPA_AUDIO_DSD_FLAG_NONE) From 9c3cf39fdd6904d7a80efc7f1f8ecb45991defdc Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Fri, 8 Jul 2022 17:21:37 +0200 Subject: [PATCH 25/38] output/PipeWire: catch exceptions in ParamChanged() Fixes a potential crash bug. --- src/output/plugins/PipeWireOutputPlugin.cxx | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/src/output/plugins/PipeWireOutputPlugin.cxx b/src/output/plugins/PipeWireOutputPlugin.cxx index 78ac228ba..66133a2b2 100644 --- a/src/output/plugins/PipeWireOutputPlugin.cxx +++ b/src/output/plugins/PipeWireOutputPlugin.cxx @@ -24,6 +24,7 @@ #include "../Error.hxx" #include "mixer/plugins/PipeWireMixerPlugin.hxx" #include "pcm/Silence.hxx" +#include "lib/fmt/ExceptionFormatter.hxx" #include "system/Error.hxx" #include "util/BitReverse.hxx" #include "util/Domain.hxx" @@ -656,7 +657,14 @@ PipeWireOutput::ParamChanged([[maybe_unused]] uint32_t id, { if (restore_volume) { restore_volume = false; - ::SetVolume(*stream, channels, volume); + + try { + ::SetVolume(*stream, channels, volume); + } catch (...) { + FmtError(pipewire_output_domain, + FMT_STRING("Failed to restore volume: {}"), + std::current_exception()); + } } #if defined(ENABLE_DSD) && defined(SPA_AUDIO_DSD_FLAG_NONE) From 0c54f2944691254c853a1d33c1fa336aeb441ce4 Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Fri, 8 Jul 2022 17:30:56 +0200 Subject: [PATCH 26/38] output/PipeWire: document field "volume" --- src/output/plugins/PipeWireOutputPlugin.cxx | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/output/plugins/PipeWireOutputPlugin.cxx b/src/output/plugins/PipeWireOutputPlugin.cxx index 66133a2b2..ee0463cc9 100644 --- a/src/output/plugins/PipeWireOutputPlugin.cxx +++ b/src/output/plugins/PipeWireOutputPlugin.cxx @@ -86,7 +86,10 @@ class PipeWireOutput final : AudioOutput { uint32_t target_id = PW_ID_ANY; - float volume = 1.0; + /** + * The current volume level (0.0 .. 1.0). + */ + float volume = 1.0f; PipeWireMixer *mixer = nullptr; unsigned channels; From 5c17b2966a2d3bd76ae34538f1bf6b268b014b35 Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Fri, 8 Jul 2022 17:43:13 +0200 Subject: [PATCH 27/38] output/PipeWire: use std::accumulate --- src/output/plugins/PipeWireOutputPlugin.cxx | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/output/plugins/PipeWireOutputPlugin.cxx b/src/output/plugins/PipeWireOutputPlugin.cxx index ee0463cc9..b60e12d53 100644 --- a/src/output/plugins/PipeWireOutputPlugin.cxx +++ b/src/output/plugins/PipeWireOutputPlugin.cxx @@ -57,6 +57,7 @@ #include #include +#include #include #include @@ -222,11 +223,9 @@ private: } void ControlInfo(const struct pw_stream_control *control) noexcept { - float sum = 0; - unsigned c; - for (c = 0; c < control->n_values; c++) - sum += control->values[c]; - + float sum = std::accumulate(control->values, + control->values + control->n_values, + 0.0f); sum /= control->n_values; if (mixer != nullptr) From 7b45d01462e30d5669055b0cf0c12b38d17d5237 Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Fri, 8 Jul 2022 17:32:16 +0200 Subject: [PATCH 28/38] output/PipeWire: update field "volume" --- src/output/plugins/PipeWireOutputPlugin.cxx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/output/plugins/PipeWireOutputPlugin.cxx b/src/output/plugins/PipeWireOutputPlugin.cxx index b60e12d53..601f6585b 100644 --- a/src/output/plugins/PipeWireOutputPlugin.cxx +++ b/src/output/plugins/PipeWireOutputPlugin.cxx @@ -226,10 +226,10 @@ private: float sum = std::accumulate(control->values, control->values + control->n_values, 0.0f); - sum /= control->n_values; + volume = std::cbrt(sum / control->n_values); if (mixer != nullptr) - pipewire_mixer_on_change(*mixer, std::cbrt(sum)); + pipewire_mixer_on_change(*mixer, volume); pw_thread_loop_signal(thread_loop, false); } From 792d6584b99427f7503b088decf9f8e184518f46 Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Fri, 8 Jul 2022 17:59:52 +0200 Subject: [PATCH 29/38] output/PipeWire: move code to OnChannelVolumes() --- src/output/plugins/PipeWireOutputPlugin.cxx | 22 ++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/src/output/plugins/PipeWireOutputPlugin.cxx b/src/output/plugins/PipeWireOutputPlugin.cxx index 601f6585b..d187a5bc9 100644 --- a/src/output/plugins/PipeWireOutputPlugin.cxx +++ b/src/output/plugins/PipeWireOutputPlugin.cxx @@ -222,11 +222,11 @@ private: o.Drained(); } - void ControlInfo(const struct pw_stream_control *control) noexcept { - float sum = std::accumulate(control->values, - control->values + control->n_values, + void OnChannelVolumes(const struct pw_stream_control &control) noexcept { + float sum = std::accumulate(control.values, + control.values + control.n_values, 0.0f); - volume = std::cbrt(sum / control->n_values); + volume = std::cbrt(sum / control.n_values); if (mixer != nullptr) pipewire_mixer_on_change(*mixer, volume); @@ -234,13 +234,17 @@ private: pw_thread_loop_signal(thread_loop, false); } - static void ControlInfo(void *data, - [[maybe_unused]] uint32_t id, + void ControlInfo([[maybe_unused]] uint32_t id, + const struct pw_stream_control &control) noexcept { + if (control.name != nullptr && + StringIsEqual(control.name, "Channel Volumes")) + OnChannelVolumes(control); + } + + static void ControlInfo(void *data, uint32_t id, const struct pw_stream_control *control) noexcept { auto &o = *(PipeWireOutput *)data; - if (control->name != nullptr && - StringIsEqual(control->name, "Channel Volumes")) - o.ControlInfo(control); + o.ControlInfo(id, *control); } #if defined(ENABLE_DSD) && defined(SPA_AUDIO_DSD_FLAG_NONE) From f08944253bcb7a5426d212eb35f886399420258f Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Fri, 8 Jul 2022 17:59:07 +0200 Subject: [PATCH 30/38] output/PipeWire: check SPA_PROP_channelVolumes, not control name Since PipeWire 0.3.53, there is no control name anymore, therefore the name check doesn't work anymore, breaking volume change events. This obsoletes the crash bug fix in commit 2ee57f9b0d8d08c554392e6b832a335593a7555d --- NEWS | 3 +-- src/output/plugins/PipeWireOutputPlugin.cxx | 6 ++++-- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/NEWS b/NEWS index c9f5e3919..355281c61 100644 --- a/NEWS +++ b/NEWS @@ -5,12 +5,11 @@ ver 0.23.8 (not yet released) - cdio_paranoia: fix crash if no drive was found - cdio_paranoia: faster cancellation - cdio_paranoia: don't scan for replay gain tags -* output - - pipewire: fix crash with PipeWire 0.3.53 * mixer - better error messages - alsa: fix setting volume before playback starts - pipewire: fix crash bug + - pipewire: fix volume change events with PipeWire 0.3.53 * support libfmt 9 ver 0.23.7 (2022/05/09) diff --git a/src/output/plugins/PipeWireOutputPlugin.cxx b/src/output/plugins/PipeWireOutputPlugin.cxx index d187a5bc9..cfa45292d 100644 --- a/src/output/plugins/PipeWireOutputPlugin.cxx +++ b/src/output/plugins/PipeWireOutputPlugin.cxx @@ -236,9 +236,11 @@ private: void ControlInfo([[maybe_unused]] uint32_t id, const struct pw_stream_control &control) noexcept { - if (control.name != nullptr && - StringIsEqual(control.name, "Channel Volumes")) + switch (id) { + case SPA_PROP_channelVolumes: OnChannelVolumes(control); + break; + } } static void ControlInfo(void *data, uint32_t id, From e807ed58704c29c3f894c7fb65442afdde4679b3 Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Fri, 8 Jul 2022 18:11:00 +0200 Subject: [PATCH 31/38] output/PipeWire: ignore SPA_PROP_channelVolumes if n_values==0 After connecting, PipeWire sometimes sends SPA_PROP_channelVolumes with no values, and this led to "volume=-NaN". --- src/output/plugins/PipeWireOutputPlugin.cxx | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/output/plugins/PipeWireOutputPlugin.cxx b/src/output/plugins/PipeWireOutputPlugin.cxx index cfa45292d..a64471a15 100644 --- a/src/output/plugins/PipeWireOutputPlugin.cxx +++ b/src/output/plugins/PipeWireOutputPlugin.cxx @@ -223,6 +223,9 @@ private: } void OnChannelVolumes(const struct pw_stream_control &control) noexcept { + if (control.n_values < 1) + return; + float sum = std::accumulate(control.values, control.values + control.n_values, 0.0f); From 02b00f9146a98d78b15eb36dfdba2b4879096add Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Fri, 8 Jul 2022 18:17:33 +0200 Subject: [PATCH 32/38] output/PipeWire: don't force initial volume=100% Closes https://github.com/MusicPlayerDaemon/MPD/issues/1484 --- NEWS | 1 + src/output/plugins/PipeWireOutputPlugin.cxx | 20 +++++++++++++------- 2 files changed, 14 insertions(+), 7 deletions(-) diff --git a/NEWS b/NEWS index 355281c61..4a9237bc8 100644 --- a/NEWS +++ b/NEWS @@ -10,6 +10,7 @@ ver 0.23.8 (not yet released) - alsa: fix setting volume before playback starts - pipewire: fix crash bug - pipewire: fix volume change events with PipeWire 0.3.53 + - pipewire: don't force initial volume=100% * support libfmt 9 ver 0.23.7 (2022/05/09) diff --git a/src/output/plugins/PipeWireOutputPlugin.cxx b/src/output/plugins/PipeWireOutputPlugin.cxx index a64471a15..524ab3d87 100644 --- a/src/output/plugins/PipeWireOutputPlugin.cxx +++ b/src/output/plugins/PipeWireOutputPlugin.cxx @@ -89,8 +89,12 @@ class PipeWireOutput final : AudioOutput { /** * The current volume level (0.0 .. 1.0). + * + * This get initialized to -1 which means "unknown", so + * restore_volume will not attempt to override PipeWire's + * initial volume level. */ - float volume = 1.0f; + float volume = -1; PipeWireMixer *mixer = nullptr; unsigned channels; @@ -669,12 +673,14 @@ PipeWireOutput::ParamChanged([[maybe_unused]] uint32_t id, if (restore_volume) { restore_volume = false; - try { - ::SetVolume(*stream, channels, volume); - } catch (...) { - FmtError(pipewire_output_domain, - FMT_STRING("Failed to restore volume: {}"), - std::current_exception()); + if (volume >= 0) { + try { + ::SetVolume(*stream, channels, volume); + } catch (...) { + FmtError(pipewire_output_domain, + FMT_STRING("Failed to restore volume: {}"), + std::current_exception()); + } } } From bc6924d3033346799e4daeb22b7b588c7587165f Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Fri, 8 Jul 2022 22:53:19 +0200 Subject: [PATCH 33/38] output/snapcast: fix busy loop while paused Removing the LockHasClients(); this code was copied from the "httpd" output plugin, but unlike "httpd", the SnapCast output plugin does not feed silence while paused, so we need to implement a delay to avoid busy-looping the CPU. As a side effect, this eliminates the suttering after resuming playback, because the timer now gets reset even if there is a client. Closes https://github.com/MusicPlayerDaemon/MPD/issues/1394 --- NEWS | 2 ++ src/output/plugins/snapcast/SnapcastOutputPlugin.cxx | 5 ++--- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/NEWS b/NEWS index 4a9237bc8..80737dc1c 100644 --- a/NEWS +++ b/NEWS @@ -5,6 +5,8 @@ ver 0.23.8 (not yet released) - cdio_paranoia: fix crash if no drive was found - cdio_paranoia: faster cancellation - cdio_paranoia: don't scan for replay gain tags + - snapcast: fix busy loop while paused + - snapcast: fix stuttering after resuming playback * mixer - better error messages - alsa: fix setting volume before playback starts diff --git a/src/output/plugins/snapcast/SnapcastOutputPlugin.cxx b/src/output/plugins/snapcast/SnapcastOutputPlugin.cxx index 1abe2a45a..acbb8f92d 100644 --- a/src/output/plugins/snapcast/SnapcastOutputPlugin.cxx +++ b/src/output/plugins/snapcast/SnapcastOutputPlugin.cxx @@ -214,9 +214,8 @@ SnapcastOutput::RemoveClient(SnapcastClient &client) noexcept std::chrono::steady_clock::duration SnapcastOutput::Delay() const noexcept { - if (!LockHasClients() && pause) { - /* if there's no client and this output is paused, - then Pause() will not do anything, it will not fill + if (pause) { + /* Pause() will not do anything, it will not fill the buffer and it will not update the timer; therefore, we reset the timer here */ timer->Reset(); From 6b430ba271f6d45b3f58c256cb8fede3b04f3b7a Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Sat, 9 Jul 2022 00:21:27 +0200 Subject: [PATCH 34/38] output/PipeWire: activate stream in Drain() --- NEWS | 1 + src/output/plugins/PipeWireOutputPlugin.cxx | 11 +++++++++++ 2 files changed, 12 insertions(+) diff --git a/NEWS b/NEWS index 80737dc1c..48ae6f8af 100644 --- a/NEWS +++ b/NEWS @@ -5,6 +5,7 @@ ver 0.23.8 (not yet released) - cdio_paranoia: fix crash if no drive was found - cdio_paranoia: faster cancellation - cdio_paranoia: don't scan for replay gain tags + - pipewire: fix playback of very short tracks - snapcast: fix busy loop while paused - snapcast: fix stuttering after resuming playback * mixer diff --git a/src/output/plugins/PipeWireOutputPlugin.cxx b/src/output/plugins/PipeWireOutputPlugin.cxx index 524ab3d87..ccdf5c0e4 100644 --- a/src/output/plugins/PipeWireOutputPlugin.cxx +++ b/src/output/plugins/PipeWireOutputPlugin.cxx @@ -866,6 +866,17 @@ PipeWireOutput::Drain() { const PipeWire::ThreadLoopLock lock(thread_loop); + if (drained) + return; + + if (!active) { + /* there is data in the ring_buffer, but the stream is + not yet active; activate it now to ensure it is + played before this method returns */ + active = true; + pw_stream_set_active(stream, true); + } + drain_requested = true; AtScopeExit(this) { drain_requested = false; }; From 493677ff81e708133f87e15157c5dd5131adad48 Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Sat, 9 Jul 2022 00:53:52 +0200 Subject: [PATCH 35/38] output/PipeWire: skip Cancel() if already drained --- src/output/plugins/PipeWireOutputPlugin.cxx | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/output/plugins/PipeWireOutputPlugin.cxx b/src/output/plugins/PipeWireOutputPlugin.cxx index ccdf5c0e4..7184edfa5 100644 --- a/src/output/plugins/PipeWireOutputPlugin.cxx +++ b/src/output/plugins/PipeWireOutputPlugin.cxx @@ -892,6 +892,9 @@ PipeWireOutput::Cancel() noexcept const PipeWire::ThreadLoopLock lock(thread_loop); interrupted = false; + if (drained) + return; + ring_buffer->reset(); } From 547a084c7ed95c09136159623240b7c92f6a2f5e Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Fri, 8 Jul 2022 23:38:45 +0200 Subject: [PATCH 36/38] output/PipeWire: call pw_stream_flush() in Cancel() Clear not only MPD's ring buffer, but also libpipewire's buffers, to avoid playing some audio from the previous song after a manual song change. Fixes part 1 of https://github.com/MusicPlayerDaemon/MPD/issues/1354 --- NEWS | 1 + src/output/plugins/PipeWireOutputPlugin.cxx | 5 +++++ 2 files changed, 6 insertions(+) diff --git a/NEWS b/NEWS index 48ae6f8af..8056bb7ba 100644 --- a/NEWS +++ b/NEWS @@ -6,6 +6,7 @@ ver 0.23.8 (not yet released) - cdio_paranoia: faster cancellation - cdio_paranoia: don't scan for replay gain tags - pipewire: fix playback of very short tracks + - pipewire: drop all buffers before manual song change - snapcast: fix busy loop while paused - snapcast: fix stuttering after resuming playback * mixer diff --git a/src/output/plugins/PipeWireOutputPlugin.cxx b/src/output/plugins/PipeWireOutputPlugin.cxx index 7184edfa5..0f8550d41 100644 --- a/src/output/plugins/PipeWireOutputPlugin.cxx +++ b/src/output/plugins/PipeWireOutputPlugin.cxx @@ -895,7 +895,12 @@ PipeWireOutput::Cancel() noexcept if (drained) return; + /* clear MPD's ring buffer */ ring_buffer->reset(); + + /* clear libpipewire's buffer */ + pw_stream_flush(stream, false); + drained = true; } bool From c8dae95eff60419fdff88d55400b6cbaacac137d Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Sat, 9 Jul 2022 00:59:35 +0200 Subject: [PATCH 37/38] output/PipeWire: after Cancel(), refill buffer before resuming playback Deactivate the stream in Cancel(). This fixes stuttering after a manual song change by refilling the whole ring buffer before reactivating the stream. Closes https://github.com/MusicPlayerDaemon/MPD/issues/1354 --- NEWS | 1 + src/output/plugins/PipeWireOutputPlugin.cxx | 9 +++++++++ 2 files changed, 10 insertions(+) diff --git a/NEWS b/NEWS index 8056bb7ba..141371245 100644 --- a/NEWS +++ b/NEWS @@ -7,6 +7,7 @@ ver 0.23.8 (not yet released) - cdio_paranoia: don't scan for replay gain tags - pipewire: fix playback of very short tracks - pipewire: drop all buffers before manual song change + - pipewire: fix stuttering after manual song change - snapcast: fix busy loop while paused - snapcast: fix stuttering after resuming playback * mixer diff --git a/src/output/plugins/PipeWireOutputPlugin.cxx b/src/output/plugins/PipeWireOutputPlugin.cxx index 0f8550d41..16febe909 100644 --- a/src/output/plugins/PipeWireOutputPlugin.cxx +++ b/src/output/plugins/PipeWireOutputPlugin.cxx @@ -901,6 +901,15 @@ PipeWireOutput::Cancel() noexcept /* clear libpipewire's buffer */ pw_stream_flush(stream, false); drained = true; + + /* pause the PipeWire stream so libpipewire ceases invoking + the "process" callback (we have no data until our Play() + method gets called again); the stream will be resume by + Play() after the ring_buffer has been refilled */ + if (active) { + active = false; + pw_stream_set_active(stream, false); + } } bool From 1f28790476f7bca935edec67eecae909741ff287 Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Sat, 9 Jul 2022 01:05:38 +0200 Subject: [PATCH 38/38] release v0.23.8 --- NEWS | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/NEWS b/NEWS index 141371245..54be560c5 100644 --- a/NEWS +++ b/NEWS @@ -1,4 +1,4 @@ -ver 0.23.8 (not yet released) +ver 0.23.8 (2022/07/09) * storage - curl: fix crash if web server does not understand WebDAV * input