Skip to content

Normalize reverb time to sample rate #838

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Normalize reverb time to sample rate #838

wants to merge 3 commits into from

Conversation

probonopd
Copy link
Owner

For consistent decay across different sample rates

Closes #563

For consistent decay across different sample rates
Closes #563
Copy link

Build for testing:
MiniDexed_2025-04-18-28d7802
Use at your own risk.

@probonopd
Copy link
Owner Author

@Banana71 if you have some time to test this, I'd appreciate it. Thanks!

@Banana71
Copy link

Thanks @probonopd,

Is it correct that the basis for the calculation is 44.1 kHz?
Then the reverb tails will be a bit longer in the future, since in the past we always worked with 48 kHz. I might adjust the reverb times in the performances.

@probonopd
Copy link
Owner Author

Excellent question. How could we know for sure? I can change the calculation to 48 kHz if you think that is the correct base.

@Banana71
Copy link

Yes,
Test successful at 48,000 Hz, the reverb now sounds longer, as expected.
What would be correct?
With 48,000 Hz as the basis for calculation, the performance would sound unchanged.

@probonopd
Copy link
Owner Author

Thank you @Banana71. Please test my latest change and then let me know if you think this is ready for being merged. Thanks!

Copy link

Build for testing:
MiniDexed_2025-04-18-b15ffad
Use at your own risk.

@probonopd
Copy link
Owner Author

This build should hopefully work as expected?

@probonopd
Copy link
Owner Author

Does not work as expected: At sample rates < 48000, the reverb time gets shorter. Now the behavior is exactly the opposite.
At a sample rate of 96000, miniDexed crashes.

Copy link

Build for testing:
MiniDexed_2025-04-20-8116434
Use at your own risk.

@probonopd
Copy link
Owner Author

Didn't have a chance to test this yet, but if you have some extra time, maybe you'll want to give this a try?

@Banana71
Copy link

This doesn't work yet.
Higher sample rates result in shorter reverb times.
Lower sample rates than 48000 result in longer reverb times.
Peter

@probonopd
Copy link
Owner Author

Running out of ideas. https://github.com/probonopd/MiniDexed/pull/838/files seems simple enough; there must be more to it than meets the eye.

@probonopd
Copy link
Owner Author

@diyelectromusic do you have any idea?

@probonopd probonopd added the help wanted Extra attention is needed label Apr 22, 2025
@diyelectromusic
Copy link
Collaborator

@diyelectromusic do you have any idea?

Ok, here are a few notes - I have no idea what is relevant yet or not, but anyway...

So reverb is set using reverb->size(val/99.0) from a UI parameter to a ratio between 0.0 and 1.0 here: https://github.com/probonopd/MiniDexed/blob/main/src/minidexed.cpp#L1002

This is used to set rv_time_k the coefficient for reverb, which is the initial value for rv_time in the doReverb() function. However rv_time_k is limited to a value between 0.2 and rv_time_k_max which appears to be 0.95. There is also an initial attenuation value that is set...

rv_time is used to update the values in the DSP with a scaling factor given by rv_time_scaler which is set according to the lodamp setting parameter https://github.com/probonopd/MiniDexed/blob/main/src/effect_platervbstereo.h#L84 which is set from the UI reverb settings (again 0 to 99 range generating a 0.0 to 1.0 parameter).

I can't follow all the DSP code involving "acc", but I think that a higher rv_time_k initial value means that rv_time is higher which possibly means that the reverb will take longer... as it is multiplying by a higher fraction each time?

Anyway, I think the sample rate calculation has to come in, in the size() function where all the checking is carried out - or actually, maybe the right way to do it is to adjust rv_time_scaler...? But I suspect the maximum will always have to be 0.95 regardless of the sample rate, so the length seems always to depend on the sample rate to me...

So, back to the change then... using the line:

 rv_time = rv_time_k * (48000.0f / m_samplerate);

Means that sample rates less then 48000 will make rv_time_k greater and possible even greater than 1.0 which presumably won't be good. Sample rates above 48000 will make rv_time smaller and then when it is used later it will be smaller again due to rv_time_scaler I think?

So I don't know if it is actually possible to scale with sample rate as the reverb factors are 0-99 mapped onto 0.0 - 1.0 regardless of how often the samples are calculated and the limits of 0.2 to 0.95 were presumably set for a reason?

Sorry, I'm not seeing an easy way to do this...

Kevin

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reverb time depends on the sample rate
3 participants