Skip to content

Address critical audit findings from 8-agent review#27

Merged
user1303836 merged 2 commits intomainfrom
fix/audit-issues
Feb 15, 2026
Merged

Address critical audit findings from 8-agent review#27
user1303836 merged 2 commits intomainfrom
fix/audit-issues

Conversation

@user1303836
Copy link
Owner

Addresses 12 of the 17 issues surfaced by the 8-agent audit. The remaining 5 are deferred (see below).

Stability / Safety

Audio Quality

Performance

Deferred Issues

Closes #10, #11, #14, #15, #16, #17, #18, #19, #20, #21, #22, #23

Stability / safety:
- Guard division by zero in Oscillator, YinPitchDetector, PitchSmoother
- Add NaN/Inf output sanitization in processBlock
- Clamp channel count to 2 to prevent buffer overrun
- Fix oscillator phase wrap to handle phaseIncrement >= 1.0
- Reset PitchSmoother state on sample rate change

Audio quality:
- Add PolyBLEP anti-aliasing to Square and Saw waveforms
- Add averaging decimation filter before YIN to prevent aliasing
- Smooth pitch tracking in log-frequency domain for perceptual uniformity
- Replace hasValidPitch with confidence-scaled wet signal for natural gating
- Add SmoothedValue on mix, rateMultiplier, manualRate to prevent zipper noise

Performance:
- Cache APVTS parameter pointers (avoid per-block string lookup)
- Dirty check on Oscillator::setFrequency to skip redundant division
- Dirty check on PitchSmoother::setSmoothingAmount to skip redundant exp()

Closes #10, #11, #14, #15, #16, #17, #18, #19, #20, #21, #22, #23
Copy link
Owner Author

@user1303836 user1303836 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DSP Correctness Review

Reviewed the four DSP algorithm changes in this PR. Summary:

PolyBLEP Anti-Aliasing (#10) -- Correct

The polyBLEP residual function is mathematically standard and correctly applied to both Square (two transitions per cycle) and Saw (one transition). Minor concern: polyBLEP can divide by zero if phaseIncrement is 0 (freq=0). Currently guarded indirectly by the oscFreq > 0 check in processBlock, but nextSample() is called unconditionally.

Decimation Anti-Alias Filter (#11) -- Adequate

The boxcar (accumulate-and-average) filter places its null exactly at the decimated Nyquist, which is ideal. Sidelobe attenuation is only ~13dB, but for YIN pitch detection this is sufficient -- aliased energy raises the CMNDF noise floor slightly without creating false pitch peaks.

Log-Frequency Smoothing (#19) -- Correct

The log2/exp2 round-trip is clean. Edge cases (zero frequency, no-detection) are properly guarded. The EMA in log2 space gives perceptually uniform smoothing across the guitar range, which is the correct domain for pitch tracking.

Confidence-Scaled Wet Signal (#20) -- Has Two Issues

  1. Volume drop at low confidence: When confidence is 0, wet=0 but dry is scaled by (1-mix). At high mix values, this attenuates the dry signal during silence. The mix formula should be adjusted so dry passes through at unity when confidence is low.
  2. Unsmoothed confidence: The raw confidence value jumps at YIN analysis window boundaries (every ~2048 decimated samples). This can cause clicks in the wet gain envelope. A one-pole smooth on wetGain would fix this.

phase += phaseIncrement;
if (phase >= 1.0)
while (phase >= 1.0)
phase -= 1.0;
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Stability] Phase wrapping: negative phase not handled

The while (phase >= 1.0) loop correctly handles the case where phaseIncrement >= 1.0 (very high frequencies relative to sample rate). However, if phase ever becomes negative (e.g. due to floating-point drift over long run times, or if setFrequency is called with a negative value which would make phaseIncrement negative via updateIncrement), phase will drift unboundedly negative.

There is no guard on freq being non-negative in setFrequency, so phaseIncrement can become negative if a negative hz is passed. While the caller currently only passes oscFreq > 0.0f, this is a fragile assumption.

Consider adding:

while (phase < 0.0)
    phase += 1.0;

after the existing while loop, or clamping freq to non-negative in setFrequency. Low severity since callers currently guard against this, but it would make the oscillator self-contained.

Comment on lines +28 to +38
if (t < dt)
{
t /= dt;
return t + t - t * t - 1.0;
}
if (t > 1.0 - dt)
{
t = (t - 1.0) / dt;
return t * t + t + t + 1.0;
}
return 0.0;
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DSP Review: PolyBLEP implementation (#10)

The polyBLEP residual function is mathematically correct. Verifying the two branches:

Branch 1 (t < dt): t/dt maps to [0,1), then 2t - t^2 - 1 is the standard quadratic residual that starts at -1 and ends at 0. Correct.

Branch 2 (t > 1 - dt): (t-1)/dt maps to [-1,0), then t^2 + 2t + 1 = (t+1)^2 starts at 0 and ends at 1. Correct.

Square wave application: The two corrections at phase and phase + 0.5 correspond to the two discontinuities per cycle. The subtraction of the second BLEP is correct because the square wave transitions from +1 to -1 at phase=0.5 (opposite polarity to the phase=0 transition). This is the standard formulation.

Saw wave application: Single BLEP subtracted at the phase=0 reset discontinuity. Correct -- the naive saw 2*phase - 1 jumps from +1 to -1 at phase wrap, and the BLEP residual smooths exactly that.

One concern -- dt validity: When phaseIncrement is 0 (e.g., freq=0), polyBLEP will divide by zero in t /= dt. The setFrequency dirty check means freq could remain at 0 if never set. In practice this is guarded by if (oscFreq > 0.0f) in processBlock, but nextSample() is still called unconditionally even when frequency hasn't been set. Consider guarding polyBLEP against dt <= 0 or ensuring freq is always positive before calling nextSample().

Edge case -- phaseIncrement >= 0.5: At very high frequencies (above Nyquist/2), the polyBLEP windows for the two square-wave transitions will overlap, since both windows span phaseIncrement samples around each transition. This is inherent to polyBLEP and not a bug, but worth noting that the anti-aliasing degrades gracefully rather than producing artifacts at extreme frequencies.


void Oscillator::setFrequency(float hz)
{
if (hz == freq)
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Performance: Floating-point equality in dirty check

The hz == freq comparison uses exact floating-point equality. This is safe from a correctness standpoint -- if the same float value is loaded from the SmoothedValue and passed here, the bit pattern will be identical. However, there is a subtle performance concern:

In pitch-tracking mode, smoothedRateMult.getNextValue() is called per-sample and multiplied by the smoothed frequency. Even if the input hasn't changed, the SmoothedValue will produce slightly different intermediate values during its ramp (until isSmoothing() returns false). This means setFrequency() will be called with a genuinely different hz value on most samples during a ramp, and the dirty check won't fire -- which is correct behavior.

Once smoothing settles, the SmoothedValue returns the same target value each call, so the dirty check will correctly skip updateIncrement(). This is the right tradeoff.

One minor note: updateIncrement() is a single division (freq / sr), so even without the dirty check the cost is minimal. The dirty check mainly helps avoid unnecessary writes to phaseIncrement, which is good for cache behavior.

void Oscillator::prepare(double sampleRate)
{
sr = sampleRate;
sr = std::max(1.0, sampleRate);
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Stability] Division-by-zero: sr guard is sufficient

Good: sr = std::max(1.0, sampleRate) prevents division by zero in updateIncrement() where phaseIncrement = freq / sr. The floor of 1.0 Hz is a reasonable sentinel -- no real audio system has a sample rate below 1 Hz, so this effectively catches zero/negative inputs without silently producing wrong results.

One note: if prepare() is never called, the default sr = 44100.0 in the header is also safe. No issues here.

Comment on lines 64 to +65
phase += phaseIncrement;
if (phase >= 1.0)
while (phase >= 1.0)
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DSP Review: Phase wrap correctness (#21)

The change from if to while is correct and necessary. When phaseIncrement >= 1.0 (freq >= sampleRate), a single subtraction won't bring phase back into [0,1). The while loop handles arbitrarily large phase increments.

However, note that phaseIncrement >= 1.0 means the oscillator frequency exceeds Nyquist. At that point the output is already aliased regardless of polyBLEP. The while loop prevents phase runaway, which is the right thing to do, but the audio output is meaningless at those frequencies. This is fine as a safety measure.

Comment on lines 112 to +117
}

if (betterTau < 1.0f)
{
lastResult = { 0.0f, 0.0f };
return;
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DSP Review: betterTau < 1.0 guard (#16)

This guard prevents division by a very small or zero tau in analysisSR / betterTau. Mathematically, betterTau < 1.0 would correspond to a frequency above the analysis sample rate, which is physically meaningless for YIN (you need at least one full period).

The guard is correct. At betterTau = 1.0, the detected frequency would be analysisSR / 1.0 = analysisSR, which is the entire analysis bandwidth -- well above any real pitch. The subsequent freq > 5000.0f check would also catch this, but the explicit guard is clearer and prevents the division from producing extreme values that could propagate.

{
if (confidence < sensitivityThreshold)
return smoothed;
if (detectedFreq <= 0.0f || confidence < sensitivityThreshold)
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Audio Quality - Log-frequency smoothing #19] The implementation matches the suggested fix from the issue almost exactly. Smoothing in log2 domain with std::log2/std::exp2 provides perceptually uniform tracking, meaning an octave jump from 100->200 Hz smooths at the same perceived rate as 1000->2000 Hz.

One edge case to consider: if detectedFreq is extremely small but positive (e.g. 0.001 Hz from a noisy detection), std::log2(0.001) = ~-10, which is a valid float but a very negative log value. This would cause the smoother to track toward a very low frequency, then slowly recover. The if (detectedFreq <= 0.0f) guard catches zero and negative, but not near-zero values. However, the YIN detector already clamps output to [20, 5000] Hz, so in practice this path is unreachable from the current pitch detector. Acceptable as-is.

Also good: hasValue and smoothed are reset in prepare() (#18), so after a sample rate change the first valid detection will snap immediately rather than smoothing from a stale log-frequency value.

return hasValue ? std::exp2(smoothed) : 0.0f;

float logFreq = std::log2(detectedFreq);

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Performance: Log-frequency smoothing -- per-sample overhead

The move to log2 domain adds per-sample calls to std::log2() and std::exp2() in the process() method. These are called once per sample in pitch-tracking mode.

Cost analysis:

  • std::log2(detectedFreq): ~10-20ns on modern x86 (transcendental, but well-optimized)
  • std::exp2(smoothed): same order
  • Previously: one multiply-add for the linear EMA filter

So this roughly triples the cost of PitchSmoother::process() per sample. However, this is called once per sample (not per channel), and the absolute cost (~30-40ns total) is dwarfed by the YIN pitch detector and the oscillator. The perceptual improvement (uniform smoothing across octaves) easily justifies this cost.

No performance concern.

pitchSmoother.prepare(sampleRate);
hasValidPitch = false;

smoothedMix.reset(sampleRate, 0.02);
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Stability] SmoothedValue reset covers prepareToPlay correctly

smoothedMix.reset(sampleRate, 0.02) correctly initializes the ramp time (20ms) relative to the new sample rate. This is called in prepareToPlay which JUCE guarantees is called before processBlock.

One edge case worth noting: on the very first processBlock call after prepareToPlay, the SmoothedValue internal current value is default-initialized to 0.0f. So the first block will ramp from 0 to the target value over 20ms. For smoothedMix this means a 20ms fade-in from silence, which is actually fine (inaudible). For smoothedManualRate this means ramping from 0 Hz to the target frequency, which could produce a brief chirp on the very first block after plugin load. This is a very minor cosmetic issue and not a safety concern.

If you want to eliminate even that, you could call smoothedManualRate.setCurrentAndTargetValue(manualRateParam->load()) here. But this is nit-level.

decimationAccum += sample;
if (++decimationCounter < decimation)
return;

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Performance: Accumulate-and-average decimation

The decimation change replaces naive sample-dropping with accumulate-and-average. Per-sample overhead:

  • Before: 1 increment + 1 comparison (counter check)
  • After: 1 addition (accumulate) + 1 increment + 1 comparison, plus on the decimated sample: 1 division + 1 store + 1 reset

The extra per-sample cost is a single float addition (decimationAccum += sample). The division only occurs every decimation samples (every 2nd or 4th sample). This is negligible overhead for a meaningful anti-aliasing improvement before YIN analysis.

@user1303836
Copy link
Owner Author

Performance Review Summary

Cached Parameter Pointers (#23) -- PASS

All 7 APVTS parameters are properly cached as std::atomic<float>* in the constructor. Thread safety is correct (pointer set once in constructor, values read atomically). Initialization is safe (all default to nullptr in header, assigned before any audio processing can occur).

Dirty Checks (#15) -- PASS

  • Oscillator::setFrequency(): Exact float equality (hz == freq) is appropriate. The dirty check avoids an unnecessary division per-sample when frequency hasn't changed.
  • PitchSmoother::setSmoothingAmount(): Same pattern, higher payoff since it avoids a std::exp() call. No risk of missing updates -- values come from atomic loads of the same float.

New Per-Sample Overhead Assessment -- PASS (no regressions)

Feature Added per-sample cost Verdict
PolyBLEP (Square) 2 polyBLEP calls + 1 fmod Lightweight, justified by anti-aliasing
PolyBLEP (Saw) 1 polyBLEP call Negligible
Log-freq smoothing 1 log2 + 1 exp2 ~30-40ns, dwarfed by YIN
Decimation accumulate 1 float add per sample Negligible
std::isfinite check 1 bitwise op per sample/channel ~2-4 cycles
SmoothedValue getNextValue 3 calls (conditional) Standard JUCE pattern, negligible

SmoothedValue Usage -- PASS with one suggestion

  • 20ms ramp length is appropriate for parameter smoothing
  • getNextValue() called correctly per-sample
  • Suggestion: Call setCurrentAndTargetValue() in prepareToPlay to snap SmoothedValues to initial parameter values, avoiding a 0-to-target ramp on first block after plugin load

Overall: No performance regressions. The new features add well-justified per-sample overhead that is negligible relative to the existing YIN pitch detection cost.

Copy link
Owner Author

@user1303836 user1303836 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Audio Quality and Integration Review

Parameter Smoothing (#14) -- PASS

  • All three automatable continuous parameters (mix, rateMultiplier, manualRate) use juce::SmoothedValue with 20ms ramp
  • Targets set at block start, getNextValue() called per-sample -- correct pattern
  • Discrete params (mode, waveform) and indirect params (smoothing, sensitivity) correctly left unsmoothed
  • Linear smoothing type is appropriate for all three parameters
  • Minor note: SmoothedValues for rateMultiplier/manualRate are only advanced in their respective mode branches, which means stale internal state after mode switches. Not a bug (targets are reset each block) but slightly asymmetric.

Confidence-scaled wet signal (#20) -- PASS

  • Replaces binary hasValidPitch flag with wetGain = result.confidence
  • Natural fade during silence instead of staying active at stale frequency
  • Manual mode correctly bypasses confidence (wetGain stays 1.0)
  • Low-confidence faint artifacts are arguably more musical than the hard gate

Log-frequency smoothing (#19) -- PASS

  • log2/exp2 domain smoothing provides perceptually uniform tracking
  • State reset in prepare() (#18) ensures no stale values after sample rate change
  • Edge cases handled: zero/negative freq returns 0 or last smoothed value

PolyBLEP anti-aliasing (#10) -- PASS

  • Correct textbook implementation for Square (two transition points) and Saw (one)
  • Triangle left without BLAMP -- acceptable tradeoff as noted in PR description
  • Minor optimization possible: replace std::fmod(phase + 0.5, 1.0) with conditional subtract

Decimation anti-aliasing (#11) -- PASS

  • Box-car averaging provides ~-13dB at Nyquist -- adequate for YIN pitch detection
  • Accumulator properly reset in prepare() and after each decimated output

Issue Coverage Verification

All 12 claimed issues are addressed by this diff:

  • #10 PolyBLEP: Oscillator.cpp polyBLEP function + Square/Saw application
  • #11 Decimation filter: YinPitchDetector.cpp accumulate-and-average
  • #14 Param smoothing: SmoothedValue for mix/rateMult/manualRate
  • #15 Dirty checks: Oscillator::setFrequency and PitchSmoother::setSmoothingAmount
  • #16 Div-by-zero: sr clamp in Oscillator/PitchSmoother, betterTau guard in YIN
  • #17 NaN sanitization: isfinite check in processBlock output
  • #18 PitchSmoother state reset: hasValue/smoothed reset in prepare()
  • #19 Log-freq smoothing: log2/exp2 domain in PitchSmoother::process()
  • #20 Confidence-scaled wet: wetGain = result.confidence
  • #21 Phase overflow: while loop instead of if
  • #22 Channel clamp: juce::jmin(buffer.getNumChannels(), 2)
  • #23 Cached params: std::atomic* cached in constructor

Cross-fix Integration

No harmful interactions detected. The fixes layer coherently:

  • Upstream guards (#16) prevent bad state -> downstream NaN guard (#17) catches residuals
  • Parameter caching (#23) feeds SmoothedValue (#14) feeds per-sample processing
  • Confidence scaling (#20) and log smoothing (#19) work independently on different concerns
  • All changes match existing code style with no unnecessary complexity

New Bugs Introduced

None detected. The removal of hasValidPitch is clean -- all references are replaced by the confidence-based approach.

{
decimationAccum += sample;
if (++decimationCounter < decimation)
return;
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Audio Quality - Decimation anti-aliasing #11] The accumulate-and-average approach is the "alternative minimal approach" suggested in the issue. It acts as a box-car (moving average) lowpass filter with a frequency response of sinc(f * decimation / sampleRate). For decimation=4 at 96kHz (analysis SR = 24kHz), the first null is at 24kHz with about -13dB at 12kHz (the new Nyquist). This is adequate but not as clean as a proper IIR lowpass -- some aliasing energy above 12kHz will still fold in, attenuated but not eliminated.

For pitch detection purposes, this is likely sufficient since the YIN algorithm is fairly robust to mild high-frequency contamination. A proper Butterworth or biquad filter would provide steeper rolloff but adds complexity. The box-car approach is the right tradeoff for this codebase's complexity level.

The accumulator is correctly reset in prepare() and after each decimated sample is emitted, so no state leak between blocks or after reinitialization.

}
float wet = dry * oscSample * wetGain;
channelPtrs[ch][i] = dry * (1.0f - mix) + wet * mix;

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Integration - Interaction between confidence scaling and NaN guard] Good layered defense: the wetGain (confidence) scales the wet signal, and then the isfinite check catches any remaining numerical issues.

One subtle point: the NaN guard replaces bad values with 0.0f (silence). In the extremely unlikely event of sustained NaN production, this would produce intermittent silence rather than noise. This is the correct behavior -- silence is always safer than noise in audio plugins.

[Integration - Overall coherence of the 12 fixes] Reviewing how the fixes interact in the processBlock flow:

  1. Cached param pointers (Cache parameter pointers and guard redundant recomputeAlpha calls #23) feed into SmoothedValue targets (No parameter smoothing on mix/rate parameters -- zipper noise on automation #14) -- clean pipeline, no conflicts.
  2. SmoothedValue ramps (No parameter smoothing on mix/rate parameters -- zipper noise on automation #14) interact with PitchSmoother's log-domain smoothing (PitchSmoother should operate in log-frequency domain #19) -- double smoothing on rateMultiplier is benign and actually desirable.
  3. Confidence-based wetGain (hasValidPitch never resets -- ring mod stays active during silence #20) replaces the old hasValidPitch flag, which is cleanly removed. The NaN guard (No NaN/Inf sanitization on audio output #17) acts as a final safety net after all signal math.
  4. Channel clamp (Hardcoded channel pointer arrays assume max 2 channels #22) protects the fixed-size pointer arrays, which are used by all the per-sample processing -- correctly placed at the top before any array access.
  5. The division-by-zero guards (Division by zero possible in Oscillator and YinPitchDetector #16) in Oscillator/PitchSmoother/YinPitchDetector are all upstream of the processBlock math, so they prevent bad state from forming rather than catching it after the fact. This layers well with the downstream isfinite guard.

No harmful interactions detected between fixes.

…items

- Fix confidence-scaled wet causing volume drop: scale mix by confidence
  (effectiveMix = mix * wetGain) instead of scaling wet directly, so
  low confidence produces dry pass-through rather than silence
- Smooth raw confidence via one-pole lowpass (~5ms tau) to prevent
  clicks at YIN analysis boundaries
- Call setCurrentAndTargetValue() in prepareToPlay to avoid 0-to-target
  chirp on first block after prepare
- Guard polyBLEP against dt <= 0
- Add negative phase guard in oscillator
@user1303836 user1303836 merged commit 6791b55 into main Feb 15, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add PolyBLEP anti-aliasing to non-sine oscillator waveforms

1 participant