MusicPlayerDaemon
First implementation of the `visualization` output plugin
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.
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
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 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?
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.