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
Smoothly ramp gain changes. #5992
Comments
Commented by: fabian-zimmermann-z Looks like every knop which changes the loudness would created noise/creaking. Tried PRE/MAIN, HEAD_VOL, BALANCE and even Fader.. any suggestions? |
Commented by: bkgood I've noticed this with EQs and gains before, not sure that I've heard it with line faders. I doubt it's a problem with your system specifically. I wouldn't be surprised if this has a duplicate somewhere but I haven't found anything. We're unable to use jackEQ as we still need to provide EQs for Windows, CoreAudio, ALSA and OSS users. |
Commented by: fabian-zimmermann-z Ah ok, but maybe you could re-use some lines of the code / find new ideas: |
Commented by: shanx-shashank Okay, I think I have zeroed down the problem here (and hopefully have a solution as well). Problem is: Solution: Trade-off: I have attached a patch and chosen step size 0.05. This sounds good to me. Give it a try. If you dont like it try changing 'm_step' variable in ctor of the class. |
Commented by: rryan Thanks for working on this shanxS. Are you able to reproduce and confirm that the smooth-fading fixes it? I could never reproduce this. It's true that a lot of controls in Mixxx could benefit from interpolating their values. We have similar code in the EnginePregain to smooth-fade the replay-gain. I think that this is common enough that it should be built in as a feature of ControlObject in the future. As for the patch, I think the smooth-fading should be latency-independent. Adding smooth-fading introduces lag into how long it takes for Mixxx to respond to user input so making the lag latency-dependent makes the experience worse for users with high latency. For example, with a step size of 0.01 and a latency of 100ms, if you turn the low EQ from half on to full on (control value delta of 0.5) then it will take 50 steps to go from 0.5 to 1.0. At a latency of 100ms, 50 steps will happen in 5 seconds. So it will take Mixxx 5 whole seconds to fully respond to the user input. Instead, I think either the step size should be a function of the latency or it should be time-based like in EnginePregain. |
Commented by: shanx-shashank Hey RJ!
I can see the point you are trying to make with latency and ramp-ing. Here is the idea:
Potential problem: Coming to the point of implementing ramp-ing in as a feature of ControlObject: |
Commented by: shanx-shashank A few corrections in my last comment:
|
Commented by: daschuer Hi shanxS, just looked at your patch. This will allow to have a smooth gain change on each sample across process calls. The drawback is, that it puts an additional latency of one buffer size to the all slope like controls, like the kill switches. |
Commented by: shanx-shashank Thanks for review Daniel! Talking about jumping from and to zero, I can see 2 cases: CASE 2: Not all EQs are 0 Also, I should make this change for other 2 EQs (even though we dont have issue there ?) as well for the sake of code consistency. Shall I go ahead with above changes ? |
Commented by: rryan I understand what you mean about making the ramping sample-based instead of time-based, but what I meant by my earlier comment is that it is a tradeoff. The trade-off is between ramping the parameter and the added latency that the ramping introduces. For a low-latency user, the ramping will work as intended if it is time-based. For a high-latency user, the ramping might not work at all. I think this is the right trade-off since the alternative is that severe latency is introduced into control changes for high latency users (See my previous example about 5s latency for a control change to be fully processed). I think in the end we could solve this for high-latency users by using a small PA user-buffer size combined with a fixed step-size per buffer-size as your original patch uses. Both Daniel and I have looked at using smaller user buffers before and I was thinking we should try this in 1.12. This would mean that even if the host-buffer size is 100ms worth of audio, we would process it in tiny (2ms?) chunks. This way the parameter changes could be smoothed even at high latencies. |
Commented by: rryan To clarify, the solution I proposed above would call for a non-fixed step-size. The step-size would have to be based on time but the passage of time is dictated by the buffer size requested of the callback. This is a fib in terms of wall-clock time since we will process that time non-deterministically but it is sound in terms of the timeline of the samples played out the soundcard. |
Commented by: shanx-shashank RJ, Maybe I am missing your point here. Correct me if I am wrong. Feasible solution will:
My second patch has:
To me it looks like both of us are talking about same thing. |
Commented by: daschuer I have made some tests with sine waves in different frequencies. It also turns out that we can easily produce unwanted frequencies by sudden gain changes seeks and start or stops. I am a little lost with the theory behind it maybe one expert can explain, please! In EngineBuffer a 3,3 ms triangular ramp is used to start a new stream. This produces hear-able frequencies with my test files, but they are not noticeable with real music. When the stream ends, a random noise 3.3 ms triangular ramp is added. This is hear-able with my test files and not with real music. If I set the ramp to 12 ms I can still here additional frequencies. Finally with 50 ms ramp all additional frequencies are gone but the ramp itself becomes heatable. The noise at the end is always hear-able. So its always a question of "Clean sound" vs. "Low Latency" For me, we should add the fix to all three channels. The current patch is a good fix for the original problem, so it fine for me to commit and work on the whole thing later (if required) |
Commented by: daschuer All gain like controls are effected by this bug. (gain, volume, crossfader, balance, eq ...) |
Commented by: daschuer You cab hear it with real music when you kill mid and high channels and the move e.g. the crossfader. |
Commented by: daschuer Here Stream start and end waves with the 3 ms fade from EngineBuffer start. |
Commented by: daschuer |
Commented by: rryan Sorry shanxS, I didn't see your v2 patch. Ramping at the sample granularity rather than buffer granularity seems promising. I was worried about the performance implications of putting the ramping in SampleUtil. If we apply a ramp to each individual sample then that introduces a loop dependence (a value of is changing based on the iterations through the loop) and it makes vectorizing the loop harder since every sample gets multiplied by a different value. If the compiler is good enough, it can optimize away the loop dependence as long as it can determine through static analysis that 'a' is a simple function of # of loop iterations. I'm less optimistic about the compiler being able to vectorize the loop but I will look at a decompilation to check. I added ramping of the other 2 channels to your patch and measured the time difference. Here is what I got:
If the measurement isn't being thrown off by external factors (e.g. caching), the difference is only about 400ns on average. Another issue I have is that the v2 patch is not based on stream time but rather it just divides the total distance to change by the buffer size so that the entire ramp is done in one callback. This works for larger buffer sizes since it makes the change more gradual but for low buffer sizes the if the parameter change is large enough it could cause the jump to be just as bad as if we were not ramping (i.e. the gain difference between two samples could be above the threshold that would generate bad frequencies / pops). I wonder if there is an easy way to characterize what the maximum gain change is between samples that would not produce distortion? I have some minor nits about the patch :)
Attached patch has these changes plus the setup I used for timing the change. |
Commented by: rryan Oh also -- make sure to configure your text editor to use 4-spaces for indents instead of tabs. |
Commented by: ywwg merged (a while ago) |
Issue closed with status Fix Released. |
Reported by: fabian-zimmermann-z
Date: 2011-09-19T18:00:24Z
Status: Fix Released
Importance: High
Launchpad Issue: lp854082
Tags: eq
Attachments: lowEqCreak.patch, loEqCreak_patch2, StreamStart.png, StreamEnd.png, ramp3.patch
Hello,
detected the following problem, maybe somebody is able to help:
I get creaking sounds (sounds like dirty potentiometer) if I do the following:
It also happens:
would it be possible to fix this or implement http://jackeq.sourceforge.net/ - it's working like a charm without any noise.
Thanks,
Fabian
The text was updated successfully, but these errors were encountered: