LB302: Prevent cutoff frequency modification when the sampling rate changes.

[thmueller64]

Fixes issue #5615. It turned out that the problem appears when the 24dB Filter is switched on and Env Mod > 0.

Findings:
The term where I made the change gets normalized a few lines down by 1/(Engine::mixer()->processingSampleRate()/2), so the term is most likely the cutoff frequency and should therefore not depend on the sampling rate.
Yet, the calculation of the cutoff frequency involves a term with the current sampling rate times the value of the knob k: (k)*(w)*(k)*(Engine::mixer()->processingSampleRate()/2.f). The w should be set proportional to 1/Engine::mixer()->processingSampleRate() by the base class, but the author of the code explicitly set his own static values along with the comment "// do not call base class". When the sampling rate changes during the export, this term also changes and consequently the cutoff frequency.
A simple fix is to calculate the cutoff frequency with 44.1kHz fixed and maybe also check if the cutoff is above the nyquist frequency which might happen at a lower samplerate than 44.1kHz. Cutoffs up to 22.05kHz are sufficient in all cases.

[LmmsBot]

🤖 Hey, I'm @LmmsBot from github.com/lmms/bot and I made downloads for this pull request, click me to make them magically appear! 🎩

Linux

• Linux (AppImage): lmms-1.2.2.686-linux-x86_64.AppImage (build link)

Windows

• Windows 64-bit: lmms-1.2.2.686-mingw-win64.exe (build link)
• Windows 32-bit: lmms-1.2.2.686-mingw-win32.exe (build link)
• Windows 32-bit: lmms-1.2.2-msvc2017-win32.exe (build link)
• Windows 64-bit: lmms-1.2.2-msvc2017-win64.exe (build link)

macOS

• macOS : lmms-1.2.2.686-mac10.13.dmg (build link)

🤖

{"platform_name_to_artifacts": {"Linux": [{"artifact": {"title": {"title": "(AppImage)", "platform_name": "Linux"}, "link": {"link": "https://8123-15778896-gh.circle-artifacts.com/0/lmms-1.2.2.686-linux-x86_64.AppImage"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/8123?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}], "Windows": [{"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://8125-15778896-gh.circle-artifacts.com/0/lmms-1.2.2.686-mingw-win64.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/8125?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://8124-15778896-gh.circle-artifacts.com/0/lmms-1.2.2.686-mingw-win32.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/8124?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/7n5p9pdpriwftoe4/artifacts/build/lmms-1.2.2-msvc2017-win32.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/34583286"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/km2m4lr2timeq722/artifacts/build/lmms-1.2.2-msvc2017-win64.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/34583286"}], "macOS": [{"artifact": {"title": {"title": "", "platform_name": "macOS"}, "link": {"link": "https://8122-15778896-gh.circle-artifacts.com/0/lmms-1.2.2.686-mac10.13.dmg"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/8122?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}]}, "commit_sha": "048a3084d05985e7f7217ed0c0404a3e8f04ed1d"}

[SecondFlight]

If this is and really should be a constant 44100.0f, could it be added as a named constant? It looks like this bug was introduced by mistake in 910daa9 and I can see it happening again, as someone seeing 44100.0f is likely to think it should be replaced by a dynamic call.

In fact, that commit replaces the original author's LB_HZ constant. The line affected by this PR is only one of a few lines where the constant was switched with a call to engine::getMixer()->getSampleRate(), which has since been superseded by engine::mixer()->processingSampleRate().

[SecondFlight]

See comment above.

[thmueller64]

Yes, a named constant would be better.
It most likely should be a constant. What happens when the sampling rate changes is that the filters cutoff frequency changes for the same settings otherwise, compare the figures below. The w is constant btw, see line 212.

[SecondFlight]

Thanks for the explanation there. It seems really odd at face value, but DSP still doesn't come naturally to me, especially filter coefficients and such. I think it's fine as long as it's easy to see that 44100.0f is intentional.

[thmueller64]

That is just the kfco in the code, for which I think it is at least approximately the cutoff frequency at the start of a note, plotted against the knob position. I just noticed that if the plots above are valid then only for short notes. The vcf_c0 in w = vcf_e0 + vcf_c0 is set to 1 at the beginning of a note (for which I generated the plots) and then gets successively multiplied with the decay vcf_c0 *= fs->envdecay in lb302filter::envRecalc(). After a few moments w is nearly zero and the kfco reduces to 50.f + (k)*((2300.f-1600.f*(fs->envmod)) so the Env Mod knob is decreasing the cutoff now instead of increasing it like at the beginning of a note. You can hear this very well when you adjust the Env Mod knob for a pattern of many short notes versus a single long note.

Anyways, this does not change my opinion that there shouldn't be a dependency of kfco on the dynamic sampling rate, which changes the filters behavior completely for the same knob positions. What do you think?

[SecondFlight]

Looks good to me. I'll merge this in a day or two if there are no objections.