So I'm looking into the porta issues in #1, specifically the file aurora_dawn.xm.

I haven't yet cracked why it sounds so interesting (Milky sounds like it's not even applying porta to the note, I think it might be something to do with the tempo and ticks, seeing what I'm reading through a few docs. Maybe something with how we do SLIDE_TOWARDS or update_frequency), but I'm gonna try and keep plodding along with it and see what I find.

But I did find this issue, which is why that big spike was happening. It was taking the effect number as well as the actual effect value (instead of 7 there, it was grabbing a much larger number because of the M).

I haven't had a good look around at xm implementations, and I haven't done much audio work before, but I think porta's only meant to slide towards between the current and new note, right? I find it odd that even a larger than normal tone_portamento_param would make the note frequency spike – if anything, I'd expect it to just go between the two notes much faster than usual. But again, I'm not too familiar with XM, so I'll keep plodding along and see if I can work out the issue!

edit: Having a play with MT, it looks like it is just doing the porta very fast, I suspect due to the tempo variable I saw some documentation about a bit earlier. I'll do some testing and see if I can get libxm sounding just like MT/etc sounds.

I'm writing a Rust wrapper for libxm, and noticed that there's no bounds for moddata in xm_create_context(). This is obviously bad if moddata is corrupted or invalid.

From what it appears, you seem to be aware of this issue already; it's listed in the README as:

• Loading a bogus file (that yet has a valid 60-byte header) will probably result in a segmentation fault.

Are there any plans to address this issue in the near future?

This is an weird patch, but it fixes this test case (where the note delay effect ED6 fails because the instrument is wiped for each new row).

Minimal test case: https://dl.dropboxusercontent.com/u/47853382/AT4RE%20-%20Lock%20Folder%20XP%203.6crk%20-%20test.xm

Full song: https://dl.dropboxusercontent.com/u/47853382/AT4RE%20-%20Lock%20Folder%20XP%203.6crk.xm

A safer xm_create_context_safe() function was added for creating the XM context; the now-deprecated xm_create_context() remains for backwards compatibility.

I originally attempted to error-handle all invalid reads, but that turned out extraordinarily verbose and difficult. :) So I did what @Artefact2 suggested and padded out-of-bounds reads with 0.

All of the READ_x macros will now return 0 if the provided offset exceeds the bounds of moddata. All occurrences of memcpy were also replaced with a new READ_MEMCPY macro that pads with zeros.

Fixes #6

Some XM modules contain a pattern table that references an undefined pattern (whose index is higher than the number of patterns). Both ft2-clone and MilkyTracker agree that those are empty patterns of 64 rows and play them accordingly.

This commit does the same in libxm, avoiding a segfault.

Rxy is possibly the most quirky command in the XM format, and it's hard to get right in all its dark corners. This gets libxm closer to FT2-Clone and XMPlay (even closer than MilkyTracker) but still not quite perfect.

The main bug fixed here is that the volume change of a Rxy command only takes effect if the volume column is empty, and only if there is no volume envelope in the instrument (and in this case, the envelope is not even restarted, contrary to what E9x does).

To fix this, I changed the code to first retrigger the note keeping the volume and the envelope like it is, and then updating it if needed.

It's important to notice that, even the volume part of Rxy doesn't apply, the value is still remembered in case a subsequent R0y is issued; so the part handling Rxy in xm_handle_note_and_instrument isn't affected.

Another bug (more obvious) is that the multi_retrig_add lookup tables is defined in 64th of a volume step, but the value was not scaled by 64, so the volume change was always saturating the volume.

Co-developed with @bryc who also prepared the testcases.

It looks like in FT2, panning is made using an exponential scale, based on sqrt. This PR allows to match panning performed by MilkyTracker and OpenMPT.

MilkyTracker: https://github.com/milkytracker/MilkyTracker/blob/be016986168a4d03b8d160303fa61296070a6e2c/src/milkyplay/ChannelMixer.cpp#L583

FT2-clone: https://github.com/8bitbubsy/ft2-clone/blob/8af2e42736b61a36a0d54bede93a47a1bab35f5d/src/ft2_audio.c#L192

The attached test makes it easy to reproduce the issue.

Co-developed with @bryc who also prepared the test.

When an instrument is played without a note, the current playing sample should restart its envelope, keeping its position. So it's important not to change the current instrument.

Also, the same behavior happens even if the instrument is invalid (and no note is specified); on the other hand, if the instrument is invalid and a note is specified, the current note is being cut.

Co-developed with @bryc, who also prepared the tests

This fixes the clicks and staticy noises in the modules in #1. That is, the ones that make these messages:

xm_sample(): final sample value is 136597905678638606450688.000000, this is a bug
xm_sample(): final sample value is 296616456960244306673664.000000, this is a bug
xm_sample(): final sample value is 144714319948683478564864.000000, this is a bug

There are two things that cause this problem in the xm_next_of_sample(xm_channel_context_t* ch) function – either the sample length is 0, and when it grabs u = ch->sample->data[a]; it reads into memory that does not belong to it, so we have that if at the start to check the sample length is more than zero.

What can also happen is that the 'loop end point' extends past the actual end of the sample. Just by one tick, so ch->sample_position was getting set to 132 at the end of a sample that was only 132 long, resulting in the same sort of read into memory that does not belong to it.

Anyways, this fixes both of those issues and the clipping. I'm fairly sure Milkytracker handles this in the same way, clamping the loop end to the sample length.

edit: I'm not sure whether this sort of fix should also be applied to the other loop types. I had to quickly type this up a few minutes before running to work, but otherwise I'll look into it when I get home.

https://modarchive.org/module.php?33498

Gets stuck in a loop using 100% of the CPU at the 1:18 mark.

Spd[05/75] Pos[0C/29] Pat[06/32] Row[22/40] Loop[00/01] 00:01:18.88

In octaves 6 and 7, there was not enough precision left to accurately compute the period, because too many bits were discarded during the scaling down.

To fix this, scale the frequency table by 1024 after switching to uint32_t, and adjust calculations accordingly.

Co-developed with @bryc.

In #27, we refactored xm_frequency to split arpeggio (note offset) from vibrato (period offset). Unfortunately, we did test it properly only for amiga frequencies.

It turns out the linear frequency implementation is wrong. The note offset cannot be added to the period, but must be added to the note itself. In case of linear frequencies, that's trivial to do once we get the frequency, as we can simply scale it.

This fixes binary_world.xm and so finally...

Closes #1

Just to log it, there's another bug in Rxy which I'm not planning to fix for now. If a row contains no note but an instrument with the Rxy effect, what happens is that on tick 0, the ghost note of the previous instrument plays (as per standard behavior with rows that contain instruments without notes), but on the first retrigger the instrument is changed to the new one. So actually the instrument changes mid-row.

This looks harder to implement because the logic to change instruments is not part of xm_trigger_note, but I haven't investigated into it. If you want, I can open an issue to log this.

Originally posted by @rasky in https://github.com/Artefact2/libxm/issues/20#issuecomment-822318552

Write a little helper program (xmanalyze?) that loads a module and sees which effects are used. The program spits out CFLAGS to build libxm with only these effects enabled. Maybe this saves a couple dozen bytes for demos and such.

Things that could be somewhat easily #ifdef'd out:

• Amiga frequencies
• Linear frequencies
• Waveform control stuff
• etc.

Gave it a spin with some tricky songs, and unfortunately I noticed some bugs that also appear in several other player libs. Note that it might be the songs that are at fault, with certain libs (xmplay/bassmod, openmpt/modplug, milkytracker/milkyplay) taking steps to cover it up. I don't know enough about the file format to judge. . binary_world.xm goes bad 39sec in, and OpenMPT has slight issues too http://modarchive.org/module.php?138953 http://clyp.it/hhp3pfex . ~~around 51sec, the lead echo in colond.xm is played at a slightly different pitch than the main lead, resulting in phasing~~ http://modarchive.org/module.php?174415 http://clyp.it/gfuaa1do . ~~as jt_strng.xm hits the higher notes at 35sec, they go off-pitch.~~ libxm gets a fair bit of static as well but I suppose that's intended http://modarchive.org/module.php?46508 http://clyp.it/nh15rq5h . ~~synthetic_attack.xm plays correctly but segfaults at the end:~~ (fixed in ac0bbdb, the module file is unsane. Opening then resaving the module in MilkyTracker fixes it.) http://modarchive.org/module.php?142503 ^{#0 xm_row (ctx=0x7f8d92832010, output=, numsamples=128) at /libxm-master/src/play.c:847 #1 xm_tick (ctx=0x7f8d92832010, output=, numsamples=128) at /libxm-master/src/play.c:948 #2 xm_sample (ctx=0x7f8d92832010, output=, numsamples=128) at /libxm-master/src/play.c:1293 #3 xm_generate_samples (ctx=0x7f8d92832010, output=, numsamples=128) at /libxm-master/src/play.c:1340}

. ~~zalza-tequila_groove.xm throws assertion failures,~~ (fixed in f24bcab) and some static at the 49sec and 58sec marks: http://modarchive.org/module.php?66518 ^{xm_sample(): final sample value is 136597905678638606450688.000000, this is a bug xm_sample(): final sample value is 296616456960244306673664.000000, this is a bug xm_sample(): final sample value is 144714319948683478564864.000000, this is a bug} . All tests done with master as of writing. Also, what are those (*) in the effects table?