Fix PeakController for short attacks

[rdrpenguin04]

Long story short, current is on top, fixed is on the bottom:

And in case you're worried about artifacts (similar to the original, before the current version), there aren't any:

The file I used to test is uploaded to Discord. Basically, it shows activating a peak controller with 0 attack and 0 decay, then a peak controller with high (0.95-ish?) attack and high decay. As shown, the current version does not execute a fast attack; it takes 34 milliseconds to transition from 0 dbFS to -30 dbFS, 68 milliseconds to get to -60 dbFS, and in that time the transient the peak controller is supposed to be following might have ended in the first 8 milliseconds.

PeakController presently having such a long minimum tail is preventing one of my friends from updating to the newest LMMS because it breaks all of his sidechains, and there is no other way to replicate the sidechain effect. This will fix it.

[sakertooth]

@LostRobotMusic would probably be the best person we have to review this, hopefully they can take a look.

[LostRobotMusic]

PeakController presently having such a long minimum tail is preventing one of my friends from updating to the newest LMMS because it breaks all of his sidechains

This makes it sound like there was a major backwards compatibility break somewhere. Can somebody link to the PR that did this?

[rdrpenguin04]

The PR that broke this was #7566 . I've talked with cyberrumor about the PR some.

[tresf]

major backwards compatibility break

Agreed however I think the severity is going to depend on how the sidechain is being used. According to this comment #7383 (comment) I'm unsure what the baseline for comparison should be.

I tried to dogfood this but sadly none of my own projects seem directly affected by this bug. I also don't understand the code well enough to make any constructive criticisms.

As shown, the current version does not execute a fast attack; it takes 34 milliseconds to transition from 0 dbFS to -30 dbFS, 68 milliseconds to get to -60 dbFS, and in that time the transient the peak controller is supposed to be following might have ended in the first 8 milliseconds.

This seems reason alone to merge this.

The file I used to test is uploaded to Discord.

As we discussed in #dev-casual, this certainly helps visualize the issue, but it makes it a bit harder to hear it, which is what I assume drove your friend to report it.

I'd like to be better about getting these trivial bugs merged quickly, especially when they're easy enough to revert.

[tresf]

... as much as it pains me to preserve bad formatting, the rest of the file is rampant with it, so there's no need to touch more lines than we have to... (there's some deleted lines you could add back too, but GitHub doesn't allow code suggestions on deleted lines)

Suggested change

	PeakController::PeakController(Model * _parent,
	PeakControllerEffect * _peak_effect) :
	Controller( ControllerType::Peak, _parent, tr("Peak Controller")),
	m_peakEffect(_peak_effect),
	m_currentSample(0.0f)
	PeakController::PeakController( Model * _parent,
	PeakControllerEffect * _peak_effect ) :
	Controller( ControllerType::Peak, _parent, tr( "Peak Controller" ) ),
	m_peakEffect( _peak_effect ),
	m_currentSample( 0.0f )

[rdrpenguin04]

I mean, wouldn't it be better to fix more formatting than less?

[tresf]

I don't understand this question... What part of PeakController.cpp do you plan on "fixing more" of?

[tresf]

Suggested change

	setSampleExact(true);
	if(m_peakEffect)
	setSampleExact( true );
	if( m_peakEffect )

[tresf]

Suggested change

	m_peakEffect->effectChain()->removeEffect(m_peakEffect);
	m_peakEffect->effectChain()->removeEffect( m_peakEffect );

[tresf]

Suggested change

	m_valueBuffer.fill(m_currentSample);
	m_valueBuffer.fill( m_currentSample );

[tresf]

Suggested change

	m_valueBuffer.fill(0);
	m_valueBuffer.fill( 0 );

[LostRobotMusic]

I unfortunately can't approve this PR in its current state. LMMS's old Peak Controller behavior generally worked but had a bug, so #7566 fixed that bug and added several more bugs in the process, and now this PR (#7868) fixes one of that PR's bugs and adds even more bugs in the process. We gotta slow down and take some measurements.

I have my buffer size set to 1024 frames for all tests (which LMMS should run in groups of 256 frames internally) and a tempo of 180 BPM. I only used sample-exact effects in the test, and I directly recorded my audio output instead of exporting.
peak-controller-test.zip

Here's my old LMMS build from a few years ago, first with an attack/release value of 0, then somewhere around 0.99:

I had to cherry-pick the worst example, but as can be seen, there's a bug that causes an occasional discontinuity to occur, which is quite bad. I assume this is what #7566 was attempting to fix. However, that PR wound up causing several issues on its own which should have been caught in testing (using attack/release values of 0 first and 0.7 second):

The first issue, which is immediately visible, is that for some reason, an attack/release of exactly 0 has an extremely long attack/release time, which is extremely bad. The second issue (which is more easily visible upon zooming in, sorry) is that when the attack/release values are higher, the peak controller signal becomes completely filled with slope discontinuities that result in quite an odd shape. All of this is a major regression from the original behavior.

This PR (#7868) attempts to fix the first issue, but further introduces issues of its own which are self-explanatory:

I'm unfortunately too busy to look into what the situation was with #7566 right now, but unless these issues are caused by some simple fixable bug in that PR's changes, I recommend reverting it entirely.

[cyberrumor]

I believe the stair step artifacts that we can see in the images above are what is causing the zipper noises, since they're present before but not after #7566 is applied. I haven't tested with this PR yet, but I can look at it in the morning.

I suspect the stair step artifacts are caused by the RMS being set per-sample, so whenever the sample changes, the RMS jumps and the peak controller uses this value for its signal for the duration of the sample. I haven't looked at the code recently or proved this, so it could be unfounded. But if true, we could eliminate stairstep artifacts as well as latency by implementing a multi-sample sliding window for RMS calculation. We could seek to prove this by measuring the size of each stair step, changing the sample rate, then expecting the size of the stair steps to have changed by the same amount.

#7566 attempts to work around the issue via lerp/eerp, which is what introduces the delay (the soft rounded edges instead of squares).

I would prefer if further changes to this file serve to approach the goal of sliding multi-sample window for RMS calculation, assuming my suspicions are correct. If reverting #7566 is necessary in order to achieve that more easily, then we should do it. Would that also make it so this patch isn't required?

Perhaps my patch should be reverted anyway since it introduces the latency regression, which is basically fundamental to the way I was trying to work around the problem. We'll have audible zipper artifacts upon revert until we implement a full solution.

I can try to prove my theory and then attempt the multi-sample sliding window thing at some point, but I'm not very experienced with cpp, so it might be beyond me.

[cyberrumor]

I put up a revert here: #7871

[rdrpenguin04]

I'll open a new PR with attempts to fix the root issue