Soundfonts that use loop offsets rather than sample loop points do not loop.

[DaforLynx]

FluidSynth version

FluidSynth runtime version 2.2.4
Copyright (C) 2000-2021 Peter Hanappe and others.
Distributed under the LGPL license.
SoundFont(R) is a registered trademark of Creative Technology Ltd.

Describe the bug

When playing samples with loop points set in the instrument's "loop start offset" and "loop end offset" (as described in Polyphone), with loop points set from 0 - 0 in the sample itself, the sample will not loop.

Expected behavior

The sample should loop properly from the loop start offset to the loop end offset, if there is no loop points set in the sample itself. This is the case for other soundfont players including Musescore's, SynthFont's, and in Polyphone itself.

Steps to reproduce

1. Download the sample MIDI file and sample SF2 file below.
2. Open a command window in the fluidsynth parent folder.
3. Type ./fluidsynth "DPPt.sf2" "Lake.mid" with the appropriate file locations.

Additional context

As stated before, this seems to be a common feature in most other soundfont players. Worth noting is that I used VGMTrans to create the soundfont.

When discovering this, I tested it with the loop points set in the sample itself rather than using offset values, and it worked perfectly. Try replacing the old soundfont with the "fixed" soundfont in which I changed the string sound to the working method. And for the record it'd be an extremely arduous process for me to go through every sample of every soundfont coming out of VGMTrans and change these values.

Sample MIDI file
Sample SF2
"Fixed" Sample SF2

The method on the left does not work in fluidsynth, the method on the right does.

[derselbst]

Sample loops - we're now entering the gray zone of the SoundFont spec.

Technically, the SoundFont spec has various requirements to the sample loops. E.g. they must be at least 32 samples long, cf. Section 7.10 of the SF2.04 spec. There must even be padding around the sample loops and other fun to guarantee an artifact-free playback. Unfortunately, the spec makes no statement about how to deal with sample loops after they've been processed (clobbered) by the loop offset modulators. That's one of the reasons why FluidSynth ignores most of those sample-loop-requirements and even accepts completely broken loops, which have their loopstart before the actual start of the sample data. Similar for loopend.

But FluidSynth does one thing: If loopstart == loopend, it considers that loop to be disabled at sample level:

fluidsynth/src/sfloader/fluid_sfont.c

Lines 789 to 798 in 015c6af

	if(sample->loopstart == sample->loopend)
	{
	/* Some SoundFonts disable loops by setting loopstart = loopend. While
	* technically invalid, we decided to accept those samples anyway. Just
	* ensure that those two pointers are within the sampledata by setting
	* them to 0. Don't report the modification, as this change has no audible
	* effect. */
	sample->loopstart = sample->loopend = 0;
	return FALSE;
	}

So, whereas previously loopstart == loopend == sample->start, now loopstart and loopend become zero. That means, they are pointing to the beginning of the smpl chunk.

Great. Now, once the loop offset modulators are running, we are reaching this code snippet:

fluidsynth/src/synth/fluid_voice.c

Lines 1035 to 1059 in 015c6af

	case GEN_STARTLOOPADDROFS: /* SF2.01 section 8.1.3 # 2 */
	case GEN_STARTLOOPADDRCOARSEOFS: /* SF2.01 section 8.1.3 # 45 */
	if(voice->sample != NULL)
	{
	fluid_real_t lstart_fine = fluid_voice_gen_value(voice, GEN_STARTLOOPADDROFS);
	fluid_real_t lstart_coar = fluid_voice_gen_value(voice, GEN_STARTLOOPADDRCOARSEOFS);
	z = voice->sample->loopstart + (int)lstart_fine + 32768 * (int)lstart_coar;
	UPDATE_RVOICE_I1(fluid_rvoice_set_loopstart, z);
	}
	break;
	case GEN_ENDLOOPADDROFS: /* SF2.01 section 8.1.3 # 3 */
	case GEN_ENDLOOPADDRCOARSEOFS: /* SF2.01 section 8.1.3 # 50 */
	if(voice->sample != NULL)
	{
	fluid_real_t lend_fine = fluid_voice_gen_value(voice, GEN_ENDLOOPADDROFS);
	fluid_real_t lend_coar = fluid_voice_gen_value(voice, GEN_ENDLOOPADDRCOARSEOFS);
	z = voice->sample->loopend + (int)lend_fine + 32768 * (int)lend_coar;
	UPDATE_RVOICE_I1(fluid_rvoice_set_loopend, z);
	}
	break;

loopstart and loopend will now be set to their respective position, however FROM the beginning of the smpl chunk. And not, from the beginning of the sample (which would be the correct case).

Now loopstart and loopend are pointing to some data points at the beginning of the smpl chunk. Those data points are unlikely part of the belonging sample. Because of that, we are very likely to trigger this line:

fluidsynth/src/rvoice/fluid_rvoice.c

Line 186 in 015c6af

voice->dsp.loopstart = min_index_loop;

and that line:

fluidsynth/src/rvoice/fluid_rvoice.c

Line 196 in 015c6af

voice->dsp.loopend = min_index_loop;

Now, loopstart == loopend == sample->start, which means, the loop is too short. Unlucky. And we're silently putting the sample to non-looped mode:

fluidsynth/src/rvoice/fluid_rvoice.c

Line 215 in 015c6af

voice->dsp.samplemode = FLUID_UNLOOPED;

The easiest fix I can think of is #1018. Pls. have a try. In case it works well, I'm not quite sure if we can already take this for 2.2.5 or whether we postpone the fix to fluidsynth 2.3.0. The test effort could be quite tremendous. And I would probably need @mawe42's opinion on that.

[mawe42]

Ahh... yet another creative way to (ab)use the many holes in the SoundFont spec. :-)

Setting loopstart = loopend = sample->start as proposed in #1018 will probably work in this case. I've got a nagging feeling that we have some more checks like this in other places, though. But it's been quite a while since I looked at that part of the codebase, so I might be mistaken.

As the spec is open to interpretation here, I would definitely vote to do what most other SF2 players do. But it would definitely be interesting to know how Polyphone, Muse and the others handle the case if the sample doesn't have 0 - 0 loop pointers, but 4000 - 4000, for example. If that works also as intended by the SF author, then maybe it's a sign that we still to too much loop validation and should even remove the check for loopstart = loopend.

[DaforLynx]

I've spent the past 3 hours trying to figure out how to compile this and I've found no success, so I'm not going to be able to test anytime soon. But if you test it with the linked files you should be able to see if it works.

[derselbst]

I've spent the past 3 hours trying to figure out how to compile this and I've found no success

That's a strange statement. What's the error you came up with? Or have you spent 3 hours trying to understand the documentation? We also have various CI builds to make sure everything builds successfully. So, just stating "I spent hours" is hard to take serious.

But if you test it with the linked files you should be able to see if it works.

I did that, ofc. And it sounds differently. But I cannot tell whether it sounds as intended.

[DaforLynx]

I wasn't entirely kidding about the "hours" part. It's nothing wrong with the compiler, or even necessarily this project specifically. It's just that there's so much stuff I have to find and download. I'm not a developer by trade or anything - have no experience with C or C++, if you would believe. The problem point was trying to install pkg-config by myself. It was surprisingly difficult for me to figure out how to compile it. Turns out I could just nick the binary from some repository somewhere. So I think I successfully did that, but now I'm getting weird unspecific errors when compiling fluidsynth. That's where I stopped, but I'll try again tomorrow. Sorry for being so negative.

[derselbst]

It was surprisingly difficult for me to figure out how to compile it.

Are you on Windows? In this case, you can just take one of the precompiled binaries:
https://dev.azure.com/tommbrt/tommbrt/_build/results?buildId=6740&view=artifacts&pathAsName=false&type=publishedArtifacts

[DaforLynx]

It was surprisingly difficult for me to figure out how to compile it.

Are you on Windows? In this case, you can just take one of the precompiled binaries: https://dev.azure.com/tommbrt/tommbrt/_build/results?buildId=6740&view=artifacts&pathAsName=false&type=publishedArtifacts

That, ah, makes things significantly easier. I just assumed compilations were automatically deleted after CI.

[DaforLynx]

But I cannot tell whether it sounds as intended.

I can confirm it is sounding as intended. And somehow it sounds even better/more accurate than either Musescore or SynthFont (Musescore by a long shot - that program does strange things with the looping in these soundfonts). It did do something weird with pitch bends in one of my MIDI files but that's not relevant. Do you need me to test anything else with this change?

[DaforLynx]

Example. This is 100% how it should sound. It also works with all of the other soundfonts I used VGMTrans to rip (I just realized it might not be VGMTrans that is doing the loop offset thing, but rather the Synthfont feature that converts DLS to SF2).

powershell_2GQCUJaSsf.mp4

[derselbst]

Do you need me to test anything else with this change?

Ok, thanks for the report and for confirming. That's it for you. I will discuss the technical details with Marcus in #1018 during the next days / weeks.

[sykhro]

Worth noting is that I used VGMTrans to create the soundfont

Out of curiosity, are you using an old version? We use fluidsynth for playback in VGMTrans nowadays, via SF2.

[DaforLynx]

Out of curiosity, are you using an old version? We use fluidsynth for playback in VGMTrans nowadays, via SF2.

Possibly. I couldn't find the version but the folder is labelled 9_29_09. Not sure if I even got it from GitHub. I don't think that would matter, though, because I used VGMTrans to export the DLS, which I then converted to SF2 with Synthfont. Sorry for the confusion.

Edit: yeah...my version of VGMTrans looks very different from current builds

Edit 2: I'm so mad.