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.

Sign up for GitHub

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

Jump to bottom

Peak Controller Attack/Decay isn't a smooth curve #7383

Closed
1 task done
0xf0xx0 opened this issue Jul 16, 2024 · 2 comments · Fixed by #7566
Closed
1 task done

Peak Controller Attack/Decay isn't a smooth curve #7383

0xf0xx0 opened this issue Jul 16, 2024 · 2 comments · Fixed by #7566
Labels
bug

Comments

@0xf0xx0
Copy link
Contributor

0xf0xx0 commented Jul 16, 2024

System Information

EndeavourOS

LMMS Version(s)

12632e6

Most Recent Working Version

No response

Bug Summary

This is the issue encountered in #1281 and #2534, I'm avoiding necroing them. As Unfa stated, the Atk/Dcy knobs seem to act more like buttons. Turning them both to 100% removes the clicking, at the cost of damping the audio being controlled. It's more noticable when the audio being used to control the controller is chaotic.
A reproducible project is attached.

Expected Behaviour

The knobs should have an effect at all values.

Steps To Reproduce

  1. Load in two audio tracks; one to control the peak controller, and one to be controlled
  2. put the peak controller on the furst track
  3. bind the peak controller to the second tracks volume

Logs

No response

Screenshots / Minimum Reproducible Project

broken-peak-controller.zip

Please search the issue tracker for existing bug reports before submitting your own.

  • I have searched all existing issues and confirmed that this is not a duplicate.
@0xf0xx0 0xf0xx0 added the bug label Jul 16, 2024
@michaelgregorius
Copy link
Contributor

Visual debugging

If I understand correctly what ultimately controls the volume levels is computed in PeakController::updateValueBuffer where the member m_valueBuffer of the parent class Controller is updated.

For debugging purposes the values of the buffer should be dumped into a raw audio file that can be opened and inspected with an audio editor, e.g. Audacity. I assume that it will look very crude and potentially with high frequency changes.

Does anybody know if there already exists a facility in LMMS for debugging purposes like these?

Observations in the code

The following things caught my eye when checking the code:

  • The RMS value of the side chain signal is only computed once per whole frame in PeakControllerEffect::processAudioBuffer. This means that the reaction to changes in the audio signal is very slow, i.e. the "direction" can only change once per frame. Another consequence is that the result will be different if the number of frames is changed or differs for some reason. The result of the method is an updated value of m_lastSample which is evaluated in PeakController::updateValueBuffer.
  • It's not really clear to me if PeakController::updateValueBuffer is called continuously because it seems to be called in the constructor of Controller (so only once), in float Controller::value( int offset ) and Controller::valueBuffer. This whole design seems quite unreliable. If updateValueBuffer is not called continuously then it wouldn't be a surprise that this leads to dropouts.

The "ideal" fix

An "ideal" fix for all these problems would be to implement flexible routing and sidechaining as a first class citizen in LMMS. See for example how it is done in Bitwig where using sidechain effects is straight forward and a breeze. If an effect has a sidechain input it will offer you a drop down list with all potential places in the signal chain/mixer that might be interesting to insert. This would obviously be a large undertaking though.

@michaelgregorius
Copy link
Contributor

michaelgregorius commented Jul 20, 2024 •
edited
Loading

The peak controller definitively generates a broken and distorted signal. Here's how it looks for the attached project (click for full size):

7383-Broken-PeakController

And here the full file with the debug output:
PeakControllerSignal.zip

Edit: better picture of the signal.

cyberrumor added a commit to cyberrumor/lmms that referenced this issue Oct 26, 2024
@cyberrumor
Fix PeakController attack/decay, lerp between samples (LMMS#7383)
c4c84bd 
Historically, the PeakController has had issues like attack/decay knobs
acting like on/off switches, and audio artifacts like buzzing or
clicking. This patch aims to address those issues.

The PeakController previously used lerp (linear interpolation) when
looping through the sample buffer, which was in updateValueBuffer. This
lerp utilized attack/decay values from control knobs. This is not the
correct place to utilize attack/decay because the only temporal data
available to the function is the frame and sample size. Therefore the
coefficient should simply be the sample rate instead.

Between each sample, processImpl would set m_lastSample to the RMS
without any sort of lerp. This resulted in m_lastSample producing
stair-like patterns over time, rather than a smooth line.

For context, m_lastSample is used to set the value of whatever is
connected to the PeakController. The basic lerp formula is:

m_lastSample = m_lastSample + ((1 - attack) * (RMS - m_lastSample))

This is useful because an attack of 0 sets m_lastSample to RMS, whereas
an attack of 1 would set m_lastSample to m_lastSample. This means our
attack/decay knobs can be used on a range from "snap to the next value
immediately" to "never stray from the last value".

* Remove attack/decay from PeakController frame lerp.
* Set frame lerp coefficient to 100.0 / sample_rate to fix buzzing.
* Add lerp between samples for PeakController to fix stairstep bug.
* The newly added lerp utilizes (1 - attack or decay) as the
  coefficient, which means the knobs actually do something now.
cyberrumor added a commit to cyberrumor/lmms that referenced this issue Oct 26, 2024
@cyberrumor
Fix PeakController attack/decay, lerp between samples (LMMS#7383)
5b692a5 
Historically, the PeakController has had issues like attack/decay knobs
acting like on/off switches, and audio artifacts like buzzing or
clicking. This patch aims to address those issues.

The PeakController previously used lerp (linear interpolation) when
looping through the sample buffer, which was in updateValueBuffer. This
lerp utilized attack/decay values from control knobs. This is not the
correct place to utilize attack/decay because the only temporal data
available to the function is the frame and sample size. Therefore the
coefficient should simply be the sample rate instead.

Between each sample, processImpl would set m_lastSample to the RMS
without any sort of lerp. This resulted in m_lastSample producing
stair-like patterns over time, rather than a smooth line.

For context, m_lastSample is used to set the value of whatever is
connected to the PeakController. The basic lerp formula is:

m_lastSample = m_lastSample + ((1 - attack) * (RMS - m_lastSample))

This is useful because an attack of 0 sets m_lastSample to RMS, whereas
an attack of 1 would set m_lastSample to m_lastSample. This means our
attack/decay knobs can be used on a range from "snap to the next value
immediately" to "never stray from the last value".

* Remove attack/decay from PeakController frame lerp.
* Set frame lerp coefficient to 100.0 / sample_rate to fix buzzing.
* Add lerp between samples for PeakController to fix stairstep bug.
* The newly added lerp utilizes (1 - attack or decay) as the
  coefficient, which means the knobs actually do something now.
cyberrumor added a commit to cyberrumor/lmms that referenced this issue Oct 26, 2024
@cyberrumor
Fix PeakController attack/decay, lerp between samples (LMMS#7383)
083c009 
Historically, the PeakController has had issues like attack/decay knobs
acting like on/off switches, and audio artifacts like buzzing or
clicking. This patch aims to address those issues.

The PeakController previously used lerp (linear interpolation) when
looping through the sample buffer, which was in updateValueBuffer. This
lerp utilized attack/decay values from control knobs. This is not the
correct place to utilize attack/decay because the only temporal data
available to the function is the frame and sample size. Therefore the
coefficient should simply be the sample rate instead.

Between each sample, processImpl would set m_lastSample to the RMS
without any sort of lerp. This resulted in m_lastSample producing
stair-like patterns over time, rather than a smooth line.

For context, m_lastSample is used to set the value of whatever is
connected to the PeakController. The basic lerp formula is:

m_lastSample = m_lastSample + ((1 - attack) * (RMS - m_lastSample))

This is useful because an attack of 0 sets m_lastSample to RMS, whereas
an attack of 1 would set m_lastSample to m_lastSample. This means our
attack/decay knobs can be used on a range from "snap to the next value
immediately" to "never stray from the last value".

* Remove attack/decay from PeakController frame lerp.
* Set frame lerp coefficient to 100.0 / sample_rate to fix buzzing.
* Add lerp between samples for PeakController to fix stairstep bug.
* The newly added lerp utilizes (1 - attack or decay) as the
  coefficient, which means the knobs actually do something now.
@sakertooth sakertooth mentioned this issue Oct 27, 2024
Merged
@messmerd messmerd mentioned this issue Jan 15, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix PeakController attack/decay, lerp between samples (#7383) cyberrumor/lmms
2 participants
@michaelgregorius @0xf0xx0