feat: add real-time per-pedal pre/post spectrum analyzer#373
Open
Arbaaz123676 wants to merge 10 commits into
Open
feat: add real-time per-pedal pre/post spectrum analyzer#373Arbaaz123676 wants to merge 10 commits into
Arbaaz123676 wants to merge 10 commits into
Conversation
github-actions
Bot
added
Audio Processing
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:
📝 WalkthroughWalkthroughAdds per-pedal pre/post spectrum analysis across audio engine and GUI layers. ChangesPer-pedal Spectrum Analyzer
Sequence Diagram(s)sequenceDiagram
rect rgba(173, 216, 230, 0.5)
Note over PedalWidget,AnalyzerCapture: Analyzer Registration
PedalWidget->>AudioEngine: register_pedal_analyzer(node_id)
AudioEngine->>AnalyzerCapture: register_pedal_analyzer(node_id)
AnalyzerCapture-->>AudioEngine: slot claimed (bool)
AudioEngine-->>PedalWidget: success
end
rect rgba(144, 238, 144, 0.5)
Note over AudioGraphExecutor,AnalyzerCapture: Audio Thread Capture
AudioGraphExecutor->>AnalyzerCapture: capture_pedal(node_id, input, output, count)
AnalyzerCapture->>AnalyzerCapture: write ring buffer, accumulate hop count
alt hop threshold reached
AnalyzerCapture->>AnalyzerCapture: try_lock, copy snapshot, increment sequence
end
end
rect rgba(255, 223, 128, 0.5)
Note over PedalWidget,AnalyzerCapture: GUI Poll and Render
PedalWidget->>AudioEngine: get_pedal_analyzer_sequence(node_id)
AudioEngine->>AnalyzerCapture: get_pedal_analyzer_sequence
AnalyzerCapture-->>PedalWidget: sequence counter
alt new sequence available
PedalWidget->>AudioEngine: copy_pedal_analyzer_snapshot(node_id, in_buf, out_buf)
AudioEngine->>AnalyzerCapture: copy_pedal_analyzer_snapshot
AnalyzerCapture-->>PedalWidget: FFT input/output buffers
end
PedalWidget->>PedalWidget: update SpectrumAnalyzer, draw Pre/Post overlay
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ 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 |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Contributor
PR Preview ReadyPreview URL: https://amplitron.sudipmondal.co.in/pr-previews/pr-373/ Built from commit |
![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: 2
🧹 Nitpick comments (2)
src/audio/engine/analyzer_capture.h (1)
80-80: 💤 Low valueConsider extracting the magic number
4to a named constant.The array size
4is hardcoded here and repeated in multiple methods throughoutanalyzer_capture.cpp(lines 89, 96, 110, 121, 134, 155). Extracting this to a named constant would improve maintainability.Suggested improvement
class AnalyzerCapture : public IAnalyzerProvider { public: static constexpr int ANALYZER_FFT_SIZE = 2048; static constexpr int ANALYZER_FFT_MASK = ANALYZER_FFT_SIZE - 1; static constexpr int ANALYZER_HOP_SIZE = 1024; + static constexpr int MAX_PEDAL_ANALYZERS = 4;Then use
MAX_PEDAL_ANALYZERSin the array declaration and loop bounds.🤖 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/engine/analyzer_capture.h` at line 80, Extract the hardcoded magic number 4 to a named constant such as MAX_PEDAL_ANALYZERS defined in the analyzer_capture.h file (this could be a constexpr static member or a file-level constant). Replace the hardcoded 4 in the pedal_captures_ array declaration with this constant, and then identify and replace all other occurrences of the hardcoded 4 used as loop bounds and array sizes throughout analyzer_capture.cpp (which appear in multiple methods) with this same constant to ensure consistency and maintainability across the codebase.src/gui/pedalboard/pedal_widget.cpp (1)
173-260: ⚡ Quick winConsider extracting overlay rendering to a helper method.
The spectrum analyzer overlay rendering logic (88 lines) could be extracted into a private
render_spectrum_overlay()helper method to improve readability and modularity. This would also make the mainrender()method easier to follow.♻️ Proposed refactor outline
Add a private helper method in the header:
void render_spectrum_overlay(ImDrawList* dl, ImVec2 pedal_pos, float pedal_width, float zoom);Then extract lines 174-260 into this method, passing the necessary context. The main
render()would simply call:if (analyzer_open_) { render_spectrum_overlay(dl, p0, pedal_width, zoom); }🤖 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/pedalboard/pedal_widget.cpp` around lines 173 - 260, Extract the spectrum analyzer overlay rendering logic (lines 173-260) from the main render method into a new private helper method called render_spectrum_overlay() that accepts ImDrawList* dl, ImVec2 pedal_pos, float pedal_width, and float zoom as parameters. Move all the code from the if (analyzer_open_) block including the spectrum data polling, overlay background drawing, reference dB line rendering, frequency tick marking, curve rendering with the draw_curve lambda, and legend labeling into this new helper method. Then replace the entire if (analyzer_open_) block in the main render() method with a single call to render_spectrum_overlay(dl, p0, pedal_width, zoom), ensuring that all member variables accessed in the extracted code remain accessible.
🤖 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/analyzer_capture.h`:
- Line 58: Rename the class member variable `node_id` to `node_id_` in the
struct definition to comply with the coding guidelines requiring a trailing
underscore for class member variables. After renaming the variable declaration,
update all references to `node_id` throughout the corresponding implementation
file to use the new name `node_id_`, ensuring consistency across all usages of
this member variable.
- Around line 69-78: The `reset()` method has a critical data race on
`capture_index_` and `samples_since_publish_` because these non-atomic variables
are written by both the GUI thread (in `reset()`) and the audio thread (in
`capture_pedal`), with insufficient synchronization. In
`register_pedal_analyzer`, move the `reset()` call to execute before the
compare-and-swap operation that assigns the node_id to the slot, ensuring the
audio thread never observes an uninitialized state. In
`unregister_pedal_analyzer`, after the CAS clears the node_id, the audio thread
may still be executing in the capture block and will race with the subsequent
`reset()` call; either move `reset()` to before the CAS operation as well, or
consider making `capture_index_` and `samples_since_publish_` atomic variables
to eliminate the synchronization dependency on the mutex.
---
Nitpick comments:
In `@src/audio/engine/analyzer_capture.h`:
- Line 80: Extract the hardcoded magic number 4 to a named constant such as
MAX_PEDAL_ANALYZERS defined in the analyzer_capture.h file (this could be a
constexpr static member or a file-level constant). Replace the hardcoded 4 in
the pedal_captures_ array declaration with this constant, and then identify and
replace all other occurrences of the hardcoded 4 used as loop bounds and array
sizes throughout analyzer_capture.cpp (which appear in multiple methods) with
this same constant to ensure consistency and maintainability across the
codebase.
In `@src/gui/pedalboard/pedal_widget.cpp`:
- Around line 173-260: Extract the spectrum analyzer overlay rendering logic
(lines 173-260) from the main render method into a new private helper method
called render_spectrum_overlay() that accepts ImDrawList* dl, ImVec2 pedal_pos,
float pedal_width, and float zoom as parameters. Move all the code from the if
(analyzer_open_) block including the spectrum data polling, overlay background
drawing, reference dB line rendering, frequency tick marking, curve rendering
with the draw_curve lambda, and legend labeling into this new helper method.
Then replace the entire if (analyzer_open_) block in the main render() method
with a single call to render_spectrum_overlay(dl, p0, pedal_width, zoom),
ensuring that all member variables accessed in the extracted code remain
accessible.
🪄 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: 897e22d2-b7c8-4522-b519-e4e5385e9203
📒 Files selected for processing (10)
src/audio/engine/analyzer_capture.cppsrc/audio/engine/analyzer_capture.hsrc/audio/engine/audio_engine.hsrc/audio/engine/audio_engine_api.cppsrc/audio/engine/audio_engine_process.cppsrc/audio/engine/audio_graph_executor.cppsrc/audio/engine/audio_graph_executor.hsrc/audio/engine/i_audio_engine.hsrc/gui/pedalboard/pedal_widget.cppsrc/gui/pedalboard/pedal_widget.h
Collaborator
|
@Arbaaz123676 CI test failing |
Collaborator
|
@Arbaaz123676 Check coderabbit warnings and push the test coverage |
… and widget rendering
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/integration/test_audio_engine.cpp`:
- Around line 548-552: The GetterTestCoverage test contains tautological
assertions that are always true and provide no real validation. Replace the
size() >= 0u assertions for test_process_buffer() and
test_process_buffer_right() with meaningful checks such as verifying the buffers
contain expected data or have specific properties. Replace the
test_audio_shadow_executor() assertion (which checks ptr == nullptr || ptr !=
nullptr) with a real postcondition that validates the executor's actual state or
properties, such as asserting it is or is not null based on expected behavior,
or checking specific attributes of the executor object.
In `@tests/ui/test_pedal_widget.cpp`:
- Around line 170-190: The test ignores the return value of engine.initialize()
but later dereferences engine.analyzer_capture_ on line 189 through the
capture_pedal call, which could be null if initialization failed. Add an
assertion or check immediately after engine.initialize() to verify that
initialization succeeded before proceeding with any code that accesses
engine.analyzer_capture_ and its methods like capture_pedal. This ensures the
test will not crash or flap due to uninitialized state.
🪄 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: ac5f70b4-e335-4211-a3c8-c8d693a5699d
📒 Files selected for processing (5)
tests/integration/test_audio_engine.cpptests/test_fixtures.htests/ui/test_gui_keyboard_shortcuts.cpptests/ui/test_pedal_board_menu.cpptests/ui/test_pedal_widget.cpp
✅ Files skipped from review due to trivial changes (1)
- tests/ui/test_pedal_board_menu.cpp
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/analyzer_capture.cpp`:
- Around line 96-104: The reset() method on pedal_captures_[i] is called before
the compare_exchange_strong operation succeeds, creating a race condition where
another thread could claim the slot between the initial load check and the
reset, causing data corruption. Move the reset() call to execute only after the
compare_exchange_strong succeeds by placing it inside the if block that verifies
the CAS operation was successful, ensuring we only reset slots that we have
actually claimed.
- Around line 155-166: The capture_pedal method is missing input validation that
could lead to undefined behavior when called from the audio thread. Add
null-pointer validation for the input and output parameters and bounds
validation for the count parameter (ensure count is positive) at the start of
the capture_pedal method, before the loop begins. Follow the same validation
pattern used in other analyzer methods like copy_analyzer_snapshot and
copy_pedal_analyzer_snapshot in this file to ensure consistency.
🪄 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: 71d96b5f-0523-4cad-af9b-ec951e640d89
📒 Files selected for processing (7)
src/audio/engine/analyzer_capture.cppsrc/audio/engine/analyzer_capture.hsrc/gui/pedalboard/pedal_widget.cppsrc/gui/pedalboard/pedal_widget.htests/integration/test_audio_engine.cpptests/test_fixtures.htests/ui/test_pedal_widget.cpp
🚧 Files skipped from review as they are similar to previous changes (6)
- tests/test_fixtures.h
- tests/ui/test_pedal_widget.cpp
- tests/integration/test_audio_engine.cpp
- src/gui/pedalboard/pedal_widget.h
- src/gui/pedalboard/pedal_widget.cpp
- src/audio/engine/analyzer_capture.h
Owner
|
@Arbaaz123676 Can you explain how to use it? Please attach a video recording |
Contributor
Author
Real.time.per.pedal.spectrum.analysis.mp4 |
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.
Summary
This PR adds a real-time per-pedal pre/post spectrum analyzer, allowing users to visualize how individual pedals affect the incoming audio signal.
The implementation captures audio before and after pedal processing, performs FFT analysis using the existing spectrum analysis pipeline, and renders a lightweight floating overlay directly above the selected pedal.
Changes
SpectrumAnalyzer.clang-format).Technical Notes
Validation
748/748).Issue
Fixes #73
Summary by CodeRabbit