.it sample vibrato rate and depth are off

[AliceLR]

This .it file uses a sample vibrato configuration that is immediately noticeable in Impulse Tracker. When played with xmp, the vibrato is performed but is much harder to hear because the depth is not as deep as it should be. Additionally, the vibrato sounds slower than when played in IT.

sample_vibrato.it.zip

[AliceLR]

Reversing the direction of the bitshift when converting sample vibrato to instrument vibrato makes the depth sound closer to Impulse Tracker. (it_load.c, line 817)

				sub->vde = ish.vid << 1;

The rate is still too slow, though.

[AliceLR]

The other thing I ended up doing was to change line 816 to add 1 to the rate:

				sub->vra = ish.vis + 1;	/* sample to sub-instrument vibrato */

I don't fully understand how the instrument vibrato works in libxmp, though, or whether or not this is actually a good idea. It did make the .it files I was using to test sound closer to IT, though.

[AliceLR]

From some experimentation in Impulse Tracker the vibrato properties listed here seem to function:

• ViS/Vibrato Speed: how fast the vibrato cycles (steps through the table per tick?). When this is 0, nothing happens. When at 1, the vibrato is very slow. At 64, the vibrato cycles quite fast. (range: 0-64)
• ViD/Vibrato Depth: magnitude of vibrato in fine linear slide steps. (range: 0-32, not 64 like the doc claims)
• ViT/Vibrato Waveform: not relevant to this issue.
• ViR/Vibrato Rate: sweep rate of vibrato in 256ths of a fine linear slide step. The depth begins at 0 and increases by this value until it reaches ViD. (range: 0-255, not 64 like the doc claims)

How they are translated to xmp subinstrument values (master):

				sub->vra = ish.vis;	/* sample to sub-instrument vibrato */
				sub->vde = ish.vid >> 1;
				sub->vwf = ish.vit;
				sub->vsw = (0xff - ish.vir) >> 1;

How these values are actually used in libxmp (read_event.c, 146):

		libxmp_lfo_set_depth(&xc->insvib.lfo, sub->vde);
		libxmp_lfo_set_rate(&xc->insvib.lfo, sub->vra >> 2);
		libxmp_lfo_set_waveform(&xc->insvib.lfo, sub->vwf);
		xc->insvib.sweep = sub->vsw;

lfo.c uses a waveform table of size 64 instead of 256, hence the right shift. This is why my patch above adding 1 to sub->vra appeared to help. If libxmp isn't going to use a 256 value sine table in lfo.c then sub->vra should probably at the very least be rounded in read_event.c (by adding 2 or 3 before the shift) instead of truncated.

sub->vsw goes on to become a divisor for the lfo output (making the rate relationship to the depth inverse instead of linear like the document suggests... I'll leave researching that to someone else). The important part here is the divisor settles at 4096.

sub->vde ends up as a multiplier for the table value of the lfo. Using square as an example, the maximum value for the vibrato ends up being 255 * (32 >> 1) / 4096 = 0.996

note to period (linear): p = (240.0 - n - (f / 128.0)) * 16.0
period to bend (linear): b = 100 * (8 * (((240 - n) << 4) - p'))

The effective calculation for pitch bend:

b = 100 * (8 * (((240 - n) << 4) - 16 * (240 - n - (f / 128)) - vib)

b = 100 * 8 * ((240 - n) * 16 - (240 - n - (f / 128)) * 16 - vib)

b = 100 * 8 * 16 * ((240 - n) - (240 - n - (f / 128)) - vib / 16)

b = 100 * 8 * 16 * ((f / 128) - vib / 16)

vib caps out at about 1 with the current handling, so a setting of 32 for ViD works out to about 8 finetune steps (instead of 32). I think this is why reversing the bitshift corrects the depth. A quick check shows every other loader that uses sub->vde also shifts left, not right, so this was probably just a mistake that never got caught before this report.

$ grep -nrI "\->vde" src/*
src/loaders/it_load.c:785:                              sub->vde = ish.vid >> 1;
src/loaders/mdl_load.c:667:         sub->vde = hio_read8(f) << 1;       /* vibrato depth */
src/loaders/rtm_load.c:477:                     sub->vde = ri.vibdepth << 2;
src/loaders/xm_load.c:562:                      sub->vde = xi.y_depth << 2;

So, summary of what I think the correct fix for this is now:

• Reverse the shift on ish.vid in the IT loader.
• Round sub->vra when providing it to libxmp_lfo_set_rate (and maybe eventually expand WAVEFORM_SIZE and the sine table to 256 positions instead).

edit: added relevant code links.

[AliceLR]

I was wrong, the original source of that bitshift was an intentional change, so I wonder if something else changed in the sample vibrato handling that broke the original fix: b942c8e

The depth >> 2 has been around much longer than that commit so it wasn't that...

For what it's worth, I tested the cited IT module with xmp both before and after the patch and after sounds closer to Impulse Tracker's playback to me. There's something weird with a different sample's volume (might be related to the volume envelope) but it definitely is not relevant to sample vibrato or sample global volume.