decoder/psgplay: Support Atari SNDH files via PSG play library#2444
decoder/psgplay: Support Atari SNDH files via PSG play library#2444frno7 wants to merge 1 commit intoMusicPlayerDaemon:masterfrom
Conversation
|
This PSG play MPD plugin for Atari ST and STE SNDH chip music is very similar to the preexisting SID play MPD plugin for C64 chip music. Note! Although the dependency in |
|
Unfortunately, your library is not compatible with C++: I'm unable to build MPD with your code. |
What C++ compiler and version are you using? I’m able to successfully compile it with as demonstrated with |
|
My compiler is the standard GCC from Debian stable: But this really isn't about the compiler. The code in your library headers is not valid C++. |
|
I removed lots of useless code from your library headers and then your headers did build in C++ mode. Turns out that all these C++-incompatible defintions in your headers are unused. I say they simply do not belong there; they should not be part of the public API of your library. |
Indeed, but I’d like to be able to reproduce these errors, to verify I’ve fixed them properly. :-) Without being able to reproduce the error, I’m in the blind. Both our compilers are GCC 14, but mine is slightly newer (14.3) than yours (14.2). Have you configured your build environment in any way? The only fix for C++ and GCC 14.3.1 I had to do last week (before submitting this pull request) was commit frno7/psgplay@8ed589e, which presumably you’ve already got. The problem with the GCC 14.2.0 C++ compiler refusing anonymous structs and flexible array members certainly look fixable. |
|
What was your meson setup command? |
Heh, well, MPD certainly doesn’t use the whole public API. As briefly mentioned about the API, apart from the ‘simple’ stereo audio API (that the MPD plugin uses), there’s also a more advanced, low-level, 250 kHz ‘digital’ API that more directly interfaces the sound chip YM2149 hardware, in case people want to make their own custom analogue filters, display popular three-channel oscilloscope effects in their players, etc. (hence the errors from
I followed the official MPD guide on compiling from source, with the addition that I had |
|
|
||
| #include <fmt/format.h> | ||
|
|
||
| #include <fstream> |
There was a problem hiding this comment.
Using iostreams is not acceptable in MPD. iostreams is a horrible library that adds huge amounts of bloat, both to the code and to runtime.
There was a problem hiding this comment.
What file reading (and/or perhaps streaming?) operation would you recommend as a replacement within the MPD plugin? The PSG play library itself doesn’t have any functions to read (nor write) any files: it needs a (complete) array of bytes containing the SNDH music data.
There was a problem hiding this comment.
MPD has two APIs to read files:
FileReaderis a low-level wrapper for a file descriptor/handleInputStreamis the generic streaming API that MPD uses to access both local files, remote files (NFS, WebDav etc.) and actual streams (HTTP, HLS)
Since there appears to be no way around reading the whole file, I suggest you switch to MPD's stream decoder API, because that will allow your plugin to read files from anywhere, not just local files.
But instead of InputStream::Read(), decoders should call the decoder_read*() wrappers because they make the read operation cancellable (if the user presses "stop", it will cancel the read instead of having to wait until your NFS server returns all the data).
There was a problem hiding this comment.
I suggest you switch to MPD's stream decoder API
Other, preexisting, plugins (GME, SID, VGM), using the WithContainer concept (taking Path rather than InputStream), don’t seem to use InputStream? They also manipulate Path objects to suffix /tune_xxx.* subtunes, etc. Is a stream’s URI the closest thing to a path for the purpose of making containers (having subtunes)?
decoders should call the
decoder_read*()wrappers
It seems a (new) decoder_read_whole() would be useful, to read and return a complete file via InputStream, also for the preexisting SID plugin (which currently doesn’t seem to be streamable)?
| static unsigned | ||
| psgplay_subtune_track(const char *base) noexcept | ||
| { | ||
| if (memcmp(base, SUBTUNE_PREFIX, sizeof(SUBTUNE_PREFIX) - 1) != 0) |
There was a problem hiding this comment.
Out-of-bounds read here. Do you know how memcmp() works?
I suggest using StringStartsWith(), or better: StringAfterPrefix(). It does what you want, but without the out-of-bounds read and simpler, less error prone.
There was a problem hiding this comment.
This code is exactly what the preexisting MPD SID play plugin already has:
MPD/src/decoder/plugins/SidplayDecoderPlugin.cxx
Lines 136 to 140 in 94a02d4
I agree it isn’t pretty, but there’s value in using the same construct for exactly the same purpose in both plugins, isn’t it?
There was a problem hiding this comment.
Oh! This piece of code is indeed just as bad and should be rewritten.
| if (!stream) | ||
| return { }; | ||
|
|
||
| return std::vector<uint8_t>((std::istreambuf_iterator<char>(stream)), |
There was a problem hiding this comment.
What if the file weighs one terabyte? This would certainly crash MPD (and make your computer unusable for an hour or so).
Also std::vector is a lousy choice here. If you read even just a megabyte, it will first zero-initialize the whole allocation and then read into it. The initialization is useless bloat.
Is there is reason you need to slurp the whole file into memory? Can't you stream the file, like the other decoders do?
If you really need to do that, use AllocatedArray<std::byte> (std::byte is the canonical type for bytes).
There was a problem hiding this comment.
What if the file weighs one terabyte?
Atari hardware only had a 24-bit address bus, limiting addressable space to 16 MiB (at most). We can have that as a software limit in the plugin, if you like?
Is there is reason you need to slurp the whole file into memory? Can't you stream the file, like the other decoders do?
As mentioned on how playing SNDH works, the SNDH file format is an Atari ST machine code executable form of music. A substantial part of Atari ST hardware must be emulated to play such files using other kinds of computers. This is exactly the same principle as the preexisting SID play plugin. The ‘music’ is 68000 machine code, and so it’s not made to be ‘streamed’ in a conventional way (with a ‘beginning’ or ‘end’ to files).
There was a problem hiding this comment.
Sounds like a reasonable limit.
| std::ios::in | std::ios::binary); | ||
|
|
||
| if (!stream) | ||
| return { }; |
There was a problem hiding this comment.
Bad error handling. If you can't open a file, throw an exception (that will be handled/logged somewhere down the call stack), but don't fail silently.
This function isn't documented; there's no documentation saying that errors return an empty vector (and I don't think this is a good idea).
| { | ||
| const auto container = psgplay_container_from_path(path_fs); | ||
|
|
||
| const AudioFormat audio_format(44100, SampleFormat::S16, 2); |
There was a problem hiding this comment.
This can be static constexpr; that would eliminate the code to allocate stack space and initialize it completely, and instead be just a few passive bytes in .rodata.
| enum { N = 4096 }; | ||
| struct psgplay_stereo buffer[N]; | ||
|
|
||
| /* psgplay_read_stereo returns the number of samples */ |
There was a problem hiding this comment.
Is this (PCM) samples or (stereo) frames?
One (stereo) frame consists of two PCM samples.
There was a problem hiding this comment.
The return value is the number of stereo sample pairs, which is to say, the number of MPD stereo frames. I can clarify the documentation to reflect this.
There was a problem hiding this comment.
Yes, please. Optimizing for minimum confusion is always worth the effort.
| container.track, | ||
| audio_format.sample_rate); | ||
| if (!pp) | ||
| return; |
There was a problem hiding this comment.
This calls psgplay_free(nullptr) which is illegal according to your API documentation.
There was a problem hiding this comment.
This calls
psgplay_free(nullptr)which is illegal according to your API documentation.
Hmm... where does it say NULL is invalid? NULL is actually very much permitted, and handled accordingly. This is exactly the way the C standard library handles NULL in free(void *p): If p is NULL, no operation is performed. However, I can certainly add this statement to the validity of NULL to the API documentation of psgplay_free.
There was a problem hiding this comment.
/**
* psgplay_free - free a PSG play object previously initialised
* @pp: PSG play object to free
*/
void psgplay_free(struct psgplay *pp);
The null pointer is not "a PSG play object previously initialised".
Even if your code handles it, your API documentation disallows it.
There was a problem hiding this comment.
Ah, indeed. I will add the validity of NULL to the documentation of psgplay_free, thanks!
| const ssize_t k_samples = | ||
| psgplay_read_stereo(pp, nullptr, | ||
| s_samples - t_samples); | ||
| if (k_samples <= 0) |
There was a problem hiding this comment.
Will this stop playback if I seek back to the beginning of a track?
(s_samples=0, t_samples=0, therefore k_samples must be 0 as well)
There was a problem hiding this comment.
Indeed, I’ve made this paragraph of code conditional on if (s_frames > t_frames) { ... } now (as I’ve replaced samples with frames for clarity on the stereo pair of samples).
But why is there no way to use your library without including this one header? I commented out the
So you did not reduce the Meson (btw. these are instructions for users. Developers should not use |
The stereo API ( However, these digital stereo filters are somewhat work-in-progress, and as it turns out, they need to be reworked because people want to compose multiple filters, in filter chains, which the current stereo filter API won’t allow. I’ve deliberately refrained from using these advanced features in the MPD plugin, until the matter of configuring stereo filter chains has been resolved satisfactorily. Given that the library is version 0.8 as of now, a filter rework might be due for 2.0 or something like that. :-) The situation is similar with MPD/src/decoder/plugins/SidplayDecoderPlugin.cxx Lines 341 to 347 in 94a02d4 However, SNDH tags can also be parsed at a lower level, for example using the
Not consciously, effectively: What warning level would you expect for triggering the errors you had with your compiler?
Ah, thanks! |
The Sure, there are optional features, and if a program wants to use them, it needs to include
That's the default level (the highest one), and the same I used. That was my last idea to explain why I had pedantic warnings enabled but you had not. |
Hmm. If I simply (only) remove
Maybe the filter stuff could go into a new
According to #2444 (comment) I actually do have |
Strangely, I don't get that warning, but you can fix it easily by adding a forward declaration for that struct. Just a line saying |
Yeah, true. Maybe the simplest (general) fix is to wrap the optional parts of the API in |
|
Sure, if some code in your headers is not valid C++ code, you should protect it with a preprocessor directive. But in this particular case, MPD needs none of this, and keeping internal header dependencies straight is enough to make the problem go away. |
Fixed in commit frno7/psgplay@9062f5f, where I don’t think there was much choice for the SNDH tag API at the moment.
Fixed in commits frno7/psgplay@01891df and frno7/psgplay@71f37a1, thanks!
Fixed in commit frno7/psgplay@874e1e7.
I suppose we can! Apart from cloning https://github.com/frno7/psgplay recursively, it only needs a |
No, certainly not clone from within a Meson file! Either we integrate it as GH Actions shell commands (and checkout with a proper GH action), or add it as a Meson Wrap. (I suppose somebody like you who put so much time into writing a huuuuge Makefile can't be convinced to switch to Meson completely...) |
Hmm. So something like [wrap-git]
directory = psgplay.git
url = https://github.com/frno7/psgplay
revision = v0.9
depth = 1then, in a
What kind of minimal
If Make is good enough for the Linux kernel build system, it’s good enough for me. If they switch to something else (such as Meson), I might follow suit. I suppose it might happen just about when MPD itself makes a wholesale replacement of its C++ with Rust, the language du jour, hmm? ;-) |
Yes.
No, it should not run
Ha, Linux kbuild is horrible enough to keep me away from
Good one! |
Hmm... That would inflict Meson on multiple subrepos, having additional source files, so we’d be leaving ‘minimal territory’ for a ‘grand enterprise’. The path forward seems to be the alternative Github action shell commands, then? |
|
There are 35 objects in the static library. That's not many, and build complexity doesn't come from the number of source files. A well-designed library with 35 sources can probably be built with a 40-line But of course, you're not a Meson fanboy like me, and having it only in a GH-Action is better than having no CI test. Yet the Meson wrap would have made your library available to all MPD users without them having to care about your specific |
|
Note: I don't suggest integrating this into GHActions/MesonWrap now. It's fine for me to finish and merge only the decoder plugin now, and figure out CI or Meson integration later in a new PR. Don't let this delay the merge. |
2 participants
No description provided.