From bd78307940ae7e36ea8e8c5958c0440db8853c04 Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Mon, 29 Jul 2024 23:10:46 +0200 Subject: [PATCH 1/7] input/{async,thread}: add an additional Cond field This eliminates the ScopeExchangeInputStreamHandler kludge. --- src/input/AsyncInputStream.cxx | 23 +++++++++++------------ src/input/AsyncInputStream.hxx | 6 ++++++ src/input/ThreadInputStream.cxx | 8 +++----- src/input/ThreadInputStream.hxx | 5 +++++ 4 files changed, 25 insertions(+), 17 deletions(-) diff --git a/src/input/AsyncInputStream.cxx b/src/input/AsyncInputStream.cxx index 4b21f3391..dcb35fd39 100644 --- a/src/input/AsyncInputStream.cxx +++ b/src/input/AsyncInputStream.cxx @@ -18,9 +18,7 @@ */ #include "AsyncInputStream.hxx" -#include "CondHandler.hxx" #include "tag/Tag.hxx" -#include "thread/Cond.hxx" #include "event/Loop.hxx" #include @@ -142,10 +140,7 @@ AsyncInputStream::Seek(std::unique_lock &lock, deferred_seek.Schedule(); - CondInputStreamHandler cond_handler; - const ScopeExchangeInputStreamHandler h(*this, &cond_handler); - cond_handler.cond.wait(lock, - [this]{ return seek_state == SeekState::NONE; }); + caller_cond.wait(lock, [this]{ return seek_state == SeekState::NONE; }); Check(); } @@ -162,6 +157,7 @@ AsyncInputStream::SeekDone() noexcept open = true; seek_state = SeekState::NONE; + caller_cond.notify_one(); InvokeOnAvailable(); } @@ -185,8 +181,6 @@ AsyncInputStream::Read(std::unique_lock &lock, { assert(!GetEventLoop().IsInside()); - CondInputStreamHandler cond_handler; - /* wait for data */ CircularBuffer::Range r; while (true) { @@ -196,8 +190,7 @@ AsyncInputStream::Read(std::unique_lock &lock, if (!r.empty() || IsEOF()) break; - const ScopeExchangeInputStreamHandler h(*this, &cond_handler); - cond_handler.cond.wait(lock); + caller_cond.wait(lock); } const size_t nbytes = std::min(read_size, r.size); @@ -219,8 +212,10 @@ AsyncInputStream::CommitWriteBuffer(size_t nbytes) noexcept if (!IsReady()) SetReady(); - else + else { + caller_cond.notify_one(); InvokeOnAvailable(); + } } void @@ -245,8 +240,10 @@ AsyncInputStream::AppendToBuffer(const void *data, size_t append_size) noexcept if (!IsReady()) SetReady(); - else + else { + caller_cond.notify_one(); InvokeOnAvailable(); + } } void @@ -258,6 +255,7 @@ AsyncInputStream::DeferredResume() noexcept Resume(); } catch (...) { postponed_exception = std::current_exception(); + caller_cond.notify_one(); InvokeOnAvailable(); } } @@ -280,6 +278,7 @@ AsyncInputStream::DeferredSeek() noexcept } catch (...) { seek_state = SeekState::NONE; postponed_exception = std::current_exception(); + caller_cond.notify_one(); InvokeOnAvailable(); } } diff --git a/src/input/AsyncInputStream.hxx b/src/input/AsyncInputStream.hxx index 3563c162f..4a6de1fda 100644 --- a/src/input/AsyncInputStream.hxx +++ b/src/input/AsyncInputStream.hxx @@ -21,6 +21,7 @@ #define MPD_ASYNC_INPUT_STREAM_HXX #include "InputStream.hxx" +#include "thread/Cond.hxx" #include "event/InjectEvent.hxx" #include "util/HugeAllocator.hxx" #include "util/CircularBuffer.hxx" @@ -41,6 +42,11 @@ class AsyncInputStream : public InputStream { InjectEvent deferred_resume; InjectEvent deferred_seek; + /** + * Signalled when the caller shall be woken up. + */ + Cond caller_cond; + HugeArray allocation; CircularBuffer buffer; diff --git a/src/input/ThreadInputStream.cxx b/src/input/ThreadInputStream.cxx index 030bb9ad8..de9bae5ae 100644 --- a/src/input/ThreadInputStream.cxx +++ b/src/input/ThreadInputStream.cxx @@ -18,7 +18,6 @@ */ #include "ThreadInputStream.hxx" -#include "CondHandler.hxx" #include "thread/Name.hxx" #include @@ -95,10 +94,12 @@ ThreadInputStream::ThreadFunc() noexcept nbytes = ThreadRead(w.data, w.size); } catch (...) { postponed_exception = std::current_exception(); + caller_cond.notify_one(); InvokeOnAvailable(); break; } + caller_cond.notify_one(); InvokeOnAvailable(); if (nbytes == 0) { @@ -136,8 +137,6 @@ ThreadInputStream::Read(std::unique_lock &lock, { assert(!thread.IsInside()); - CondInputStreamHandler cond_handler; - while (true) { if (postponed_exception) std::rethrow_exception(postponed_exception); @@ -155,8 +154,7 @@ ThreadInputStream::Read(std::unique_lock &lock, if (eof) return 0; - const ScopeExchangeInputStreamHandler h(*this, &cond_handler); - cond_handler.cond.wait(lock); + caller_cond.wait(lock); } } diff --git a/src/input/ThreadInputStream.hxx b/src/input/ThreadInputStream.hxx index 131c79d9a..38a90ca1d 100644 --- a/src/input/ThreadInputStream.hxx +++ b/src/input/ThreadInputStream.hxx @@ -56,6 +56,11 @@ class ThreadInputStream : public InputStream { */ Cond wake_cond; + /** + * Signalled when the caller shall be woken up. + */ + Cond caller_cond; + std::exception_ptr postponed_exception; HugeArray allocation; From 70a0a781c8581df7a3b29863ef736aab815bfa4b Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Wed, 29 Jan 2025 12:58:36 +0100 Subject: [PATCH 2/7] input/async: move the IsEOF() check to a separate block --- src/input/AsyncInputStream.cxx | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/input/AsyncInputStream.cxx b/src/input/AsyncInputStream.cxx index dcb35fd39..ec59680df 100644 --- a/src/input/AsyncInputStream.cxx +++ b/src/input/AsyncInputStream.cxx @@ -187,9 +187,12 @@ AsyncInputStream::Read(std::unique_lock &lock, Check(); r = buffer.Read(); - if (!r.empty() || IsEOF()) + if (!r.empty()) break; + if (IsEOF()) + return 0; + caller_cond.wait(lock); } From d7212624b099b9f4e39b39ac9189cdb5613ef5b3 Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Wed, 29 Jan 2025 12:54:42 +0100 Subject: [PATCH 3/7] input/{async,thread}: clear the CircularBuffer when it becomes empty First step towards fixing https://github.com/MusicPlayerDaemon/MPD/issues/2186 --- src/input/AsyncInputStream.cxx | 6 ++++++ src/input/ThreadInputStream.cxx | 8 ++++++++ 2 files changed, 14 insertions(+) diff --git a/src/input/AsyncInputStream.cxx b/src/input/AsyncInputStream.cxx index ec59680df..da41d3989 100644 --- a/src/input/AsyncInputStream.cxx +++ b/src/input/AsyncInputStream.cxx @@ -200,6 +200,12 @@ AsyncInputStream::Read(std::unique_lock &lock, memcpy(ptr, r.data, nbytes); buffer.Consume(nbytes); + if (buffer.empty()) + /* when the buffer becomes empty, reset its head and + tail so the next write can fill the whole buffer + and not just the part after the tail */ + buffer.Clear(); + offset += (offset_type)nbytes; if (paused && buffer.GetSize() < resume_at) diff --git a/src/input/ThreadInputStream.cxx b/src/input/ThreadInputStream.cxx index de9bae5ae..382ec8fdc 100644 --- a/src/input/ThreadInputStream.cxx +++ b/src/input/ThreadInputStream.cxx @@ -146,6 +146,14 @@ ThreadInputStream::Read(std::unique_lock &lock, size_t nbytes = std::min(read_size, r.size); memcpy(ptr, r.data, nbytes); buffer.Consume(nbytes); + + if (buffer.empty()) + /* when the buffer becomes empty, + reset its head and tail so the next + write can fill the whole buffer and + not just the part after the tail */ + buffer.Clear(); + wake_cond.notify_all(); offset += nbytes; return nbytes; From c0a9434f3414e42e5f94ea7c9ed5423a637f6e87 Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Wed, 29 Jan 2025 12:44:26 +0100 Subject: [PATCH 4/7] command/file: "albumart" tries to send larger chunks if available If we only receive very little data from the InputStream, try a second Read() call to get more data. This works around tiny reads at input buffer boundaries with the io_uring input plugin. These tiny reads are inefficient, and we can afford to wait one more low-level I/O iteration to finish (but not more). Closes https://github.com/MusicPlayerDaemon/MPD/issues/2186 --- NEWS | 2 ++ src/command/FileCommands.cxx | 13 +++++++++++++ 2 files changed, 15 insertions(+) diff --git a/NEWS b/NEWS index f39025288..75ce69e6d 100644 --- a/NEWS +++ b/NEWS @@ -1,4 +1,6 @@ ver 0.23.17 (not yet released) +* protocol + - "albumart" tries to send larger chunks if available * storage - nfs: require libnfs 4.0 or later * database diff --git a/src/command/FileCommands.cxx b/src/command/FileCommands.cxx index e28cc2eb5..fbf2ee70a 100644 --- a/src/command/FileCommands.cxx +++ b/src/command/FileCommands.cxx @@ -215,7 +215,20 @@ read_stream_art(Response &r, const std::string_view art_directory, if (buffer_size > 0) { std::unique_lock lock(is->mutex); is->Seek(lock, offset); + + const bool was_ready = is->IsReady(); + read_size = is->Read(lock, buffer.get(), buffer_size); + + if (was_ready && read_size < buffer_size / 2) + /* the InputStream was ready before, but we + got only very little data; probably just + some data left in the buffer without doing + any I/O; let's wait for the next low-level + read to complete to get more data for the + client */ + read_size += is->Read(lock, buffer.get() + read_size, + buffer_size - read_size); } r.Fmt("size: {}\n", art_file_size); From 8fcb6e148f71380f54233c0f7b153afb95ee1dde Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Wed, 29 Jan 2025 15:35:53 +0100 Subject: [PATCH 5/7] decoder/list: probe "ffmpeg" before "sndfile" and "audiofile" For FFmpeg's DTS-WAV support, see code comment. Closes https://github.com/MusicPlayerDaemon/MPD/issues/2158 --- NEWS | 2 ++ src/decoder/DecoderList.cxx | 19 +++++++++++++------ 2 files changed, 15 insertions(+), 6 deletions(-) diff --git a/NEWS b/NEWS index 75ce69e6d..123e5e886 100644 --- a/NEWS +++ b/NEWS @@ -5,6 +5,8 @@ ver 0.23.17 (not yet released) - nfs: require libnfs 4.0 or later * database - inotify: trigger update after symlink was created +* decoder + - ffmpeg: prefer over sndfile and audiofile for its DTS-WAV support * support libfmt 11.1 ver 0.23.16 (2024/12/03) diff --git a/src/decoder/DecoderList.cxx b/src/decoder/DecoderList.cxx index 965380089..0504365b6 100644 --- a/src/decoder/DecoderList.cxx +++ b/src/decoder/DecoderList.cxx @@ -73,12 +73,6 @@ constexpr const struct DecoderPlugin *decoder_plugins[] = { #ifdef ENABLE_OPUS &opus_decoder_plugin, #endif -#ifdef ENABLE_SNDFILE - &sndfile_decoder_plugin, -#endif -#ifdef ENABLE_AUDIOFILE - &audiofile_decoder_plugin, -#endif #ifdef ENABLE_DSD &dsdiff_decoder_plugin, &dsf_decoder_plugin, @@ -120,6 +114,19 @@ constexpr const struct DecoderPlugin *decoder_plugins[] = { #ifdef ENABLE_FFMPEG &ffmpeg_decoder_plugin, #endif + + /* these WAV-decoding plugins are below ffmpeg_decoder_plugin + to give FFmpeg a chance to decode DTS-WAV files which is + technically DTS Coherent Acoustics (DCA) stream wrapped in + fake 16-bit stereo samples; neither libsndfile nor + libaudiofile detect this, but FFmpeg does */ +#ifdef ENABLE_SNDFILE + &sndfile_decoder_plugin, +#endif +#ifdef ENABLE_AUDIOFILE + &audiofile_decoder_plugin, +#endif + &pcm_decoder_plugin, nullptr }; From 0e8cd3b961a29058ee5f4d507170243d237994ef Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Wed, 29 Jan 2025 16:07:39 +0100 Subject: [PATCH 6/7] client/Process: explicitly disallow "idle" and "noidle" in command lists These commands cannot possibly work with command lists because command lists are supposed to be atomic, but suspended command execution conflicts with that. Closes https://github.com/MusicPlayerDaemon/MPD/issues/2167 --- NEWS | 1 + doc/protocol.rst | 3 +++ src/client/Process.cxx | 14 ++++++++++++++ 3 files changed, 18 insertions(+) diff --git a/NEWS b/NEWS index 123e5e886..9690d84a0 100644 --- a/NEWS +++ b/NEWS @@ -1,6 +1,7 @@ ver 0.23.17 (not yet released) * protocol - "albumart" tries to send larger chunks if available + - explicitly disallow "idle" and "noidle" in command lists * storage - nfs: require libnfs 4.0 or later * database diff --git a/doc/protocol.rst b/doc/protocol.rst index 6c105c3e2..2677f5f83 100644 --- a/doc/protocol.rst +++ b/doc/protocol.rst @@ -156,6 +156,9 @@ fails, no more commands are executed and the appropriate ``list_OK`` is returned for each successful command executed in the command list. +Only synchronous commands can be used in command lists. Commands that +suspend execution (``idle`` and ``noidle``) are not allowed. + Ranges ====== diff --git a/src/client/Process.cxx b/src/client/Process.cxx index 041e339c7..69bd78862 100644 --- a/src/client/Process.cxx +++ b/src/client/Process.cxx @@ -52,6 +52,13 @@ Client::ProcessCommandList(bool list_ok, return CommandResult::OK; } +[[gnu::pure]] +static bool +IsAsyncCommmand(const char *line) noexcept +{ + return StringIsEqual(line, "idle") || StringIsEqual(line, "noidle"); +} + CommandResult Client::ProcessLine(char *line) noexcept { @@ -67,6 +74,13 @@ Client::ProcessLine(char *line) noexcept return CommandResult::CLOSE; } + if (cmd_list.IsActive() && IsAsyncCommmand(line)) { + FmtWarning(client_domain, + "[{}] not possible in comand list: \"{}\"", + num, line); + return CommandResult::CLOSE; + } + if (StringIsEqual(line, "noidle")) { if (idle_waiting) { /* send empty idle response and leave idle mode */ From b080ca8627c2ae1e6d5fec158b3e1aaaff2791bd Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Wed, 29 Jan 2025 17:11:53 +0100 Subject: [PATCH 7/7] release v0.23.17 --- NEWS | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/NEWS b/NEWS index 9690d84a0..ee9eddaa3 100644 --- a/NEWS +++ b/NEWS @@ -1,4 +1,4 @@ -ver 0.23.17 (not yet released) +ver 0.23.17 (2025/01/29) * protocol - "albumart" tries to send larger chunks if available - explicitly disallow "idle" and "noidle" in command lists