fix: streaming contract, flush lifecycle, and polyphase interpolation defects - #56
Merged
Conversation
At severe non-integer downsampling ratios the fixed-point accumulator overshoots the per-call limit by up to one step, making consumed exceed the history length. The old guard then skipped the trim while still rebasing the accumulator: unbounded history growth and re-read of stale samples (stuttering, over-emission).
#51) Each FIR stage's Process retains exactly N-1 history samples (the filter delay line), so N-1 padding zeros drain it. The stages padded N zeros, pushing one extra all-zero reference position through the filter and emitting a phantom output sample: Process(470)+Flush at 44100 to 48000 QualityHigh returned 514 where ceil(470*ratio)+1 = 513 is the maximum. Padding combination: N-1 in all three stages (no contingency needed). - DFTStage.Flush: tapsPerPhase -> tapsPerPhase-1 - DFTDecimationStage.Flush: numTaps -> numTaps-1 - PolyphaseStage.Flush: tapsPerPhase -> tapsPerPhase-1 The polyphase lower-bound contingency did not trigger; tapsPerPhase-1 satisfies both bounds of TestFlushLength_Canonical across all 6 rate pairs x 2 presets. TestStreamingEquivalence stays bit-exact because Flush is altered identically on the one-shot and chunked sides.
…llback newHalfBandStage silently swallowed newPolyphaseStage's error and substituted stubStage (nearest-neighbor resampling), so a caller of the public New() constructor could get a "working" resampler with drastically degraded audio and no signal that anything failed. newHalfBandStage now returns (pipeline.Stage, error) and createStage propagates the tuple directly, matching the StagePolyphase and StageFFT branches. The rest of the chain (createStage -> newConstantRateResampler -> New()) already propagated errors correctly. Inspection shows this failure is currently unreachable through any public New() input: BuildPipeline only ever emits StageHalfBand specs with Ratio pinned to 0.5 or 2.0, both of which route through deterministic, bounds-clamped filter design math in engine.NewResampler that cannot fail. The signature change makes that guarantee explicit instead of relying on silent substitution.
…on flush CubicStage.Process emitted immediately from a fictional zero-filled history instead of withholding output like the FIR stages do, so the final cubicLatencySamples of real input were silently dropped while the first samples were computed from implicit pre-silence. Process now withholds emission until cubicLatencySamples real samples have primed the history window, and Flush pads that many zeros through the same path to drain the true tail, then resets to fresh state. Also removes the dead histPos field and fixes the stale "doesn't buffer" doc comment.
Adds two cubic-stage flush tests requested by second review: TestCubicStage_FlushTailTracksRamp checks the flushed tail's actual values, not just its length: the first sample must still track the ramp's real final value, with later samples decaying toward the zero padding. Uses a bounded 0..1 ramp rather than TestCubicStage_FlushEmitsTail's unbounded one, since that ramp's huge final-value-to-zero discontinuity makes cubic interpolation overshoot substantially in the interior of the affected segment (a known, minor characteristic already documented on CubicStage.Flush, not a defect), which would make a tight tolerance fail on the correct implementation. TestCubicStage_FlushAfterPartialPriming covers Process being fed fewer samples than cubicLatencySamples before Flush: confirmed no panic, the total count still lands within the established tolerance, and the terminal-flush lifecycle holds.
…et, and latency tests
NewPolyphaseStage builds the cubic sub-phase interpolation banks (B/C/D) by sampling the prototype at the current phase and its neighbours. The old getCoeff wrapped the neighbour phase within the same tap (phase % numPhases), which at each phase boundary picked a prototype sample numPhases-1 positions away instead of the adjacent one. filterBank.coeffs is the flat prototype, so the adjacent sample to (tap t, phase L-1) is coeffs[t*L + L] (phase 0 of tap t+1) and to (tap t, phase 0) is coeffs[t*L - 1] (phase L-1 of tap t-1); out-of-range positions clamp to 0.0. The wrap injected a large coefficient discontinuity at each boundary. It was invisible for exact-rational ratios (44100<->48000 == 80/147, where the fixed-point sub-phase x is identically 0 so the banks are never consulted), which is why the whole existing regression/soxr suite (all exact-rational or integer/DFT ratios) never caught it. For ratios with active sub-phase interpolation it collapsed THD+N by 77 to 111 dB: measured -54 dB wrapped versus -141 dB flat at 44100->64000 QualityHigh, and similar at 32000->44100, 8000->44100, 16000->44100, 11025->48000, 44100->192000. Flat matches soxr's HQ-class THD+N. phase_wrap_measure_test.go records the measurement (a degenerate exact-ratio probe documenting the x==0 masking, plus an active-ratio probe that reaches the code) and guards the fix: it asserts the production banks match the flat reconstruction and that the active-ratio flat path stays >= 40 dB better than the old wrap. No existing golden moved; full suite green.
…ProcessInto Two contract gaps in the cubic (QualityQuick) streaming path, both surfaced by this branch (cubic Flush now emits a real tail, and QualityQuick now maps to the cubic stage): 1. Resampler.Flush early-returned the cubic tail and bypassed the samplesOut accounting done on the FIR path, so GetStatistics undercounted samplesOut by the flush-tail length. The cubic branch now adds its emitted tail to samplesOut before returning. 2. CubicStage.Process allocated a fresh output slice every call, violating the documented zero-allocation ProcessInto contract for QualityQuick. The stage now reuses a persistent output buffer via growStableLen, mirroring the FIR stages: processZeroCopy fills the reused buffer with append (so an off-by-one in the size bound can never write out of range) and returns an alias, while the owning Process wrapper returns a copy. The engine's ProcessZeroCopy path calls processZeroCopy; Process keeps the owning wrapper. Tests: TestCubicStage_FlushUpdatesSamplesOut pins the samplesOut invariant and TestProcessInto_ZeroAllocs_QualityQuick pins zero allocations on the warm ProcessInto loop. Chunked-equivalence and cubic content tests stay bit-exact.
The type-level streaming example discarded the Process and Flush errors with `_`, unlike every other example on this branch. Handle them with log.Fatal to match doc.go's float32 example style.
…nfig.Validate
CubicStage.Process forwarded processZeroCopy's empty result during the
priming window, but that slice has cap > 0 and aliases the internal output
buffer; a caller appending to it was corrupted by the next Process call.
Return a fresh []F{} literal on the empty path, matching the sibling stages.
Config.Validate used NaN-blind comparisons (c.InputRate <= 0, ratio < min),
so New/NewMultiChannel/NewStereo/NewSimple and the preset constructors
accepted NaN rates and built a passthrough pipeline that emitted garbage.
Reject NaN with the positive-comparison idiom already used in
internal/engine/resampler.go.
DFTStage.Process and DFTDecimationStage.Process had duplicate copy branches; the decimation factor==1 path returned an aliased input slice. Collapse both into a single unconditional owned copy after the empty guard so Process always returns caller-owned memory regardless of factor. StageAdapter.GetMemoryUsage summed only the base polyphase coefficient bank, undercounting the four cubic-interpolation banks (a, b, c, d) by 4x; sum all four and add the missing cubic-stage branch for symmetry with GetLatency. Also: soften the cubic priming comment to describe the bounded startup neighbor accurately, add the latency invariant note, reconcile the getCoeff THD figure to the committed 86.26 dB measurement, cross-reference Resampler.Latency() from StageAdapter.GetLatency, apply the buffer growth-slack policy to DFT phaseBufs, use the min() builtin for the polyphase consumed cap, hoist deficitIn below the cubic early return, and drop the issue #51 parentheticals from production comments.
Reconcile the polyphase THD figure to the committed 86.26 dB measurement in the CHANGELOG (previously an uncommitted 77-111 dB range). Tag every Unreleased Fixed/Changed bullet with (#51) for issue-ref consistency and state the NaN-rejection coverage confidently now that Config.Validate rejects NaN. README/doc.go Latency section: add a callout that Latency() exists on the NewEngine (SimpleResampler/SimpleResamplerFloat32) path only, while New(config) users get the input-domain GetLatency()/GetInfo() figure that is not for FIFO priming; soften the withhold/already-fed wording to the measured +-2 sample tolerance; align the Real-Time heading casing across both surfaces. doc.go: point multi-channel streaming readers to ProcessMulti/FlushMulti instead of the Stereo Processing section, which never mentions ProcessMulti. stages.go: reword the stubStage doc; it is test-only now, not an unimplemented stub. examples/streaming: document the first-callback precondition and the per-callback allocation and FIFO reslice tradeoffs.
Add a float32 latency case mirroring one row of TestLatency_MatchesMeasuredDeficit, a float32 QualityQuick ProcessInto zero-alloc test, and stage-adapter unit tests covering GetLatency's cubic branch and GetMemoryUsage's decimation, cubic, and four-bank polyphase paths (constructed directly since the public pipeline cannot reach the cubic branch). Broaden the float32 streaming equivalence test with a downsample ratio, a second quality, and a second chunk size. Name the severe-ratio magic numbers as derived consts and modernize the callback loop to range-over-int.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 59 minutes Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (31)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces comprehensive improvements and bug fixes for streaming, latency, and quality in the audio resampler. Key changes include fixing a polyphase phase-boundary coefficient interpolation bug (which significantly improves THD+N for active sub-phase interpolation ratios), resolving severe downsampling corruption, ensuring Flush is terminal and does not over-pad, and returning owned buffers from Process to prevent aliasing. Additionally, it introduces a Latency() method to report startup deficits for real-time FIFO priming, adds robust NaN rate validation, ensures proper error propagation for half-band stages, and optimizes the QualityQuick cubic interpolation path to be zero-allocation once warm. New tests and documentation examples have been added to validate these streaming contracts and behaviors. I have no feedback to provide as no review comments were submitted.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Benchmark Comparison |
Extract the per-plan chunked run in TestStreamingEquivalence_Float64 into a helper to drop cognitive complexity below 50; replace append([]float64(nil), x...) with slices.Clone(x) in the streaming and flush-lifecycle tests. In the streaming example, name the loop count and tone constants, use range-over-int and max(), and start the FIFO zero-length with capacity so the priming zeros are appended (satisfies makezero).
There was a problem hiding this comment.
Pull request overview
This PR fixes multiple engine and streaming-contract defects that caused audible clicks/distortions in chunked real-time usage, and adds tests and documentation to pin the corrected behavior (chunked Process + single terminal Flush matches one-shot processing bit-exactly).
Changes:
- Fix polyphase coefficient boundary interpolation (avoid wrapped neighbor), severe downsampling history/accumulator behavior, and
Flushpadding/lifecycle so streaming is click-free and deterministic. - Introduce
Latency()onSimpleResampler/SimpleResamplerFloat32for real-time FIFO priming, and document the streaming contract (singleFlushat end-of-stream, per-channel instance). - Add extensive regression tests covering streaming equivalence, flush length/lifecycle, NaN validation, QualityQuick cubic behavior, memory/latency accounting, and buffer/aliasing invariants.
Reviewed changes
Copilot reviewed 31 out of 31 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| streaming_equivalence_test.go | Pins bit-exact equivalence between chunked streaming (Process + final Flush) and one-shot processing. |
| stages.go | Propagates half-band stage construction errors instead of silently falling back to a stub. |
| stages_test.go | Tests half-band stage creation and ensures no stub fallback occurs in pipelines. |
| resample.go | Makes config validation NaN-safe for sample rates and ratio bounds. |
| README.md | Documents streaming/FIFO usage, terminal Flush, and adds a runnable streaming example link. |
| quality_quick_test.go | Ensures QualityQuick via NewEngine maps to cubic (low-latency) behavior. |
| processinto_test.go | Adds warm-path zero-allocation assertions for ProcessInto on QualityQuick (float64/float32). |
| pipeline_builder.go | Updates half-band stage creation to return (Stage, error) and propagate errors. |
| nan_validation_test.go | Adds tests ensuring NaN sample rates are rejected by constructors. |
| latency_test.go | Verifies Latency() matches measured startup deficit (float64 and float32). |
| internal/engine/stage_adapter.go | Improves latency/memory reporting to account for more stages and coefficient banks. |
| internal/engine/stage_adapter_test.go | Adds coverage for new latency/memory accounting branches (cubic/decimation/banks). |
| internal/engine/severe_ratio_test.go | Adds regression test for severe non-integer downsampling monotonicity and bounded history. |
| internal/engine/reset_state_test.go | Strengthens reset determinism tests and adds reset-after-flush coverage (float64/float32). |
| internal/engine/resampler.go | Makes validation NaN-safe, fixes cubic ProcessZeroCopy path, adds terminal cubic-tail flush accounting, and implements Latency(). |
| internal/engine/polyphase_stage.go | Fixes coefficient neighbor indexing, severe-downsample history consumption, and makes Flush terminal with correct padding. |
| internal/engine/phase_wrap_measure_test.go | Adds measurement/regression harness validating the phase-boundary interpolation fix via THD+N and bank equality. |
| internal/engine/dft_stage.go | Ensures Process always returns owned memory, improves buffer growth policy, and makes Flush terminal with correct padding. |
| internal/engine/debug_latency_test.go | Replaces/debug refactors latency test to validate Latency() against measured deficit. |
| internal/engine/cubic.go | Fixes cubic priming/tail behavior, enforces owned-return contract, and adds reusable output buffer for warm zero-alloc streaming. |
| internal/engine/cubic_flush_test.go | Adds comprehensive cubic-stage tail, lifecycle, ownership, and streaming-equivalence tests. |
| internal/engine/buffer_integrity_test.go | Strengthens tests to assert determinism and that returned buffers are not corrupted across calls. |
| flush_multi_test.go | Improves multi-channel flush test to validate sample content equality, not just length, and pins empty-flush behavior. |
| flush_lifecycle_test.go | Pins terminal Flush lifecycle semantics across ratios/qualities (including QualityQuick). |
| flush_length_test.go | Pins Process+Flush total length bounds to prevent phantom padding samples. |
| examples/streaming/main.go | Adds runnable real-time FIFO streaming example using Latency()-primed buffering and deficit-driven sizing. |
| doc.go | Updates package docs with streaming contract, FIFO/latency guidance, and safer example error handling. |
| convenience.go | Documents streaming usage, makes Flush semantics explicit, adds Latency() accessors, and corrects QualityQuick engine mapping. |
| convenience_float32_test.go | Strengthens float32 flush test to require a real drained tail after warm processing. |
| CHANGELOG.md | Documents behavioral changes and fixes affecting streaming, flush lifecycle/length, QualityQuick mapping, and NaN validation. |
| aliasing_test.go | Adds regression test ensuring unity-ratio output does not alias caller input. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+166
to
+172
| allocs := testing.AllocsPerRun(100, func() { | ||
| r.Reset() | ||
| _, _ = r.ProcessInto(input, output) | ||
| }) | ||
| if allocs != 0 { | ||
| t.Fatalf("ProcessInto allocated %.0f times per call; expected 0", allocs) | ||
| } |
Owner
Author
There was a problem hiding this comment.
Fixed in 84e4371. The helper now captures the error into a pre-declared variable inside the closure and asserts it with require.NoError after AllocsPerRun returns. Same treatment applied to the float32 helper and the three sibling zero-alloc tests that shared the pattern.
Comment on lines
+567
to
+573
| allocs := testing.AllocsPerRun(100, func() { | ||
| r.Reset() | ||
| _, _ = r.ProcessInto(input, output) | ||
| }) | ||
| if allocs != 0 { | ||
| t.Fatalf("ProcessInto allocated %.0f times per call; expected 0", allocs) | ||
| } |
Owner
Author
There was a problem hiding this comment.
Fixed in 84e4371, together with the float64 helper and the three inline sibling tests using the same pattern. Allocation assertions still measure 0 allocs.
The AllocsPerRun closures in the zero-alloc tests and helpers discarded the ProcessInto/ProcessFloat32Into error, so an erroring call (which may allocate nothing) would still satisfy the 0-alloc assertion and mask a regression. Capture the error in a pre-declared var (assigning to an already-escaped var inside the closure does not allocate) and require.NoError after AllocsPerRun returns, across all five sites (two warm helpers plus the three inline sibling tests).
This was referenced Jul 17, 2026
tphakala
added a commit
that referenced
this pull request
Jul 17, 2026
…ad code retirement (#57) * refactor(engine): extract shared cubic interp bank builder The phase-wrap measurement test previously re-derived the Catmull-Rom bank construction to build its differential reference, so a formula change would have required lockstep edits. NewPolyphaseStage and the test now share buildCubicInterpBanks and the test varies only the boundary-indexing policy. The bank-equality assertion pins the extraction bit-for-bit. Refs #53 * test(engine): pin full-pipeline THD at active-interpolation ratios quality_regression_test.go previously exercised only exact-rational or integer ratios where the sub-phase interpolation banks are never consulted, so the phase-boundary indexing fix from #56 was guarded only at stage level. Add THD assertions through the whole resampler at 32000->44100 and 44100->64000 for High, Medium, and Low. High gets a dedicated tighter limit (-145 dB, worst measured -151.63 dB); Medium and Low fall within margin of the existing constants and reuse them. Refs #54 * fix: report the true startup deficit from constantRateResampler.GetLatency GetLatency summed per-stage group-delay heuristics whose terms live in different rate domains, so multi-stage pipelines mis-reported the startup deficit (672 reported vs 703 measured at 44100 to 96000 QualityHigh). The engine Resampler now exposes StartupDeficit(), the un-rounded deficit in output samples that Latency() ceils, CubicStage mirrors it, and GetLatency accumulates each stage's fractional deficit through the downstream stage ratios before rounding once. Accuracy is now within 2 samples across presets and ratios, pinned by a numeric test on the New(config) path. Refs #52 * refactor: retire duplicate polyphase design path and dead LinearStage internal/filter/polyphase.go carried a parallel polyphase design implementation that diverged from the engine (edge clamping, DC-gain normalization, unchecked GetCoefficient, duplicated cubic constants) and was consumed only by the cmd/analyze-filter diagnostic, so the tool analyzed a filter the product never ships. Remove both along with the production-dead LinearStage and its constants; the engine's design code in internal/engine is now the single polyphase implementation. kaiser.go stays, the engine depends on it. Refs #55 * refactor: harden startup-deficit contract and align docs from review Gate review follow-ups: README no longer describes the retired group-delay semantics of GetLatency on the New(config) path; the duck-typed deficit assertion uses a named startupDeficitStage interface with compile-time assertions in stages.go so a refactor of the StageAdapter embedding fails the build instead of silently degrading to the group-delay fallback; the cubic deficit formula lives only in CubicStage.StartupDeficit with the engine delegating to it; the config path latency test now covers QualityQuick and a new test pins the fallback branch including its downstream-ratio conversion; TestCubicStageGetters asserts the new getter; stale or imprecise comments corrected (StageAdapter.GetLatency consumer note, THD margin arithmetic, duplicated bank-build description, what the bank-equality pin does and does not guard). Refs #52 #53 #54 * docs(test): attribute phase-boundary defect to issue #51 alongside PR #56 CodeRabbit review: the CHANGELOG attributes the defect to #51, so the active-interpolation THD comment now names both the reporting issue and the fixing PR instead of the PR alone.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #51.
This branch fixes the streaming contract issues behind the clicks reported in #51 and several defects found while investigating, with the streaming path pinned bit-exact before any engine change (chunked Process plus one final Flush now provably equals one-shot processing, enforced by a new equivalence test).
Engine fixes: the polyphase phase-boundary coefficient interpolation used a wrapped neighbor, degrading THD+N massively at ratios with active sub-phase interpolation (measured 86.26 dB improvement at 44100 to 64000; exact-rational ratios like 44100 to 48000 are bit-identical, which is why the existing suite never saw it); severe non-integer downsampling (beyond roughly 1:16) corrupted output and grew internal history without bound; Flush over-padded each FIR stage by one zero, emitting the phantom samples observed in #51 (Process+Flush now totals within [floor(nratio), ceil(nratio)+1]); Flush is now terminal (second Flush returns empty, Process after Flush starts a fresh stream); Process returns caller-owned memory at every ratio including unity and empty results; the cubic (QualityQuick) stage no longer computes its first output segments from a fictional zero history and now drains its true tail on flush; NaN sample rates are rejected by all constructors; half-band stage construction errors propagate instead of silently substituting a nearest-neighbor stub.
API and docs: new Latency() on SimpleResampler and SimpleResamplerFloat32 returns the startup deficit in output samples for priming real-time FIFOs; QualityQuick through NewEngine and NewEngineFloat32 now uses cubic interpolation, matching New() and the documented contract; GetLatency and GetMemoryUsage account for all stages and coefficient banks; the streaming contract (variable per-call output length, Flush end-of-stream only, one instance per channel) is documented on the API, in doc.go, and in the README, with a runnable real-time FIFO example at examples/streaming that uses deficit-driven input sizing (static floor sizing underruns on long runs, static ceil grows the FIFO without bound).
Tests: new pins for chunked-streaming equivalence, flush length and lifecycle, severe-ratio integrity, latency accuracy (float64 and float32), zero-allocation ProcessInto for QualityQuick, and stage-adapter accounting; several previously vacuous tests (latency, buffer integrity, multi-reset, flush-multi) now assert real behavior.
Behavior changes for existing users are listed in the CHANGELOG: flush output length shrinks by the phantom padding, a second Flush returns empty, QualityQuick via NewEngine has near-zero latency, and non-exact-rational ratio output changes bit-for-bit (it is now correct). Follow-ups tracked in #52, #53, #54, #55.