search

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

Added some fixes to Farandole modules. #37

Closed neumatho closed 2 years ago

neumatho commented 3 years ago
  1. Fixed loop detection. "Budda on a bicycle" have a looped sample without the flag set.
  2. Fixed the player to handle empty rows (row number = 0). "Alterations of time" have that at the last position.
  3. Handle invalid pattern numbers in position list. "Beyond the shores of avalon" have that at the last two positions.
  4. Added support for effect 1 and 2 (porta slide up/down).
  5. Implemented effect 3 (tone porta) the right way (hopefully) by adding a new effect. Implementation is based on documentation by Daniel Potter found at Modland + the original player code. This makes "Amazon dawn" sound better.
  6. Now plays with the real tempos. Farandole uses another concept than all other players to find the tempo.
  7. Added support for effect D and E (fine tempo up/down). "A journey into sound" uses this a lot.
  8. Fixed effect 6 (vibrato) by adding a new effect. The PT vibrato effect was too slow + it also need to increment on tick 0. "A journey into sound" sounds better now.
  9. Fixed effect 4 (retrigger) by adding a new effect. The argument is not a delay count, but number of times to retrig.

I will say, after these fixes, MikMod and NostalgicPlayer will be the most accourate player of Farandole modules there exists, except the Farandole itself. All players I have tested with, have different issues, mostly with the speed and note portamento.

There are still some effects that need to be implemented, but I could not find any modules that uses them. Modules mentioned above is attached.

TestModules.zip

sezero commented 3 years ago

I don't like the abi changes introduced in mikmod.h. Other than that, will try reviewing and testing. @AliceLR: Please review if you can?

neumatho commented 3 years ago

Do you mean the new FAR tempo variables in the MODULE structure? I can move them to somewhere else if you want, but the only reason I added them there, is because other counter variables etc. used by the player are stored there as well.

sezero commented 3 years ago

Do you mean the new FAR tempo variables in the MODULE structure?

Yes. Unfortunately the MODULE structure of libmikmod is not opaque, and one cannot know how the library is used. For the moment, I'd like to preserve ABI compatibility with libmikmod/3.

neumatho commented 3 years ago

Ok, do you want me to move them out as global variables instead or is there some other structure you want them in?

I guess the new flag is ok, since this is required.

sezero commented 3 years ago

Ok, do you want me to move them out as global variables instead or is there some other structure you want them in?

Globals are bad, but the library is infested with globals, so I guess you should go with globals for the time being. That aside, I still haven't had a chance to review properly yet - also waiting for comments of @AliceLR.

neumatho commented 3 years ago

I agree, that globals are bad, but since MikMod is written in C and not C++, there isn't much other to do other than having a structure which is giving around, just like MODULE. Maybe you should consider having a PLAYER or something like that structure, where such stuff can be stored.

I will move them to globals and then they can be moved again if you find some other/better solution in the future.

neumatho commented 2 years ago

Now floating points are not used in this patch anymore.

sezero commented 2 years ago

@AliceLR: What do you say, can you review?

Fix needed: In mplayer.c::SetFARTempo(): WORD should be UWORD

About the two new farcurtempo and fartempobend globals: They are not per-channel, yes, but can we not move them to struct MP_CONTROL, after the other four you added, and reference them like mod->control[0].xxx? E.g. like the patch below (compile-tested only: not run-tested):

diff --git a/libmikmod/include/mikmod_internals.h b/libmikmod/include/mikmod_internals.h
index f52428c..f779398 100644
--- a/libmikmod/include/mikmod_internals.h
+++ b/libmikmod/include/mikmod_internals.h
@@ -536,6 +536,8 @@ typedef struct MP_CONTROL {
     SLONG   fartoneportaspeed;   /* FAR tone porta increment value */
     SLONG   farcurrentvalue;     /* Because we're using fixing points as speed and the current period is an integer, we need to store the current value here for next round */
     UBYTE   farretrigcount;      /* Number of retrigs to do */
+    UBYTE   farcurtempo;         /* Farandole current speed */
+    SWORD   fartempobend;        /* Used by the Farandole fine tempo effects and store the current bend value */

     UBYTE   glissando;  /* glissando (0 means off) */
     UBYTE   wavecontrol;
diff --git a/libmikmod/playercode/mplayer.c b/libmikmod/playercode/mplayer.c
index 75f68f2..3ec6ace 100644
--- a/libmikmod/playercode/mplayer.c
+++ b/libmikmod/playercode/mplayer.c
@@ -46,10 +46,6 @@ extern long int random(void);
 /* The currently playing module */
 MODULE *pf = NULL;

-/* Different playing information */
-UBYTE farcurtempo;   /* Farandole current speed */
-SWORD fartempobend;  /* Used by the Farandole fine tempo effects and store the current bend value */
-
 #define NUMVOICES(mod) (md_sngchn < (mod)->numvoices ? md_sngchn : (mod)->numvoices)

 #define    HIGH_OCTAVE     2   /* number of above-range octaves */
@@ -2314,9 +2310,9 @@ static void DoFarTonePorta(MP_CONTROL* a)
 }

 /* Find tempo factor */
-static SLONG GetFARTempoFactor()
+static SLONG GetFARTempoFactor(MP_CONTROL* a)
 {
-   return farcurtempo == 0 ? 256 : (128 / farcurtempo);
+   return a->farcurtempo == 0 ? 256 : (128 / a->farcurtempo);
 }

 /* Set the right speed and BPM for Farandole modules */
@@ -2369,7 +2365,7 @@ static void SetFARTempo(MODULE* mod)

       You can make yourself a little exercise to prove that the above is correct :-) */

-   WORD realTempo = fartempobend + GetFARTempoFactor();
+   UWORD realTempo = mod->control[0].fartempobend + GetFARTempoFactor(&mod->control[0]);

    SLONG gus = 1197255 / realTempo;

@@ -2477,7 +2473,7 @@ static int DoFAREffect4(UWORD tick, UWORD flags, MP_CONTROL* a, MODULE* mod, SWO

                a->farretrigcount--;
                if (a->farretrigcount > 0)
-                   a->retrig = ((fartempobend + GetFARTempoFactor()) / dat / 8) - 1;
+                   a->retrig = ((mod->control[0].fartempobend + GetFARTempoFactor(&mod->control[0])) / dat / 8) - 1;
            }
        }
        else
@@ -2508,13 +2504,13 @@ static int DoFAREffectD(UWORD tick, UWORD flags, MP_CONTROL* a, MODULE* mod, SWO

    if (dat != 0)
    {
-       fartempobend -= dat;
+       mod->control[0].fartempobend -= dat;

-       if ((fartempobend + GetFARTempoFactor()) <= 0)
-           fartempobend = 0;
+       if ((mod->control[0].fartempobend + GetFARTempoFactor(&mod->control[0])) <= 0)
+           mod->control[0].fartempobend = 0;
    }
    else
-       fartempobend = 0;
+       mod->control[0].fartempobend = 0;

    SetFARTempo(mod);

@@ -2527,13 +2523,13 @@ static int DoFAREffectE(UWORD tick, UWORD flags, MP_CONTROL* a, MODULE* mod, SWO

    if (dat != 0)
    {
-       fartempobend += dat;
+       mod->control[0].fartempobend += dat;

-       if ((fartempobend + GetFARTempoFactor()) >= 100)
-           fartempobend = 100;
+       if ((mod->control[0].fartempobend + GetFARTempoFactor(&mod->control[0])) >= 100)
+           mod->control[0].fartempobend = 100;
    }
    else
-       fartempobend = 0;
+       mod->control[0].fartempobend = 0;

    SetFARTempo(mod);

@@ -2546,7 +2542,7 @@ static int DoFAREffectF(UWORD tick, UWORD flags, MP_CONTROL* a, MODULE* mod, SWO

    if (!tick)
    {
-       farcurtempo = dat;
+       mod->control[0].farcurtempo = dat;
        mod->vbtick = 0;

        SetFARTempo(mod);
@@ -3340,8 +3336,8 @@ void Player_HandleTick(void)
                if (!(pf->sngpos=pf->reppos)) {
                    pf->volume=pf->initvolume>128?128:pf->initvolume;
                    if (pf->flags & UF_FARTEMPO) {
-                       farcurtempo = pf->initspeed;
-                       fartempobend = 0;
+                       pf->control[0].farcurtempo = pf->initspeed;
+                       pf->control[0].fartempobend = 0;
                        SetFARTempo(pf);
                    }
                    else {
@@ -3393,8 +3389,8 @@ static void Player_Init_internal(MODULE* mod)
    mod->sngpos=0;

    if (mod->flags & UF_FARTEMPO) {
-       farcurtempo = mod->initspeed;
-       fartempobend = 0;
+       mod->control[0].farcurtempo = mod->initspeed;
+       mod->control[0].fartempobend = 0;
        SetFARTempo(mod);
    }
    else {
neumatho commented 2 years ago

You're right, that the two tempo variables are not channel depending, since they tell the current tempo of the whole module. Ofcourse, they can moved to MP_CONTROL, but personally, I think it is misleading to place them there, since they are not voice depending.

Maybe you could introduce a new structure holding these variables, and in the future, maybe move some other globals that have something to do with the module itself. The structure could be called MODULE_PLAYBACK or something like that. It could then be given in all the functions where MODULE also is given, or at least where needed.

Ofcourse, you then need another global variable to hold this, since you won't change the MODULE structure. I guess, it is not possible to make MODULE_PLAYBACK derive from MODULE, since it is C and not C++? Else that could be a solution without having the extra global.

But it is your call what you think is best, also for future changes.

neumatho commented 2 years ago

Do you want me to do anything, so this can get approved?

sezero commented 2 years ago

All I can think of in order to not add more globals is making a linked list, like

struct _fartempos {
    struct _fartempos *next;
    MODULE *mod;
    UBYTE farcurtempo;   /* Farandole current speed */
    SWORD fartempobend;
} fartempos = NULL;

in mplayer.c, initialize it from FAR_Load and search for the correct mod in the list. Then remove the relevant element when freeing the mod.

What do you guys think?

neumatho commented 2 years ago

So instead of two new globals, you will create one new global with this structure. But I do not understand, why you want to create a linked list?

I will suggess, to make it more future guaranteed, we could make two structures like this:

struct _farinfo {
    UBYTE farcurtempo;  /* Farandole current speed */
    SWORD fartempobend;
}

struct _extraformatinfo {
    _farinfo *farinfo = NULL;
} extraformatinfo;

Then if other formats in the future need some extra info, we just extend the _extraformatinfo structure.

Btw, I'm not 100% sure if the syntax above is correct, since I'm not a daily C coder, but I hope you get the idea :-)

sezero commented 2 years ago

A linked list carrying the module pointer would help in case of multiple mods opened.

@AliceLR what do you think?

neumatho commented 2 years ago

Ok, then it make sense. We could then extend my suggestion with:

struct _farinfo {
    UBYTE farcurtempo;  /* Farandole current speed */
    SWORD fartempobend;
}

struct _extraformatinfo {
    struct _extraformatinfo *next;
    MODULE *mod;
    _farinfo *farinfo = NULL;
} extraformatinfo;
neumatho commented 2 years ago

What about some lock mechanism on this structure? Thinking if you're loading a new module while playing another one, both the loader and player can access and/or modify this structure at the same time. Is there a standard way to do this in C?

AliceLR commented 2 years ago

The only valid way to get a MODULE * is via an API function that heap allocates it, right? How about an internal/opaque extended type until a public API change can be done? (I still think it wouldn't be the end of the world to just put it in MP_CONTROL/control[0] with a comment explaining the problem.)

typedef struct MODULE_EXT {
    MODULE mod;
    UBYTE farcurtempo;
    SWORD fartempobend;
} MODULE_EXT;

Then to access extended fields, just type pun the MODULE *:

MODULE_EXT *ext = (MODULE_EXT *)mod;

Also, sorry I haven't done the testing I said I would yet. A libxmp patch I was working on that was supposed to be done last week turned into four patches that broke >100 regression tests...

AliceLR commented 2 years ago

First issue I've found so far: tempo slides are very badly broken and result in a SIGFPE when playing back "The Rain in the Ruin" by Zowie. It seems like what's going on here is it's decrementing the fine tempo too quickly at order 22h (d1 should only decrement it by 1 per row, and FAR doesn't really have the same concept of a "tick" that other formats do) and then it repeatedly underflows until the resulting tempo is 0, hence the SIGFPE. So, presumably two bugs here: don't decrement on every "tick", and check the final tempo for underflow to <=0 after decrementing it. (Just assuming, I haven't looked at the code of this patch recently so there isn't a license issue when I fix this format in libxmp.)

The tone portamento in this module sounds correct, at least.

edit: i might be partially wrong with the above description because from further investigation, "new" FAR tempos are truly the worst thing

neumatho commented 2 years ago

I will move the globals into the MP_CONTROL struct with some comments and then look at the issue mentioned above.

neumatho commented 2 years ago

Found two bugs. The first one was a porting error from my side, when I ported the code from C# to C. Using the wrong data type (should be signed, but was unsigned). The other error was a missing range check in the FAR effect F (set tempo) handler.

AliceLR commented 2 years ago

The other error was a missing range check in the FAR effect F (set tempo) handler.

Effect F should not be modifying the fine tempo. The original code ignores setting the current tempo if eax is 0. I'm not sure how this case is happening in The Rain and the Ruin because it (and everything else I have) uses the new tempo mode, which shouldn't have this issue (the fine tempo down effect bounding is adequate to avoid division by zero). I think this could technically occur by setting the fine tempo at a faster coarse tempo and then slowing down the coarse tempo, though.

edit: I did confirm that setting tempo 3, sliding fine tempo to -32, and then setting tempo to 4 results in a case where eax is 0.

AliceLR commented 2 years ago

Something unusual I noticed while checking into why retrigger plays more notes than I expected: OCount is checked prior to decrement, meaning that the OCount of "3" actually works out to 4 pseudo-ticks. This module format is cursed

neumatho commented 2 years ago

I can confirm that if fine tempo is -32 (FarTempoBend) and a tempo of 4 (GetFarTempoFactor returns 32 (128/4)), then realTempo will be 0 in SetFarTempo, which will made a division by zero when calculation gus. eax will also be zero, but it crashes before that. Also double checked with the Farandole code and can see no tempo is set in this case, as you also mentioned. So I added an extra if statement there.

Is this retrigger thing something I should look at? Any test modules with this?

AliceLR commented 2 years ago

Here's the effects testing module I made: EFFECTS.FAR.zip

It covers every effect aside from 01 and 02, which just control volume ramping in Farandole Composer's mixer. It sounds the same with and without ramping so I'm not sure what the point is. See the description text (press F10 in Farandole Composer or open the module in OpenMPT) for more information. Some observations (not all of these are necessarily relevant to this patch, but I wanted to dump this info somewhere):

Tempo

Tempo edge cases

Portamento and Vibrato

Volslides and balance

Retrigger and "note offset"

"Fulfill loop"

AliceLR commented 2 years ago

With the latest commit it seems that both The Rain in the Ruin and "vanish.far" by Haj crash. "vanish.far" crashes as soon as it's loaded. It uses retrigger in a couple of places, but it's also very painful to listen to, so I'll post a better real example of it when I find one.

edit: nevermind, git fetch didn't pull that remote. Testing again. edit: vanish.far still crashes. The Rain and the Ruin no longer crashes, but the slowdown at order 34 still wraps several times when it shouldn't (it should slow from tempo 4+00 to tempo 4-31 and then change to tempo 6+00 in the final pattern).

neumatho commented 2 years ago

Big thanks @alicelr for the report. Vanish.far do work on my machine, but your test module fails with a division-by-zero.

This is what I have done so far:

Need to look more into the other effects to see if anything need to be changed there.

AliceLR commented 2 years ago

I will retest the few modules I've gone through with that patch when I get a chance, thanks. It's going pretty slow because I've been trying to check MikMod + libxmp + OpenMPT at the same time and occasionally fix libxmp bugs. The most recent bug I found in all replayers is that patterns of length 0, when referenced in the order list, get initialized to a blank pattern with the configured default break point (64 rows or 3F by default). An example of a module that uses this is Guillermo Luijk Delgado/to the polish of valencia.far, but there are several others I haven't got to yet.

Most modules so far play fine, though.

It appears that 16-bit samples was not used in real modules.

Not only this, but I had to fight for a while to even get Farandole Composer to consistently load and play that sample correctly when reloading that module. I think this (like its issues with effect 03 Fulfill Loop) might be due to GUS misuse, but the original playercode source doesn't include anything that interfaces with the GUS, so I don't know.

AliceLR commented 2 years ago

I confirmed that The Rain in the Ruin and to the polish of valencia.far are correct now with regards to tempo (the latter uses tempo 3-03 and was noticeably off before). I think vanish.far is also correct (it doesn't crash, anyway), but I hate listening to that module and I'm not going to replay it enough to be sure. It has something weird with portamento but I'm 99% certain there's probably a test module somewhere that doesn't hurt to listen to.

edit: oh no, all of Haj's modules sound like this :( edit: Haj/before.far relies on underflowing the frequency of one of the GUS voices, WTF? edit: made a Gist with the effects summary from above and my progress on comparing modules so far.

AliceLR commented 2 years ago
  • I didn't implement the old/new tempo mode, because I didn't see it necessary to do so. But I can add it, if you want me to so.

Also, I think it's OK to skip old tempo mode—if something is ever found actually relying on that, it can be fixed later. Nothing I've seen aside from my test module uses effects 04 or 05 (or any of the other 0? effects, for that matter).

AliceLR commented 2 years ago

Over halfway through the FAR modules I have right now. Some issues I've found (I've also been updating the gist I linked above):

edit: note, these are just the things in the modules I've gone through so far, I'll list C? and whatever else when I get to it. As far as I know, one module uses it.

AliceLR commented 2 years ago
  1. Fixed loop detection. "Budda on a bicycle" have a looped sample without the flag set.

Which sample? The only sample that looks looped to me is sample 1 "BellSynth1", and it does have the loop flag set (bit 3 of the looping mode byte) both in the version of the module attached and the version I have from ModLand. Did a different module have this bug?

neumatho commented 2 years ago

Which sample? The only sample that looks looped to me is sample 1 "BellSynth1", and it does have the loop flag set (bit 3 of the looping mode byte)

You're absolutely right. I double checked and I noticed that when I ported the loader in my player, I didn't saw that it was the loop variable that was used. I saw it as the type flag and there it is zero. I remove that part again in the next commit. My mistake.

neumatho commented 2 years ago

New fixes. I think I need help with the rest of the fixes, since I have nothing to compare against to hear if it is played correctly. Also you seem to be quite in-deep on how it should work now, @AliceLR.

I do not know if I should give to write access to this branch or we just complete it and create a new branch. It is better than before, but not yet complete, so it is up to you @sezero.

sezero commented 2 years ago

@AliceLR has write access here in mikmod, so she should have write access to your PR branch I guess?

neumatho commented 2 years ago

@AliceLR - I have made an invitation to my repository, so you should have write access now.

AliceLR commented 2 years ago

I think I should be able to push to this branch since I have write access here, but just in case, I accepted the invitation. Thanks.

I finally got though all of the FAR modules I have, and things mostly look good except where otherwise mentioned. I will pull the new changes and test those particular modules again.

Any frequency/period mode changes and new effects not handled here can wait for a second patch I think. The only module that uses C? aside from the test module is the palace festival.far, and it looks like it was used accidentally in place of B?. Aside from that, I think 03 is the only effect I didn't touch on before, and I still haven't figured out the exact circumstances that allow it to work correctly.

There's also another thing—it seems that Dan Potter disappeared from the Internet around 2008, so I don't know if there's a good way to confirm licensing on the old FAR player routine. I think using it as a basis for support elsewhere without restrictions is what he intended and it should be safe, but that call is up to @sezero (I can quote relevant parts of the docs/source on this if needed). If you didn't use it directly for this patch, it shouldn't really matter either way.

sezero commented 2 years ago

it seems that Dan Potter disappeared from the Internet around 2008, so I don't know if there's a good way to confirm licensing on the old FAR player routine. I think using it as a basis for support elsewhere without restrictions is what he intended and it should be safe, but that call is up to @sezero

Should be safe, yes

AliceLR commented 2 years ago

I confirmed that the most recent patch fixes those modules. I'm working on a commit with a couple more minor updates: updating the tempo comment, some minor line wrapping, moving declarations to the start of blocks for ANSI C, fixes for effect 4, and also a tempo crash fix that I missed before.

AliceLR commented 2 years ago

It turns out the tone portamento param is only the number of rows until completion for tempo 4, and it scales differently for other tempos. Another patch incoming after I finish testing it. 🤦🏻‍♀️

Example module: the tone portamentos in amazon dawn.far are double the speed they should be because it uses tempo 2. I think I noticed it before but brushed it off as a slight difference (it's actually not).

AliceLR commented 2 years ago

I think this patch is ready to squash and merge (at least, aside from the tone portamento issue, I haven't found anything else wrong). Any thoughts?

sezero commented 2 years ago

If there is nothing else to do, we can merge. I'm still not happy with the farcurtempo and fartempobend placement, but that can be handled later I guess?

AliceLR commented 2 years ago

I don't have any better ideas for where to them—my idea of putting them on the end of an internal type is probably asking for trouble and MikMod doesn't have anything like libxmp's internal structs or "extras" system. The linked list idea could work as long as all of the API functions accessing that use the lock, but I think it should be a more generalized system (in case more variables need to be added for anything else).

AliceLR commented 2 years ago

If there is nothing else to do, we can merge.

OK, will go ahead and do this.