UniMod issues

[sezero]

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?

[sezero]

I pushed commit ad97d36d532ed87bd9d074fee4a4618fc19c19a7 to fix UN04 flags.

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.

For UN05, yes? @neumatho: Have you tested this well?

[neumatho]

Short answer -> no. I do not have that many UNI mods to test on. Most of the flags are guesses, but based on the old header file.

[sezero]

OK, if I apply the following one-liner, the five UN05 unimod modules from http://ftp.modland.com/pub/modules/MikMod%20UNITRK/Gatekeeper/ play as they do with mikmod-2.14-unix:

diff --git a/libmikmod/loaders/load_uni.c b/libmikmod/loaders/load_uni.c
index a54f94f..f240011 100644
--- a/libmikmod/loaders/load_uni.c
+++ b/libmikmod/loaders/load_uni.c
@@ -486,7 +486,7 @@ static BOOL loadsmp5(void)
            if(s->flags&  4) q->flags|=SF_DELTA;
        } else {
            if(s->flags&128) q->flags|=SF_REVERSE;
-           if(s->flags& 64) q->flags|=SF_SUSTAIN;
+           if(s->flags& 64) q->flags|=SF_OWNPAN;
            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;

Tested by recording using the raw output driver of mikmod-2.14-unix and then playing back with sox (play -s -t raw -2 -c 2 -r 44100 music.raw)

This is the only change needed before closing this ticket.

@AliceLR, @neumatho: What do you say?

[AliceLR]

SF_SUSTAIN being there at all seems highly suspicious to me since (as far as I know anyway) MikMod has never properly supported IT sustain loops. I can check the old versions of MikMod I have locally but I assume that's already been done.

edit: also, there's no corresponding flag for bidirectional sustain loops, and mikmod.h doesn't even define one. Also, it doesn't make sense that it'd be changed from a feature that is supported to one that is not supported between versions.

[sezero]

SF_SUSTAIN isn't employed anywhere in the mikmod source

[neumatho]

If you have looked in the old sources and it is not there, the fix seems ok to me.

[AliceLR]

okay this is even worse than i expected

MikMod 3.0.3:

/* Sample format [loading and in-memory] flags: */
#define SF_16BITS       1
#define SF_SIGNED       2
#define SF_STEREO       4
#define SF_DELTA        8
#define SF_BIG_ENDIAN   16

/* General Playback flags */

#define SF_LOOP         32
#define SF_BIDI         64
#define SF_SUSTAIN      128
#define SF_REVERSE      256

/* Module-only Playback Flags */

#define SF_OWNPAN       512
#define SF_UST_LOOP     1024

    /* Load sample headers */

    s = of.samples;
    for(v=0; v<of.numsmp; v++, s++)
    {   s->flags      = _mm_read_M_UWORD(modfp);

libmikmod 3.1.5, the next version after 3.0.3 with a UNI loader:

                        s->flags    =_mm_read_I_UWORD(modreader);

libmikmod 3.1.12 (I skipped some versions):

                /* 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;

edit: didn't realize the later versions have two different loaders in load_uni.c. Ugh...

[sezero]

If you have looked in the old sources and it is not there, the fix seems ok to me.

Yeah, SF_SUSTAIN is not in the old sources back when UN05 support was available

[sezero]

okay this is even worse than i expected

It really is. The last source you quote is for unimod 6, i.e. APUN from aplayer, which I can't find some modules to test with.

The patch I (Thomas actually) suggest is for unimod-5 (UN05, made using the mikcvt tool.)

What do you say?

[AliceLR]

Yeah, I didn't realize there were multiple loaders in there at first. I think this change is fine.

[sezero]

The patch is in. Closing this ticket.