First implementation of the `visualization` output plugin #1910

sp1ff · 2023-12-10T01:59:49Z

NOTE TO REVIEWERS: this was discussed previously, but life got busy.

This commit contains the first implementation of a visualization output plugin. It servers a very simple network protocol, backed by a very simple audio analysis. I'm using it as my daily driver, but it needs review & hardening.

Update: Converting this to a draft while I work through the CI issues. Will squash all commits before marking "ready for review".

Update #2: CI passing, unit test suite considerably beefed-up. Using this build as my daily driver. @MaxKellermann ready for review.

MaxKellermann · 2023-12-10T07:12:13Z

Hey, I'm happy you continued that effort. That is certainly a useful feature for many MPD users.
I hope I can take time for a proper review soon, but meanwhile, please check out your coding style:

indent with tabs! Your code tabs and spaces at random
return type should be on its own line as the length of C++ types can get out of hand and this would add indentation to all parameters
for multi-line comments, use multi-line comment syntax (/* ... */) and not muliple instances of the single-comment syntax (//)
MPD has meanwhile switched to SPDX copyright headers, please do the same for your new code
in some comments, you have non-ASCII emojis - I prefer to leave the code plain ASCII, except if there are good reasons to use Unicode, but those emojis don't look particularly useful
functions/method/structs should be CamelCase, same for source file names
if a function never throws, decorate it with noexcept
top-level definitions (e.g. structs) have pretty generic names in our code, but they may conflict with structs of the same name in other compilation units, conflicting each other's symbol namespaces; better use prefixed names, or even better: put everything in a C++ namespace

MaxKellermann · 2023-12-10T07:13:54Z

src/output/plugins/visualization/VisualizationServer.cxx

+	for (auto p0 = clients.begin(), p1 = clients.end(); p0 != p1; ) {
+		auto p = p0++;
+		if ((*p)->IsClosed()) {
+			LogInfo(vis_server_domain, "Reaping closed client.");
+			clients.erase(p);
+		}


This loops endlessly if at least one client isn't closed, doesn't it?

I don't believe so... the iterator p0 is unconditionally incremented on line 53, so the iteration will always make progress whether or not anyone is closed.

MaxKellermann · 2023-12-10T07:14:41Z

src/output/plugins/visualization/VisualizationServer.hxx

+	/// Clients have both a reference to the PCM cache as well as a
+	/// SoundAnalysis instance while the plugin is opened. We'll create new
+	/// clients with our present state
+	std::list<std::unique_ptr<VisualizationClient>> clients;


What's the point of wrapping the item type in unique_ptr? All this does is add a layer of indirection, doesn't it?

Yes. I wrapped it because VisualizationClient, being a Buffered Socket is not copy constructable. Your comment made me realize I could emplace elements in the list rather than push them. I've removed the unique_ptr.

MaxKellermann · 2023-12-10T07:15:07Z

src/output/plugins/visualization/VisualizationServer.hxx

+	/// clients with our present state
+	std::list<std::unique_ptr<VisualizationClient>> clients;
+	/// invoked periodically to clean-up dead clients
+	CoarseTimerEvent reaper;


Why have a timer, why not remove dead clients right away?

In order to remove dead clients right away, the server would need a means of notification. Other plugins use a "back reference" from client to server as this means. This, however, introduces a cyclic dependency (server owns clients, clients have a reference to server) between the two, which I prefer to avoid.
I wrote-up my thoughts in VisualizationOutputPlugin.cxx, line 415 (sorry, can't get GitHub to link directly to it).

MaxKellermann · 2023-12-10T07:16:50Z

src/output/plugins/visualization/protocol.hxx

+
+    using std::byte;
+
+    constexpr byte zero = byte{0};


This coding style looks clumsy. This is easier:

constexpr byte zero{0};

Why the redundant byte?

Indeed-- changed.

MaxKellermann · 2023-12-10T07:17:53Z

src/output/plugins/visualization/protocol.hxx

+    *pout++ = h2;
+    *pout++ = h3;
+
+    *pout++ = sixteen;


I don't even get why you declare symbolic constants when all the name does is describe the integer value in numbers. Why not just:

*pout++ = std::byte{16};

I try to avoid literals in code (well, other than their declaration). Perhaps I've adhered too literally to what was meant to be a general guideline. Changed.

MaxKellermann · 2023-12-10T07:19:17Z

src/output/plugins/visualization/VisualizationServer.cxx

+	FmtInfo(vis_server_domain, "VisualizationServer::OnAccept({})", gettid());
+
+    // Can we allow an additional client?
+	if (max_clients && clients.size() > max_clients) {


Off-by-one error? This allows max_clients+1 clients.

Yes-- fixed.

MaxKellermann · 2023-12-10T07:20:42Z

src/output/plugins/visualization/SoundInfoCache.hxx

+	/// ring buffer size/capacity, in bytes
+	std::size_t cb_ring;
+	/// this is the ring buffer
+	std::unique_ptr<uint8_t[]> ring;


MPD has class AllocatedArray which wraps these two fields nicely - it could be used to simplify your code.

Took a quick read over class AllocatedArray. Can you tell me what it offers above & beyond unique_ptr<uint8_t []>?

It is mostly the same as unique_ptr, but also remembers the size of the array allocation. Thus, you have one field instead of two.

MaxKellermann · 2023-12-10T07:21:47Z

src/output/plugins/visualization/SoundInfoCache.hxx

+	typedef std::chrono::steady_clock::duration Duration;
+	typedef std::chrono::microseconds Time;


I don't get these two types. One is called "duration" and it represents a duration. The other is called "time" and it represents ..... a time? Huh?

IME "time" generally represents a singular moment in time; 07:23:00 December 17, 2023, for instance. A duration is the span of time between two such moments; 10 seconds, e.g.
Do you think I should rename one or both types?

In C++-speak, what you call "time" is a "time point", and C++ has a dedicated type for that (called time_point). You however (ab)use the "duration" type to express a time point. But how can you express a time point in milliseconds? What's the reference time point? It's not documented in your code.

Ahhhhh... I completely missed that. Thank you. Fixed.

MaxKellermann · 2023-12-10T07:22:12Z

src/output/plugins/visualization/SoundInfoCache.hxx

+	/// # of bytes currently in the ring buffer (as distinct from capacity)
+	std::size_t cb;
+	/// Valid PCM data exists in buf[p0, p1)
+	ptrdiff_t p0, p1;


Why is this a signed integer?

No good reason; must have been an artifact of a first implementation. Changed, in any event.

MaxKellermann · 2023-12-20T10:10:23Z

When you fix PR commits, please amend them instead of adding fixup commits. I don't want to merge known-bad commits; a PR is "good" when all individual commits are "good".

MaxKellermann · 2023-12-20T10:11:38Z

src/output/plugins/meson.build

+libfftw3_dep = dependency('fftw3f', version: '>= 3.3.10', required: get_option('fftw3'))
+output_features.set('ENABLE_FFTW3', libfftw3_dep.found())
+output_features.set('ENABLE_VISUALIZATION_OUTPUT', get_option('visualization'))
+if get_option('visualization') and libfftw3_dep.found()


This logic seems somewhat wrong. The user asks Meson to enable visualization, but then Meson doesn't find libfftw and disables the plugin, even though the user's request could not be fulfilled.

Good point.

Now we have two Meson options: one to enable "FFTW support" - but what does it mean, what does it do?
Second option is "Visualization output plugin", I understand that. So I disable it, but MPD still looks for libfftw - why?
The I enable "visualization", but this still depends on the "fftw" options where I can have contradicting values.
The simples solution would be to have one single option, let's say "visualization", which is used as "required" parameter for looking up libfftw. This would be less obscure and there wouldn't be a way to have contradicting options.

MaxKellermann · 2023-12-20T10:14:13Z

src/output/plugins/visualization/LowLevelProtocol.hxx

+#include <algorithm>
+#include <cstdint>
+
+#include <netinet/in.h>


That header isn't portable, is it?
I suggest using MPD's util/ByteOrder.hxx header, which is portable and even faster!
It also has a PackedBE32 class which you can use to write big-endian 32 bit integers to a byte stream (without reverting to std::copy()).

Huh... I had no idea (I only work on Linux). Replaced... but I don't see how to use PackedBE{16,32} to copy the raw bytes into a buffer without recourse to std::copy (or its equivalent)?

Since the Packed classes are just bytes, you can simply cast a pointer to PackedBE16* and access the object from there.

MaxKellermann · 2023-12-20T10:15:52Z

src/output/plugins/visualization/LowLevelProtocol.hxx

+	return std::copy(p, p + 2, pout);
+}
+
+/* Convert an IEEE 754 single-precision floating-point number to wire format;


Please add a static_assert to ensure that this platform really uses IEEE754 (see https://en.cppreference.com/w/cpp/types/numeric_limits/is_iec559). All commonly used do, but I want this to be safe just in case somebody compiles MPD on some very exotic future CPU architecture.

MaxKellermann · 2023-12-20T10:16:02Z

src/output/plugins/visualization/LowLevelProtocol.hxx

+template <typename OutIter>
+OutIter
+SerializeFloat(float f, OutIter pout) {
+	auto m = htonl(*(uint32_t*)&f);


This cast is undefined behavior, isn't it?

Casting one object type to another is undefined behavior in C++ (there are a few exceptions).

MaxKellermann · 2023-12-20T10:16:37Z

src/output/plugins/visualization/Protocol.hxx

+};
+
+/**
+ * \brief Attempt to parse a CLIHLO message from the given buffer


What is CLIHLO?

Added references to the message documentation.

MaxKellermann · 2023-12-20T10:23:07Z

src/output/plugins/visualization/SoundAnalysis.cxx

+
+	size_t max_coeffs_idx = num_samples/2;
+
+	for (uint8_t c = 0; c < num_channels; ++c) {


Using uint8_t here instead of unsigned has no effect at best, and can add overhead and code bloat. It's a harmful micro-optimization.

Wasn't intended as an optimization or any sort; just chose the type to match that of num_channels. Changed, but can you say more about how making c a uint8_t could add overhead & code bloat?

MaxKellermann · 2023-12-20T10:23:56Z

src/output/plugins/visualization/SoundAnalysis.hxx

+	static constexpr size_t DEF_LO_CUTOFF = 200;
+	static constexpr size_t DEF_HI_CUTOFF = 10000;


What does DEF_ mean and why is this a size_t when the non-static fields with similar names are float?

Short for "default"-- re-named. They're integral because ConfigBlock offers no accessors for floating-point values.

MaxKellermann · 2023-12-20T10:24:14Z

src/output/plugins/visualization/SoundAnalysis.hxx

+
+public:
+	SoundAnalysisParameters() noexcept;
+	SoundAnalysisParameters(const ConfigBlock &config_block);


Should be explicit to avoid implicit unwanted conversions.

MaxKellermann · 2023-12-20T10:24:42Z

src/output/plugins/visualization/SoundAnalysis.hxx

+		/* In both `out` & `pout`, the coefficients are laid out as:
+		 * 	   | coeffs for chan #0... | coeffs for chan #1... | ... |
+		 * so outer loop will be on channel. */
+		for (uint8_t chan = 0; chan < num_channels; ++chan) {


Another bad micro-optimization by using uint8_t

MaxKellermann · 2023-12-20T10:25:05Z

src/output/plugins/visualization/SoundInfoCache.cxx

+}
+
+Visualization::SoundInfoCache::SoundInfoCache(const AudioFormat &audio_format,
+											  const Duration &buf_span):


This commit contains the first implementation of an output plugin that streams sound analysis to clients (presumably visualizers of some sort).

MaxKellermann · 2024-03-11T11:57:03Z

.github/workflows/build_android.yml

@@ -40,7 +40,7 @@ jobs:
          sudo apt-get update
          sudo apt-get install -y --no-install-recommends \
            ninja-build \
-            quilt 
+            quilt


MaxKellermann · 2024-03-11T11:57:32Z

src/lib/fmt/ThreadIdFormatter.hxx

+#define THREAD_ID_FORMATTER_HXX
+
+#include <fmt/format.h>
+#include <sstream>


Please please please no iostreams in MPD!
Why do we even need this whole header??

MaxKellermann · 2024-03-11T11:57:53Z

src/output/plugins/meson.build

@@ -160,6 +160,28 @@ else
  wasapi_dep = dependency('', required: false)
 endif

+libfftw3_dep = dependency('fftw3f', version: '>= 3.3.8', required: get_option('fftw3'))
+output_features.set('ENABLE_FFTW3', libfftw3_dep.found())


This macro is never used

MaxKellermann

I read only part of the code, but found so many things that were wrong or more complicated than they should be, I stopped reading further.

MaxKellermann · 2024-03-11T11:58:43Z

src/output/plugins/visualization/LowLevelProtocol.hxx

+	return std::copy(p, p + 2, pout);
+}
+
+static_assert(std::numeric_limits<float>::is_iec559);


It is not clear why this line exists - if you move it into the function that relies on this assertion, it would be clearer.

MaxKellermann · 2024-03-11T11:59:46Z

src/output/plugins/visualization/LowLevelProtocol.hxx

+template <typename OutIter>
+OutIter
+SerializeU16(uint16_t n, OutIter pout) {
+	auto m = PackedBE16(n);


Weird coding style. PackedBE16 m{n} is simpler.

MaxKellermann · 2024-03-11T12:01:35Z

src/output/plugins/visualization/LowLevelProtocol.hxx

+ */
+template <typename OutIter>
+OutIter
+SerializeU16(uint16_t n, OutIter pout) {


What's the reason you decided to make this a template instead of just passing a std::byte*?

MaxKellermann · 2024-03-11T12:02:01Z

src/output/plugins/visualization/LowLevelProtocol.hxx

+SerializeU16(uint16_t n, OutIter pout) {
+	auto m = PackedBE16(n);
+	auto p = (std::byte*)(&m);
+	return std::copy(p, p + 2, pout);


Using std::copy after creating a copy on the stack seems pretty clumsy. Now we have three copies of the value. Can we do better than that?

MaxKellermann · 2024-03-11T12:02:54Z

src/output/plugins/visualization/LowLevelProtocol.hxx

+	auto r = PackedBE32(*(const uint32_t*)&(c[0]));
+	auto i = PackedBE32(*(const uint32_t*)&(c[1]));


Yet more undefined behavior here. Why are you not calling SerializeFloat here twice? Too simple?

MaxKellermann · 2024-03-11T12:16:23Z

src/output/plugins/visualization/VisualizationClient.hxx

+		/* FSM initial state; the socket has been established, but no
+		 * communication has taken place; we are expecting a CLIHLO
+		 * message to arrive (i.e. a READ/POLLIN notification) */
+		Init,


enum choices should be UPPER_CASE

MaxKellermann · 2024-03-11T12:18:45Z

src/output/plugins/visualization/VisualizationClient.cxx

+
+inline
+typename std::chrono::microseconds::rep
+NowTicks() {


This function is only used for your excessive debug prints, but those should be removed before I merge this. What's even the point of this function, why do you insert those time stamps in all log messages?

MaxKellermann · 2024-03-11T12:20:54Z

src/output/plugins/visualization/VisualizationServer.cxx

+	if (!clients.empty()) {
+		LogInfo(vis_server_domain, "Scheduling another reaping in 3 "
+			"seconds.");
+		reaper.Schedule(std::chrono::seconds(3));


This periodically calls ReapClients, all the time, while MPD runs! This is bad, because it causes unnecessary CPU wakeups. Never wake up the CPU unless you explicitly have a reason to do so.
I also don't understand the point of this whole method. Why does it exist? If a client is closed, why not delete it immediately?

MaxKellermann · 2024-03-11T12:23:41Z

src/output/plugins/visualization/VisualizationServer.cxx

+{
+	state = HavePcmData{pcache };
+
+	for (auto p0 = clients.begin(), p1 = clients.end(); p0 != p1; ) {


This iteration happens in the OutputThread, but other methods happen in the i/o thread (via EventLoop), thus all methods in this class may crash all the time.

MaxKellermann · 2024-03-11T12:24:26Z

src/output/plugins/visualization/VisualizationServer.hxx

+	/// only valid when the plugin is open
+	struct HavePcmData {
+		// I wish C++ had a `not_null` class
+		std::shared_ptr<SoundInfoCache> pcache;


Why is this a shared_ptr? What's the point of adding reference counting overhead here?

sp1ff · 2024-03-26T23:36:56Z

@MaxKellermann This isn't going well. Before I embark on another round of low-level changes (addressing "excessive logging", for instance), I think it might be beneficial to first verify that that basic architecture of the plugin is acceptable (not much use updating code that's going to need to be re-written, after all). I've documented that at some length here, where I hope you'll find the answers to some of your questions above.

In the interests of time, if you do want changes, it would help me if you could indicate what you'd like to see instead of what I've written. I realize you've done that in some cases, but the review so far contains many open-ended questions (such as "why?"). When reviewing others' code, I've gotten better results with concrete suggestions.

Finally, and in particular, could you tell me a bit about casting being undefined behavior? When, e.g. I cast a sixteen bit value to a uint16_t I expect to get the bit pattern. This has worked for me on multiple platforms, in both kernel & user mode, for nearly three decades now; that said, I'm always interesting in honing my craft. Could you say more?

MaxKellermann · 2024-04-03T21:38:07Z

contains many open-ended questions (such as "why?"). When reviewing others' code, I've gotten better results with concrete suggestions.

When I see weird code, I want to know why you wrote it that way. I assume you have good reasons which are just outside of my grasp. So I ask "why".

Maybe you don't have good reasons, but only after I find that out I can make suggestions.

could you tell me a bit about casting being undefined behavior?

You mustn't cast an unaligned pointer to a pointer to a 16 bit integer (and then dereference it). This works on most CPUs, but may crash on others (with SIGBUS) - but for certain the C++ language does not allow it. Maybe you got away with that mistake for three decades, but that doesn't mean it's correct and doesn't mean it will never crash for anybody. Let's write the code to be correct, maybe more correct than it is necessary for the CPUs you happen to have been using for the last three decades, hoping that MPD will remain stable for everybody.

MaxKellermann · 2024-05-06T11:09:52Z

Is this going anywhere?

MaxKellermann reviewed Dec 10, 2023

View reviewed changes

sp1ff requested a review from MaxKellermann December 19, 2023 00:55

MaxKellermann requested changes Dec 20, 2023

View reviewed changes

sp1ff force-pushed the soundinfo branch 3 times, most recently from 173d4a8 to 1c7c4e4 Compare January 11, 2024 01:45

sp1ff requested a review from MaxKellermann January 11, 2024 01:50

sp1ff force-pushed the soundinfo branch 2 times, most recently from 7f52ae9 to 119c0d2 Compare February 19, 2024 23:56

sp1ff marked this pull request as draft February 20, 2024 00:04

sp1ff force-pushed the soundinfo branch 3 times, most recently from 7116d35 to 64d8629 Compare March 3, 2024 02:17

First implementation of the visualization output plugin.

a25ae7f

This commit contains the first implementation of an output plugin that streams sound analysis to clients (presumably visualizers of some sort).

sp1ff force-pushed the soundinfo branch from 64d8629 to a25ae7f Compare March 5, 2024 21:27

sp1ff marked this pull request as ready for review March 5, 2024 21:39

MaxKellermann reviewed Mar 11, 2024

View reviewed changes

MaxKellermann requested changes Mar 11, 2024

View reviewed changes

MaxKellermann added the waiting Waiting for more information from reporter label May 6, 2024

		typedef std::chrono::steady_clock::duration Duration;
		typedef std::chrono::microseconds Time;


		size_t max_coeffs_idx = num_samples/2;

		for (uint8_t c = 0; c < num_channels; ++c) {

		static constexpr size_t DEF_LO_CUTOFF = 200;
		static constexpr size_t DEF_HI_CUTOFF = 10000;

		auto r = PackedBE32((const uint32_t)&(c[0]));
		auto i = PackedBE32((const uint32_t)&(c[1]));

First implementation of the visualization output plugin #1910

Are you sure you want to change the base?

First implementation of the visualization output plugin #1910

Conversation

sp1ff commented Dec 10, 2023 • edited

MaxKellermann commented Dec 10, 2023

Choose a reason for hiding this comment

sp1ff Dec 17, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sp1ff Dec 17, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sp1ff Dec 17, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sp1ff Dec 17, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaxKellermann commented Dec 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaxKellermann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sp1ff commented Mar 26, 2024

MaxKellermann commented Apr 3, 2024

MaxKellermann commented May 6, 2024

First implementation of the `visualization` output plugin #1910

First implementation of the `visualization` output plugin #1910

sp1ff commented Dec 10, 2023 •

edited

sp1ff Dec 17, 2023 •

edited

sp1ff Dec 17, 2023 •

edited

sp1ff Dec 17, 2023 •

edited

sp1ff Dec 17, 2023 •

edited