feat: Add JACK audio backend support for Linux(#58)#220
Conversation
|
Warning Review limit reached
More reviews will be available in 7 minutes and 3 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR adds a conditional JACK audio backend for Linux, integrating it alongside the existing PortAudio backend. CMake adds a ChangesJACK Backend Integration
Sequence Diagram(s)The main sequence diagram for JACK initialization and callback flow is embedded in the "JACK backend implementation" layer above, showing how Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
(cherry picked from commit 9bfd74aa6a9e8f80eaa131ae88da44b7642f5178) (cherry picked from commit 302bd88b3e3cbdf278b7d9e125d32fbef6d5aca5)
Code Coverage Report 📊Line Coverage: 81.0% ✅ Coverage meets threshold: 81.0% >= 60% Full Coverage Summary |
PR Preview RemovedThe GitHub Pages preview for this PR has been removed because the PR was closed. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
README.md (1)
170-177: ⚡ Quick winClarify JACK prerequisite as optional on Linux.
Since the README now lists an optional JACK backend, add a short prerequisite note here (e.g., JACK dev package required only when enabling JACK) to keep build/setup guidance consistent.
🤖 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 `@README.md` around lines 170 - 177, Update the prerequisites list in README.md to clarify that the JACK development package is only required on Linux when enabling the JACK backend: locate the prerequisites block where audio libs like "PortAudio" and "SDL2" are listed and add a short parenthetical or bullet note next to "JACK" (or add a new line) saying "JACK dev package (Linux only, required only if enabling JACK backend)"; ensure the phrasing references the JACK backend option so users understand it's optional and platform-specific.scripts/setup_dependencies.sh (1)
98-98: ⚡ Quick winAvoid unconditional JACK install on macOS.
brew install ... jackadds an unnecessary dependency when JACK backend is Linux-focused. Consider gating this behind an explicit flag (or removing it from macOS defaults) to keep setup lean and reduce install failures.🤖 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 `@scripts/setup_dependencies.sh` at line 98, The script currently always installs JACK via the line "brew install cmake portaudio sdl2 jack" in scripts/setup_dependencies.sh; change this to conditionally install jack on macOS by gating it behind an explicit flag (e.g., INSTALL_JACK) or omit it for Darwin. Update the install flow around the "brew install cmake portaudio sdl2 jack" invocation so that jack is appended only when INSTALL_JACK is true (or when the platform is not macOS), leaving cmake/portaudio/sdl2 unconditional.
🤖 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 100-114: The JACK include dirs variable (${JACK_INCLUDE_DIRS}) is
discovered but never added to targets; update the CMakeLists targets to add it:
in the Amplitron target_include_directories block (target name "Amplitron")
append a conditional entry to include ${JACK_INCLUDE_DIRS} when defined, and do
the same in the amplitron-tests target_include_directories block (target name
"amplitron-tests"); ensure you follow the existing pattern (wrap with
if(JACK_INCLUDE_DIRS) ... target_include_directories(... PUBLIC
${JACK_INCLUDE_DIRS}) ... endif()) so src/audio/audio_backend_jack.cpp can find
<jack/jack.h>.
In `@README.md`:
- Around line 137-139: The fenced code block containing the signal chain example
(the triple-backtick block that shows "Guitar ──[1/4" jack]──> USB Guitar Cable
──[USB-C]──> Laptop ──> Guitar Amp Simulator ──> Laptop Speakers / Headphones")
needs a language tag; change the opening ``` to ```text so Markdown linting
recognizes it as plain text.
- Around line 289-294: The fenced code block showing the signal chain (the line
beginning "Input → [Noise Gate] → [Compressor] → [Overdrive] → EQ → [Cabinet] →
[Delay] → Reverb → Output" and the following two explanatory lines) is missing a
language tag; update the opening fence from ``` to ```text (or another
appropriate language) so the block is explicitly marked as plain text for proper
rendering and syntax highlighting.
In `@src/audio/audio_backend_jack.cpp`:
- Around line 138-161: AudioEngine::start() may call jack_activate() on a client
that create_audio_backend() already activated, causing a failure; update the
logic to avoid duplicate activation by first checking whether the backend client
is already active (either by adding and consulting a flag on AudioBackendState
like state->activated set when create_audio_backend() activates the client, or
by querying the JACK client activation state if available) and only call
jack_activate(state->client) when the client is not already active;
alternatively remove activation from create_audio_backend() so start() is the
single activation point (refer to AudioEngine::start(), create_audio_backend(),
backend_, AudioBackendState and state->client to locate the code to change).
- Around line 66-93: create_audio_backend currently registers ports, sets the
process callback and activates the JACK client prematurely which leaves
ports_registered false and causes duplicate port registration and callbacks
before state->engine is set; change create_audio_backend (AudioBackendState
*create_audio_backend) to only allocate AudioBackendState, open the JACK client
(jack_client_open) and return the state without calling jack_port_register,
jack_set_process_callback or jack_activate, so that
AudioEngine::start/ensure_ports_registered handles jack_port_register,
jack_set_process_callback and jack_activate (and sets ports_registered = true)
and jack_process won't run before state->engine is assigned.
---
Nitpick comments:
In `@README.md`:
- Around line 170-177: Update the prerequisites list in README.md to clarify
that the JACK development package is only required on Linux when enabling the
JACK backend: locate the prerequisites block where audio libs like "PortAudio"
and "SDL2" are listed and add a short parenthetical or bullet note next to
"JACK" (or add a new line) saying "JACK dev package (Linux only, required only
if enabling JACK backend)"; ensure the phrasing references the JACK backend
option so users understand it's optional and platform-specific.
In `@scripts/setup_dependencies.sh`:
- Line 98: The script currently always installs JACK via the line "brew install
cmake portaudio sdl2 jack" in scripts/setup_dependencies.sh; change this to
conditionally install jack on macOS by gating it behind an explicit flag (e.g.,
INSTALL_JACK) or omit it for Darwin. Update the install flow around the "brew
install cmake portaudio sdl2 jack" invocation so that jack is appended only when
INSTALL_JACK is true (or when the platform is not macOS), leaving
cmake/portaudio/sdl2 unconditional.
🪄 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: 1a753ce3-b4b4-4c55-8a02-367b31de9763
📒 Files selected for processing (6)
CMakeLists.txtREADME.mdscripts/setup_dependencies.shsrc/audio/audio_backend_jack.cppsrc/gui/gui_settings.cpptests/test_audio_backend_jack.cpp
There was a problem hiding this comment.
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)
276-287:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winGate JACK backend selection (sources/defs/linking) on successful JACK detection, not just
WITH_JACK.In
CMakeLists.txt, the JACK backend source selection and target setup are guarded only byif(WITH_JACK AND UNIX AND NOT APPLE)(lines 276-287, and similarly 521-535 and 645-659), while JACK detection (e.g.,JACK_FOUND,JACK_INCLUDE_DIRS,JACK_LIBRARIESaround ~101-108) is not used to gate those later blocks. This can still select/link the JACK backend when JACK dev packages are missing, leading to hard build failures (e.g., linking with*-NOTFOUND).Introduce a
HAVE_JACKflag derived from successful header+library detection (and ensureJACK_INCLUDE_DIRS/JACK_LIBRARIESare only used when present), use it to guard the backend sources and theAmplitron+amplitron-testscompile definitions/linking, and fall back to the PortAudio backend when JACK isn’t actually usable.🤖 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 276 - 287, Define and use a HAVE_JACK flag that is set only when JACK detection succeeds (e.g., when JACK_FOUND is true and JACK_INCLUDE_DIRS/JACK_LIBRARIES are valid) and replace the current checks of WITH_JACK in the backend selection and linking blocks; specifically, set HAVE_JACK after the existing find_package/find_path/find_library JACK checks, then gate BACKEND_SOURCES selection (the if(...) that currently uses WITH_JACK AND UNIX AND NOT APPLE), the Amplitron target compile definitions/linking, and amplitron-tests linking on HAVE_JACK so the JACK backend is only selected/linked when headers and libs are present, and ensure the PortAudio backend is chosen as the fallback when HAVE_JACK is false.
🤖 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.
Outside diff comments:
In `@CMakeLists.txt`:
- Around line 276-287: Define and use a HAVE_JACK flag that is set only when
JACK detection succeeds (e.g., when JACK_FOUND is true and
JACK_INCLUDE_DIRS/JACK_LIBRARIES are valid) and replace the current checks of
WITH_JACK in the backend selection and linking blocks; specifically, set
HAVE_JACK after the existing find_package/find_path/find_library JACK checks,
then gate BACKEND_SOURCES selection (the if(...) that currently uses WITH_JACK
AND UNIX AND NOT APPLE), the Amplitron target compile definitions/linking, and
amplitron-tests linking on HAVE_JACK so the JACK backend is only selected/linked
when headers and libs are present, and ensure the PortAudio backend is chosen as
the fallback when HAVE_JACK is false.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/audio/audio_backend_jack.cpp (1)
109-121:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRecreate backend state when
shutdown()has closed the JACK client.Line 114 only re-creates when
backend_is null, but Line 131 nullsstate->clientwhile keepingbackend_allocated. After one shutdown,initialize()succeeds with a disconnected backend andstart()will always fail.🔧 Suggested fix
bool AudioEngine::initialize() { if (initialized_) return true; - if (!backend_) - { - backend_ = create_audio_backend(); - } + auto *state = static_cast<AudioBackendState *>(backend_); + if (!state || !state->client) + { + destroy_audio_backend(state); + backend_ = create_audio_backend(); + } initialized_ = true; last_error_.clear(); return true; }Also applies to: 124-136
🤖 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.cpp` around lines 109 - 121, The initialize() path currently only recreates backend_ when it's null, but shutdown() leaves backend_ allocated while nulling state->client, so subsequent initialize()/start() run against a disconnected backend; update AudioEngine::initialize to detect a closed backend (e.g., check backend_ and backend_->state->client or equivalent state member) and recreate the backend by calling create_audio_backend() (and replace or delete the stale backend_) when the client/state is null or closed before setting initialized_; reference AudioEngine::initialize, shutdown(), backend_, state->client and start() when making 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.
Outside diff comments:
In `@src/audio/audio_backend_jack.cpp`:
- Around line 109-121: The initialize() path currently only recreates backend_
when it's null, but shutdown() leaves backend_ allocated while nulling
state->client, so subsequent initialize()/start() run against a disconnected
backend; update AudioEngine::initialize to detect a closed backend (e.g., check
backend_ and backend_->state->client or equivalent state member) and recreate
the backend by calling create_audio_backend() (and replace or delete the stale
backend_) when the client/state is null or closed before setting initialized_;
reference AudioEngine::initialize, shutdown(), backend_, state->client and
start() when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 15ceb041-f0aa-4097-be05-9c9c983774cb
📒 Files selected for processing (5)
CMakeLists.txtsrc/audio/audio_backend_jack.cppsrc/audio/audio_engine.cppsrc/audio/audio_engine.htests/test_audio_backend_jack.cpp
✅ Files skipped from review due to trivial changes (1)
- src/audio/audio_engine.h
|
@MohitBareja16 review it please |
github-actions
Bot
added
level:intermediate
2 participants
What does this PR do?
This PR reverts the previously merged change that introduced a JACK audio backend for Linux.
Related Issue
closes #58
Type of Change
Select the primary intent(s) of this PR:
How Was This Tested?
Recommended verification for reviewers / CI:
If CI or local build fails due to unrelated missing externals, address those separately.
Checklist
backup-before-rollback-20260524165253Screenshots / Demo
N/A
Summary by CodeRabbit
New Features
Documentation
Chores