Smoothly ramp gain changes.

[Fabian Zimmermann]

Hello,

detected the following problem, maybe somebody is able to help:

I get creaking sounds (sounds like dirty potentiometer) if I do the following:

* load a track
* kill high and mid
* change/move low-eq slowly ==> sound is creaking

It also happens:

* in simple ALSA-Mode (without Jack)
* different Audio-Interfaces (Hercules MK2, Terratec Phase26)
* happens in Windows, too
* even without any MIDI-Interfaces connected (simple mouse-usage)
* and also in "advanced-eq-mode"
* 1.9 and latest HEAD affected

would it be possible to fix this or implement http://jackeq.sourceforge.net/ - it's working like a charm without any noise.

Thanks,

Fabian

[Fabian Zimmermann (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?

[William Good (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.

[Fabian Zimmermann (fabian-zimmermann-z)]

Ah ok, but maybe you could re-use some lines of the code / find new ideas:

https://github.com/swh/lv2/

[shanxS (shanx-shashank)]

Okay, I think I have zeroed down the problem here (and hopefully have a solution as well).

Problem is:
While turning the knob, the gain is changing in big steps and hence the creaking sound.

Solution:
Lets “ramp” the gain so that the change is made gradually over several samples, rather than in big steps.

Trade-off:
More 'gradual' the change will result in slow response of filter but will avoid the creaking sound.
So an optimum 'step' size is required which will not reduce the creaking sound but will keep filter reasonably responsive.

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.

[RJ Skerry-Ryan (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.

[shanxS (shanx-shashank)]

Hey RJ!

> Are you able to reproduce and confirm that the smooth-fading fixes it?
Yes, on my Linux box (core i3, 4GB) I can reproduce this issue.

I can see the point you are trying to make with latency and ramp-ing.
This patch is latency based.

Here is the idea:
1. Latency effects the size of audio buffer engine needs to process between each callback of PortAudio. Latency is inversely proportional to buffer size, its simple relation between sampling freq. and time

2. Creaking is because the jumps in 'gain' are of high magnitude

3. If, the jump is 'ramped' to samples (opposed to a frame, like in my prev. patch) Then the ramping will be finished in 1 audio frame itself.

Potential problem:
I cant run Mixxx on latencies below 11 ms, so I couldnt test on lower latencies.

Coming to the point of implementing ramp-ing in as a feature of ControlObject:
I think, ramp- ing should be more a feature of functions in SampleUtil class.
OR a part of ::process() functions which do some kind of DSP.
That is because (maybe in future) someone might need exact value
of the CO (and not the ramp-ed one). This is just a thought.

[shanxS (shanx-shashank)]

A few corrections in my last comment:
1. Latency is inversely proportional to buffer size.
No, its directly proportional

2. The above patch focuses on resolving just the Low EQ creaking, ideally, same thing should be done for all 3 EQs to maintain code consistency.

[Daniel Schürmann (daschuer)]

Hi shanxS,

just looked at your patch.
It fixes the issue from #0 for me (Ubuntu Precise 32 bit)
I like the Idea to have it buffer size depended.

This will allow to have a smooth gain change on each sample across process calls.
This is useful for all rotary type controls.

The drawback is, that it puts an additional latency of one buffer size to the all slope like controls, like the kill switches.
Can we handle this with disabling fading ins some cases like jumping from and to zero?

[shanxS (shanx-shashank)]

Thanks for review Daniel!

Talking about jumping from and to zero, I can see 2 cases:
CASE 1: All EQs jump to 0
We can use memset(...) here, the way SampleUtil::copyWithGain(...) uses when `gain = 0'.

CASE 2: Not all EQs are 0
In this case the calling thread has to complete the loop (for non zero EQs). So I guess, we can let the other EQ (which are set to 0) ramp down to zero because this will help in keeping code more readable is not a big penalty in terms of performance.

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 ?

[RJ Skerry-Ryan (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.

[RJ Skerry-Ryan (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.

[shanxS (shanx-shashank)]

RJ,

Maybe I am missing your point here. Correct me if I am wrong.

Feasible solution will:
1. Have non-fixed step size.
2. Step size will be determined by buffer size requested by the callback.

My second patch has:
1. Non fixed step size
2. Step size is based on buffer size requested by callback
3. To ramp upto any value it will take just 1 frame (1 buffer quantum), so it'll work fine for both low and high latencies.

To me it looks like both of us are talking about same thing.
What am I getting wrong ?

[Daniel Schürmann (daschuer)]

I have made some tests with sine waves in different frequencies.
It turns out that all EQ channels are effected by this bug (with this exotic test case).

It also turns out that we can easily produce unwanted frequencies by sudden gain changes seeks and start or stops.
The good news is that all of this is not or hard to notice with real music files.

I am a little lost with the theory behind it maybe one expert can explain, please!
Here my theory for now:
A rectangular like gain change always produces a bunch of hear-able frequency, so thats not what we want.
So all gain changes must take place in a way without hear-able frequencies.
In theory below 20 Hz. So this results in a fade duration of 1/20 Hz / 4 = 0,0125 s
A triangular pulses has also hear-able frequencies. But I am not sure which ramp form is the theoretical ideal form. Maybe a sine quarter?

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.
I am not sure if it is required to have a special case for the kill switches. This might be required for buffer sizes above 20 ms.
But will the user make extensively use of this on those long latencies? I think not ....
According to the theory above we might will have problem with low latencies. But fixing this will introduce additional complexity ...

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)

[Daniel Schürmann (daschuer)]

All gain like controls are effected by this bug. (gain, volume, crossfader, balance, eq ...)
You can easily reproduce it with mixxx/src/test/soundFileFormats/1kHzR440HzLReference_32i96kStereo.wav.bz2

[Daniel Schürmann (daschuer)]

You cab hear it with real music when you kill mid and high channels and the move e.g. the crossfader.

[Daniel Schürmann (daschuer)]

Here Stream start and end waves with the 3 ms fade from EngineBuffer start.
Recorded with mix with a flat static EQ.

[RJ Skerry-Ryan (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:

Debug [Main]: Stat("EngineFilterBlock SampleUtil::copy3WithGain","count=13556,sum=1.13961e+07ns,average=840.665ns,min=507ns,max=47897ns,variance=381644ns^2,stddev=617.773ns")
Debug [Main]: Stat("EngineFilterBlock SampleUtil::rampCopy3WithGain","count=13556,sum=1.68933e+07ns,average=1246.19ns,min=664ns,max=33024ns,variance=377100ns^2,stddev=614.085ns")

If the measurement isn't being thrown off by external factors (e.g. caching), the difference is only about 400ns on average.
Based on this, I'm not too worried about the performance.

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 :)
* Try to avoid the C rules of defining variables at the start of a function and then using them later. Try to define the variable as close to its first use as possible.
* I see a new include for engine/engineasync.h. I assume that's not relevant to this bug?
* The vars in the constructor should be initialized to 1 (or better yet, filterpotLow->get()). As written the first callback ramps from 0 to 1.

Attached patch has these changes plus the setup I used for timing the change.

[RJ Skerry-Ryan (rryan)]

Oh also -- make sure to configure your text editor to use 4-spaces for indents instead of tabs.
http://mixxx.org/wiki/doku.php/coding_guidelines

[Owen Williams (ywwg)]

This should be fixed by https://github.com/mixxxdj/mixxx/pull/39

(That particular patch only fixes EQs, but the solution is generalizable)

[RJ Skerry-Ryan (rryan)]

Owen is done with a general solution and should merge it shortly:
https://github.com/mixxxdj/mixxx/pull/39

Thanks for working on this shanxS -- it helped inform the general solution!

[Owen Williams (ywwg)]

merged (a while ago)

[Swiftb0y (swiftb0y)]

Mixxx now uses GitHub for bug tracking. This bug has been migrated to:
https://github.com/mixxxdj/mixxx/issues/5992