sezero / mikmod

Mikmod Sound System (mirror of git repo at https://sf.net/projects/mikmod/)
http://mikmod.sourceforge.net/
69 stars 21 forks source link

.it max instument count #33

Closed yousernaym closed 3 years ago

yousernaym commented 3 years ago

The song hyo-cher.it from Modarchive fails to load because it uses instrument numbers greater than 99. I changed line 280 in load_it.c to allow it. Not sure about the implications but it seems to have solved the problem without breaking anything.

if((ins)&&(ins<100))    //<- Changed to 253
    UniInstrument(ins-1);
else if(ins==253)
    UniWriteByte(UNI_KEYOFF);
else if(ins!=255) { /* crap */
    _mm_errno=MMERR_LOADING_PATTERN;
    return NULL;
}
sezero commented 3 years ago

It's your commit https://github.com/yousernaym/mikmod/commit/66110993845b3e51e04a6d7741c8bd177d997db6 yes?

@AliceLR: Can you comment?

yousernaym commented 3 years ago

Yeah

yousernaym commented 3 years ago

Ok, something is still wrong though. First instrument stops playing after a few seconds.

AliceLR commented 3 years ago

Regarding the change to the instrument bound, I believe <253 is correct. I don't know why it's <100.

The first instrument breaks after a few seconds because this IT misuses the channel volume effect and MikMod stores the channel volume as a signed byte. MFF causes the volume to be -1. Values over M40 should probably be ignored but I'm not 100% sure (OpenMPT ignores them and this is a MPT 1.16 IT...).

I'll check the behavior for both of these issues in Impulse Tracker.

AliceLR commented 3 years ago

Impulse Tracker only allows up to 99 instruments and crashes when it encounters instruments >=100, hence that original bound. That said, the file format supports them just fine and I don't see the harm in allowing them (especially considering MPT and OpenMPT are both capable of creating ITs with that many instruments).

Impulse Tracker, OpenMPT, Modplug Tracker 1.16, and libxmp all ignore invalid Mxx parameters. The fix for that bug is to just change playercode/mlutil.c line 219 to this:

            case 0xd: /* Mxx Set Channel Volume */
                /* Ignore invalid values >64. */
                if (inf <= 0x40)
                    UniEffect(UNI_ITEFFECTM,inf);
                break;
sezero commented 3 years ago

OK then, I will apply the following patch soon:

diff --git a/libmikmod/loaders/load_it.c b/libmikmod/loaders/load_it.c
index 5b6f5bc..f19bada 100644
--- a/libmikmod/loaders/load_it.c
+++ b/libmikmod/loaders/load_it.c
@@ -277,7 +277,10 @@ static UBYTE* IT_ConvertTrack(ITNOTE* tr,UWORD numrows)
                UniNote(note);
        }

+       /* Impulse Tracker only allows up to 99 instruments and crashes when it
+          encounters instruments >=100. But the file format supports them just
+          fine and there are many MPT-created ITs with that many instruments. */
-       if((ins)&&(ins<100))
+       if((ins)&&(ins<253))
            UniInstrument(ins-1);
        else if(ins==253)
            UniWriteByte(UNI_KEYOFF);
diff --git a/libmikmod/playercode/mlutil.c b/libmikmod/playercode/mlutil.c
index 1c8005b..dacc6fb 100644
--- a/libmikmod/playercode/mlutil.c
+++ b/libmikmod/playercode/mlutil.c
@@ -217,7 +217,9 @@ void S3MIT_ProcessCmd(UBYTE cmd, UBYTE inf, unsigned int flags)
                UniEffect(UNI_S3MEFFECTD,inf);
                break;
            case 0xd: /* Mxx Set Channel Volume */
-               UniEffect(UNI_ITEFFECTM,inf);
+               /* Ignore invalid values > 64. */
+               if (inf <= 0x40)
+                   UniEffect(UNI_ITEFFECTM,inf);
                break;
            case 0xe: /* Nxy Slide Channel Volume */
                UniEffect(UNI_ITEFFECTN,inf);
sezero commented 3 years ago

Fixed by commit 9a3821b7fc31e, thanks.