fix: accurate GetLatency startup deficit, polyphase hardening, and dead code retirement - #57
Merged
Merged
Conversation
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
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
…tency 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
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
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
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 51 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 (1)
WalkthroughThe PR corrects multi-stage startup-deficit latency reporting, centralizes cubic coefficient-bank construction, expands active-interpolation regression tests, removes duplicate polyphase and linear-stage implementations, updates documentation, and adjusts CI command builds. ChangesLatency and interpolation updates
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant constantRateResampler
participant StageAdapter
participant Resampler
Caller->>constantRateResampler: GetLatency()
constantRateResampler->>StageAdapter: inspect StartupDeficit()
StageAdapter->>Resampler: obtain fractional deficit
Resampler-->>constantRateResampler: return deficit in output samples
constantRateResampler-->>Caller: return ceil(total deficit)
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Benchmark Comparison |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/engine/quality_regression_test.go`:
- Around line 165-170: Update the regression comment in the active-interpolation
THD coverage to replace the phase-boundary defect reference “#56” with “#51”,
while preserving the existing “#54” regression tag and all other wording.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f16eafeb-bd6d-44e3-81c3-20c8b1240df0
📒 Files selected for processing (22)
.github/workflows/ci.ymlCHANGELOG.mdREADME.mdcmd/analyze-filter/analyze_filter_gain.gocodecov.ymlconstant.gointernal/engine/buffer_integrity_test.gointernal/engine/constants.gointernal/engine/cubic.gointernal/engine/edge_cases_test.gointernal/engine/extra_engine_test.gointernal/engine/phase_wrap_measure_test.gointernal/engine/polyphase_stage.gointernal/engine/quality_regression_test.gointernal/engine/resampler.gointernal/engine/reset_state_test.gointernal/engine/stage_adapter.gointernal/filter/polyphase.gointernal/filter/polyphase_test.golatency_test.goresample.gostages.go
💤 Files with no reviewable changes (8)
- cmd/analyze-filter/analyze_filter_gain.go
- codecov.yml
- internal/engine/constants.go
- internal/engine/edge_cases_test.go
- .github/workflows/ci.yml
- internal/filter/polyphase_test.go
- internal/filter/polyphase.go
- internal/engine/buffer_integrity_test.go
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
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
1 participant
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 all four follow-up issues from #56 in one bundle, since they touch the same polyphase and latency subsystem.
#52 (bug):
constantRateResampler.GetLatencysummed per-stage group-delay heuristics across mixed rate domains, mis-reporting the startup deficit on multi-stage pipelines (672 reported vs 703 measured at 44100 to 96000 QualityHigh; the numbers in the issue body predate #56, see the issue comment). The engineResamplernow exposesStartupDeficit(), the un-rounded deficit in output samples thatLatency()ceils, andGetLatencyaccumulates each stage's fractional deficit through the downstream stage ratios before rounding once. Accuracy is within 2 samples across presets and ratios, pinned by a numeric test covering QualityQuick through QualityHigh plus a dedicated test for the non-deficit-stage fallback. Compile-time assertions instages.gokeep every production stage type on the accurate path, and theResamplerinterface doc plus README now describe the new semantics.#53: the Catmull-Rom interpolation bank construction now lives in one shared
buildCubicInterpBankshelper used by bothNewPolyphaseStageand the phase-wrap measurement test, so the test varies only the boundary-indexing policy. The bank-equality assertion pins the extraction bit-for-bit.#54: new
TestQualityRegression_THD_ActiveInterpolationpins full-pipeline THD at 32000 to 44100 and 44100 to 64000, the ratio class where the sub-phase interpolation banks are actually consulted (the #56 defect class). High gets a dedicated -145 dB limit (worst measured -151.63 dB); Medium and Low reuse the existing constants with 6.0 and 9.5 dB margins. The wider ratio survey mentioned in the issue is not included.#55: retired the diverged duplicate polyphase design path (
internal/filter/polyphase.goplus its test file), its only consumercmd/analyze-filter, the production-deadLinearStagewith its constants and test references, the CI build line, and the stale codecov ignore.kaiser.gostays; the engine depends on it. Net -1100 lines.Review: plan and full diff peer-reviewed via agy (Opus 4.6 and Gemini 3.1 Pro), then a 12-agent gate pass (reuse, correctness, quality, integration, regression, test quality, Go performance) plus Gemini cross-validation; all findings fixed in the final commit, with hot-path benchmarks statistically unchanged vs master and allocations identical.
Closes #52
Closes #53
Closes #54
Closes #55
Summary by CodeRabbit
New Features
Flushterminal; subsequent processing starts a fresh stream.Bug Fixes
Documentation