Different fixes to DSMI loader

neumatho commented 3 years ago

This commit contains several fixes.

Removed the failure if an effect is out-of-range, but ignored instead. This makes the module "Escape From Dulce Base" playable. Module attached.
When porting this loader to C#, I noticed some strange thing, namely that the channel_remap array was defined as int, but loaded as bytes. The place where it is used, will then get some very high values and crash. Luckly?, this code was never executed, because of another bug. Version 10 modules, which have this remap table, did not use it. This is changed now.
In the search for finding some modules that uses the channel_remap, I stumbled over Otto Chrons Github page, where the original DSMI code is published. There are some test modules, which are in version 8 and 9. I decided to add support for those versions, so they can be played. One of the modules is in version 14, but I have attached all of them anyway. In the progress, I made some addional fixes while analyzering Ottos loader.

Escape From Dulce Base.zip Test.zip

sezero commented 3 years ago

@AliceLR: can you review?

AliceLR commented 3 years ago

I took a quick glance over the changes and they look fine to my (limited) knowledge of the AMF format. Saga Musix also reported a large number of AMF issues in libxmp recently (https://github.com/libxmp/libxmp/issues/350), so while I'm verifying these changes I'll cross reference against his laundry list and see if there's anything else that needs to be fixed... :)

sezero commented 3 years ago

OK, waiting.

neumatho commented 3 years ago

Well, if you open the module (Avoid.amf) in a hex-editor of some kind, so you can see all the binary values. Then this track starts at position 0x1559. The first two bytes is the length (0x0174) and this byte is 0xa3. The next 3 bytes are 0x09, 0xf4 and 0x06. The command (0xf4) is illegal, but we ignore that. The next 3 bytes read are 0x70, 0x00 and 0xc3. Here is the row number byte (0x70) too big and the load fails.

If you continue to look at the bytes, you will find a block which only contains zeros, which I think is very strange. The other bytes will also fail by the different range checks in the loader. The read length however, match to where the track ends, so that's why I just skip it and make it an empty track.

sagamusix commented 3 years ago

The track decodes just fine (as silence) though, even if it may contain nonsense. Ignoring it outright based on a sample size of 1 seems just wrong to me, because as you point out in your comment DSMI doesn't ignore it.

neumatho commented 3 years ago

Maybe the track decode fine in OpenMPT, but MikMod fails because of all those checks. Then these should be removed, but that's Ozkans choice.

If you look at the original loader (https://github.com/ochrons/dsmi/blob/master/src/AMFLOAD.C), you can see the loop from line 146 to 166, which loads all the tracks. Line 150-151 is where it read the length and this extra byte. The extra byte is not used further in the loop, so that's why I say it is ignored.

However, I agree that the detection is not the best way, but that's what I could come up with. It didn't fail for all the modules I have. All of them works just fine. Maybe an extra check could be made on the file version too.

Ottos loader also loads this track into memory without any checks at all. I do not know, what the player parts do with it then. It could just ignore it.

AliceLR commented 3 years ago

Currently verifying these changes now. Avoid.amf plays fine in DMP4 and all 27 orders play normally as far as I can tell (i.e. none are blank or garbage). I'll inspect it with a hex editor and report back.

sezero / mikmod

Different fixes to DSMI loader #36