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

Envelopes with 0 points #44

Closed sezero closed 3 years ago

sezero commented 3 years ago

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?

sezero commented 3 years ago

@AliceLR: Does the suggested patch above look OK to you? Any better solutions?

AliceLR commented 3 years ago

This looks fine for XM, will check into IT to see if this case is possible for it. If affected, the IT EF_VOLENV bit later in ProcessEnvelope would need to be copied there. (edit: Also, that seems like a bad name for this flag, because it doesn't indicate any volume envelope, but specifically IT volume envelopes that initiate fadeout when they end.)

AliceLR commented 3 years ago

Confirmed that Impulse Tracker 2.14p5 does immediately initiate fadeout when given a volume envelope with 0 points. OpenMPT, libxmp, and MikMod all currently get this wrong (but to be fair, I don't think 0 points on an enabled envelope is supposed to be valid anyway).

0pt.zip

sezero commented 3 years ago

Solution?

AliceLR commented 3 years ago

I checked the IT loader and I don't think anything needs to be changed, because the loader disables an envelope if it has fewer than 2 points. Your patch is fine as-is.

sezero commented 3 years ago

Thanks, applied.

Whenever you have the time, we have a backlog of patches/issues in mikmod: can you help?