extra notes heard with some sf3 sound fonts

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.

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.

Seems the long awaited libsndfile-1.0.26 had finally been released in November 2015.

no progress here?

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 > (end + 1) || loopstart >= loopend || loopstart <= 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…

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->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)…

5aea0ab5

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.

https://github.com/musescore/MuseScore/pull/7728

No more fixes for 3.x

It is part of https://github.com/musescore/MuseScore/pull/9000 though, so you could use the artifacts of that