feat: Add JACK audio backend for ultra-low latency on Linux (#58)#115
Closed
vansh-09 wants to merge 13 commits into
Closed
feat: Add JACK audio backend for ultra-low latency on Linux (#58)#115vansh-09 wants to merge 13 commits into
vansh-09 wants to merge 13 commits into
Conversation
Contributor
📝 WalkthroughWalkthroughAdds an optional JACK audio backend selectable via CMake (WITH_JACK), normalizes native library discovery, implements JACK-specific types and lifecycle with a process callback, wires conditional build/link/include for Amplitron and tests, adds guarded integration tests, and refactors related tests and GUI code. ChangesJACK Audio Backend Feature
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant AudioEngine as AudioEngine
participant JACK as JACK Server
participant Callback as jack_process Callback
App->>AudioEngine: initialize()
AudioEngine->>JACK: jack_client_open()
AudioEngine->>JACK: jack_set_process_callback(jack_process)
AudioEngine->>JACK: jack_port_register(input_port)
AudioEngine->>JACK: jack_port_register(output_port)
AudioEngine->>JACK: query sample_rate, buffer_size
App->>AudioEngine: start()
AudioEngine->>JACK: jack_activate()
JACK->>Callback: jack_process(nframes)
Callback->>AudioEngine: process_audio()
AudioEngine->>JACK: write to output buffers
App->>AudioEngine: stop()
AudioEngine->>JACK: jack_deactivate()
App->>AudioEngine: shutdown()
AudioEngine->>JACK: jack_client_close()
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 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 |
Collaborator
|
@vansh-09 It is failing in Linux, please fix it. |
Collaborator
|
@vansh-09 Actually, the compiler is not recognizing jack_nframes_t in your engine. Try including the header file and include directories in cmakelists.txt. |
![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: 3
🤖 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 265-295: The WITH_JACK branch sets BACKEND_SOURCES to JACK files
before confirming JACK was actually found; after the pkg_check_modules /
find_path / find_library attempts check whether JACK_INCLUDE_DIRS and
JACK_LIBRARIES (or JACK_FOUND) are set, and if they are still missing, emit a
warning via message(WARNING ...) and set WITH_JACK to OFF (or clear the
JACK-specific vars), then fall back to the PortAudio sources by re-evaluating
BACKEND_SOURCES; ensure you reference and update the existing symbols
(WITH_JACK, JACK_INCLUDE_DIRS, JACK_LIBRARIES, JACK_FOUND, BACKEND_SOURCES) so
compilation will use portaudio if JACK resolution fails.
In `@src/audio/audio_backend_jack_lifecycle.cpp`:
- Around line 34-46: The initialize() method currently reopens JACK and
overwrites backend_->client if called twice; add a guard at the top of
AudioEngine::initialize() that returns true immediately when initialized_ is
already set, or alternatively call the teardown/close path before reopening;
specifically, check the member initialized_ (or set one if absent) and return
true, or call the existing shutdown/close helper (or implement one to call
jack_client_close on backend_->client and reset backend_) before calling
create_audio_backend() and jack_client_open(), and ensure you set initialized_ =
true only after successful jack_client_open and clear it on failure/teardown.
- Around line 141-142: set_input_device and set_output_device currently return
true without changing state; update them to validate the provided device index,
assign it to the corresponding member (input_device_ / output_device_), and then
apply the change to the running engine (e.g. reconfigure or restart the JACK
client / invoke the existing audio reinitialization routine). Ensure the
functions return false for invalid indices or if reconfiguration fails and true
only on successful assignment+apply so UI/settings stay in sync with the engine.
🪄 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: 98ba64fe-75cd-4895-9dbb-2aaf73e210e5
📒 Files selected for processing (9)
CMakeLists.txtscripts/setup_dependencies.shsrc/audio/audio_backend_jack.cppsrc/audio/audio_backend_jack_internal.hsrc/audio/audio_backend_jack_lifecycle.cppsrc/audio/audio_engine.hsrc/gui/pedal_widget_body.cpptests/test_jack_backend.cpptests/test_json_serialization.cpp
Comment on lines
+265
to
+295
| if(WITH_JACK) | ||
| if(NOT JACK_INCLUDE_DIRS OR NOT JACK_LIBRARIES) | ||
| find_package(PkgConfig QUIET) | ||
| if(PkgConfig_FOUND) | ||
| pkg_check_modules(JACK QUIET jack) | ||
| endif() | ||
| if(NOT JACK_FOUND) | ||
| find_path(JACK_INCLUDE_DIRS jack/jack.h | ||
| PATHS /usr/include /usr/local/include /opt/homebrew/include | ||
| "C:/Program Files/JACK/include" | ||
| "${CMAKE_SOURCE_DIR}/external/jack") | ||
| find_library(JACK_LIBRARIES NAMES jack | ||
| PATHS /usr/lib /usr/local/lib /opt/homebrew/lib | ||
| "C:/Program Files/JACK/lib" | ||
| "${CMAKE_SOURCE_DIR}/external/jack") | ||
| endif() | ||
| endif() | ||
| if(JACK_LIBRARY_DIRS) | ||
| link_directories(${JACK_LIBRARY_DIRS}) | ||
| endif() | ||
| set(BACKEND_SOURCES | ||
| src/audio/audio_backend_jack.cpp | ||
| src/audio/audio_backend_jack_lifecycle.cpp | ||
| ) | ||
| else() | ||
| set(BACKEND_SOURCES | ||
| src/audio/audio_backend_portaudio.cpp | ||
| src/audio/audio_backend_portaudio_lifecycle.cpp | ||
| src/audio/audio_backend_portaudio_devices.cpp | ||
| ) | ||
| endif() |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and examine the CMakeLists.txt file
git ls-files | grep -i cmakeRepository: sudip-mondal-2002/Amplitron
Length of output: 86
🏁 Script executed:
# Read the CMakeLists.txt to understand the context around lines 265-295
wc -l CMakeLists.txtRepository: sudip-mondal-2002/Amplitron
Length of output: 90
🏁 Script executed:
# Read the relevant sections mentioned in the review
sed -n '260,300p' CMakeLists.txtRepository: sudip-mondal-2002/Amplitron
Length of output: 1663
🏁 Script executed:
# Also check the other locations mentioned: 517-518, 529-530, 535-536, 642-643, 652-654, 659-660
sed -n '510,540p' CMakeLists.txtRepository: sudip-mondal-2002/Amplitron
Length of output: 1081
🏁 Script executed:
# Check the target_include_directories and target_link_libraries usage
sed -n '630,670p' CMakeLists.txtRepository: sudip-mondal-2002/Amplitron
Length of output: 1419
🏁 Script executed:
# Check all the "Also applies to" locations mentioned
echo "=== Lines 517-518 ==="
sed -n '515,520p' CMakeLists.txt
echo -e "\n=== Lines 529-530 ==="
sed -n '527,532p' CMakeLists.txt
echo -e "\n=== Lines 535-536 ==="
sed -n '533,538p' CMakeLists.txt
echo -e "\n=== Lines 642-643 ==="
sed -n '640,645p' CMakeLists.txt
echo -e "\n=== Lines 652-654 ==="
sed -n '650,656p' CMakeLists.txt
echo -e "\n=== Lines 659-660 ==="
sed -n '657,662p' CMakeLists.txtRepository: sudip-mondal-2002/Amplitron
Length of output: 1486
🏁 Script executed:
# Search for all WITH_JACK usages throughout the file
rg -n "WITH_JACK" CMakeLists.txtRepository: sudip-mondal-2002/Amplitron
Length of output: 655
🏁 Script executed:
# Check if JACK libraries are actually used in compilation targets to understand impact
grep -n "add_executable\|add_library\|target_sources" CMakeLists.txt | head -20Repository: sudip-mondal-2002/Amplitron
Length of output: 556
🏁 Script executed:
# Check PORTAUDIO handling for comparison
sed -n '200,265p' CMakeLists.txtRepository: sudip-mondal-2002/Amplitron
Length of output: 2196
🏁 Script executed:
# Check if there are validation checks after find_package/pkg_check_modules for other libraries
rg -A 10 "find_package|pkg_check_modules" CMakeLists.txt | head -100Repository: sudip-mondal-2002/Amplitron
Length of output: 4174
🏁 Script executed:
# Search for any checks of PORTAUDIO_FOUND or similar validation after find operations
rg -B 5 -A 5 "if\(NOT.*FOUND" CMakeLists.txtRepository: sudip-mondal-2002/Amplitron
Length of output: 1807
🏁 Script executed:
# Final check: verify that JACK source files are included unconditionally when WITH_JACK is true
sed -n '265,295p' CMakeLists.txt | grep -A 5 "if(WITH_JACK)"Repository: sudip-mondal-2002/Amplitron
Length of output: 290
🏁 Script executed:
# Verify PORTAUDIO is always required (never disabled like JACK should be)
rg -B 3 -A 3 "PORTAUDIO_FOUND\|if.*PORTAUDIO" CMakeLists.txt | head -30Repository: sudip-mondal-2002/Amplitron
Length of output: 53
Add fallback logic when JACK is requested but not found to prevent compilation failure.
When WITH_JACK=ON but the JACK libraries are not found on the system, the code still sets BACKEND_SOURCES to compile JACK audio backend sources without verifying that JACK_INCLUDE_DIRS and JACK_LIBRARIES were successfully resolved. This causes compilation to fail when the jack/jack.h header is missing, rather than gracefully falling back to the PortAudio backend.
The current code checks if(NOT JACK_INCLUDE_DIRS OR NOT JACK_LIBRARIES) to conditionally search for JACK, but lacks validation after the find operations to confirm they succeeded. Add a check after the pkg-config and find_path/find_library attempts: if JACK is still not found, disable WITH_JACK with a warning message and re-evaluate BACKEND_SOURCES accordingly.
Suggested CMake fix
if(WITH_JACK)
if(NOT JACK_INCLUDE_DIRS OR NOT JACK_LIBRARIES)
find_package(PkgConfig QUIET)
if(PkgConfig_FOUND)
pkg_check_modules(JACK QUIET jack)
endif()
if(NOT JACK_FOUND)
find_path(JACK_INCLUDE_DIRS jack/jack.h
PATHS /usr/include /usr/local/include /opt/homebrew/include
"C:/Program Files/JACK/include"
"${CMAKE_SOURCE_DIR}/external/jack")
find_library(JACK_LIBRARIES NAMES jack
PATHS /usr/lib /usr/local/lib /opt/homebrew/lib
"C:/Program Files/JACK/lib"
"${CMAKE_SOURCE_DIR}/external/jack")
endif()
endif()
+ if(NOT JACK_INCLUDE_DIRS OR NOT JACK_LIBRARIES)
+ message(WARNING "WITH_JACK=ON but JACK not found; falling back to PortAudio backend.")
+ set(WITH_JACK OFF CACHE BOOL "Build with JACK audio backend support" FORCE)
+ endif()
+endif()
+
+if(WITH_JACK)
+ if(JACK_LIBRARY_DIRS)
+ link_directories(${JACK_LIBRARY_DIRS})
+ endif()
+ set(BACKEND_SOURCES
+ src/audio/audio_backend_jack.cpp
+ src/audio/audio_backend_jack_lifecycle.cpp
+ )
+else()
+ set(BACKEND_SOURCES
+ src/audio/audio_backend_portaudio.cpp
+ src/audio/audio_backend_portaudio_lifecycle.cpp
+ src/audio/audio_backend_portaudio_devices.cpp
+ )
+endif()
-
- if(JACK_LIBRARY_DIRS)
- link_directories(${JACK_LIBRARY_DIRS})
- endif()
- set(BACKEND_SOURCES
- src/audio/audio_backend_jack.cpp
- src/audio/audio_backend_jack_lifecycle.cpp
- )
-else()
- set(BACKEND_SOURCES
- src/audio/audio_backend_portaudio.cpp
- src/audio/audio_backend_portaudio_lifecycle.cpp
- src/audio/audio_backend_portaudio_devices.cpp
- )
-endif()Per coding guidelines, external libraries should validate successful resolution and fallback when not available—this change ensures WITH_JACK complies with that pattern.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if(WITH_JACK) | |
| if(NOT JACK_INCLUDE_DIRS OR NOT JACK_LIBRARIES) | |
| find_package(PkgConfig QUIET) | |
| if(PkgConfig_FOUND) | |
| pkg_check_modules(JACK QUIET jack) | |
| endif() | |
| if(NOT JACK_FOUND) | |
| find_path(JACK_INCLUDE_DIRS jack/jack.h | |
| PATHS /usr/include /usr/local/include /opt/homebrew/include | |
| "C:/Program Files/JACK/include" | |
| "${CMAKE_SOURCE_DIR}/external/jack") | |
| find_library(JACK_LIBRARIES NAMES jack | |
| PATHS /usr/lib /usr/local/lib /opt/homebrew/lib | |
| "C:/Program Files/JACK/lib" | |
| "${CMAKE_SOURCE_DIR}/external/jack") | |
| endif() | |
| endif() | |
| if(JACK_LIBRARY_DIRS) | |
| link_directories(${JACK_LIBRARY_DIRS}) | |
| endif() | |
| set(BACKEND_SOURCES | |
| src/audio/audio_backend_jack.cpp | |
| src/audio/audio_backend_jack_lifecycle.cpp | |
| ) | |
| else() | |
| set(BACKEND_SOURCES | |
| src/audio/audio_backend_portaudio.cpp | |
| src/audio/audio_backend_portaudio_lifecycle.cpp | |
| src/audio/audio_backend_portaudio_devices.cpp | |
| ) | |
| endif() | |
| if(WITH_JACK) | |
| if(NOT JACK_INCLUDE_DIRS OR NOT JACK_LIBRARIES) | |
| find_package(PkgConfig QUIET) | |
| if(PkgConfig_FOUND) | |
| pkg_check_modules(JACK QUIET jack) | |
| endif() | |
| if(NOT JACK_FOUND) | |
| find_path(JACK_INCLUDE_DIRS jack/jack.h | |
| PATHS /usr/include /usr/local/include /opt/homebrew/include | |
| "C:/Program Files/JACK/include" | |
| "${CMAKE_SOURCE_DIR}/external/jack") | |
| find_library(JACK_LIBRARIES NAMES jack | |
| PATHS /usr/lib /usr/local/lib /opt/homebrew/lib | |
| "C:/Program Files/JACK/lib" | |
| "${CMAKE_SOURCE_DIR}/external/jack") | |
| endif() | |
| endif() | |
| if(NOT JACK_INCLUDE_DIRS OR NOT JACK_LIBRARIES) | |
| message(WARNING "WITH_JACK=ON but JACK not found; falling back to PortAudio backend.") | |
| set(WITH_JACK OFF CACHE BOOL "Build with JACK audio backend support" FORCE) | |
| endif() | |
| endif() | |
| if(WITH_JACK) | |
| if(JACK_LIBRARY_DIRS) | |
| link_directories(${JACK_LIBRARY_DIRS}) | |
| endif() | |
| set(BACKEND_SOURCES | |
| src/audio/audio_backend_jack.cpp | |
| src/audio/audio_backend_jack_lifecycle.cpp | |
| ) | |
| else() | |
| set(BACKEND_SOURCES | |
| src/audio/audio_backend_portaudio.cpp | |
| src/audio/audio_backend_portaudio_lifecycle.cpp | |
| src/audio/audio_backend_portaudio_devices.cpp | |
| ) | |
| 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 265 - 295, The WITH_JACK branch sets
BACKEND_SOURCES to JACK files before confirming JACK was actually found; after
the pkg_check_modules / find_path / find_library attempts check whether
JACK_INCLUDE_DIRS and JACK_LIBRARIES (or JACK_FOUND) are set, and if they are
still missing, emit a warning via message(WARNING ...) and set WITH_JACK to OFF
(or clear the JACK-specific vars), then fall back to the PortAudio sources by
re-evaluating BACKEND_SOURCES; ensure you reference and update the existing
symbols (WITH_JACK, JACK_INCLUDE_DIRS, JACK_LIBRARIES, JACK_FOUND,
BACKEND_SOURCES) so compilation will use portaudio if JACK resolution fails.
Comment on lines
+34
to
+46
| bool AudioEngine::initialize() | ||
| { | ||
| // Try to open JACK client without auto-starting server | ||
| const char *client_name = "Amplitron"; | ||
| jack_options_t options = JackNoStartServer; | ||
| jack_status_t status; | ||
|
|
||
| // Create backend state if not present | ||
| if (!backend_) | ||
| backend_ = create_audio_backend(); | ||
|
|
||
| backend_->client = jack_client_open(client_name, options, &status, nullptr); | ||
| if (!backend_->client) |
Contributor
There was a problem hiding this comment.
initialize() should guard against duplicate initialization.
If initialize() is called twice, the existing client can be overwritten without a prior close/shutdown path. Add an early guard (if (initialized_) return true;) or perform explicit teardown before reopening.
🤖 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 `@src/audio/audio_backend_jack_lifecycle.cpp` around lines 34 - 46, The
initialize() method currently reopens JACK and overwrites backend_->client if
called twice; add a guard at the top of AudioEngine::initialize() that returns
true immediately when initialized_ is already set, or alternatively call the
teardown/close path before reopening; specifically, check the member
initialized_ (or set one if absent) and return true, or call the existing
shutdown/close helper (or implement one to call jack_client_close on
backend_->client and reset backend_) before calling create_audio_backend() and
jack_client_open(), and ensure you set initialized_ = true only after successful
jack_client_open and clear it on failure/teardown.
Comment on lines
+141
to
+142
| bool AudioEngine::set_input_device(int) { return true; } | ||
| bool AudioEngine::set_output_device(int) { return true; } |
Contributor
There was a problem hiding this comment.
Device setters return success without updating engine state.
set_input_device/set_output_device always return true but never update input_device_ / output_device_. This can desync settings/UI from engine state.
Suggested fix
-bool AudioEngine::set_input_device(int) { return true; }
-bool AudioEngine::set_output_device(int) { return true; }
+bool AudioEngine::set_input_device(int device_index)
+{
+ if (device_index != 0) return false;
+ input_device_ = 0;
+ return true;
+}
+
+bool AudioEngine::set_output_device(int device_index)
+{
+ if (device_index != 0) return false;
+ output_device_ = 0;
+ return true;
+}🤖 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 `@src/audio/audio_backend_jack_lifecycle.cpp` around lines 141 - 142,
set_input_device and set_output_device currently return true without changing
state; update them to validate the provided device index, assign it to the
corresponding member (input_device_ / output_device_), and then apply the change
to the running engine (e.g. reconfigure or restart the JACK client / invoke the
existing audio reinitialization routine). Ensure the functions return false for
invalid indices or if reconfiguration fails and true only on successful
assignment+apply so UI/settings stay in sync with the engine.
sudip-mondal-2002
added
mentor:MohitBareja16
type:feature
Contributor
PR Preview RemovedThe GitHub Pages preview for this PR has been removed because the PR was closed. |
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 `@tests/test_jack_backend.cpp`:
- Around line 12-15: Replace the silent early returns after failed jack backend
initialization with explicit test skips: where the code currently does "if
(!engine.initialize()) { return; }" (and the similar block later), call
GTEST_SKIP() with a clear message like "GTEST_SKIP() << \"jackd unavailable:
engine.initialize() failed\";" so the test is reported as skipped instead of
passed; update both occurrences that call engine.initialize() to use
GTEST_SKIP() and include the initialize() failure context in the message.
🪄 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: acfad33f-a6cc-4baa-aefc-a6a8c8bbbb17
📒 Files selected for processing (4)
CMakeLists.txtsrc/audio/audio_backend_jack_lifecycle.cppsrc/gui/pedal_widget_body.cpptests/test_jack_backend.cpp
✅ Files skipped from review due to trivial changes (1)
- src/gui/pedal_widget_body.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
- src/audio/audio_backend_jack_lifecycle.cpp
- CMakeLists.txt
Comment on lines
+12
to
+15
| if (!engine.initialize()) | ||
| { | ||
| return; | ||
| } |
Contributor
There was a problem hiding this comment.
Use explicit test skips instead of silent early returns.
Line 12 and Line 24 currently return when initialize() fails, which marks the test as passed and can mask real regressions. Use GTEST_SKIP() so CI reports these as skipped (e.g., when jackd is unavailable).
Proposed patch
TEST(JackBackend_InitializeAndStart)
{
AudioEngine engine;
if (!engine.initialize())
{
- return;
+ GTEST_SKIP() << "JACK backend unavailable or jackd not running";
}
ASSERT_TRUE(engine.start());
engine.stop();
engine.shutdown();
}
@@
TEST(JackBackend_StartStopSmoke)
{
AudioEngine engine;
if (!engine.initialize())
{
- return;
+ GTEST_SKIP() << "JACK backend unavailable or jackd not running";
}
ASSERT_TRUE(engine.start());
std::this_thread::sleep_for(std::chrono::milliseconds(200));
engine.stop();
engine.shutdown();
}Also applies to: 24-27
🤖 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/test_jack_backend.cpp` around lines 12 - 15, Replace the silent early
returns after failed jack backend initialization with explicit test skips: where
the code currently does "if (!engine.initialize()) { return; }" (and the similar
block later), call GTEST_SKIP() with a clear message like "GTEST_SKIP() <<
\"jackd unavailable: engine.initialize() failed\";" so the test is reported as
skipped instead of passed; update both occurrences that call engine.initialize()
to use GTEST_SKIP() and include the initialize() failure context in the message.
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/audio_engine.h`:
- Around line 77-175: The header contains duplicated public method declarations
(e.g., is_analyzer_enabled, get_analyzer_sequence, copy_analyzer_snapshot,
set_input_gain, set_output_gain, toggle_metronome, set_metronome_bpm,
set_metronome_volume,
get_metronome_enabled/get_metronome_bpm/get_metronome_volume, push_param_change,
push_effect_enabled, push_effect_mix, etc.); remove one of the duplicate blocks
so each public method is declared exactly once, move the metronome state atoms
(metronome_enabled_state_, metronome_bpm_state_, metronome_volume_state_) into
the actual class private: section (so they live with other private members like
recorder_ and cpu_load_), and delete the orphaned duplicated block that follows
the class closing brace (the stray private:/members after the closing };) so the
class is well-formed and builds cleanly.
🪄 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: 6362614c-a371-4769-a940-5d8684f8a626
📒 Files selected for processing (2)
CMakeLists.txtsrc/audio/audio_engine.h
Removed analyzer-related methods and member variables from the audio engine header.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
CMakeLists.txt (1)
251-256:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRestrict JACK default enablement to Linux, not all non-Apple UNIX.
UNIX AND NOT APPLEalso enables JACK by default on BSD-like targets. The PR objective says default-on should be Linux-only. Please gate this on Linux explicitly.Suggested change
-if(UNIX AND NOT APPLE) +if(CMAKE_SYSTEM_NAME STREQUAL "Linux") set(DEFAULT_WITH_JACK ON) else() set(DEFAULT_WITH_JACK OFF) 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 251 - 256, The DEFAULT_WITH_JACK logic currently uses "UNIX AND NOT APPLE" which also enables JACK on BSDs; change the check to detect Linux explicitly (use CMAKE_SYSTEM_NAME == "Linux") so DEFAULT_WITH_JACK is only ON for Linux, and keep the option(WITH_JACK ...) intact; update the block that sets DEFAULT_WITH_JACK and the surrounding if/else that references DEFAULT_WITH_JACK to use the explicit Linux check instead of UNIX AND NOT APPLE.
🤖 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/gui/pedal_widget_body.cpp`:
- Around line 295-303: There is an orphaned duplicate UI block after
render_ir_cabinet_display that closes the function scope incorrectly; remove the
entire leftover block containing the stray ImGui::PopStyleColor(3), the
unmatched "} else {" and the "No IR loaded" branch so the closing PopStyleColor
for the if (ir_cab->has_ir()) path (after the Clear button) remains inside
render_ir_cabinet_display and the duplicate "No IR loaded" code is eliminated;
look for the duplicate code that references ImGui::PopStyleColor, the "} else {"
branch and the "No IR loaded" text and delete that whole segment.
---
Outside diff comments:
In `@CMakeLists.txt`:
- Around line 251-256: The DEFAULT_WITH_JACK logic currently uses "UNIX AND NOT
APPLE" which also enables JACK on BSDs; change the check to detect Linux
explicitly (use CMAKE_SYSTEM_NAME == "Linux") so DEFAULT_WITH_JACK is only ON
for Linux, and keep the option(WITH_JACK ...) intact; update the block that sets
DEFAULT_WITH_JACK and the surrounding if/else that references DEFAULT_WITH_JACK
to use the explicit Linux check instead of UNIX AND NOT APPLE.
🪄 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: 79c53419-cc50-4240-b250-a23dbab22cf0
📒 Files selected for processing (4)
CMakeLists.txtsrc/audio/audio_engine.hsrc/gui/pedal_widget_body.cpptests/test_json_serialization.cpp
💤 Files with no reviewable changes (1)
- src/audio/audio_engine.h
Comment on lines
295
to
303
| @@ -269,7 +301,6 @@ void PedalWidget::render_cabinet_ir_display(ImVec2 p0, float pedal_width) { | |||
| ImGui::TextUnformatted(no_ir); | |||
| ImGui::PopStyleColor(); | |||
| } | |||
Contributor
There was a problem hiding this comment.
Critical: Duplicate/orphaned code causes syntax errors.
Lines 295–303 appear to be leftover from a merge or copy-paste error:
- Line 294 already closes
render_ir_cabinet_display—everything after is outside the function scope. } else {on line 296 has no matchingif, causing a syntax error.- The "No IR loaded" logic is already handled in lines 287–292.
- The orphaned
PopStyleColor(3)was meant to be inside theif (ir_cab->has_ir())block (after the Clear button).
This matches the reported Linux build failure. Remove lines 295–303 entirely.
🐛 Proposed fix: Remove the duplicate code block
ImGui::PopStyleColor();
}
}
- ImGui::PopStyleColor(3);
- } else {
- const char* no_ir = "No IR loaded";
- ImVec2 ni_size = ImGui::CalcTextSize(no_ir);
- ImGui::SetCursorScreenPos(ImVec2(cx - ni_size.x * 0.5f, display_y));
- ImGui::PushStyleColor(ImGuiCol_Text, Theme::TextDim());
- ImGui::TextUnformatted(no_ir);
- ImGui::PopStyleColor();
- }
void PedalWidget::render_looper_display(ImVec2 p0, float pedal_width) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
🤖 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 `@src/gui/pedal_widget_body.cpp` around lines 295 - 303, There is an orphaned
duplicate UI block after render_ir_cabinet_display that closes the function
scope incorrectly; remove the entire leftover block containing the stray
ImGui::PopStyleColor(3), the unmatched "} else {" and the "No IR loaded" branch
so the closing PopStyleColor for the if (ir_cab->has_ir()) path (after the Clear
button) remains inside render_ir_cabinet_display and the duplicate "No IR
loaded" code is eliminated; look for the duplicate code that references
ImGui::PopStyleColor, the "} else {" branch and the "No IR loaded" text and
delete that whole segment.
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
3 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?
Adds a Linux-only JACK audio backend for Amplitron and makes JACK the default backend on Linux builds. The new backend wires JACK's real-time process callback directly into the existing audio engine path, which gives low-latency audio routing for pro-audio and live-monitoring setups. This PR also adds guarded JACK integration tests and updates the build/configuration flow so JACK is enabled by default on Linux.
Related Issue
Fixes #58
Type of Change
How Was This Tested?
./amplitron-testsbuild/directoryjackdis not runningChecklist
./amplitron-tests)std::mutex::lock()on the hot path)Screenshots / Demo
No UI changes in this PR.
Summary by CodeRabbit
New Features
Removed
Tests
Chores
Style