extra notes heard with some sf3 sound fonts

• Dec 1, 2015 - 04:28
Type
Functional
Frequency
Few
Severity
S4 - Minor
Reproducibility
Randomly
Status
PR created
Regression
No
Workaround
No
Project
Tags

I've converted a sound font from sf2 to sf3 using sfconvert (https://github.com/wschweer/sftools/). When I play a short composition using the converted font, extra notes are heard. This doesn't occur with the original sf2.

Here are links to the fonts, since they can't be attached in the bug tracker;

http://moffatt.id.au/~hamish/sfconvert/rs_piano.sf2 - the original font
http://moffatt.id.au/~hamish/sfconvert/rs_piano.sf3 - the font converted with sfconvert -z

and here is the test composition:

http://moffatt.id.au/~hamish/sfconvert/bach.mid - my test file


Comments

Also I don't hear the extra notes with the included FluidR3Mono_GM.sf3, but I do hear it with other sf2->sf3 conversions I've done myself using sfconvert.

There is a known bug for the decoding of the samples inside the sf3 file, present in libsndfile 1.0.25 (which is used by MuseScore to decompress the Ogg Vorbis samples). See here:
https://musescore.org/en/node/22086
and here:
https://musescore.org/en/node/21814

The soundfont shipped with MuseScore uses a hardcoded renormalization factor in the sftool, which is set here:
https://github.com/wschweer/sftools/blob/master/sfont.cpp#L1234
you can try to increase this factor and see if the problem disappears.
Unfortunately libsdnfile 1.0.26, which solves the renormalization/overflow problem has not been released yet. That library will solve the problem and the renormalization factor correction will no more be needed.

Are you sure that this is the same problem? I don't hear pops and clicks, I hear very nicely played notes at unexpected times.

I discussed this with Fabian Greffrath who made a patch for vanilla Fluidsynth to support sf3 (https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=740710). He discovered that setting loopend = loopstart = 0 in the decoder/decompression code worked around the problem. It's probably not the correct fix though.

Sorry, I wrote before actually testing the files attached.

The problem is that in the original sf2 file the loopend is exactly equal to the end of the sample.
When sf files are imported into MuseScore, end is decreased by one here:
https://github.com/musescore/MuseScore/blob/master/fluid/sfont.cpp#L654
end -= (start + 1);
so that when the sample is actually played, sample->end is smaller by one with respect to sample->loopend in this sf2 case. This is taken care further on in the code and everything works as expected.

In the case of the sf3 file, however, loopend is coded in the sf3 file already as the relative loopend (i.e. with start=0). Therefore, when the sample is loaded, loopend in this case is larger than end (by 1) and therefore the loopstart and loopend are changed in the if-clause here:
https://github.com/musescore/MuseScore/blob/master/fluid/sfont3.cpp#L33
if (loopend > end ||loopstart >= loopend || loopstart <= start) {
this results in wrong boundaries for the sample loop in this case, since loopstart is moved to smaller numbers.
The equivalent part of the code for the sf2 is here:
https://github.com/musescore/MuseScore/blob/master/fluid/sfont.cpp#L1627
which is not hit in this sf2, since loopend is exactly equal to end.

Now, to solve this bug we can do one of these:
-1- change https://github.com/musescore/MuseScore/blob/master/fluid/sfont3.cpp#L33
so that the check loopend > end is not performed;
-2- change https://github.com/musescore/MuseScore/blob/master/fluid/sfont3.cpp#L33
so that the check reads loopend > (end + 1);
-3- something else?

Personally, I would suggest point -2- above so that the behavior for the sf3 is similar to the one for the sf2 case.

Reported version 2.1  
Regression No
Workaround No

I’m currently investigating a crash on a soundfont, and this may play into that…

Since end = frames - 1; the calculations seem a little off anyway.

If we use if (loopend &gt; (end + 1) || loopstart &gt;= loopend || loopstart &lt;= start) { then we probably also must do this:

            if ((end - start + 1) >= 20) {
                  loopstart = start + 8;
                  loopend = end - 7;
                  }
            else { // loop is fowled, sample is tiny (can't pad 8 samples)
                  loopstart = start + 1;
                  loopend = end;    
                  }

Please, anyone with clues about this, have a look…

In reply to by mirabilos

Right, if (p->loopend > p->end || p->loopstart >= p->loopend || p->loopstart <= p->start) { in sfont.cpp:SFont::load_shdr uses SoundFont spec semantics for p-&gt;end (namely, one past the last sample); if (loopend > end ||loopstart >= loopend || loopstart <= start) { in sfont3.cpp:Sample::decompressOggVorbis uses MuseScore-internal semantics (end points to the last sample), so they are all off.

I’m finding a number of connected bugs (e.g. decompression failure in SF3 doesn’t mark Sample as invalid) and fixing them all together.

I’m factoring out the end -= 1; to just before optimize() is called, so it’s done in the same location for SF2 and SF3.

Ah, wonderfully, with this we can revert commit 5aea0ab5b50b9037ba8ed9dd100dcf06396a4c9d as well, as I found the root causes (yes, plural)…

Hmm… if I see (at least in MuseScore 2, for which I’m also providing the fix) this right, then the soundfont headers are read when the soundfont is loaded, but the actual sample data is only loaded (and Vorbis-decompressed for SF3) once the instrument is assigned… but the file is opened and closed once per sample, not once per “loading action”, there, so there’s potential for quite the speedup…

Edit: no, 3.x branch also does this; Preset::loadSamples() could probably open the file once, then pass the opened handle to Sample::load() for each. But that’s outside the scope of these bugfixes.