Prevent loopstart mangling by fixup_sample if equal to sample start

[mawe42]

fixup_sample in defsloader attempts to repair certain broken sample loop points. But it also mangles the
pointers if sample start and loop start are equal, which leads to weird behavior observed in #171. After applying this fix, the pitch seems to be correct again.

Signed-off-by: Marcus Weseloh marcus@weseloh.cc

[derselbst]

I dont even understand why we checked for <=. The change you suggested seems to be absolutely comprehensive. And (as far as I understand the soundfont spec) it also seems to be legal.

I think the user should be informed in case of invalid loops, as they can cause some real trouble as we just saw. However as I just tested this is would be very spammy. Might only become a debug message... will think about it, have some tests and merge it later.

[mawe42]

Yes, having more debug messages if loop points are being changed by FS would be great. I get the impression that there are quite a few problems with weird soundfonts that could benefit if we could just tell people to switch on debug messages and send the output.

[derselbst]

Actually all messages of any level are printed out (regardless of the --verbose option passed to fluidsynth or not) except for debug messages which are only available if compiled with #define DEBUG. I'm ok with it. Dbg msg. should not contain any relevant information. The invalid loop message for example should be at FLUID_INFO level. So should info msg. only be printed out with the --verbose option? If yes, how to handle the case if fluidsynth is not started via its executable but as a lib? Yet another API call?? Here I consider flooding the user with a whole bunch of info messages once as the minor problem.

[mawe42]

Ah, good point. Yes, I agree that it should be FLUID_INFO. And it would be more in line with other programs to only print INFO if --verbose is specified.
When using the lib embedded into another program, you have the option of setting a custom logger, via the already available fluid_set_log_function. That way the host program can choose which levels to print and which to ignore.

[derselbst]

Decided to inform the user once with a summarizing FLUID_WARN msg. Sample specific messages are printed via FLUID_DBG. This should most useful as well as least annoying.

[mawe42]

Sounds good. Although the "extending loop to full sample" is not technically correct. The code tries to offset loopstart and loopend by 8 if the sample is long enough, otherwise by 1. I do wonder why the code does this. Why not simply set loopstart to sam->start and loopend to sam->end?

[derselbst]

The code tries to offset loopstart and loopend by 8 if the sample is long enough,

Well ok, I call that "full" sample, hoping that noone can actually hear the difference :)

I do wonder why the code does this.

Because Emu Systems thought this would be a good idea:

A minimum of eight valid data points are required to be present before the loop start and after the loop end. The eight data points (four on each side) surrounding the two equivalent loop points should also be
forced to be identical. By forcing the data to be identical, all interpolation algorithms are guaranteed to
properly reproduce an artifact-free loop.

[mawe42]

Ah, I see, thanks! But the description is a little bit unclear. Especially with regard to the "four on each side" part. Is it four data points on each side of the loop, or four on each side of the two loop points? I guess it's the former. Then fixup_sample should really offset by 4, not by 8, shouldn't it?

This might seem overly picky... but as we've seen in #171, even such a small offset can make a huge difference to perceived pitch if the sample is extremely short and only consists of a single wave form period.

[derselbst]

But the description is a little bit unclear.

It is indeed. I understand it as the latter one, so I guess the implementation is correct.

[mawe42]

Ahh, there are more details further down in the spec:

The values of dwStart, dwEnd, dwStartloop, and dwEndloop must all be within the range of the sample data field included in the SoundFont compatible bank or referenced in the sound ROM. Also, to allow a variety of hardware platforms to be able to reproduce the data, the samples have a minimum length of 48 data points, a minimum loop size of 32 data points and a minimum of 8 valid points prior to dwStartloop and after dwEndloop. Thus dwStart must be less than dwStartloop-7, dwStartloop must be less than dwEndloop-31, and dwEndloop must be less than dwEnd-7. If these constraints are not met, the sound may optionally not be played if the hardware cannot support artifact-free playback for the parameters given.

According to this, the samples in the soundfont in #171 are actually invalid. And the fix in this pull request, allowing sam->loopstart to be equal to sam->start, is against the spec. But I guess fixup_sample is all about supporting broken soundfonts in the first place and fix them so that SF can play them correctly. As we've seen, having start = loopstart (and I guess also end = loopend) is not a problem for SF, at least with the default interpolation method. Would probably be good to check if the other interpolation methods can also cope with this change.

[derselbst]

Investigating #149. It is so headache inducing.

And the fix in this pull request, allowing sam->loopstart to be equal to sam->start, is against the spec.

But previously it was also against spec because we didnt necessarily had these 8 padding points right?

But I guess fixup_sample is all about supporting broken soundfonts in the first place and fix them so that SF can play them correctly.

Yep, I'll have another disimprovement commit ready.

Would probably be good to check if the other interpolation methods can also cope with this change.

Agreed, though I cant imagine any problem this might cause...

[mawe42]

But previously it was also against spec because we didnt necessarily had these 8 padding points right?

Yes, of course. And I think fixup_sample is not about making samples to conform with the specs, but to take really broken samples and change them so that they can be played like they were probably intended to. And as FS doesn't seem to need the 8 point padding, maybe it's best to really only do the minimal amount of mangling. So only fix loop points that are outside of the sample and leave the rest as is?

[derselbst]

So only fix loop points that are outside of the sample and leave the rest as is?

And completely forgetting about

if ((sam->end - sam->start) >= 20)
{
    if(inv_start)
      sam->loopstart = sam->start + 8;

     if(inv_end)
        sam->loopend = sam->end - 8;
}

and only using what's now in the else branch? Ok, I would be fine with that. Esp. because padding only make sense if the surrounding data is identical, as pointed out by spec, to help interpolation algorithms. But we dont make sure this is the case. We dont even check for it. So yeah, I think let's just use the logic of the else branch.

[mawe42]

And completely forgetting about [...] and only using what's now in the else branch?

Yes, but maybe go even further than that. The +1 / -1 offset might not be necessary either. After all, the loops we saw in #171 had loopstart = sam->start and worked fine.

And considering the comment by Element in #149:

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. [...] 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.

Maybe just do that? Only clamp sam->loopstart to sam->start? And leave the rest alone?

[derselbst]

Maybe just do that? Only clamp sam->loopstart to sam->start? And leave the rest alone?

But I'm afraid if sam.loopend really exceeds sam.end we may get a buffer overrun somewhere.

[derselbst]

The +1 / -1 offset might not be necessary either.

Yes, but to me it's not clear what reservations element had in this case:

I had first thought to clamp these values to the start/end, but that results in issues at the instrument level, in this case.

[mawe42]

Looking at the code again, I think we should check sam->loopend against sdtachunk_size, just like sam->end. If I understand correctly, then sdtachunk_size is the actual length of the sample data, including the zero-padding data that the SF format specifies. So if we just make sure that start and loopstart are > 0, end and loopend are < sdtachunk_size, then there's no risk of buffer overruns.

[mawe42]

sam->end is should be the position AFTER the last sample data, pointing to the first zero-byte after valid sample data. So no need for the +1 here

[derselbst]

Digging in the code I agree. sam.end comes directly from the sdhr chunk which specifies it after valid sample data and we dont seem to correct it by -1. But then fluidsynths documentation is incorrect.

[derselbst]

Oh wait, fixup_sample() acutally corrects it by -1 later on. But talking about this particular line of code it still points behind sample data. Insane

[derselbst]

I think we should check sam->loopend against sdtachunk_size

But is loopend really allowed to point to the padded area, i.e. after the end of valid sample data? I'm feeling very uncomfortable doing so. if there is any garbage in that padded area we get undefined output.

[mawe42]

I must admit I don't like this whole "silently changing looping points, trying to make the best of it" thing at all. We don't even tell people that their font is broken. So when it sounds horrible, FluidSynth is to blame. If were just to reject the broken data and loudly complain about it, then maybe people would try to fix their fonts or font editors... But maybe that is just wishful thinking.

But is loopend really allowed to point to the padded area, i.e. after the end of valid sample data? I'm feeling very uncomfortable doing so. if there is any garbage in that padded area we get undefined output.

Well, if there is garbage and not the zero bytes as the spec requires, we'll get either horrible screeching noises... or pink or white noise in the best case. But that would only happen if the SoundFont editor that wrote the file is really broken. From what I've seen so far, most problems were just off-by-one errors.

This whole fixup_sample function is about making broken soundfonts that don't adhere to the spec play (mostly) without problems. I guess it comes down to what level of "brokenness" we want to cater for and at which point do we simply reject a sample.

For example, as I see it, the broken "Final Fantasy" font in #149 has wrong sample->end(!) pointers. They point to the last word of valid data, not to the first zero word in the padded area. The loopend pointer points to end + 1, so at the place where the end pointer should be. According to spec, loopend should be at least end - 7, but as we've learned, that is not really important for FS. So loading those samples without touching any of those broken pointers would make the font sound as intended by the designer. But if we check for loopend > end and adjust the loopend or reject this sample, that font won't work (properly) in FS. Even the 1 word offset of moving the loopend to sample end would make the Guitar sample sound weird, because the loop in that sample is so short. Same with the ocarina.

Sorry... lots of text... But if you ask me: I would rather have a loud screeching noise if a sample is broken than a fairly short or quiet clicking artefact or slightly out of tune sound.

[derselbst]

Ok, so you share elements opinion of mangling as little as necessary with loop points. I'm fine with that. Hopefully this commit takes everything into account what we've discussed so far.

[mawe42]

That looks great! Just two small comments about +1 / -1 offsets. Would be great to have another set of eyes on this, though. Preferably Element :-)

[mawe42]

Why the +1? Seems like an arbitrary offset...

[mawe42]

loopend has the same semantic as end, I think. so setting it to end-1 means that the loop reaches end-2 and then jumps to loopstart. I think it should be sam->loopend = sam->end here.

[derselbst]

@Element-Green @elementgreen This PR made some (hopefully useful) improvements to fixup_sample(). Could you tell us why you've chosen those +1 / -1 offsets. This would cause the loop to play from start+1 to end-2:

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

...and perhaps review the changes?

[derselbst]

Merging now. I want to glue it together with #183 and test. @elementgreen is informed and will hopefully comment on this when he has time.

[derselbst]

Stumbled over an issue. Soundfonts that dont use sample loops at all may set loopstart=loopend, triggering (sam->loopstart >= sam->loopend). Now, as I understand the spec it is always illegal to set loopstart=loopend:

In concept, the loop end point is never actually played during looping; instead the loop start point follows the point just prior to the loop end point.

and

the samples have a minimum length of 48 data points, a minimum loop size of 32 data points and
a minimum of 8 valid points prior to dwStartloop and after dwEndloop. Thus dwStart must be less than dwStartloop-7, dwStartloop must be less than dwEndloop-31, and dwEndloop must be less than dwEnd-7.

Meaning the logic we have in this PR is correct, even if the sample loop is ultimately not used, do you agree @mawe42?

[elementgreen]

Sorry for the delay in replying to this. Some of the more recent changes I did to the Swami code base involved issues around loops. The SoundFont loading logic is something I originally contributed a long while back, so it had some things in common with the FluidSynth code base.

Here is a link to the relevant commit to Swami:
https://sourceforge.net/p/swami/code/ci/1a6fc9692ab2711c7f1bbdd1bcd0a808d77491cb/

In summary:
The loop start and loop end are converted from absolute positions to offsets. So if the loop_start or loop_end are before the sample start position in the smpl chunk, they will be set to offset 0. In all other cases the loop points will be left as is, even if it is an invalid loop according to the spec. The loop check is then done at the instrument level. This is because, despite the loop being invalid at the sample level, the loop offset generators sometimes correct this, making it valid. If the loop was to get mangled at the sample level, then the instrument zone will not offset the loop properly, when it could have.

The check for a valid loop should therefore occur in FluidSynth when calculating instrument zones.

[derselbst]

So if the loop_start or loop_end are before the sample start position in the smpl chunk, they will be set to offset 0. In all other cases the loop points will be left as is, even if it is an invalid loop according to the spec.

Pretty much what this PR attempts. Additionally it assures that loopend is within the sdta chunk.

This is because, despite the loop being invalid at the sample level, the loop offset generators sometimes correct this, making it valid.

Looking at the spec this seems to be something that Emu didnt take into account.

The check for a valid loop should therefore occur in FluidSynth when calculating instrument zones.

Ok, for now I'm not feeling that familiar with the codebase to actually introduce such a change. Issue #188 created.

Anyway, thanks so far. I'll see if it makes sense to merge some of your changes into fixup_sample().