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

Add ability to change sample rate in the settings menu #7719

Merged
merged 10 commits into from
Mar 25, 2025

Conversation

sakertooth
Copy link
Contributor

@sakertooth sakertooth commented Feb 17, 2025

A slider was added in the audio settings menu that allows users to change the sample rate. The slider only allows the choice of a few standard sample rates, those currently being 44100, 48000, 88200, 96000, and 192000.

Copy link
Member

@tresf tresf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are some issues with this PR.

  • The UI defaults to 8000 now
  • The spinbox encourages selecting arbitrary sample rates. I don't think that we want this.
  • ExportProjectDialog uses different values. Shouldn't these match?
    const auto samplerates = std::array{44100, 48000, 88200, 96000, 192000};

Also, I would love a small explanation about the use-case for this PR to justify values such as 8000 (this may be common sense or a very common value to some that I do not know about, so perhaps it would benefit this PR to explain this value in a few words?).

@sakertooth
Copy link
Contributor Author

@tresf, The reason for 8000 as a minimum was because it was the lowest value listed in the table here.

If it's a bad minimum it can be changed no problem.

I'll change the UI element to be a slider, I just have to figure out how to limit it to only a few defined values. The sample rates listed in other places should match.

@LostRobotMusic
Copy link
Contributor

LostRobotMusic commented Feb 18, 2025

Also, I would love a small explanation about the use-case for this PR to justify values such as 8000 (this may be common sense or a very common value to some that I do not know about, so perhaps it would benefit this PR to explain this value in a few words?).

The points Tresf is bringing up here are correct, and this PR would break a lot of things and encourage users to do things that aren't good.

Before going into the issues, the first thing to point out is that there is absolutely no quality advantage to changing the sample rate to arbitrary values. Changing the sample rate simply changes the maximum frequency the audio can support (Nyquist, which is half of the sample rate). The standard sample rate of 44100 Hz already has a Nyquist of 22050 Hz, far above the upper range of human hearing. This means, regardless of whether the audio is 44100 Hz, 47628 Hz, or 9000000000 Hz, it will sound absolutely identical.

If you go far below 44100 Hz, the audio won't even be able to store frequencies across the entire range of human hearing. For example, a sample rate of 8000 Hz (the current UI default) would only be able to store frequencies up to 4000 Hz. It would sound like you put an infinitely steep 4000 Hz lowpass filter on everything.

Here are some of the issues that would pop up from setting arbitrary sample rate values:

  1. A lot of audio processing algorithms are designed or optimized for specific samples rates, or at least a specific range, and will break or crash when pushed outside of them.

    For example, the goal of an antialiasing filter during downsampling is to remove all audio below the new Nyquist frequency, and to do this it must use a much lower cutoff frequency due to the filter's limited slope. For the highest quality, they'd want to get as close as they can to the upper end of human hearing, usually around 20,000 Hz. If the sample rate is so low that Nyquist ends up being pushed below this target frequency, there would usually be an instant crash the moment the filter attempts to calculate its coefficients, or at minimum, some very major and painful audio glitches (or infs and NaNs).

    It's also extremely common for VSTs and other audio software to have filters with pre-calculated coefficients for certain common sample rates, in which case with any other value they simply would not function properly or would crash.

  2. Most audio samples are supplied in either 44100 Hz or 48000 Hz. If the user works with a different sample rate, the audio will have to be resampled, causing aliasing or quality loss.

  3. Most websites don't support arbitrary sample rates due to compatibility concerns. Once the user uploads their audio in a non-standard sample rate, the site would have to resample it, which among other potential issues, would in most cases push many of the samples above 0 dBFS, adding clipping that previously was not there.

This is quite a lot of downside with no upside (especially point #1 which is the most important). For compatibility reasons, the user should be encouraged to only use standard sample rates. I recommend 44100, 48000, 96000, 192000, and 384000. If we still want to support arbitrary sample rates (almost nobody does, for the reasons I explained), it should be an advanced feature that the average user is heavily dissuaded from ever touching.

@sakertooth sakertooth marked this pull request as draft February 19, 2025 00:17
@sakertooth sakertooth marked this pull request as ready for review March 2, 2025 10:21
@tresf
Copy link
Member

tresf commented Mar 2, 2025

@sakertooth hard crash when the settings dialog is raised. Remove ~/.lmmsrc.xml and it won't open at all now. This is the crash log that was produced the first time: https://gist.github.com/tresf/5b604d71e1824bb5bdf22fe3e5230a46

@sakertooth
Copy link
Contributor Author

@sakertooth hard crash when the settings dialog is raised. Remove ~/.lmmsrc.xml and it won't open at all now. This is the crash log that was produced the first time: https://gist.github.com/tresf/5b604d71e1824bb5bdf22fe3e5230a46

Not too sure. Just pushed some new commits though, let me know if anything changes 👍

@tresf
Copy link
Member

tresf commented Mar 2, 2025

@sakertooth hard crash when the settings dialog is raised. Remove ~/.lmmsrc.xml and it won't open at all now. This is the crash log that was produced the first time: https://gist.github.com/tresf/5b604d71e1824bb5bdf22fe3e5230a46

Not too sure. Just pushed some new commits though, let me know if anything changes 👍

Still crashes. Here' the bt:

* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0xec67c7e00e430032)
    frame #0: 0x0000000101253068 QtWidgets`QWidget::show() + 28
QtWidgets`QWidget::show:
->  0x101253068 <+28>: ldr    x8, [x19, #0x28]
    0x10125306c <+32>: ldr    w1, [x8, #0xc]
    0x101253070 <+36>: ldr    x8, [x0]
    0x101253074 <+40>: ldr    x8, [x8, #0xa8]
Target 0: (lmms) stopped.

Here's the full bt: https://gist.github.com/tresf/699f188b8df8ad9902d65614737606a8

@sakertooth
Copy link
Contributor Author

Ah, I think I know why.

@sakertooth
Copy link
Contributor Author

I figured out why but in doing so I found a quite sinister bug in SetupDialog. Calling setSampleRate(SUPPORTED_SAMPLERATES[m_sampleRateSlider->value()]) in the constructor works, but setSampleRate(m_sampleRate) crashes the program. The same can be said with setBufferSize(m_bufferSizeSlider->value()) and setBufferSize(256). This is because when these calls happen, the code tries to show the restart warning label which hasn't been created yet, but something about calling QSlider::value on the sliders works for some reason. Really sinister/odd..

@tresf
Copy link
Member

tresf commented Mar 3, 2025

I figured out why but in doing so I found a quite sinister bug

The latest rounds of patches fixes the crash when the Setup Dialog is raised.

I tested a few non-44100 sample rates with SDL and PortAudio without any observable issues.

The only thing that stands out to me is that the export dialog should probably default to the same sample rate as the Setup Dialog for consistency purposes. This way if a plugin misbehaves on playback at certain sample rates, the user will be encouraged to use the same sample rate for export, theoretically resulting in a better more "consistent" experience, quoting @LostRobotMusic:

  • It's also extremely common for VSTs and other audio software to have filters with pre-calculated coefficients for certain common sample rates, in which case with any other value they simply would not function properly or would crash.

  • Most audio samples are supplied in either 44100 Hz or 48000 Hz. If the user works with a different sample rate, the audio will have to be resampled, causing aliasing or quality loss.

If impacting the export dialog in this way is out of scope for this PR, it may make sense as a follow-up PR.

Note

Functionally, approving in current form for macOS on above contingency.

  • Testing on other platforms should be done prior to merge.
  • Code review should be done prior to merge.

@sakertooth
Copy link
Contributor Author

The only thing that stands out to me is that the export dialog should probably default to the same sample rate as the Setup Dialog for consistency purposes.

I agree. I'll try to get this done soon.

@LostRobotMusic
Copy link
Contributor

LostRobotMusic commented Mar 3, 2025

This way if a plugin misbehaves on playback at certain sample rates, the user will be encouraged to use the same sample rate for export, theoretically resulting in a better more "consistent" experience

On top of this, even when working with plugins that do support all of these sample rates, any change in sample rate can (and will, in the large majority of cases) result in a drastic change in the sound.

For example, LMMS's Equalizer filter shapes have cramping near Nyquist, meaning a sample rate change would completely change what your filter shape even is. The amount of aliasing present in audio rate modulations and distortion plugins and such would also change. It's essential that the user is encouraged to export at the same sample rate they're working in, or we'll otherwise get flooded with bug reports of people asking why their export sounds completely different from their playback.

@sakertooth sakertooth added needs code review A functional code review is currently required for this PR needs testing This pull request needs more testing labels Mar 15, 2025
@sakertooth sakertooth merged commit 1d5f2c0 into LMMS:master Mar 25, 2025
10 checks passed
@sakertooth sakertooth deleted the change-sample-rate-settings branch March 25, 2025 20:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs code review A functional code review is currently required for this PR needs testing This pull request needs more testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants