sverx / libxm7

A C library to play XM (and MOD) modules on Nintendo DS using only ARM7 resources
MIT License
14 stars 3 forks source link

MOD playback is broken #1

Closed AntonioND closed 1 month ago

AntonioND commented 1 month ago

Try the files here, most of them are broken. https://github.com/AntonioND/wolveslayer/tree/master/nitrofs/wolveslayer/bgfx

I noticed the issue in my fork of libxm7 https://github.com/blocksds/libxm7/tree/master, but the issue is present in XM7Play too https://www.gamebrew.org/wiki/XM7Play

XM files play without issues.

sverx commented 1 month ago

I currently have no hardware and no build environment to test this, so the best I can do is to help you fix the issue in the code.

So I need to know in which way those MOD playback is broken. Do they load? Do they start playing? Is a tuning issue? Is some specific MOD effect misbehaving?

Please provide details, I'm more than happy to investigate into fixing this.

:wave:

AntonioND commented 1 month ago

Hey! Thanks for the reply. I've just created a test project that you can build with BlocksDS and test with melonDS. What happens is that the MOD file loads and starts playing. Some things are clearly broken, it doesn't sound well, but I don't know exactly what's wrong. Also, when I try to unload it, it crashes during during some free() call.

mod.zip

I've included the library itself in the project so that it's easier to play with it to find errors (and I've attached the objdump disassemblies, but I doubt this is very useful). The pre-built NDS ROM is also included.

With this small project it looks like it's a lot more manageable to find bugs, so I'm looking into it too.

AntonioND commented 1 month ago

It looks like CurrentInstrumentPtr->NumberofSamples ends up having a value that shouldn't happen for a MOD file. In one of my tests it returned 228 (instead of 1). So most likely there is some buffer overflow somewhere when loading the MOD file, and that is causing the playback issues.

AntonioND commented 1 month ago

I have found one issue in the XM7_UnloadXM() function, this fixes the crash when unloading MOD files:

    // instruments, from last to first
    for (i = (Module->NumberofInstruments - 1); i >= 0; i--)
    {
        CurrentInstrumentPtr = Module->Instrument[i];

        if (CurrentInstrumentPtr == NULL) // NEW LINE
            continue;                     // NEW LINE

        // samples, from last to first
        for (j = (CurrentInstrumentPtr->NumberofSamples - 1); j >= 0; j--)

However, this doesn't fix the MOD playback. Notes seem to be cut super early, for example.

AntonioND commented 1 month ago

I've been playing the MOD files side by side with OpenMPT and my demo, and it looks like one of the biggest issues is that the MOD files are playing one instrument in one row, and the next row they change the instrument but they don't apply volume or note, and apparently it should just be ignored. LibXM7 doesn't seem to ignore it. If I remove the additional instruments specified in otherwise empty rows in the MOD, it sounds okay. I don't know much about XM, is this supposed to be different in XM?

AntonioND commented 1 month ago

Okay, found it! XM7_REPLAY_ONTHEFLYSAMPLECHANGE_FLAG is what is causing this issue. If I remove this flag from the replay style, everything sounds perfect. What is the reason behind this flag? Why is it added by default for MOD files?

sverx commented 1 month ago

Because MOD files had that feature where if you had a line with an instrument specified and nothing else, the playback would continue using the new instrument sample data instead of the old one. FT2 instead chose to ignore that, but was common among Protracker modules, so this is the default behavior when loading a MOD file.

The library provides a way to set a different behavior after you have loaded a MOD/XM file, see: void XM7_SetReplayStyle (XM7_ModuleManager_Type* Module, u8 style)

This function sets some parameters that affect the way the module will be reproduced, mainly because there are some differences in some effect behavior if used in XM or MOD modules. Actually this simply affects the effect 0xy (Arpeggio) and activates/deactivates the 'on-the-fly sample change' feature. For your convenience some constants are defined:

#define XM7_REPLAY_STYLE_XM_PLAYER #define XM7_REPLAY_STYLE_MOD_PLAYER #define XM7_REPLAY_ONTHEFLYSAMPLECHANGE_FLAG #define XM7_REPLAY_STYLE_FT2 (currently alias of XM7_REPLAY_STYLE_XM_PLAYER, the default for XM modules) #define XM7_REPLAY_STYLE_PT (the default for MOD modules, it's both XM7_REPLAY_STYLE_MOD_PLAYER and XM7_REPLAY_ONTHEFLYSAMPLECHANGE_FLAG)

of course in your fork you can change the default for MOD files by changing this line

AntonioND commented 1 month ago

Thanks for the explanation! Yes, I call that function now and everything works fine: https://github.com/AntonioND/wolveslayer/blob/7cb786ee17587eee7276bfca7e76c3111f55ca3c/arm9/source/Sound/Music.cpp#L94-L95

I don't think I'll change the default behaviour because this is the first time I see MOD files doing this, so I assume it's not too common. I may write down a note about it, though.

In any case, I consider this issue fixed, so I'll close it and I'll just leave the PR with the fix for the crash when unloading MODs: https://github.com/sverx/libxm7/pull/2

sverx commented 1 month ago

Thank you for the fix! :+1:

AntonioND commented 1 month ago

By the way, now that this is all sorted out, I've switched this project to use LibXM7 😃: https://github.com/AntonioND/wolveslayer/commit/cc2feb871e5101443c1cc1e3de4e0d2db720e7f8

sverx commented 1 month ago

I'm happy you found my library useful, and please let me know should you stumble upon any other issue.