Allow speeds values of 128-255 for S3M/IT/DSMI/GDM/IMF.

[AliceLR]

Here is my alternate solution to pull request #35, as requested by @sezero. All investigation re: this patch is in the comments for that pull request, but the short version is the S3M, IT, DSMI, GDM, and IMF formats (which all use UNI_S3MEFFECTA) all allow speed values of 128-255. It's not clear why MikMod didn't allow this (maybe mod->sngspd was originally a signed char?).

[sezero]

@neumatho: This works OK for you?

[neumatho]

It sounds like it has been tested and the new fix is the correct way of doing it, so I have to live with that 2 minutes pause at the end :-)

Also found a lot of other modules of different types that also have long pauses at the end, so it seems to be very common to do that by the musicans.

[sezero]

It sounds like it has been tested and the new fix is the correct way of doing it, so I have to live with that 2 minutes pause at the end :-)

Also found a lot of other modules of different types that also have long pauses at the end, so it seems to be very common to do that by the musicans.

Do those modules play with the extra long silence using libxmp or openmpt?

[neumatho]

I did not test it and I cannot remember which one it was. But I just tried the module in the original pull request in OpenMPT, and there it set the speed to 128.

[sezero]

Alice?

[AliceLR]

I can't speak for any specific modules, but I have encountered numerous OctaMED and Scream Tracker 2 modules that intentionally play long stretches of silence, including some really excessively long ones (this comment lists some STMs that do this, they're marked with "[2]": https://github.com/sezero/mikmod/issues/6#issuecomment-708124148). This was probably an intentional decision by the author.

[sezero]

I think @neumatho speaks particularly about https://github.com/sezero/mikmod/files/6445324/Metromania.Partie.1.zip for the silence issue

[AliceLR]

Sorry it wasn't clear, but by "this was probably an intentional decision by the author", I did mean specifically that module. FWIW it should be possible to hex edit that out in your local copy if you don't want two minutes of silence in your playlist... :)

[sezero]

Tried playing that https://github.com/sezero/mikmod/files/6445324/Metromania.Partie.1.zip in latest xmp in properly stops at 5:40. Latest mikmod with this (and PR51) applied keeps playing after 5:40 - just silence like Thomas said (didn't bother waiting how long.)

Looking at older (lib)mikmod versions, the code this PR removes was added in libmikmod-3.1.5 The NEWS file doesn't have a specific entry for it (maybe I missed it.)

[AliceLR]

(Don't mind the force push; all I did was rebase this to the current master.)

If xmp stops at 5:40 when playing this mod then I think xmp might be wrong. To clarify, all formats that reference this effect either 1) bound their speed outside of the affected range (OctaMED) or 2) appeared to fully support speeds between 128 and 255 when I tested them (S3M, IT, BWSB, IMAGO Orpheus, and Dual Module Player). The minutes of silence at the end of Metromania Partie 1.amf are caused by the final pattern setting the speed to 128. This is how Dual Module Player behaved when I loaded this module in it.

I will check into the changes between MikMod 3.1.2 and 3.1.5.

[sezero]

Another test is, edit https://github.com/sezero/mikmod/blob/master/libmikmod/examples/test/test.c to use drv_wav instead of drv_nos, and run it against the Metromania modue: sox reports 8:20 duration.

[AliceLR]

It does appear that xmp is ignoring the A80 effect at the start of the last pattern in this module. This is a libxmp bug, but I don't know what's causing it (currently trying to diagnose it). The ~2 minute silence at the end of that module is the correct behavior.

edit: the libxmp bug is that it's being interpreted as BPM because it's using FX_SPEED instead of FX_S3M_SPEED.

[sezero]

libmodplug reports 340" duration: possible bug in libmodplug?

[sezero]

libmodplug reports 340" duration: possible bug in libmodplug?

I was quick to report this: it keeps playing after 340"...

[AliceLR]

I don't know about more recent libmodplug versions but I loaded it up in ModPlug Tracker 1.16 and can confirm that 1) it also gives an incorrect song length estimate of 5'40", but 2) it correctly sets the speed to 128 in the final pattern. Maybe it has a separate scan loop that doesn't properly handle the speed? I'm not as familiar with how it works internally as I am with libxmp and MikMod...

edit: didn't play the entire module but the final pattern lasted for about 2 minutes and 30 seconds, so I'd say at least its playback for this effect is correct.

[sezero]

OK, if you are sure that this won't be harmful, I'm willing to apply. However, can we #if 0 it out instead of removing it? Like e.g.:

diff --git a/libmikmod/playercode/mplayer.c b/libmikmod/playercode/mplayer.c
index 9653bb8..28a720a 100644
--- a/libmikmod/playercode/mplayer.c
+++ b/libmikmod/playercode/mplayer.c
@@ -1157,8 +1157,10 @@ static int DoS3MEffectA(UWORD tick, UWORD flags, MP_CONTROL *a, MODULE *mod, SWO
    if (tick || mod->patdly2)
        return 0;

+#if 0 /* See: https://github.com/sezero/mikmod/pull/42 and https://github.com/sezero/mikmod/pull/35 */
    if (speed > 128)
        speed -= 128;
+#endif
    if (speed) {
        mod->sngspd = speed;
        mod->vbtick = 0;

[AliceLR]

OK, will do. I'm 99% certain removing it is the correct behavior for all affected formats, but since I wasn't able to find any form of documentation explaining that change either, you're right that it's probably worth notating that it existed.

[sezero]

OK, applied. Let's see if anything ever breaks.