Investigate slight popping issues in some songs - An obscure one... this time

[AmiSapphire]

Mainly some notes here, as it was originally documented on Bluesky and it was getting messy there anyway...

A different bug exhibits slight popping issues in Jubilife City (Day) from Pokémon Diamond and Pearl. This will also show up in the Night variant, though different instruments obscure this somewhat. Another affected song is Battle! Black Kyurem/White Kyurem from Pokémon Black 2 and White 2, though it is quite subtle. However, you will not catch this on an unfixed later plugin build due to portamento issues and other clipping issues. (Fixing those merely revealed it.)

Compiling the NCSF plugin from older commit https://github.com/AmiSapphire/in_xsf_pre-wxWidgets/commit/f30edc495ae7415e36243c0d4b487ac08802c3db before the conversion to C++17 (source was originally in C++11) plays the song just fine. Uploaded f30edc4 MP4 file Original f30edc4 WAV file

Compiling the NCSF plugin from older commit https://github.com/AmiSapphire/in_xsf_pre-wxWidgets/commit/21e0f057c85d76d02fc503c53a1837e34cf85d41 after the conversion to C++17 plays the song with the issues as described above. Uploaded 21e0f05 MP4 file Original 21e0f05 WAV file

The theory now is that something during or after the conversion from wstring_convert to standard Windows strings somehow introduced this bug. Commits: https://github.com/AmiSapphire/in_xsf_pre-wxWidgets/commit/21c14cdc0d29d64ebaa19cb1d208672c0d277577 , https://github.com/AmiSapphire/in_xsf_pre-wxWidgets/commit/4f88cb5572c380153a49f99afb7193575e8692e4 , https://github.com/AmiSapphire/in_xsf_pre-wxWidgets/commit/c041246ceba3b56269d1b7f5bc45e64407feefb1

[AmiSapphire]

Ruh roh.

Apparently this bug may be older than originally thought. It is actually present in commit https://github.com/AmiSapphire/in_xsf_pre-wxWidgets/commit/f30edc495ae7415e36243c0d4b487ac08802c3db , however, it may or may not show up in the second loop.

I first managed to compile the source code from that point in commit https://github.com/AmiSapphire/in_xsf_pre-wxWidgets/commit/21c14cdc0d29d64ebaa19cb1d208672c0d277577 , which is between https://github.com/AmiSapphire/in_xsf_pre-wxWidgets/commit/f30edc495ae7415e36243c0d4b487ac08802c3db and https://github.com/AmiSapphire/in_xsf_pre-wxWidgets/commit/21e0f057c85d76d02fc503c53a1837e34cf85d41 , and the bug is present there, so this disproves my theory.

I reverted the tempo fix in commit https://github.com/AmiSapphire/in_xsf_pre-wxWidgets/commit/6311c584aa3f86b3da0b95ee308b6549b49ee564 as a test in the current master code, and this only replicates what is actually played in https://github.com/AmiSapphire/in_xsf_pre-wxWidgets/commit/f30edc495ae7415e36243c0d4b487ac08802c3db .

After some experimenting with the source code from that point in commit https://github.com/AmiSapphire/in_xsf_pre-wxWidgets/commit/f30edc495ae7415e36243c0d4b487ac08802c3db , I finally tried v1.10.3 without any fixes. (This means the PSG bug is also unfixed.) Jubilife City (Day) plays perfectly fine there... especially after more than two loops, so it's back to looking through 2014-era commits.

I should test v1.11 and v1.11.1 as well to confirm, but...

[AmiSapphire]

Tested v1.11 (where the PLAYER Record data implementation was introduced) and the bug exists there as well. Comparing it with the SSEQPlayer code and most other fixes are implemented there. The PLAYER Record section is conspicuously absent, however.

This is the suspect commit: https://github.com/AmiSapphire/in_xsf_pre-wxWidgets/commit/9e2737fb069f06cbf820f10f88d1efa57450e71f

How the PLAYER Record data is handled in this plugin may be a slight issue. I may have an idea, though...

Here's a note: if I cannot fix this, I may as well rip it out, as most non-NDS players do not actually use it anyway.

Here's another note: I actually managed to remove the implementation at one point to try to 'fix' another clipping issue (the one I fixed earlier), and I saved the files in question... but I overwrote them by accident. So I will have to manually do so again as a last resort.

[AmiSapphire]

This is possibly fixed.

The PLAYER Record implementation may have been using padding alongside valid data for channel info for a decade!

No commit has been pushed for this yet, because it needs more testing. But if you want to test...

--

The test fix should be placed in src/in_ncsf/SSEQPlayer/INFOEntry.cpp. At the end of the file, you will see the following:

void INFOEntryPLAYER::Read(PseudoFile &file)
{
    file.ReadLE<std::uint16_t>(); // maxSeqs
    this->channelMask = file.ReadLE<std::uint16_t>();
    file.ReadLE<std::uint32_t>(); // heapSize
}

Change it to this:

void INFOEntryPLAYER::Read(PseudoFile &file)
{
    file.ReadLE<std::uint8_t>(); // maxSeqs
    this->channelMask = file.ReadLE<std::uint8_t>();
    file.ReadLE<std::uint8_t>(); // padding
    file.ReadLE<std::uint32_t>(); // heapSize
}

and compile.

[AmiSapphire]

After ~1.5 days of testing, this is confirmed fixed with this commit: https://github.com/AmiSapphire/in_xsf_pre-wxWidgets/commit/d478b20061a5e6f37ff592dc4bd2c00504fadcbd