Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix missing support for lv2core#sampleRate (Fixes #5767) #5783

Merged
merged 3 commits into from
Nov 15, 2020

Conversation

JohannesLorenz
Copy link
Contributor

This multiplies port's min/max value with the processing sample rate
that is used for the plugin. This fixes damaged audio in GLAME
Butterworth High-/Lowpass from #5767.

This multiplies port's min/max value with the processing sample rate
that is used for the plugin. This fixes damaged audio in GLAME
Butterworth High-/Lowpass from LMMS#5767.
@JohannesLorenz
Copy link
Contributor Author

@zonkmachine I added you as reviewer, but I already checked GLAME Lowpass/Highpass are working. It's just for information.

@LmmsBot
Copy link

LmmsBot commented Nov 12, 2020

🤖 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

Windows

macOS

🤖
{"platform_name_to_artifacts": {"Linux": [{"artifact": {"title": {"title": "(AppImage)", "platform_name": "Linux"}, "link": {"link": "https://10471-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.8%2Bgb2facbf-linux-x86_64.AppImage"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/10471?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}], "Windows": [{"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://10470-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.8%2Bgb2facbfbb-mingw-win32.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/10470?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://10474-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.8%2Bgb2facbfbb-mingw-win64.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/10474?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}], "macOS": [{"artifact": {"title": {"title": "", "platform_name": "macOS"}, "link": {"link": "https://10472-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.8%2Bgb2facbfbb-mac10.13.dmg"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/10472?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}]}, "commit_sha": "c155144f636c98db03f44c0c4eba580d58e2498a"}

@PhysSong
Copy link
Member

Just asking, isn't the default value supposed to be multiplied by sample rate as well?

@JohannesLorenz
Copy link
Contributor Author

@PhysSong Good point. It shall not be multiplied, sampleRate is only for multiplying the bounds in order to be confirming to the Nyquist frequency, but having different default values with different sample rates is not wanted. I asked in the lv2 chat to be sure.

However, the GLAME filters got this wrong, too (they pre-divided the default value), so thanks for helping me opening swh/lv2#17:

 :port [
     a :InputPort, :ControlPort ;
     :name "Cutoff Frequency (Hz)" ;
     :index 0 ;
     :symbol "cutoff" ;
     :minimum 0.0001 ;
     :maximum 0.45 ;
     
     :default 0.112575 ;
     :portProperty pprops:logarithmic ;
     :portProperty :sampleRate ;
   ] ;

In this case, default is smaller than minimum * sampleRate. Should I write automatic code, that, like in this case, does multiply the default value with the samplerate? Or should we reject such plugins?

@PhysSong
Copy link
Member

It seems like LADSPA supports both sr-dependent and constant default values, but LV2 don't.
BTW, Have you looked into what other LV2 hosts do in such a case?

@JohannesLorenz
Copy link
Contributor Author

@PhysSong From what I can see in the source code, both Ardour and jalv only multiply min and max, and I can't see that they would do anything to fix pre-divided default values like in the GLAME filters. Also, opening the GLAME filters in ardour will bash the default value to the minimum, which means they don't have any special logic.

Proposal: If "sampleRate" is given and the default value is smaller than "minimum * sampleRate", then it should be multiplied with the sampleRate, too? Or maybe with a fixed sampleRate, like 44100?

@JohannesLorenz
Copy link
Contributor Author

No matter if we are going to add more commits, I'll merge this in 24 hours if no one is opposed. This is a critical fix.

@IanCaio
Copy link
Contributor

IanCaio commented Nov 14, 2020

@JohannesLorenz What about setting the default to m_min if it's out of range? Or you can first try to multiply it by the SampleRate and if the value is still invalid set it to the minimum.

@PhysSong
Copy link
Member

Audacity does multiply the default value by sample rate, but GLAME Butterworth filter is broken in Audacity, meaning that we can't use it as a reference.

What about setting the default to m_min if it's out of range?

I think clamping with warning is better, since some plugins may have default value larger than m_max. I think it's also better than blacklisting such plugins.

first try to multiply it by the SampleRate

We may open a new issue and keep the discussion there.

@JohannesLorenz
Copy link
Contributor Author

It's clamped to [min, max] now (this was also supported in devtalk, mostly because it exposes the issue to the user). I also added a warning to the command line if the default value is not in range.

Should be enough now. I'll merge after CI passes.

@JohannesLorenz JohannesLorenz merged commit 060d0dc into LMMS:master Nov 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants