feat: add Gate audio effect (noise gate) completing the dynamics set#2072
Open
yuto-trd wants to merge 5 commits into
Open
feat: add Gate audio effect (noise gate) completing the dynamics set#2072yuto-trd wants to merge 5 commits into
yuto-trd wants to merge 5 commits into
Conversation
Add GateEffect/GateNode, a downward noise gate completing the dynamics- processing set alongside the already-shipped Compressor and Limiter. The gate attenuates the signal by a configurable Range while its level sits below Threshold, with Attack/Hold/Release envelope timing: - GateEffect: additive public AudioEffect exposing Threshold (dB), Attack (ms), Hold (ms), Release (ms) and Range (dB) as animatable CoreProperty- backed IProperty<float>, mirroring CompressorEffect's declaration style. - GateNode: per-sample DSP following the CompressorNode pattern exactly — static/animated Process paths sharing one gate-gain helper, linked-stereo peak detection, hold-timer latching, one-pole attack/release smoothing in the dB domain, cached per-channel buffer handles on the hot loop, envelope continuity across chunks, reset on sample-rate change / time discontinuity, and once-per-parameter non-finite/out-of-range sanitization with recovery. - GateParameters: single source of truth for ranges/defaults shared by the effect's [Range] attributes and the node's clamps. - Registered in LibraryRegistrar so it appears in the audio-effect add menu; EN/JA display names and descriptions added to AudioStrings. Tests: GateNodeTests (DSP behaviour — silence, open/close about threshold, attack time constant, hold latching, range=0 bypass, chunk/seek/sample-rate state continuity, Reset, animated path, static/animated parity, non-finite recovery, mono, zero-length, arity guards) and GateEffectTests (property wiring, defaults, [Range] validators, range consistency). 34 new tests green; full Engine.Audio suite (234) green. NoiseSuppression (SpectralGate) is intentionally NOT included here: the board item's own body phases it separately (phase 1 C# SpectralGate, phase 2 RNNoise/ONNX out of scope), and it is a distinct FFT-based architecture rather than an envelope-follower dynamics processor. It is tracked as a follow-on phase of the same board item, not deferred in-scope work. Refs: Project #9 board item "オーディオエフェクト拡充 (Compressor / Limiter / NoiseSuppression)"
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds a noise gate audio effect with shared parameter definitions, validated animatable properties, static and animated DSP processing, state management, localization, catalog registration, and comprehensive unit tests. ChangesGate Effect Feature
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant AudioContext
participant GateEffect
participant GateNode
participant AudioBuffer
AudioContext->>GateEffect: CreateNode(context, inputNode)
GateEffect->>GateNode: create with gate parameter properties
GateEffect->>AudioContext: connect input node to GateNode
GateEffect-->>AudioContext: return GateNode
AudioContext->>GateNode: Process(context)
alt parameters animated
GateNode->>GateNode: sample chunks and process animated values
else parameters static
GateNode->>GateNode: process static values
end
GateNode->>AudioBuffer: write sanitized gated samples
GateNode-->>AudioContext: return output buffer
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Code Review BotNo comment/code divergences or documentation drift detected. Reviewed 8 file(s); skipped 0. |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a new dynamics audio effect Gate (noise gate) to Beutl’s audio engine and registers it in the app library, including localization and NUnit coverage. This completes the dynamics set alongside the already-added Compressor and Limiter.
Changes:
- Added
GateEffect+ sharedGateParameters(animatable properties, range metadata, node creation/wiring). - Added
GateNodeDSP implementation (linked peak detection, hold + attack/release smoothing, non-finite + clamp hardening, chunk-continuity behavior). - Added comprehensive unit tests and EN/JA localization strings; registered the effect in
LibraryRegistrar.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Beutl.UnitTests/Engine/Audio/GateNodeTests.cs | DSP/state/robustness coverage for GateNode including chunk continuity, resets, and non-finite handling. |
| tests/Beutl.UnitTests/Engine/Audio/GateEffectTests.cs | Verifies effect→node wiring, defaults vs GateParameters, and ScanProperties/range validators. |
| src/Beutl/Services/LibraryRegistrar.cs | Registers GateEffect so it appears in the audio-effect library menu. |
| src/Beutl.Language/AudioStrings.resx | Adds English UI strings for Gate + parameters/descriptions. |
| src/Beutl.Language/AudioStrings.ja.resx | Adds Japanese UI strings for Gate + parameters/descriptions. |
| src/Beutl.Engine/Audio/Graph/Nodes/GateNode.cs | Implements the gate DSP node with static/animated paths and parameter/output sanitization. |
| src/Beutl.Engine/Audio/Effects/GateParameters.cs | Centralizes Gate ranges/defaults shared by effect metadata and DSP clamps. |
| src/Beutl.Engine/Audio/Effects/GateEffect.cs | Defines the public audio effect surface (properties + node creation) with localization metadata. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
tests/Beutl.UnitTests/Engine/Audio/GateNodeTests.cs (2)
18-47: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor duplication between
PeakDbandPeakDbInWindow.
PeakDbcould be expressed asPeakDbInWindow(buffer, startSample, buffer.SampleCount - startSample)to avoid maintaining two near-identical peak-scan loops.🤖 Prompt for 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. In `@tests/Beutl.UnitTests/Engine/Audio/GateNodeTests.cs` around lines 18 - 47, There is duplicated peak-scan logic between PeakDb and PeakDbInWindow in GateNodeTests. Refactor PeakDb to delegate to PeakDbInWindow by passing the remaining buffer length, and keep PeakDbInWindow as the single loop implementation for computing the peak over a range.
611-647: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueWhite-box coupling to internal chunk size is acceptable but fragile.
This test hardcodes
chunkSize = 1024, matchingProcessAnimated's internal chunking. If that internal constant changes, the test will silently start failing rather than pointing at the cause. Given it's an intentional continuity check per the comment, no action required, just flagging for future maintainers.🤖 Prompt for 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. In `@tests/Beutl.UnitTests/Engine/Audio/GateNodeTests.cs` around lines 611 - 647, The Process_AnimatedPath_SmoothAcrossChunkBoundary test is tightly coupled to ProcessAnimated’s internal chunk size via the hardcoded chunkSize value. Replace the literal with the shared chunk-size source used by GateNode.ProcessAnimated, or route the assertion through a helper/constant so the test stays aligned if chunking changes. Keep the continuity check logic and the references to KeyFrameAnimation, GateNode, and ProcessAnimated intact.tests/Beutl.UnitTests/Engine/Audio/GateEffectTests.cs (1)
70-112: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider unifying duplicated (min, default, max) tuples.
AssertNameAndRange(Lines 77-81) andParameterRanges()(Lines 98-105) both enumerate the same five parameter triples. A sharedTestCaseSourcewould avoid keeping two lists in sync as parameters evolve.🤖 Prompt for 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. In `@tests/Beutl.UnitTests/Engine/Audio/GateEffectTests.cs` around lines 70 - 112, The tests duplicate the same min/default/max tuples in AssertNameAndRange and ParameterRanges, so keep the GateEffect parameter set in one shared source instead of two separate lists. Refactor the GateEffectTests helpers so both the property validation in ScanProperties_RegistersNamesAndRangeValidatorsForEveryProperty and the range assertions in GateParameters_RangeIsConsistent consume a single TestCaseSource or shared tuple provider, using the existing symbols like ParameterRanges and AssertNameAndRange to locate the change.
🤖 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.
Nitpick comments:
In `@tests/Beutl.UnitTests/Engine/Audio/GateEffectTests.cs`:
- Around line 70-112: The tests duplicate the same min/default/max tuples in
AssertNameAndRange and ParameterRanges, so keep the GateEffect parameter set in
one shared source instead of two separate lists. Refactor the GateEffectTests
helpers so both the property validation in
ScanProperties_RegistersNamesAndRangeValidatorsForEveryProperty and the range
assertions in GateParameters_RangeIsConsistent consume a single TestCaseSource
or shared tuple provider, using the existing symbols like ParameterRanges and
AssertNameAndRange to locate the change.
In `@tests/Beutl.UnitTests/Engine/Audio/GateNodeTests.cs`:
- Around line 18-47: There is duplicated peak-scan logic between PeakDb and
PeakDbInWindow in GateNodeTests. Refactor PeakDb to delegate to PeakDbInWindow
by passing the remaining buffer length, and keep PeakDbInWindow as the single
loop implementation for computing the peak over a range.
- Around line 611-647: The Process_AnimatedPath_SmoothAcrossChunkBoundary test
is tightly coupled to ProcessAnimated’s internal chunk size via the hardcoded
chunkSize value. Replace the literal with the shared chunk-size source used by
GateNode.ProcessAnimated, or route the assertion through a helper/constant so
the test stays aligned if chunking changes. Keep the continuity check logic and
the references to KeyFrameAnimation, GateNode, and ProcessAnimated intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: fb3b5d70-ca00-42fc-98e4-68e07b242e25
📒 Files selected for processing (8)
src/Beutl.Engine/Audio/Effects/GateEffect.cssrc/Beutl.Engine/Audio/Effects/GateParameters.cssrc/Beutl.Engine/Audio/Graph/Nodes/GateNode.cssrc/Beutl.Language/AudioStrings.ja.resxsrc/Beutl.Language/AudioStrings.resxsrc/Beutl/Services/LibraryRegistrar.cstests/Beutl.UnitTests/Engine/Audio/GateEffectTests.cstests/Beutl.UnitTests/Engine/Audio/GateNodeTests.cs
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1b3650cda7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Address three DSP correctness findings on the noise gate: 1. Range=0 is now an exact identity. Previously the gate-gain follower still smoothed toward 0 dB from its closed floor, so "gating disabled" (Range=0) introduced a startup attack ramp and an asymptotic sub-unity gain. NextGain now snaps the follower open and returns unit gain when Range >= 0, so output equals input sample-for-sample. 2. Linked-stereo peak detection no longer propagates NaN. The mono/stereo fast path seeded `peak = Abs(s0)`, so a NaN in one channel poisoned the peak (`a1 > NaN` is false) and mis-gated a valid channel. It now seeds 0 and compares (NaN > 0 is false), matching the >2-channel path, so a finite channel still drives the gate. 3. Digital silence no longer opens the gate at the -100 dB minimum Threshold. Silence collapses to inputDb = MinDb (-100), which compared equal to the minimum threshold (-100 >= -100) and opened the gate. Opening now also requires actual signal energy (peak > 0f), so -inf dB silence stays closed. Each fix has a regression test (Process_RangeZero_IsExactIdentity, Process_OneChannelNaN_GatesFromValidChannel, Process_MinThreshold_DigitalSilenceDoesNotOpenGate); all three fail against the prior GateNode and pass after the fix. Full Engine.Audio suite (237) green, dotnet format clean. Note: finding 2 (fast-path NaN peak seeding) also exists in the already-merged CompressorNode (its >2-channel path is safe; the mono/stereo fast path is not); LimiterNode is unaffected (it coerces non-finite samples to 0 first). That existing-node fix is intentionally out of scope for this PR. Refs: Project #9 board item "オーディオエフェクト拡充 (Compressor / Limiter / NoiseSuppression)"
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: de710f38b5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ding Address two Codex review findings on GateNode: - Seed the gain follower at the effective Range floor on the first sample after a reset instead of the -100 dB sentinel, so a below-threshold lead-in rests at the user's Range floor rather than fading in from near-silence. An above-threshold onset still ramps open from that floor. - Match LimiterNode's one-tick timestamp tolerance when detecting chunk discontinuity, so adjacent sample-boundary buffers whose start differs by one tick from independent TimeSpan rounding are not misread as a seek and do not spuriously reset the gate. Regression tests (red on the previous commit): Process_BelowThresholdLeadIn_StartsAtRangeFloorNotFullMute, Process_OneTickBoundaryRounding_DoesNotResetGate.
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 `@tests/Beutl.UnitTests/Engine/Audio/GateNodeTests.cs`:
- Around line 755-766: The test Process_OneTickBoundaryRounding_DoesNotResetGate
currently covers only a one-tick overlap; add coverage for a one-tick gap by
also creating the second context with chunkDuration + TimeSpan.FromTicks(1), and
verify the warmed-open gate remains continuous rather than resetting.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c360737f-785e-4657-ab08-8b19a289cc8e
📒 Files selected for processing (2)
src/Beutl.Engine/Audio/Graph/Nodes/GateNode.cstests/Beutl.UnitTests/Engine/Audio/GateNodeTests.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Beutl.Engine/Audio/Graph/Nodes/GateNode.cs
Confidence Score: 5/5Purely additive change with no modifications to existing types; all new code is well-guarded and pattern-consistent with sibling nodes. The DSP logic is carefully implemented: the envelope follower, hold latch, Range-floor priming, and non-finite recovery all behave correctly on inspection, and the 34 tests exercise the edge cases (silence, attack/release time constants, hold off-by-one, chunk-boundary continuity, seek/sample-rate resets, NaN/Infinity inputs). No existing API surface is changed. Files Needing Attention: The channels > 2 branches in ProcessStatic and ProcessAnimated have no test coverage, as noted in the prior review. Everything else is well-exercised. Important Files Changed
Reviews (3): Last reviewed commit: "fix(engine): correct Gate hold length, r..." | Re-trigger Greptile |
Address a CodeRabbit review: Process_OneTickBoundaryRounding_DoesNotResetGate only exercised a one-tick overlap. The discontinuity tolerance is symmetric (±1 tick), so parametrize the test over both a one-tick overlap (-1) and a one-tick gap (+1); an asymmetric implementation could pass one direction and reset on the other.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5be4d8ad62
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Address three Codex review findings on GateNode: - Read the hold latch for the current sample before spending it. Decrementing first kept the gate open for only N-1 of N configured hold samples, and a hold that converts to a single sample released immediately. - Reject an upstream buffer whose sample rate differs from the process context, as LimiterNode already does. Coefficients and the hold count come from the context rate while the output buffer carries the input rate, so a mismatch mislabelled the samples instead of resampling them. - Fall back to the declared GateParameters constant when a property's DefaultValue is itself non-finite. Returning a non-finite default defeated the range clamp (NaN clamps to NaN) and pinned the gate to its recovery sentinel. Regression tests (all red on the previous commit): Process_Hold_KeepsGateOpenForEveryConfiguredSample, Process_InputSampleRateMismatch_Throws, Process_NonFinitePropertyDefault_FallsBackToDeclaredConstant.
Contributor
|
No TODO comments were found. |
Contributor
Minimum allowed line rate is |
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.
Summary
Adds a Gate (noise gate) audio effect, completing the dynamics-processing set alongside the already-merged Compressor (#1724) and Limiter (#1725):
GateEffect(new,Beutl.Engine.Audio.Effects) — animatableThreshold/Attack/Hold/Release/Rangeproperties, faithful clone of theCompressorEffectsurface pattern (ScanProperties,[Range]/[Display]attributes,CreateNodeviaAddNode+Connect).GateNode(new,Beutl.Engine.Audio.Graph.Nodes) — per-sample DSP mirroringCompressorNode: static/animated paths sharing one gate-gain helper, linked-stereo peak detection, hold-timer latching, one-pole dB-domain attack/release envelope, pooled per-channel handle cache, envelope continuity across chunk boundaries, reset on sample-rate/time discontinuity, non-finite parameter sanitization.GateParameters(internal) — shared ranges/defaults, same convention asCompressorParameters/LimiterParameters.LibraryRegistrarregistration + EN/JA display strings, so Gate appears in the audio-effect add menu (parameter UI is auto-generated byAudioEffectEditor).All additive — no existing type's signature changes; non-breaking
feat:.Board-item scope note
The board item's 現状把握 predates #1724/#1725: Compressor and Limiter are already on
mainwith full tests, so only Gate remained in the dynamics family. NoiseSuppression (SpectralGate) is a distinct FFT/STFT architecture that the item body itself splits into a separate phase; it is intentionally not part of this PR (no TODO markers left in code — the phase boundary is documented here and in the commit body). Future naming is collision-free (SpectralGate*vsGate*).Tests
GateNodeTests+GateEffectTests— 34 new tests: silence input, threshold open/close behavior, attack/release time characteristics, hold latching, range floor, chunk-split processing equals single-block processing (glitch guard), parameter sanitization, node reuse/reset.Engine.Audionamespace: 234 passed / 0 failed (no regression).dotnet format --verify-no-changesclean; UTF-8 BOM on all new.csfiles.Review
DynamicsNoderefactor across all three.Manual verification
GateNodedrives both).Refs: Project #9 board item「オーディオエフェクト拡充 (Compressor / Limiter / NoiseSuppression)」
🤖 Generated with Claude Code
Summary by CodeRabbit