sezero / mikmod Goto Github PK

From the STM zero volume samples issue: #6 (comment). Essentially, the bounding on the STM loader's loop for reading the order list isn't quite right, and the STM loader also does not correctly handle empty patterns with a valid index between 0-63. Several .STMs rely on this empty pattern behavior.

The MikMod 3.1.9 handling is:

        /* 99 terminates the patorder list */
        while((mh->patorder[t]!=99)&&(mh->patorder[t]<mh->numpat)) {
                of.positions[t]=mh->patorder[t];
                t++;
        }
        if(mh->patorder[t]!=99) t++;
        of.numpos=t;
        of.numtrk=of.numpat*of.numchn;
        of.numins=of.numsmp=31;

The MikMod 3.1.10 handling (which seems to be identical to the current handling aside from a missing bounds check) is:

        /* 99 terminates the patorder list */
        while((mh->patorder[t]<=99)&&(mh->patorder[t]<mh->numpat)) {
                of.positions[t]=mh->patorder[t];
                t++;
        }
        if(mh->patorder[t]<=99) t++;
        of.numpos=t;
        of.numtrk=of.numpat*of.numchn;
        of.numins=of.numsmp=31;

These are both wrong. The first version allows bytes >99, which is probably the reason for the revision. The second version incorrectly includes 99 in the bound, meaning every track gets a junk order appended after the loop.

The line after the loop that increments t in both versions is also suspect. I think this might have been added in an attempt to work around the second bug I mentioned, but the resulting extra order is never valid (it's either 99 or >=mh->numpat).

I think the correct handling is something along the lines of:

static BOOL STM_Load(BOOL curious)
{
        BOOL blankpattern=0;
        int t;
...
        /* 99 terminates the patorder list */
        while(mh->patorder[t]<99) {
                /* Patterns >=64 have undefined behavior... */
                if(mh->patorder[t] >= 64) {
                        _mm_errno = MMERR_NOT_A_MODULE;
                        return 0;
                }

                of.positions[t]=mh->patorder[t];

                /* ScreamTracker 2 treats patterns >= the pattern count like blank patterns. */
                if(of.positions[t]>=mh->numpat) {
                        of.positions[t]=mh->numpat;
                        blankpattern=1;
                }

                if(++t == 0x80) {
                        _mm_errno = MMERR_NOT_A_MODULE;
                        return 0;
                }
        }
        /* Allocate an extra blank pattern. */
        if(blankpattern) of.numpat++;
        of.numpos=t;
        of.numtrk=of.numpat*of.numchn;
        of.numins=of.numsmp=31;
...
        /* Modify STM_LoadPatterns() to take the real number of patterns to load from the file as an argument.
         * An extra blank pattern will be allocated and zero initialized by AllocPatterns().
         */
        if(!STM_LoadPatterns(mh->numpat)) return 0;

This obviously needs a real patch instead of pseudocode and a lot of testing, but does this generally look correct?

From a fuzz archive from Aug.2019: Files attached with valgrind outputs.
Valgrind runs were made on an i686-linux using examples/test/test.c. An
example output is inlined below from the 1st file, the rest of them are
similar. @AliceLR: Need help with this.

Playing minimum (4 chn)
==29020== Invalid read of size 2
==29020==    at 0x8074CBE: Mix32StereoNormal (virtch.c:307)
==29020==    by 0x807728A: AddChannel (virtch.c:1073)
==29020==    by 0x807824A: VC1_WriteSamples (virtch.c:1203)
==29020==    by 0x80776D8: VC1_WriteBytes (virtch_common.c:278)
==29020==    by 0x8055F8E: VC_WriteBytes (virtch_common.c:161)
==29020==    by 0x8048D87: NS_Update (drv_nos.c:70)
==29020==    by 0x804966D: MikMod_Update (mdriver.c:311)
==29020==    by 0x8048CB6: main (test.c:84)
==29020==  Address 0x4042212 is 6 bytes before a block of size 84 alloc'd
==29020==    at 0x4006041: calloc (vg_replace_malloc.c:593)
==29020==    by 0x8048E18: MikMod_calloc (mmalloc.c:118)
==29020==    by 0x8077BB8: VC1_SampleLoad (virtch_common.c:403)
==29020==    by 0x8055F55: VC_SampleLoad (virtch_common.c:159)
==29020==    by 0x80495C8: MD_SampleLoad (mdriver.c:283)
==29020==    by 0x8055C40: DitherSamples (sloader.c:486)
==29020==    by 0x8055CFD: SL_LoadSamples (sloader.c:508)
==29020==    by 0x804BDD7: Player_LoadGeneric_internal (mloader.c:622)
==29020==    by 0x804BE8D: Player_LoadGeneric (mloader.c:647)
==29020==    by 0x804BF54: Player_LoadFP (mloader.c:675)
==29020==    by 0x804BFA9: Player_Load (mloader.c:689)
==29020==    by 0x8048C64: main (test.c:74)

102-xm.tar.gz

Not sure if this is the correct place to report issues for MikMod, but I've been doing some testing with less common formats and found this bug. Every .FAR file (two attached) I load with MikMod has two rows truncated from the end of each pattern.

This line appears to be the culprit: https://github.com/sezero/mikmod/blob/master/libmikmod/loaders/load_far.c#L253
I haven't found decent documentation on this format yet, but as far as I can tell from looking at libmodplug and libxmp this is NOT the pattern row count. Modplug ignores this byte altogether and libxmp adds 1 to it and uses it as the row index for the final row of the pattern (i.e. it is 2 less than the effective row count). Both libraries use ([pattern size from the table] - 2)/64 to calculate the actual pattern row count.

budda1.far.zip
rainruin.far.zip

Hi @sezero

I was surprised you are in charge of this classic mikmod! Thanks!

I would like to ask you if it would be possible to add an option to build without texinfo dependency, as that pulls 11MB of dependencies.

This is what currently happens if I try, since makeinfo (part of texinfo) is not found:

/bin/sh: 1: MAKEINFO_EXECUTABLE-NOTFOUND: not found
make[2]: *** [docs/CMakeFiles/info.dir/build.make:62: docs/mikmod.info] Error 127
make[1]: *** [CMakeFiles/Makefile2:320: docs/CMakeFiles/info.dir/all] Error 2
make[1]: *** Waiting for unfinished jobs....

I believe makeinfo is needed to generate html docs, which could be optional in such a basic library.

Creating an issue for this one so my research doesn't clutter the (eventual) PR.

This bug occurs with .MED files that use the .MOD secondary tempo command 9xx in tempo mode, and appears to be caused by this line of code: https://github.com/sezero/mikmod/blob/master/libmikmod/loaders/load_med.c#L223

	  case 0x9:
		if (bpmtempos) {
			if (!dat)
				dat = of.initspeed;
			UniEffect(UNI_S3MEFFECTA, dat);
		} else {
			if (dat <= 0x20) {
				if (!dat)
					dat = of.initspeed;
				else
					dat /= 4; /* This line. */
				UniPTEffect(0xf, dat);
			} else
				UniEffect(UNI_MEDSPEED, ((UWORD)dat * 125) / (33 * 4));
		}
		break;

The OctaMED documentation suggests that this should convert directly to/from the .MOD speed command and, considering MikMod's tempo conversion, it seems that there is no reason to divide this value by 4. https://modland.com/pub/documents/format_documentation/Music%20Editor%20v3.2%20(.med).txt

This change dates back to MikMod 3.1, where it appeared in the form of a speeddivisor variable that was always equal to 4 during tempo mode. This variable is applied to the 9xx command and the speed case of the Fxx command in different ways!

- Lots of bug fixes in MED loader. Should now play modules at correct speed, but
  still not perfect

                case 0x9:
                        if(dat<=0x20) {
                                if(!dat) currentspeed=of.initspeed;
                                else {
                                        if(bpmtempos) currentspeed=dat*4/speeddivisor;
                                        else currentspeed=dat/speeddivisor;
                                }
                                UniPTEffect(0xf,currentspeed);
                        } else ...

                case 0xf:
                        switch(dat) {
...
                                default:
                                        if(dat<=10) {
                                                currentspeed=dat*4/speeddivisor;
                                                UniPTEffect(0xf,currentspeed);
                                        } else
...

By 3.1.5 this variable was completely removed, just leaving a fixed division by 4 in the 9xx command and not in the Fxx command. This looks like an oversight that never got caught to me, but there might some reason I'm missing for this being there.

Removing that line makes these example .MEDs play at the same speeds that xmp plays them at. I don't have OctaMED set up to test with right now.

alain.med.zip
wizardry.med.zip
yellybeansong.med.zip

Since this division by 4 was presumably added for a reason (even if it might have been a mistake), I'm going to exhaustively test (with OctaMED, xmp, and MikMod) every .MED MMD0 and MMD1 file I can find that uses this to make sure removing the bugged line doesn't cause any breakage.

edit: maybe that was too optimistic, there are 379 MMD0 files and 816 MMD1 files that use 9xx with values <= 0x20...

Certain .STM files rely on dummying out samples with 0 volume. There's a check for this in load_stm.c, but it's commented out. I'm not sure why this is the case, but here's a .STM from Mod Archive that definitely relies on this behavior to work correctly.

georythm.stm.zip

Other players' behaviors: libxmp has the same bug but also allows the mod to load, resulting in corrupted samples that sound awful. libmodplug unconditionally dummies these samples out and the file plays correctly. libopenmpt plays the file correctly and does effectively the same check, but in a different location.

See discussion in #40: Ultra Tracker tone portamento is currently implemented by unrolling the tone portamento effect. This works within a single pattern, but does not work when the tone portamento continues between patterns. The only fix for this is to implement a true continuous effect.

https://github.com/sezero/mikmod/files/7221268/PORTA.ULT.zip

From Thomas Neumann (@neumatho):

I have a little request you may put on your todo list, namely support
of panning law, specially for FastTracker 2. I found this you may use
as reference:
https://forum.openmpt.org/index.php?topic=6084.0
http://weaselaudiolib.sourceforge.net/#panning_law
https://modarchive.org/forums/index.php?topic=3517.0

As reported by Thomas Neumann (@neumatho):

I found another IT module, that sounds very strange. It is right at the
beginning in first channel. The guitar it cut when playing a note. Where
it happens, there is a S70 effect, which is according to the description,
some kind of note cut. MikMod does a notecut, but when playing this module
in other players, they do not cut the note. So I do not know, if MikMod
plays it correctly, even if it sounds weird or there is some kind of issue
with this effect.

Crying For You.zip

Hi!
After many years, I wanted to play the collection from ftp.hornet.org, which is all zipped. It used to work with mikmod, but does not anymore. Can't say when it got broken. I just remember using Mikmod on Debian 3.0. Now on 9.0 zip files are not recognized.

The .MOD (and .XM, .MED, and whatever else uses it...) retrigger effect currently does not initialize the retrigger timer, meaning it will always retrigger on the second tick of a given row.

Test file: in Protracker 3.15, libxmp, OpenMPT, this plays one snare hit, then three evenly spaced snare hits. In MikMod, this plays one snare hit, then four unevenly spaced snare hits. Speed=24, uses E98.

retrig.mod.zip

The only relevant changelog entry I could find re: this was for libmikmod-3.1.10:

- ProTracker effect E9 (Retrig) was not played correctly.

But weirdly, the only change to this effect between libmikmod-3.1.9 and libmikmod-3.1.10 is to prevent it from triggering on tick 0:
libmikmod-3.1.9:

                case 0x9: /* retrig note */
                        /* only retrigger if data nibble > 0 */
                        if (nib) {
                                if (!a->retrig) {
                                        /* when retrig counter reaches 0, reset counter and restart
                                           the sample */
                                        if (a->period) a->kick=KICK_NOTE;
                                        a->retrig=nib;
                                }
                                a->retrig--; /* countdown */
                        }
                        break;

libmikmod-3.1.10:

        case 0x9: /* retrig note */
                /* do not retrigger on tick 0, until we are emulating FT2 and effect
                   data is zero */
                if (!tick && !((flags & UF_FT2QUIRKS) && (!nib)))
                        break;
                /* only retrigger if data nibble > 0, or if tick 0 (FT2 compat) */
                if (nib || !tick) {
                        if (!a->retrig) {
                                /* when retrig counter reaches 0, reset counter and restart
                                   the sample */
                                if (a->main.period) a->main.kick=KICK_NOTE;
                                a->retrig=nib;
                        }
                        a->retrig--; /* countdown */
                }
                break;

The worst part of this is that I loaded a test .MOD into Protracker 3.15 and it does retrigger on tick 0.

Test file: this plays one snare, then plays a single snare note with an E9A effect on the note line and an E9A effect on the following line. In Protracker, the two lines with E9A both play on ticks 0, 10, and 20. libxmp, MikMod, OpenMPT, and MPT 1.16 all do something else(!).

retrig2.mod.zip

The first .MOD is trivial to fix (and I have a working patch) but I suspect finding the "correct" behavior for the second .MOD could be a mess, which is why this is an issue and not a PR yet.

diff --git a/libmikmod/playercode/mplayer.c b/libmikmod/playercode/mplayer.c
index e346ceb..4029eca 100644
--- a/libmikmod/playercode/mplayer.c
+++ b/libmikmod/playercode/mplayer.c
@@ -987,10 +987,13 @@ static void DoEEffects(UWORD tick, UWORD flags, MP_CONTROL *a, MODULE *mod,
 		}
 		break;
 	case 0x9: /* retrig note */
-		/* do not retrigger on tick 0, until we are emulating FT2 and effect
+		/* do not retrigger on tick 0, unless we are emulating FT2 and effect
 		   data is zero */
-		if (!tick && !((flags & UF_FT2QUIRKS) && (!nib)))
-			break;
+		if (!tick) {
+			if (!(flags & UF_FT2QUIRKS) && (!nib))
+				break;
+			a->retrig = 0;
+		}
 		/* only retrigger if data nibble > 0, or if tick 0 (FT2 compat) */
 		if (nib || !tick) {
 			if (!a->retrig) {

edit: I should note that the 3.1.9 behavior is also wrong because it would result in the retrigger timer carrying over between lines, which it shouldn't. To get the behavior of this patch in line with the Protracker 3.15 behavior, the change would probably be to initialize a->retrig to 0 instead of nib. The FT2 quirks check is correct; E90 does retrigger exactly once in Fasttracker 2 but does not retrigger at all in Protracker.

edit 2: initializing a->retrig to 0 was actually required anyway so I updated the patch. It does make the playback for the second test identical to Protracker.

edit 3: in FT2 the E9As don't play the first note unless there's a note on the line. :(
Updated patch to take that into account:

diff --git a/libmikmod/playercode/mplayer.c b/libmikmod/playercode/mplayer.c
index e346ceb..6f4e812 100644
--- a/libmikmod/playercode/mplayer.c
+++ b/libmikmod/playercode/mplayer.c
@@ -987,10 +987,16 @@ static void DoEEffects(UWORD tick, UWORD flags, MP_CONTROL *a, MODULE *mod,
 		}
 		break;
 	case 0x9: /* retrig note */
-		/* do not retrigger on tick 0, until we are emulating FT2 and effect
-		   data is zero */
-		if (!tick && !((flags & UF_FT2QUIRKS) && (!nib)))
-			break;
+		/* Protracker: retriggers on tick 0 first; does nothing when nib=0.
+		   Fasttracker 2: retriggers on tick nib first, including nib=0. */
+		if (!tick) {
+			if (flags & UF_FT2QUIRKS)
+				a->retrig=nib;
+			else if (nib)
+				a->retrig=0;
+			else
+				break;
+		}
 		/* only retrigger if data nibble > 0, or if tick 0 (FT2 compat) */
 		if (nib || !tick) {
 			if (!a->retrig) {

From a fuzz archive from Aug.2019: Files attached with valgrind outputs.
Valgrind runs were made on an i686-linux using examples/test/test.c. An
example output is inlined below from the 1st file, the rest of them are
similar. @AliceLR: Need help with these.

Playing  (1 chn)
==29020== Invalid read of size 2
==29020==    at 0x804C888: ProcessEnvelope (mplayer.c:440)
==29020==    by 0x8051805: pt_UpdateVoices (mplayer.c:2831)
==29020==    by 0x805339A: Player_HandleTick (mplayer.c:3410)
==29020==    by 0x8077F3E: VC1_WriteSamples (virtch.c:1161)
==29020==    by 0x80776D8: VC1_WriteBytes (virtch_common.c:278)
==29020==    by 0x8055F8E: VC_WriteBytes (virtch_common.c:161)
==29020==    by 0x8048D87: NS_Update (drv_nos.c:70)
==29020==    by 0x804966D: MikMod_Update (mdriver.c:311)
==29020==    by 0x8048CB6: main (test.c:84)
==29020==  Address 0x404aea0 is 12 bytes after a block of size 4 alloc'd
==29020==    at 0x4006041: calloc (vg_replace_malloc.c:593)
==29020==    by 0x8048E18: MikMod_calloc (mmalloc.c:118)
==29020==    by 0x804B006: AllocTracks (mloader.c:219)
==29020==    by 0x806188D: IT_Load (load_it.c:1008)
==29020==    by 0x804BB73: Player_LoadGeneric_internal (mloader.c:570)
==29020==    by 0x804BE8D: Player_LoadGeneric (mloader.c:647)
==29020==    by 0x804BF54: Player_LoadFP (mloader.c:675)
==29020==    by 0x804BFA9: Player_Load (mloader.c:689)
==29020==    by 0x8048C64: main (test.c:74)
==29020== 
==29020== Invalid read of size 2
==29020==    at 0x804C89E: ProcessEnvelope (mplayer.c:441)
==29020==    by 0x8051805: pt_UpdateVoices (mplayer.c:2831)
==29020==    by 0x805339A: Player_HandleTick (mplayer.c:3410)
==29020==    by 0x8077F3E: VC1_WriteSamples (virtch.c:1161)
==29020==    by 0x80776D8: VC1_WriteBytes (virtch_common.c:278)
==29020==    by 0x8055F8E: VC_WriteBytes (virtch_common.c:161)
==29020==    by 0x8048D87: NS_Update (drv_nos.c:70)
==29020==    by 0x804966D: MikMod_Update (mdriver.c:311)
==29020==    by 0x8048CB6: main (test.c:84)
==29020==  Address 0x404aea2 is 14 bytes after a block of size 4 alloc'd
==29020==    at 0x4006041: calloc (vg_replace_malloc.c:593)
==29020==    by 0x8048E18: MikMod_calloc (mmalloc.c:118)
==29020==    by 0x804B006: AllocTracks (mloader.c:219)
==29020==    by 0x806188D: IT_Load (load_it.c:1008)
==29020==    by 0x804BB73: Player_LoadGeneric_internal (mloader.c:570)
==29020==    by 0x804BE8D: Player_LoadGeneric (mloader.c:647)
==29020==    by 0x804BF54: Player_LoadFP (mloader.c:675)
==29020==    by 0x804BFA9: Player_Load (mloader.c:689)
==29020==    by 0x8048C64: main (test.c:74)

101-it.tar.gz
103-xm.tar.gz

I found a module that do not play correctly at the last pattern. There is a release note on every channel and when that is played, all channels are played with their previous samples. It works fine in libxmp and OpenMPT. Module is attached.

A Life In Termoil (surround).zip

Changing MikMod_malloc to use plain malloc and not calloc revealed
actual defects, i.e. unitialized memory reads here and there.

So far, I pushed the following commits to fix the ones I encountered:

• explicitly use MikMod_calloc for SAMPLOAD and MikMod_amalloc (517966a)
• explicitly use MikMod_calloc in ReadLinedComment for FAR (4693928)
• ASY_Load: avoid uninitialized reads from DupStr (605e9f3)
• MikMod_InfoDriver: ensure proper nul termination of list (9db6d2f)
• mikmod player, mconfedit.c: make skip_number saner (2a1b1f6)

More may follow.

@AliceLR: Can you have a look at the last three commits to make sure they are correct?

As reported by Thomas Neumann (@neumatho):

You have made a fix in StartEnvelope() to prevent "out of bounds" when
an envelope has 0 points. You did that by adding the following lines:
https://github.com/sezero/mikmod/blob/master/libmikmod/playercode/mplayer.c#L364

	if (!t->pts) { /* FIXME: bad/crafted file. better/more general solution? */
		t->b=0;
		return t->env[0].val;
	}

You also need to add the same/similar lines in ProcessEnvelope(), else
you will get "out of bounds" there (in line 460).

With Gherkins & A Rhubarb.xm zero-point env happens at position 36,
but it never seems to hit line 460.
Gherkins & A Rhubarb.zip

With Vikings In The Hood!.xm zero-point env happens at position 21,
and it does hit line 460.
Vikings In The Hood!.zip

Patch suggested by Thomas Neumann:

diff --git a/libmikmod/playercode/mplayer.c b/libmikmod/playercode/mplayer.c
index 9653bb8..d1aadcb 100644
--- a/libmikmod/playercode/mplayer.c
+++ b/libmikmod/playercode/mplayer.c
@@ -402,6 +402,9 @@
 */
 static SWORD ProcessEnvelope(MP_VOICE *aout, ENVPR *t, SWORD v)
 {
+	if (!t->pts) {	/* happens with e.g. Vikings In The Hood!.xm */
+		return v;
+	}
 	if (t->flg & EF_ON) {
 		UBYTE a, b;		/* actual points in the envelope */
 		UWORD p;		/* the 'tick counter' - real point being played */

OK? Comments? @AliceLR?

After merging #58, we replaced hard-coded numsamples, etc with values
read from the module itself: Is the following patch correct? It changes
the samples read for loop's upper boundary from 64 to numsamples, and
updates sanity checks. @AliceLR: Please have a look.

 diff --git a/libmikmod/loaders/load_asy.c b/libmikmod/loaders/load_asy.c
index 22e3c7e..1a9b070 100644
--- a/libmikmod/loaders/load_asy.c
+++ b/libmikmod/loaders/load_asy.c
@@ -179,7 +179,7 @@ static int ConvertNote(MODNOTE *n)
 
 	if (instrument) {
 		/* if instrument does not exist, note cut */
-		if ((instrument > 31) || (!mh->samples[instrument - 1].length)) {
+		if ((instrument > mh->num_samples) || (!mh->samples[instrument - 1].length)) {
 			UniPTEffect(0xc, 0);
 			if (effect == 0xc)
 				effect = effdat = 0;
@@ -301,8 +301,17 @@ static BOOL ASY_Load(BOOL curious)
 
 	_mm_read_UBYTES(mh->positions, 256, modreader);
 
+	#ifdef MIKMOD_DEBUG
+	fprintf(stderr, "ASY: bpm=%d, spd=%d, numins=%d, numpat=%d\n",
+		mh->inittempo, mh->initspeed, mh->num_samples, mh->num_patterns);
+	#endif
+	if (!mh->initspeed || !mh->inittempo || mh->num_samples > 64) {
+		_mm_errno = MMERR_NOT_A_MODULE;
+		return 0;
+	}
+
 	/* read samples headers*/
-	for (t = 0; t < 64; t++) {
+	for (t = 0; t < mh->num_samples; t++) {
 		s = &mh->samples[t];
 
 		_mm_fseek(modreader, 0x126 + (t*37), SEEK_SET);
@@ -323,10 +332,7 @@ static BOOL ASY_Load(BOOL curious)
 		return 0;
 	}
 
-	if (mh->num_samples > 64) {
-		_mm_errno = MMERR_NOT_A_MODULE;
-		return 0;
-	}
+	_mm_fseek(modreader, 37*(64-mh->num_samples), SEEK_CUR);
 
 	/* set module variables */
 	of.initspeed = mh->initspeed;

It seems that libxmp can load more different module formats than MikMod. It would be nice, if those can be adapted to MikMod.

I don't mind to give it a try at some point, if it is ok to take the code from libxmp (more or less) and make a MikMod loader from them.

Hello! I'm not sure if this is the right venue for this bug in particular since this is a GitHub mirror, not necessarily the official project source, but I noticed during the build step that libmikmod assumes it's at the root of the CMake project, which in my case it isn't, it's being used as a subdirectory.

I can make a PR if it goes upstream to the SourceForge repository, it's simply a matter of replacing some instances of ${CMAKE_SOURCE_DIR} with ${PROJECT_SOURCE_DIR}. I don't know if this is ideal since the CMake version that's being targeted is so old, and ${PROJECT_SOURCE_DIR} might be a new feature. I couldn't find a way to report this issue on the main project page either.

Let me know what you think!

As far as I can tell from looking at the .MOD loader there's no support for this odd .MOD variant. It's essentially an 8CHN .MOD with an M.K. signature and the only way to detect this is to calculate the expected size of an 8CHN .MOD and see if the input file has the same length. MikMod interprets them as standard 4-channel M.K. .MODs and the playback is about what you'd expect from that.

I don't know if any of these actually exist in the wild or that these are worth supporting. This .WOW was created with the Mod's Grave 6692WOW.EXE converter from a small .669 I had available. Despite what the doc claims I haven't found any evidence that "Grave Composer" actually exists; Mod's Grave is a DOS .MOD player and .WOW seems to exist solely as a format so it can support .669s.

crystals.669.zip
CRYSTALS.WOW.zip (Warning: sounds pretty bad when played by MikMod)
grave105.arj.zip (Documentation is in German. Sorry for the double archiving, Github doesn't like .arj).

Unrelatedly, that .669 feels like it plays back a little bit slower than with libxmp and libmodplug, but I don't know if that's a bug.

Several behaviors of OctaMED affecting note translation do not currently work:

• Song transpose: when this is set, every note in a song is transposed a number of semitones up or down. Used by around 80 MMD0s/MMD1s.
• Instrument transpose: when this is set, every note played by a particular instrument is transposed a number of semitones up or down. Used by around 750 MMD0s/MMD1s, more than any other unimplemented OctaMED feature so far (aside from MMD2/3...).
• Octave wrapping: hardware mixing mode has a couple of strange behaviors that a few modules rely on: octaves 4-7 act like octave 3; octaves 8-A wrap around to two octaves below octave 1 due to a bug. I don't have numbers on the former behavior but the latter affects ~30 MMD0s/MMD1s maximum, likely fewer.

The fix for all of these is the same: the OctaMED loader should allocate instruments and implement these features with the tuning table. This is a fairly easy fix (and this will be required to implement the IFFOCT instruments down the road anyway).

Tentative patch here. It is not well-tested yet, so expect some table spam here as I do that over the next few days.
master...AliceLR:med-transpose-octave-wrap

Hi everyone!

I have just created a web audio wrapper for libmikmod.

Not a single file has been changed in the entire library!

All I did was to create a new folder inside libmikmod, called webaudio, and placed everything needed in there, including two sample HTML pages and the distributed files.

https://github.com/carlosrafaelgn/mikmod/tree/master/libmikmod/webaudio

I uploaded the example so you could see it working (tested OK on Chrome 91, Edge 91, and Firefox 90):

• https://fplay.com.br/examples/exampleSimple.html
• https://fplay.com.br/examples/exampleOptions.html

My question is:

After creating these missing files, could I create a pull request? Or should I just leave these files in my repo?... Or should I move these files elsewhere, perhaps creating an entire new repo for them?

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;
}

As reported by Thomas Neumann (@neumatho):

I found a module that has some slide issues. It happens at position
4 and 5 at channel 2 and 3. There is a lot of E10 effects and the note
is sliding, but it sounds like it is sliding too far.  I do not know,
if it is the speed or there is missing some bounds checking. Maybe you
can investigate it further.

10000 Marbles.zip

I just brew installed mikmod 3.2.8 on my M3 macbook pro with some classic airpods pro connected, and tuned into some of my old favorite xm modules. They played glitchilly at double speed, indicating to me that every other sample was being discarded or something like that.

Now that I'm trying to replicate with wired headphones, they sound fine. Changing back to bluetooth breaks it again. The timer ticks at two seconds per second of wall time. There are lots of glitchy discontinuities, which is why I think it's dropping samples rather than playing at double sample rate.

Sorry, here's another issue to organize my research on OctaMED-related bugs.

Aside from the missing effects I noted here I've found issues with several other effects and another set of missing OctaMED effects (this time, they were allegedly introduced in OctaMED 3).

Links to relevant documents:

• OctaMED 2 effects document.
• OctaMED 4 effects document.

My progress on this issue: master...AliceLR:fix-med-effects. Feedback re: any of this, but particularly how I added the new effects with regards to the UNI format definitely welcome.

Play note twice, delay note, play note three times (FF1, FF2, FF3)

FF1 uses: 90 MMD0, 154 MMD1
FF2 uses: 49 MMD0, 38 MMD1
FF3 uses: 19 MMD0, 43 MMD1

Despite the strange documentation in the OctaMED 2 effects document (which is probably why these were implemented the way they were), it's clarified in later documents that FF1 is equivalent to 1F03, FF2 is equivalent to 1F30, and FF3 is equivalent to 1F02 (i.e. all three commands assume a secondary tempo of 6). I've verified the following for various versions of OctaMED:

This is easier to demonstrate with tests than with any modules I found. Each plays at speeds 6, 9, or 12:

1. two snare hits with no commands;
2. two snare hits, the second with FF1, then two snare hits, the second with 1F03;
3. two snare hits, the second with FF2, then two snare hits, the second with 1F30;
4. two snare hits, the second with FF3, then two snare hits, the second with 1F02.

ffcmds6.med.zip
ffcmds9.med.zip
ffcmds12.med.zip

Set pitch (FFD)

Total MMD0/MMD1 uses: 47.

This is supposed to immediately set the pitch of the current playing note to the new note without retriggering it. This required a new UNI effect (though all it does is stops the new note from playing...).

Portamento/Vibrato continue + volslide, Tremolo (0x05, 0x06, 0x07)

0x05 uses: 11 MMD0, 58 MMD1
0x06 uses: 15 MMD0, 152 MMD1
0x07 uses: 1 MMD0, 22 MMD1

These are apparently OctaMED 3 commands but I don't have the OctaMED 3 documentation; they're missing from the OctaMED 2 document but they're present in the OctaMED 4 document. These are ProTracker-compatible effects and are trivially implemented. Commands 0x06 and 0x07 don't exist in previous OctaMED versions so they should be safe. Command 0x05 was legacy vibrato prior to OctaMED 3, and all 11 MMD0s that use that command seem to have intended to use that.

All MMD0 modules using 0x05:

All MMD0 modules using 0x06:

So I put a version check on effect 0x05 and just let the other two translate to their ProTracker equivalents unconditionally. The implementation for the legacy vibrato was also wrong (the entire byte is the depth and the rate is fixed). I'm not sure the exact conversion to regular vibrato but I went with Rate=0xB, Depth=MIN(((old depth + 3) >> 2), 0xF) and it sounds close enough.

Portamento+volslide 0x05 example: orders 4/5 of balance.med.
Tremolo example: the bass in john paul ii.med uses it, but it's subtle. Other examples use too many features MikMod doesn't support yet.

Finetune (0x15)

Total MMD1 uses: 33.

I had to make my own test case to really test this since nothing that uses it is particularly obvious about it. It's exactly the same as ProTracker finetune.

finetune.med.zip

Other fixes

• Instrument default volume was getting ignored. (obvious in soul crystal - witching hour.med)

.XM files containing the .S3M/.IT order skip byte 0xFE will play an empty pattern instead of skipping that order.

Here's an example file I created in Modplug 1.16 a while back (it's not a particularly good track, sorry... ☹️).
vs_battle.xm.zip

This might be a Modplug-exclusive extension to the .XM format, but other .XMs in the wild might have these. libxmp handles these as expected but I can't tell if that's intentional.

Also, the S3MIT_CreateOrders behavior of stripping out 0xFE bytes from the order list causes sngpos / Player_GetOrder / Player_SetPosition to use different order numbers than the original track. This will break anything outside of MikMod relying on the original order positions (many MegaZeux games rely on hardcoded order numbers but I don't know how many of those also use 0xFE...). edit: that's unless there's a documented way to convert between the two sets of order numbers I missed.

As reported by Thomas Neumann (@neumatho):

While listening to my FastTracker 2 modules, to test the ported Xm
loader and player, I found a module which uses the XM Effect L (Set
envelope position) and it crashed with an index out of range. It
crashed on this line:
https://github.com/sezero/mikmod/blob/master/libmikmod/playercode/mplayer.c#L1584

				aout->venv.p=aout->venv.env[(dat>points)?points:dat].pos;

dat is 80 and points is 253. I tried with MikMod itself (the C code),
but there it doesn't crash, but it have the same values as I got in dat
and points. Maybe its because of pointer usage it does not always crash?

Well, I do not know much about exactly how envelopes works, but the array
venv.env, is always fixed to 32 elements. As I can see, each element have
a position and a value and what I can see for the ProcessEnvelope(), is
that P is the position and A and B is the indexes to the elements in the
array in use.

So I guess, that this function does not find the right position the way
its implemented, but it should just set P to either dat or points and update
A and B with the right indexes by looking in the array to find where P is
between two elements.

Also OpenMPT says that the panning envelope should only be updated, if the
volume envelope sustain is on:

https://wiki.openmpt.org/Manual:_Effect_Reference#Effect_Column_2

It seems like MikMod always update the panning envelope as well. I do not know, which is the correct way.

Oh, here is a link to the module I have tested with. The effect is used a couple of rows down at position 4.

https://modarchive.org/index.php?request=view_by_moduleid&query=135290

mikmod can run and process from command line, that's great.
I'd want to extract module info from command line, e.g. name. Thanks.

As reported by Thomas Neumann (@neumatho):

As I understand, then UniExpand() only makes sure, that there is enough
room in the buffer for the given number of bytes. If that's true, then
in UniNewline(), there is a call to UniExpand(). What I can see, it only
updates the len for the current row, but it need to be sure that there
is room for the next len or end marker. I will mean that the argument
should just be the number 1 (also the unitt and unipc was reversed anyway,
so it always gave a negative number).

Why is UniExpand() called at all in UniDup()? It just make a copy of the
current row, no need to expand. Well, its write the end marker, but we
know there is room for that in the UniNewline() call. Even if that's not
called, we know there is a buffer on at least 1 byte, which was allocated
in UniInit().

I propose the following patch:

diff --git a/libmikmod/playercode/munitrk.c b/libmikmod/playercode/munitrk.c
index bf41a9b..bc5ca46 100644
--- a/libmikmod/playercode/munitrk.c
+++ b/libmikmod/playercode/munitrk.c
@@ -275,7 +275,7 @@ void UniNewline(void)
 		unibuf[lastp]+=0x20;
 		unipc = unitt+1;
 	} else {
-		if (UniExpand(unitt-unipc)) {
+		if (UniExpand(unipc-unitt)) {
 			/* current and previous row aren't equal... update the pointers */
 			unibuf[unitt] = len;
 			lastp = unitt;

Comments? @AliceLR?

As reported by Thomas Neumann (@neumatho):

I noticed that in ConvertNote() in Asylum loader, you have a local
variable called lastnote. It seems like it should be a memory of the
last note found and is used as such. The problem is, that when used,
it will always be zero. Should this variable be global instead?
I mean, did I found a bug or is it by purpose the way its doing now?

https://github.com/sezero/mikmod/blob/master/libmikmod/loaders/load_asy.c#L147

Indeed, lastnote assignment doesn't seem to matter in present code,
it is never used after being assigned.

Further note by Thomas Neumann (@neumatho):

I found one module, where it go into that code path where there is no note,
but an effect. The module is "Crusader - No Remorse - Misc2" and its on
position 3-6 in channel 6. I tried to load the module into OpenMPT to check
what it does. It do not apply any notes, but the effect and instrument.
See screenshot.

I tried to listen to this channel and the same with the MikMod player, and
it sound identical. So I think, that the loader does the right thing by not
applying any notes. Therefore it could be a good idea to make some cleanup
and remove the lastnote variable :-)

strange.uni is a music track included in Onlink (the mod to the hacking game Uplink, but the track doesn't seem to be included in the original game or its bonus CD). I've attached a zip of the specific file, which is hopefully permitted. If not then it could be extracted from Onlink's data.dat yourself.
strange.zip

This track used to play on earlier versions of libmikmod but broke in later versions. The strange_loader.mod available on modarchive is still playable in newer versions of libmikmod.

The breaking commit appears to be 3a8d364 and the track worked in the previous commit 1c8dfd0

As reported by Thomas Neumann (@neumatho):

I have now ported the player part to C# and it seems to work. I have
then started on the UniMod loader. I had some files myself, but I found
some more here:

https://www.exotica.org.uk/mediawiki/index.php?title=Special:Modland&md=for&id=184

There are two modules, which is in version 4, namely "hundrapporten!"
and "mulla on porkkanaa". The rest is in version 5, which also my own
modules are. Those two modules did not sound correctly, and I found out
that the sample flags was wrong, so I have a fix here for them.

The flags are really guesses, because I could not find any information
about the flags, when its so old. I could not even find MikCvt2 (or 3
for that matter) to look at. The only flags that I'm 100% sure of, is
the loop and bidi. I know the rest need to be set, but which original
bit is the correct one, is guesses. I do not know if you have some
MikMod code that goes that far back you can look at.

But anyway, here is my fix for the UniMod loader. I have only changed
the LoadSmp5() function and this is what it's look like.

Here is the patch, as suggested by Thomas Neumann:

diff --git a/libmikmod/loaders/load_uni.c b/libmikmod/loaders/load_uni.c
index 0cbbfa5..a1d78a7 100644
--- a/libmikmod/loaders/load_uni.c
+++ b/libmikmod/loaders/load_uni.c
@@ -477,14 +477,22 @@ static BOOL loadsmp5(void)
 
 		/* convert flags */
 		q->flags=0;
-		if(s->flags&128) q->flags|=SF_REVERSE;
-		if(s->flags& 64) q->flags|=SF_SUSTAIN;
-		if(s->flags& 32) q->flags|=SF_BIDI;
-		if(s->flags& 16) q->flags|=SF_LOOP;
-		if(s->flags&  8) q->flags|=SF_BIG_ENDIAN;
-		if(s->flags&  4) q->flags|=SF_DELTA;
-		if(s->flags&  2) q->flags|=SF_SIGNED;
-		if(s->flags&  1) q->flags|=SF_16BITS;
+		if (universion >= 5) {
+			if(s->flags&128) q->flags|=SF_REVERSE;
+			if(s->flags& 64) q->flags|=SF_SUSTAIN;
+			if(s->flags& 32) q->flags|=SF_BIDI;
+			if(s->flags& 16) q->flags|=SF_LOOP;
+			if(s->flags&  8) q->flags|=SF_BIG_ENDIAN;
+			if(s->flags&  4) q->flags|=SF_DELTA;
+			if(s->flags&  2) q->flags|=SF_SIGNED;
+			if(s->flags&  1) q->flags|=SF_16BITS;
+		} else { /* UN04 flags, as suggested by Thomas Neumann */
+			if(s->flags& 64) q->flags|=SF_OWNPAN;
+			if(s->flags& 32) q->flags|=SF_SIGNED;
+			if(s->flags& 16) q->flags|=SF_BIDI;
+			if(s->flags&  8) q->flags|=SF_LOOP;
+			if(s->flags&  4) q->flags|=SF_DELTA;
+		}
 	}
 
 	d=of.instruments;s=wh;

Comments? @AliceLR?

The oldest mikcvt I found writes 'UN05' format, so I don't know
what 'UN04' used to do: See e.g. tclmikmod-2.15-dev9.tgz under
https://sourceforge.net/projects/mikmod/files/outdated_versions/mikmod/Legacy.unix/

Further note from @neumatho:

I looked at some of the old legacy code you link to below. When looking
at the sample flags there (in mikmod.h), the bit 6 (64) is SF_OWNPAN,
but in the original LoadSmp5() it is set to Sustain. I guess this is wrong
too and should be changed too.

Comments?

I haven't tested the MikMod CLI player very much in Linux but in Windows 10/MSYS2, I've encountered the following bugs consistently:

• When using OSS, the player often takes a second or more to respond to Q or Ctrl+C presses. This isn't an issue with WinMM so it might be a problem with MSYS2 OSS.
• The player display is usually a second or two ahead of playback (both OSS and WinMM).
• When using the WinMM driver, sometimes the end of the track will be cut off (presumably because the player is ahead of the audio playback). This doesn't happen when using the OSS driver.

These bugs can make testing annoying sometimes. I don't know the cause of these but I can look into them further.