Wrong loops when reading certain soundfonts

[derselbst]

When certain soundfonts/instruments are played there is a distinct beating that is probably caused by the loops being wrong. I've heard this with many different videogame related soundfonts, to take one as an example here's a for Final Fantasy VI soundfont . Many instruments have problems here, for example program 000-024 (Guitar) and 000-078 (Ocarina1) . It doesn't seem to be a problem with the soundfont since it plays correctly in other software.

To try and investigate I loaded the soundfont in Swami and Polyphone, two different soundfont editors. Swami's playback is wrong too (I know it uses fluidsynth, but I'm not sure if it shares the sf2 loading code with it). Polyphone plays it correctly.

Interestingly the sample loop points for these instruments had different data in both softwares. For example in swami the sample for 'Guitar' (s_guitar) loops from 8 to 5960 and Polyphone reports it as 5456 to 5967. Polyphone gets it right and it seems that for the offending samples Swami always had them starting at a fixed 8 and finishing with an offset of -7 the working value.

Anyway, I'm not aware if swami shares code with fluidsynth, but it probably shares this bug in interpreting the soundfont data. I browsed through both projects code and I recognize I'm not knowledgeable enough to try and fix this myself, but I hope this report has enough details for someone to do it.

I assure you this happens with many other soundfonts and can provide more examples if needed. Also I tested with the latest fluidsynth code from git to be sure and it does have the same problem.

Thanks!

Reported by: lfzawacki

Original Ticket: fluidsynth/tickets/151

[derselbst]

The function fixup_sample in fluid_defsfont.c converts the loopstart/end to offsets and two other functions receive the raw loop information from the soundfont, load_shdr and fluid_sample_import_sfont. It may be of interest to remove the comments from fluid_sample_import_sfont, possibly add a few printf statements to show the values of loopstart/end, and report all logging that is relevant to the loop points of the instruments.

Original comment by: *anonymous

[derselbst]

Here's a log of all the information in the 'fluid_sample_import_sfont' function. See attachment. The first 2 values are for the samples I cited in my bug report, the rest is the full log.

The code used is this (I just filtered some of the clutter out in the final output) :

  FLUID_LOG(FLUID_WARN, "\nLoop values for sample: %s", sample->name);
  FLUID_LOG(FLUID_WARN, "sfsample->start %05d", sfsample->start);
  FLUID_LOG(FLUID_WARN, "sfsample->loopstart %05d | sample->loopstart %05d", sfsample->loopstart, sample->loopstart);
  FLUID_LOG(FLUID_WARN, "sfsample->loopend   %05d | sample->loopend   %05d", sfsample->loopend, sample->loopend);
  FLUID_LOG(FLUID_WARN, "sample->samplerate %d", sample->samplerate);
  FLUID_LOG(FLUID_WARN, "sample->origpitch %d", sample->origpitch);
  FLUID_LOG(FLUID_WARN, "sample->pitchadj %d", sample->pitchadj);
  FLUID_LOG(FLUID_WARN, "sample->sampletype %d", sample->sampletype);

By the way, the commented code starting at line 1842 doesnt fix the sound of these samples, it just sounds wrong in a different way. Would you like a log of the final values with these lines uncommented?

Original comment by: lfzawacki

[derselbst]

Next, it would be interesting to log the sam->loopstart values from this function: fixup_sample. You may also revert the uncommented out lines from before so the log is more easily read.

The same log functions above for loopstart can be added to fixup_sample and it's not necessary to log other than sam->start and sam->loopstart.

I would also log each statement which checks for the size of the sample, such as this:
else
loop is fowled, sample is tiny (can't pad 8 samples)

And after these lines:
loop is fowled?
can pad loop by 8 samples

And add a log line just saying that "sample is tiny". It will help isolate the line that is the cause of the issue and then that line may be changed for testing.

From a guess at this point, this line seems important in that function:
sam->loopstart -= sam->start;

It would also help to log &p->name, p->start and p->loopstart in load_shdr function.

At this point, my guess is that the looping points are stored differently in this soundfont (and some others) than is typically used. Even in that case, it may be possible to edit the soundfont to test for this or whether fluidsynth doesn't handle certain types of stored looping data. However, it's too early yet to test for this. It would also help to log this above data for a soundfont which is working as expected.

Original comment by: *anonymous

[derselbst]

I just noted that there is a pattern to your informative logging. It seems that the problem samples have a loop end value which extends beyond the length of the sample.

If you could edit s_guitar in Viena or similar soundfont editor and change loop start/end to 5456 - 5968; then try 5456-5461; and see if it makes a difference in the logging and by ear. Also, in the ocarina case, it's the same, so that can also be tested by subtracting form the loop end value in an editor. Many of the other samples look ok from your logs, particularly those where the loop end value is contained within the size of the sound sample.

Another (separate) issue may be the size of the loop from start to end. If the frames are low, then that may cause it, too. So, if the above works and there is an issue with the short loop samples, then try to extend those loops beyond 64 frames and test again.

Original comment by: *anonymous

[derselbst]

5456 - 5968 and 5456-5461 dont sound right as loop points. As I stated before another editor software called Polyphone reports it more accurately as 5456 to 5967.

Here's the new log for fixup_sample and load_shdr in attachment.

Looking at these raw values for the loop points the values reported by Polyphone make a little more sense... I suppose the soundfont spec doesnt talk about loopend points which go past the sample size, but some soundfonts use it? Anyway, the loop fowled treating could be different maybe?

Original comment by: lfzawacki

[derselbst]

That's good news that you already identified the issue from Polyphone. It should be simple enough to use their code as a reference in the above functions.

Original comment by: *anonymous

[derselbst]

Success! The main difference with Polyphone's code is that it doesnt do anything with these kind of values when loop end exceed the sample length. I tested some variations on this and attached is a patch which ignores the 'fowled loop' treatment for 'sam->loopend > sam->end' .

changes.diff is a version of the patch with more logging so you can see the loop values.

loops_wrong.ogg and loops_right.ogg are versions of the same soundfont playing before and after the patch.

I tested this with 4 different soundfonts which had this problem and it works fine.

Original comment by: lfzawacki

[derselbst]

Polyphone does change the s_guitar loop position, it has code for a limit() function so it is limiting the values from 5456 to 5967. Actually, a soundfont editor shows the loop end position as 5969.

However, that is correct that removing the fowled loop treatment will mirror Polyphone. But this may not be a true solution but instead a workaround since the code is there for a reason. Test it for a while with specific soundfonts and then try to report your results. The concern is the DSP code and how it uses the looping positions for the fowled loops. Nice work otherwise!

Original comment by: *anonymous

[derselbst]

Ok. I'll test it with most of the soundfonts I usually use, see if I notice any playback errors and report back in a few days. Do you have any specific cases where I the problem with these loops might arise? I could test them too.

Original comment by: lfzawacki

[derselbst]

I've been using these changes in my fluidsynth instance for a week now and haven't noticed any detraction from the sound quality. What's the next steps from here to get these changes pushed into fluidsynth? I'm willing to help by testing with the soundfonts which originally needed that loop padding and contribute more code for this issue.

Original comment by: lfzawacki

[derselbst]

Just now getting around to joining the conversation on this one. The code in question is based on what I wrote for an older version of Swami (what was called Smurf SoundFont Editor), which Peter Hanappe adapted for FluidSynth. It got a couple overhauls when it became Swami, but a lot of the logic is still similar, having a common code ancestry, so it is not surprising the issue affects both FluidSynth and Swami the same (libInstPatch in this case - which Swami uses).

I reviewed the SoundFont 2.4 specification PDF again, in regards to the sample offsets and did some analysis of the sample data and loop information in the example SoundFont. As you noted, the loop_end is outside of the defined sample data by 1 sample point, for all the problem samples in that SoundFont. The sample start index, defines the first valid sample point in the sdta chunk, the end index defines the first point AFTER the last valid sample point (should index the first of a sequence of 46 or more 0 valued samples - though I have seen some software just leave these out altogether or have much fewer). Looking at the s_guitar sample data, it looks like the end index does indeed point to the first 0 valued sample after the sample waveform. So that seems correct.

The SoundFont 2.4 spec defines the loop start index as the first sample point of the loop, while the end is the first point AFTER the last sample of the loop. In essence the loop start and loop end can be seen as being superimposed over one-another, so that the loop_end really represents the loop start sample point. So that means that the loop_end should not be past the sample end, but can be equal (since both end and loop_end represent the next sample after the last used sample).

The logic I had for correcting fowl loops (cluck cluck indeed) seems brain dead. I can see that adjusting these values at all means that sample offsets in instrument zones would be affected. I had first thought to clamp these values to the start/end, but that results in issues at the instrument level, in this case. So it seems like your suggested patch, of just leaving these incorrect indexes alone, makes sense. I'd even go so far as to leave them alone entirely, except values less than the sample start index cannot be represented, so that at least must be corrected.

My main question is whether FluidSynth has any code which depends on loop_start and loop_end of fluid_sample_t being sane. The fluid_voice_optimize_sample() at least looks like it depends on this. There may be other places too (though I did not see any). That function should at least be updated to check for a valid loop, before doing its noise floor calculation.

A bit long winded and repeats a lot of what you guys already observed, but I wanted to be as thorough as possible.

FluidSynth development has been rather slow for a while and I have been out of the loop. I'd be happy to commit your patch though, once I'm confident there isn't any code depending on sane sample loop indexes.

Original comment by: elementgreen

[derselbst]

Hey Truth and Element,

Thanks for helping me out on this issue. I've learned a lot about the inner workings of fluisynth and soundfonts in general from this conversation.

I'm not super confident in my skills in this area to properly correct all the edge cases with these soundfonts, but I can provide you with more examples and cases and log files from soundfonts where things go wrong.

One thing I've been doing is running fluisynth with warnings for the 3 fowled loop cases and found out that I have many soundfonts have at least one sample which will fall into these cases. This won't be a big deal most of the times with fluidsynth and will pass unnoticed, but becomes a problem with Swami (where the loop values are loaded incorrectly and then saved incorrectly if the file is ever modified and saved to disk).

Anyway, I'll be following this thread and probably file some more bug reports about other specific cases where the loops sound sketchy.

Original comment by: lfzawacki

[derselbst]

Regarding the FinalFantasy SF provided here: for many samples it specifies the loopend after the end of the sample. From my understanding of the sfspec this is illegal. Anyway #184 changes the way fluidsynths mangles with incorrects looppoints and it accidentally also fixes playback of this soundfont.