fix: output silence when graph is disconnected#334
Merged
sudip-mondal-2002 merged 8 commits intoJun 3, 2026
Merged
Conversation
github-actions
Bot
added
good first issue
Contributor
|
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:
📝 WalkthroughWalkthroughAudioGraphExecutor now zeroes output and returns for oversized blocks or empty execution plans. Integration tests updated to expect silence in those cases and new split/merge disconnect tests added. Several audio-engine tests prepend a disabled Distortion. CMake gets a missing endif(); CabinetSim HPF coefficients adjusted. ChangesDisconnected Graph Silence Behavior
CMake fix
CabinetSim HPF update
Sequence Diagram sequenceDiagram
participant Tests
participant AudioGraphExecutor
participant ExecutionPlan
participant OutputBuffer
Tests->>AudioGraphExecutor: call process(input, output, num_samples)
AudioGraphExecutor->>ExecutionPlan: check execution_plan_
alt execution_plan_ empty
AudioGraphExecutor->>OutputBuffer: zero(num_samples)
AudioGraphExecutor->>Tests: return
else
AudioGraphExecutor->>AudioGraphExecutor: compare num_samples to max_block_size_
alt num_samples > max_block_size_
AudioGraphExecutor->>OutputBuffer: zero(num_samples)
AudioGraphExecutor->>Tests: return
else
AudioGraphExecutor->>ExecutionPlan: execute plan (normal processing)
AudioGraphExecutor->>OutputBuffer: write processed samples
AudioGraphExecutor->>Tests: return
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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 `@src/audio/engine/audio_graph_executor.cpp`:
- Around line 109-111: The oversized-block branch in audio_graph_executor.cpp
only zeros max_block_size_ samples leaving the rest of the caller's output
buffer with stale data; update the branch that checks if (num_samples >
max_block_size_) to clear all num_samples floats in output (use a size-safe cast
to size_t and multiply by sizeof(float) or std::fill_n) before returning so the
entire provided buffer is silent; refer to variables/functions num_samples,
max_block_size_, and output when making the change.
🪄 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
Run ID: cd4c1897-c923-4661-8284-112671ed2447
📒 Files selected for processing (2)
src/audio/engine/audio_graph_executor.cpptests/integration/test_audio_graph.cpp
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/integration/test_audio_engine.cpp (1)
26-28: 💤 Low valueConsider extracting the disabled-effect pattern to a test fixture helper.
The 3-line pattern for adding a disabled
Distortioneffect is repeated identically across 5 tests. While acceptable for test clarity, extracting this toAudioEngineTestas a helper method (e.g.,add_bypass_effect()) would reduce duplication and make the intent clearer.♻️ Example refactor
Add to
AudioEngineTestfixture (likely intests/integration/test_fixtures.h):void add_bypass_effect() { auto bypass = std::make_shared<Distortion>(); bypass->set_enabled(false); engine.add_effect(bypass); }Then replace the 3-line blocks with:
- auto bypass = std::make_shared<Distortion>(); - bypass->set_enabled(false); - engine.add_effect(bypass); + add_bypass_effect();Also applies to: 37-39, 49-51, 66-68, 297-299
🤖 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/integration/test_audio_engine.cpp` around lines 26 - 28, Duplicate 3-line pattern creating a disabled Distortion and adding it to the engine appears in multiple tests; extract this into a fixture helper to reduce duplication. Add a method on the AudioEngineTest fixture (e.g., add_bypass_effect()) that constructs a std::shared_ptr<Distortion>, calls set_enabled(false) on it, and calls engine.add_effect(...) with that pointer, then replace each repeated block (the pattern using Distortion, set_enabled, engine.add_effect) in tests with a call to add_bypass_effect().
🤖 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/integration/test_audio_engine.cpp`:
- Around line 26-28: Duplicate 3-line pattern creating a disabled Distortion and
adding it to the engine appears in multiple tests; extract this into a fixture
helper to reduce duplication. Add a method on the AudioEngineTest fixture (e.g.,
add_bypass_effect()) that constructs a std::shared_ptr<Distortion>, calls
set_enabled(false) on it, and calls engine.add_effect(...) with that pointer,
then replace each repeated block (the pattern using Distortion, set_enabled,
engine.add_effect) in tests with a call to add_bypass_effect().
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 87eaebfd-e7fb-49aa-999f-5bcf1de3ea35
📒 Files selected for processing (2)
tests/integration/test_audio_engine.cpptests/integration/test_audio_graph.cpp
Contributor
🌍 Cross-Platform Coverage ReportPlatforms: Linux + macOS + Windows ✅ Coverage meets threshold: 76.7% >= 60% Full Merged Coverage Summary |
Contributor
PR Preview RemovedThe GitHub Pages preview for this PR has been removed because the PR was closed. |
Owner
|
Add few new clean tests The original graph looks like input ->> splitter ->> A ->> Merge ->> Amp If we pass input to original, it should produce output |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/integration/test_audio_graph.cpp (1)
1116-1142: ⚡ Quick winAssert the exact branch sums, not just “non-zero”.
These three cases are deterministic with the current executor contract: full split/merge should produce
2.0f, and the single-branch cases should produce1.0f. Using> 0.0fwill still pass if a stale route leaks extra signal or a disconnected branch is accidentally still accumulated.src/audio/engine/audio_graph_executor.cpp:128-173makes the expected values exact here.Suggested assertion tightening
- ASSERT_TRUE(output_audio[0] > 0.0f); + ASSERT_TRUE(output_audio[0] == 2.0f); ... - ASSERT_TRUE(output_audio[0] > 0.0f); + ASSERT_TRUE(output_audio[0] == 1.0f); ... - ASSERT_TRUE(output_audio[0] > 0.0f); + ASSERT_TRUE(output_audio[0] == 1.0f);🤖 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/integration/test_audio_graph.cpp` around lines 1116 - 1142, The tests currently assert only that output_audio[0] > 0.0f; change these to assert exact expected branch sums per the executor contract: when both branches are active expect 2.0f and when only a single branch is active expect 1.0f. Update the three ASSERT_TRUE checks after executor.process(...) to use precise float comparisons (e.g., ASSERT_FLOAT_EQ or ASSERT_NEAR with a tiny epsilon) against 2.0f or 1.0f respectively, keeping references to the same topology changes (graph.rebuild_topology(), executor.compile(graph), executor.process(...), and the link variables l_split_a / l_split_b) so the assertions map to the correct cases.
🤖 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 `@CMakeLists.txt`:
- Around line 103-125: The config-based find_package(portaudio CONFIG QUIET)
branch sets PORTAUDIO_FOUND when TARGET portaudio exists but the subsequent
PkgConfig/pk_check_modules and manual find_path/find_library logic can overwrite
those variables; modify the fallback so the pkg_check_modules(PORTAUDIO ...) and
the manual find_path/find_library steps (and the
link_directories(${PORTAUDIO_LIBRARY_DIRS}) call) only run when the CONFIG
target was not found (e.g., wrap them in an if(NOT TARGET portaudio) or if(NOT
PORTAUDIO_FOUND) guard), preventing the CONFIG-selected target and its
PORTAUDIO_* values from being clobbered.
In `@tests/integration/test_audio_graph.cpp`:
- Around line 1099-1114: The test currently calls graph.add_link and
graph.remove_link (e.g., when creating l_in_split, l_split_a, l_split_b,
l_a_merge, l_b_merge, l_merge_amp and other link/remove calls) without checking
the return values; add assertions immediately after each mutation that validate
the operation succeeded (for example assert/EXPECT that add_link returned a
valid id and remove_link returned success/true) so the test fails fast if any
topology mutation fails and subsequent silence checks aren't masking a bad graph
shape. Ensure you add these guard assertions for every add_link/remove_link
usage in this block and the other mentioned ranges (1126-1127, 1135-1138,
1145-1147, 1155-1163).
---
Nitpick comments:
In `@tests/integration/test_audio_graph.cpp`:
- Around line 1116-1142: The tests currently assert only that output_audio[0] >
0.0f; change these to assert exact expected branch sums per the executor
contract: when both branches are active expect 2.0f and when only a single
branch is active expect 1.0f. Update the three ASSERT_TRUE checks after
executor.process(...) to use precise float comparisons (e.g., ASSERT_FLOAT_EQ or
ASSERT_NEAR with a tiny epsilon) against 2.0f or 1.0f respectively, keeping
references to the same topology changes (graph.rebuild_topology(),
executor.compile(graph), executor.process(...), and the link variables l_split_a
/ l_split_b) so the assertions map to the correct cases.
🪄 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
Run ID: 092a9022-278a-426c-b0d3-3d8f986ff4ad
⛔ Files ignored due to path filters (2)
short.wavis excluded by!**/*.wavvery_long_ir_file_name_that_should_be_truncated.wavis excluded by!**/*.wav
📒 Files selected for processing (3)
CMakeLists.txtsrc/audio/engine/audio_graph_executor.cpptests/integration/test_audio_graph.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- src/audio/engine/audio_graph_executor.cpp
Contributor
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/integration/test_audio_graph.cpp (1)
1116-1142: ⚡ Quick winAssert the exact branch sums, not just “non-zero”.
These three cases are deterministic with the current executor contract: full split/merge should produce
2.0f, and the single-branch cases should produce1.0f. Using> 0.0fwill still pass if a stale route leaks extra signal or a disconnected branch is accidentally still accumulated.src/audio/engine/audio_graph_executor.cpp:128-173makes the expected values exact here.Suggested assertion tightening
- ASSERT_TRUE(output_audio[0] > 0.0f); + ASSERT_TRUE(output_audio[0] == 2.0f); ... - ASSERT_TRUE(output_audio[0] > 0.0f); + ASSERT_TRUE(output_audio[0] == 1.0f); ... - ASSERT_TRUE(output_audio[0] > 0.0f); + ASSERT_TRUE(output_audio[0] == 1.0f);🤖 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/integration/test_audio_graph.cpp` around lines 1116 - 1142, The tests currently assert only that output_audio[0] > 0.0f; change these to assert exact expected branch sums per the executor contract: when both branches are active expect 2.0f and when only a single branch is active expect 1.0f. Update the three ASSERT_TRUE checks after executor.process(...) to use precise float comparisons (e.g., ASSERT_FLOAT_EQ or ASSERT_NEAR with a tiny epsilon) against 2.0f or 1.0f respectively, keeping references to the same topology changes (graph.rebuild_topology(), executor.compile(graph), executor.process(...), and the link variables l_split_a / l_split_b) so the assertions map to the correct cases.
🤖 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 `@CMakeLists.txt`:
- Around line 103-125: The config-based find_package(portaudio CONFIG QUIET)
branch sets PORTAUDIO_FOUND when TARGET portaudio exists but the subsequent
PkgConfig/pk_check_modules and manual find_path/find_library logic can overwrite
those variables; modify the fallback so the pkg_check_modules(PORTAUDIO ...) and
the manual find_path/find_library steps (and the
link_directories(${PORTAUDIO_LIBRARY_DIRS}) call) only run when the CONFIG
target was not found (e.g., wrap them in an if(NOT TARGET portaudio) or if(NOT
PORTAUDIO_FOUND) guard), preventing the CONFIG-selected target and its
PORTAUDIO_* values from being clobbered.
In `@tests/integration/test_audio_graph.cpp`:
- Around line 1099-1114: The test currently calls graph.add_link and
graph.remove_link (e.g., when creating l_in_split, l_split_a, l_split_b,
l_a_merge, l_b_merge, l_merge_amp and other link/remove calls) without checking
the return values; add assertions immediately after each mutation that validate
the operation succeeded (for example assert/EXPECT that add_link returned a
valid id and remove_link returned success/true) so the test fails fast if any
topology mutation fails and subsequent silence checks aren't masking a bad graph
shape. Ensure you add these guard assertions for every add_link/remove_link
usage in this block and the other mentioned ranges (1126-1127, 1135-1138,
1145-1147, 1155-1163).
---
Nitpick comments:
In `@tests/integration/test_audio_graph.cpp`:
- Around line 1116-1142: The tests currently assert only that output_audio[0] >
0.0f; change these to assert exact expected branch sums per the executor
contract: when both branches are active expect 2.0f and when only a single
branch is active expect 1.0f. Update the three ASSERT_TRUE checks after
executor.process(...) to use precise float comparisons (e.g., ASSERT_FLOAT_EQ or
ASSERT_NEAR with a tiny epsilon) against 2.0f or 1.0f respectively, keeping
references to the same topology changes (graph.rebuild_topology(),
executor.compile(graph), executor.process(...), and the link variables l_split_a
/ l_split_b) so the assertions map to the correct cases.
🪄 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
Run ID: 092a9022-278a-426c-b0d3-3d8f986ff4ad
⛔ Files ignored due to path filters (2)
short.wavis excluded by!**/*.wavvery_long_ir_file_name_that_should_be_truncated.wavis excluded by!**/*.wav
📒 Files selected for processing (3)
CMakeLists.txtsrc/audio/engine/audio_graph_executor.cpptests/integration/test_audio_graph.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- src/audio/engine/audio_graph_executor.cpp
🛑 Comments failed to post (2)
CMakeLists.txt (1)
103-125:
⚠️ Potential issue | 🟠 Major | ⚡ Quick win🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '\n--- PortAudio block ---\n' sed -n '100,145p' CMakeLists.txt printf '\n--- PortAudio discovery statements ---\n' rg -n 'find_package\(portaudio|pkg_check_modules\(PORTAUDIO|find_path\(PORTAUDIO_INCLUDE_DIRS|find_library\(PORTAUDIO_LIBRARIES|link_directories\(\$\{PORTAUDIO_LIBRARY_DIRS\}\)' CMakeLists.txtRepository: sudip-mondal-2002/Amplitron
Length of output: 2499
Guard PortAudio fallback so config-target selection isn’t overwritten
The
find_package(portaudio CONFIG QUIET)branch setsPORTAUDIO_FOUNDwhenTARGET portaudioexists, but the subsequentfind_package(PkgConfig QUIET)+pkg_check_modules(PORTAUDIO portaudio-2.0)runs unconditionally and can overwritePORTAUDIO_*(libs/include dirs/library dirs), plus repeatslink_directories().Suggested fix
find_package(portaudio CONFIG QUIET) if(TARGET portaudio) set(PORTAUDIO_FOUND TRUE) set(PORTAUDIO_LIBRARIES portaudio) else() find_package(PkgConfig QUIET) if(PkgConfig_FOUND) pkg_check_modules(PORTAUDIO portaudio-2.0) endif() if(NOT PORTAUDIO_FOUND) find_path(PORTAUDIO_INCLUDE_DIRS portaudio.h PATHS /usr/include /usr/local/include /opt/homebrew/include "C:/ Program Files/portaudio/include" "${CMAKE_SOURCE_DIR}/external/portaudio/include") find_library(PORTAUDIO_LIBRARIES NAMES portaudio portaudio_x64 PATHS /usr/lib /usr/local/lib /opt/homebrew/lib "C:/ Program Files/portaudio/lib" "${CMAKE_SOURCE_DIR}/external/portaudio/lib") endif() if(PORTAUDIO_LIBRARY_DIRS) link_directories(${PORTAUDIO_LIBRARY_DIRS}) endif() endif() - -find_package(PkgConfig QUIET) -if(PkgConfig_FOUND) - pkg_check_modules(PORTAUDIO portaudio-2.0) -endif() -if(NOT PORTAUDIO_FOUND) - find_path(PORTAUDIO_INCLUDE_DIRS portaudio.h - PATHS /usr/include /usr/local/include /opt/homebrew/include - "C:/ Program Files/portaudio/include" - "${CMAKE_SOURCE_DIR}/external/portaudio/include") - find_library(PORTAUDIO_LIBRARIES NAMES portaudio portaudio_x64 - PATHS /usr/lib /usr/local/lib /opt/homebrew/lib - "C:/ Program Files/portaudio/lib" - "${CMAKE_SOURCE_DIR}/external/portaudio/lib") -endif() -if(PORTAUDIO_LIBRARY_DIRS) - link_directories(${PORTAUDIO_LIBRARY_DIRS}) -endif()🤖 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 `@CMakeLists.txt` around lines 103 - 125, The config-based find_package(portaudio CONFIG QUIET) branch sets PORTAUDIO_FOUND when TARGET portaudio exists but the subsequent PkgConfig/pk_check_modules and manual find_path/find_library logic can overwrite those variables; modify the fallback so the pkg_check_modules(PORTAUDIO ...) and the manual find_path/find_library steps (and the link_directories(${PORTAUDIO_LIBRARY_DIRS}) call) only run when the CONFIG target was not found (e.g., wrap them in an if(NOT TARGET portaudio) or if(NOT PORTAUDIO_FOUND) guard), preventing the CONFIG-selected target and its PORTAUDIO_* values from being clobbered.tests/integration/test_audio_graph.cpp (1)
1099-1114:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winCheck every topology mutation in the test setup.
Several
add_link/remove_linkcalls here are unchecked. If one of those mutations fails, the later silence assertions can still pass against the wrong graph shape, which makes this test a false positive.Suggested guard assertions
- int l_in_split = graph.add_link(nodes[0].output_pin_ids[0], nodes[1].input_pin_ids[0]); + int l_in_split = graph.add_link(nodes[0].output_pin_ids[0], nodes[1].input_pin_ids[0]); + ASSERT_TRUE(l_in_split != -1); ... - int l_split_a = graph.add_link(nodes[1].output_pin_ids[0], nodes[2].input_pin_ids[0]); + int l_split_a = graph.add_link(nodes[1].output_pin_ids[0], nodes[2].input_pin_ids[0]); + ASSERT_TRUE(l_split_a != -1); ... - int l_split_b = graph.add_link(nodes[1].output_pin_ids[1], nodes[3].input_pin_ids[0]); + int l_split_b = graph.add_link(nodes[1].output_pin_ids[1], nodes[3].input_pin_ids[0]); + ASSERT_TRUE(l_split_b != -1); ... - int l_a_merge = graph.add_link(nodes[2].output_pin_ids[0], nodes[4].input_pin_ids[0]); + int l_a_merge = graph.add_link(nodes[2].output_pin_ids[0], nodes[4].input_pin_ids[0]); + ASSERT_TRUE(l_a_merge != -1); ... - int l_b_merge = graph.add_link(nodes[3].output_pin_ids[0], nodes[4].input_pin_ids[1]); + int l_b_merge = graph.add_link(nodes[3].output_pin_ids[0], nodes[4].input_pin_ids[1]); + ASSERT_TRUE(l_b_merge != -1); ... - int l_merge_amp = graph.add_link(nodes[4].output_pin_ids[0], nodes[5].input_pin_ids[0]); + int l_merge_amp = graph.add_link(nodes[4].output_pin_ids[0], nodes[5].input_pin_ids[0]); + ASSERT_TRUE(l_merge_amp != -1); ... - graph.remove_link(l_split_a); + ASSERT_TRUE(graph.remove_link(l_split_a)); ... - l_split_a = graph.add_link(nodes[1].output_pin_ids[0], nodes[2].input_pin_ids[0]); - graph.remove_link(l_split_b); + l_split_a = graph.add_link(nodes[1].output_pin_ids[0], nodes[2].input_pin_ids[0]); + ASSERT_TRUE(l_split_a != -1); + ASSERT_TRUE(graph.remove_link(l_split_b)); ... - graph.remove_link(l_split_a); + ASSERT_TRUE(graph.remove_link(l_split_a)); ... - l_split_a = graph.add_link(nodes[1].output_pin_ids[0], nodes[2].input_pin_ids[0]); - l_split_b = graph.add_link(nodes[1].output_pin_ids[1], nodes[3].input_pin_ids[0]); + l_split_a = graph.add_link(nodes[1].output_pin_ids[0], nodes[2].input_pin_ids[0]); + l_split_b = graph.add_link(nodes[1].output_pin_ids[1], nodes[3].input_pin_ids[0]); + ASSERT_TRUE(l_split_a != -1); + ASSERT_TRUE(l_split_b != -1); ... - graph.remove_link(l_a_merge); - graph.remove_link(l_split_b); + ASSERT_TRUE(graph.remove_link(l_a_merge)); + ASSERT_TRUE(graph.remove_link(l_split_b));Also applies to: 1126-1127, 1135-1138, 1145-1147, 1155-1163
🤖 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/integration/test_audio_graph.cpp` around lines 1099 - 1114, The test currently calls graph.add_link and graph.remove_link (e.g., when creating l_in_split, l_split_a, l_split_b, l_a_merge, l_b_merge, l_merge_amp and other link/remove calls) without checking the return values; add assertions immediately after each mutation that validate the operation succeeded (for example assert/EXPECT that add_link returned a valid id and remove_link returned success/true) so the test fails fast if any topology mutation fails and subsequent silence checks aren't masking a bad graph shape. Ensure you add these guard assertions for every add_link/remove_link usage in this block and the other mentioned ranges (1126-1127, 1135-1138, 1145-1147, 1155-1163).
sudip-mondal-2002
requested changes
Jun 2, 2026
sudip-mondal-2002
left a comment
sudip-mondal-2002
left a comment
Owner
There was a problem hiding this comment.
@MohitBareja16
The starting item in the preview
Input -> cabinet sim -> amp
without any changes doesn't create outputs
please check. Please fix
Also I would prefer the starting preset to be
Input->reverb->amp
Collaborator
Author
|
Check preview pr, I changed the starting preset to reverb @sudip-mondal-2002 |
sudip-mondal-2002
approved these changes
Jun 3, 2026
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
2 participants
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.
What does this PR do?
This PR fixes a bug in the DSP pipeline where an empty or completely disconnected audio graph would inadvertently bypass the dry input signal to the output instead of correctly muting it.
Specifically, in
AudioGraphExecutor::process, when theexecution_plan_was empty (no connected pedals) or an oversized block was received, it copied the input buffer to the output usingstd::memcpy. This has been updated to output absolute silence usingstd::memset(zeroing the buffer) instead. It also updates the respective integration tests to assert for silence instead of passthrough.Related Issue
Fixes #273
Type of Change
How Was This Tested?
./build/tests/integration/integration_testsChecklist
./amplitron-tests)std::mutex::lock()on the hot path)Screenshots / Demo
N/A - purely backend DSP logic fix (no visual changes).
Summary by CodeRabbit
Bug Fixes
Tests
Chores