Added __Real-Time Audio Visualization Panel__ has been implemented wi…#391
Open
TomHacker69 wants to merge 1 commit into
Open
Added __Real-Time Audio Visualization Panel__ has been implemented wi…#391TomHacker69 wants to merge 1 commit into
TomHacker69 wants to merge 1 commit into
Conversation
…th the following components
Contributor
📝 WalkthroughWalkthroughThree Emscripten-exported C bridge functions ( ChangesAudio Visualizer Panel
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 Warning |
![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 `@src/main.cpp`:
- Around line 319-324: The exported bridge function copy_analyzer_snapshot
currently accepts any positive sample_count and forwards it directly to
g_engine_ptr->copy_analyzer_snapshot, so enforce the documented 2048-sample
contract here before copying. Update copy_analyzer_snapshot to reject or clamp
any oversized request above the analyzer snapshot size, while keeping the
existing null-pointer and nonpositive checks intact, so the ABI never passes an
out-of-range length to the engine.
In `@web/visualizer.js`:
- Around line 84-92: The bass-energy calculation in the visualizer is using the
first 128 values from outView as if they were FFT bins, but outView is
time-domain data, so this should be changed to derive bass from actual
low-frequency content instead of an arbitrary buffer slice. Update the logic in
the bass-energy section of visualizer.js to use the appropriate frequency-domain
source or a proper low-frequency aggregation path, and keep the smoothing via
smoothedBass unchanged once bassEnergy is computed correctly.
- Around line 62-120: The snapshot sampling logic in visualizer.js leaks WASM
heap buffers when an exception occurs after _malloc but before the normal
cleanup path. Update the snapshot block around copy_analyzer_snapshot, the
Float32Array view creation, and the downstream processing so inPtr and outPtr
are always freed in a finally-style cleanup, even when the catch returns false;
alternatively, reuse persistent buffers for the panel lifetime if that better
fits the surrounding code.
🪄 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: 3bb153d4-674d-4fa3-ae58-6c9d87e511c2
📒 Files selected for processing (3)
src/main.cppweb/shell.htmlweb/visualizer.js
Comment on lines
+319
to
+324
| extern "C" EMSCRIPTEN_KEEPALIVE int copy_analyzer_snapshot(float* input_dest_ptr, | ||
| float* output_dest_ptr, | ||
| int sample_count) { | ||
| if (!g_engine_ptr || !input_dest_ptr || !output_dest_ptr || sample_count <= 0) return 0; | ||
| bool ok = g_engine_ptr->copy_analyzer_snapshot(input_dest_ptr, output_dest_ptr, sample_count); | ||
| return ok ? sample_count : 0; |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Clamp or reject oversized snapshot requests before copying.
The bridge accepts any positive sample_count and passes it to the engine, despite documenting a clamp to the analyzer size. Since this is an exported pointer+length ABI, enforce the 2048-sample contract here before copying.
Proposed fix
extern "C" EMSCRIPTEN_KEEPALIVE int copy_analyzer_snapshot(float* input_dest_ptr,
float* output_dest_ptr,
int sample_count) {
if (!g_engine_ptr || !input_dest_ptr || !output_dest_ptr || sample_count <= 0) return 0;
- bool ok = g_engine_ptr->copy_analyzer_snapshot(input_dest_ptr, output_dest_ptr, sample_count);
- return ok ? sample_count : 0;
+ constexpr int max_analyzer_samples = 2048;
+ const int samples_to_copy =
+ sample_count > max_analyzer_samples ? max_analyzer_samples : sample_count;
+ bool ok =
+ g_engine_ptr->copy_analyzer_snapshot(input_dest_ptr, output_dest_ptr, samples_to_copy);
+ return ok ? samples_to_copy : 0;
}📝 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
| extern "C" EMSCRIPTEN_KEEPALIVE int copy_analyzer_snapshot(float* input_dest_ptr, | |
| float* output_dest_ptr, | |
| int sample_count) { | |
| if (!g_engine_ptr || !input_dest_ptr || !output_dest_ptr || sample_count <= 0) return 0; | |
| bool ok = g_engine_ptr->copy_analyzer_snapshot(input_dest_ptr, output_dest_ptr, sample_count); | |
| return ok ? sample_count : 0; | |
| extern "C" EMSCRIPTEN_KEEPALIVE int copy_analyzer_snapshot(float* input_dest_ptr, | |
| float* output_dest_ptr, | |
| int sample_count) { | |
| if (!g_engine_ptr || !input_dest_ptr || !output_dest_ptr || sample_count <= 0) return 0; | |
| constexpr int max_analyzer_samples = 2048; | |
| const int samples_to_copy = | |
| sample_count > max_analyzer_samples ? max_analyzer_samples : sample_count; | |
| bool ok = | |
| g_engine_ptr->copy_analyzer_snapshot(input_dest_ptr, output_dest_ptr, samples_to_copy); | |
| return ok ? samples_to_copy : 0; |
🤖 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/main.cpp` around lines 319 - 324, The exported bridge function
copy_analyzer_snapshot currently accepts any positive sample_count and forwards
it directly to g_engine_ptr->copy_analyzer_snapshot, so enforce the documented
2048-sample contract here before copying. Update copy_analyzer_snapshot to
reject or clamp any oversized request above the analyzer snapshot size, while
keeping the existing null-pointer and nonpositive checks intact, so the ABI
never passes an out-of-range length to the engine.
Comment on lines
+62
to
+120
| const inPtr = Module._malloc(n * 4); | ||
| const outPtr = Module._malloc(n * 4); | ||
| if (!inPtr || !outPtr) { | ||
| if (inPtr) Module._free(inPtr); | ||
| if (outPtr) Module._free(outPtr); | ||
| return hasData; | ||
| } | ||
|
|
||
| const count = Module.ccall( | ||
| "copy_analyzer_snapshot", | ||
| "number", | ||
| ["number", "number", "number"], | ||
| [inPtr, outPtr, n] | ||
| ); | ||
|
|
||
| if (count > 0) { | ||
| const inView = new Float32Array(Module.HEAPF32.buffer, inPtr, n); | ||
| const outView = new Float32Array(Module.HEAPF32.buffer, outPtr, n); | ||
| currentInputSamples.set(inView); | ||
| currentOutputSamples.set(outView); | ||
| hasData = true; | ||
|
|
||
| // Compute bass energy (first ~5% of FFT bins = low frequencies) | ||
| // For time-domain, look at low-frequency content via downsampled RMS | ||
| let sum = 0; | ||
| const bassBins = Math.min(128, n); | ||
| for (let i = 0; i < bassBins; i++) { | ||
| sum += Math.abs(outView[i]); | ||
| } | ||
| bassEnergy = sum / bassBins; | ||
| smoothedBass = smoothedBass * 0.8 + bassEnergy * 0.2; | ||
|
|
||
| // Build waveform frame: downsample 2048 -> ~256 for display | ||
| const frameLen = 256; | ||
| const frame = new Float32Array(frameLen); | ||
| const step = Math.max(1, Math.floor(n / frameLen)); | ||
| for (let i = 0; i < frameLen; i++) { | ||
| let peak = 0; | ||
| const start = i * step; | ||
| const end = Math.min(start + step, n); | ||
| for (let j = start; j < end; j++) { | ||
| const abs = Math.abs(outView[j]); | ||
| if (abs > peak) peak = abs; | ||
| } | ||
| frame[i] = peak; | ||
| } | ||
| waveformHistory.push(frame); | ||
| if (waveformHistory.length > WAVEFORM_HISTORY_LEN) { | ||
| waveformHistory.shift(); | ||
| } | ||
| // Simple progress simulation (could be driven by transport position) | ||
| playbackProgress = (playbackProgress + 0.001) % 1.0; | ||
| } | ||
|
|
||
| Module._free(inPtr); | ||
| Module._free(outPtr); | ||
| return count > 0; | ||
| } catch (e) { | ||
| return false; |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Free WASM heap buffers on every exit path.
If copy_analyzer_snapshot or view construction throws after _malloc, the catch returns without freeing the allocated buffers. Wrap the allocation block in finally or reuse persistent buffers for the panel lifetime.
Proposed fix
- const inPtr = Module._malloc(n * 4);
- const outPtr = Module._malloc(n * 4);
- if (!inPtr || !outPtr) {
- if (inPtr) Module._free(inPtr);
- if (outPtr) Module._free(outPtr);
- return hasData;
- }
+ let inPtr = 0;
+ let outPtr = 0;
- const count = Module.ccall(
- "copy_analyzer_snapshot",
- "number",
- ["number", "number", "number"],
- [inPtr, outPtr, n]
- );
+ try {
+ inPtr = Module._malloc(n * 4);
+ outPtr = Module._malloc(n * 4);
+ if (!inPtr || !outPtr) return hasData;
+
+ const count = Module.ccall(
+ "copy_analyzer_snapshot",
+ "number",
+ ["number", "number", "number"],
+ [inPtr, outPtr, n]
+ );
- if (count > 0) {
+ if (count > 0) {
...
- }
+ }
- Module._free(inPtr);
- Module._free(outPtr);
- return count > 0;
+ return count > 0;
+ } finally {
+ if (inPtr) Module._free(inPtr);
+ if (outPtr) Module._free(outPtr);
+ }📝 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
| const inPtr = Module._malloc(n * 4); | |
| const outPtr = Module._malloc(n * 4); | |
| if (!inPtr || !outPtr) { | |
| if (inPtr) Module._free(inPtr); | |
| if (outPtr) Module._free(outPtr); | |
| return hasData; | |
| } | |
| const count = Module.ccall( | |
| "copy_analyzer_snapshot", | |
| "number", | |
| ["number", "number", "number"], | |
| [inPtr, outPtr, n] | |
| ); | |
| if (count > 0) { | |
| const inView = new Float32Array(Module.HEAPF32.buffer, inPtr, n); | |
| const outView = new Float32Array(Module.HEAPF32.buffer, outPtr, n); | |
| currentInputSamples.set(inView); | |
| currentOutputSamples.set(outView); | |
| hasData = true; | |
| // Compute bass energy (first ~5% of FFT bins = low frequencies) | |
| // For time-domain, look at low-frequency content via downsampled RMS | |
| let sum = 0; | |
| const bassBins = Math.min(128, n); | |
| for (let i = 0; i < bassBins; i++) { | |
| sum += Math.abs(outView[i]); | |
| } | |
| bassEnergy = sum / bassBins; | |
| smoothedBass = smoothedBass * 0.8 + bassEnergy * 0.2; | |
| // Build waveform frame: downsample 2048 -> ~256 for display | |
| const frameLen = 256; | |
| const frame = new Float32Array(frameLen); | |
| const step = Math.max(1, Math.floor(n / frameLen)); | |
| for (let i = 0; i < frameLen; i++) { | |
| let peak = 0; | |
| const start = i * step; | |
| const end = Math.min(start + step, n); | |
| for (let j = start; j < end; j++) { | |
| const abs = Math.abs(outView[j]); | |
| if (abs > peak) peak = abs; | |
| } | |
| frame[i] = peak; | |
| } | |
| waveformHistory.push(frame); | |
| if (waveformHistory.length > WAVEFORM_HISTORY_LEN) { | |
| waveformHistory.shift(); | |
| } | |
| // Simple progress simulation (could be driven by transport position) | |
| playbackProgress = (playbackProgress + 0.001) % 1.0; | |
| } | |
| Module._free(inPtr); | |
| Module._free(outPtr); | |
| return count > 0; | |
| } catch (e) { | |
| return false; | |
| let inPtr = 0; | |
| let outPtr = 0; | |
| try { | |
| inPtr = Module._malloc(n * 4); | |
| outPtr = Module._malloc(n * 4); | |
| if (!inPtr || !outPtr) return hasData; | |
| const count = Module.ccall( | |
| "copy_analyzer_snapshot", | |
| "number", | |
| ["number", "number", "number"], | |
| [inPtr, outPtr, n] | |
| ); | |
| if (count > 0) { | |
| const inView = new Float32Array(Module.HEAPF32.buffer, inPtr, n); | |
| const outView = new Float32Array(Module.HEAPF32.buffer, outPtr, n); | |
| currentInputSamples.set(inView); | |
| currentOutputSamples.set(outView); | |
| hasData = true; | |
| // Compute bass energy (first ~5% of FFT bins = low frequencies) | |
| // For time-domain, look at low-frequency content via downsampled RMS | |
| let sum = 0; | |
| const bassBins = Math.min(128, n); | |
| for (let i = 0; i < bassBins; i++) { | |
| sum += Math.abs(outView[i]); | |
| } | |
| bassEnergy = sum / bassBins; | |
| smoothedBass = smoothedBass * 0.8 + bassEnergy * 0.2; | |
| // Build waveform frame: downsample 2048 -> ~256 for display | |
| const frameLen = 256; | |
| const frame = new Float32Array(frameLen); | |
| const step = Math.max(1, Math.floor(n / frameLen)); | |
| for (let i = 0; i < frameLen; i++) { | |
| let peak = 0; | |
| const start = i * step; | |
| const end = Math.min(start + step, n); | |
| for (let j = start; j < end; j++) { | |
| const abs = Math.abs(outView[j]); | |
| if (abs > peak) peak = abs; | |
| } | |
| frame[i] = peak; | |
| } | |
| waveformHistory.push(frame); | |
| if (waveformHistory.length > WAVEFORM_HISTORY_LEN) { | |
| waveformHistory.shift(); | |
| } | |
| // Simple progress simulation (could be driven by transport position) | |
| playbackProgress = (playbackProgress + 0.001) % 1.0; | |
| } | |
| return count > 0; | |
| } finally { | |
| if (inPtr) Module._free(inPtr); | |
| if (outPtr) Module._free(outPtr); | |
| } | |
| } catch (e) { | |
| return false; |
🤖 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 `@web/visualizer.js` around lines 62 - 120, The snapshot sampling logic in
visualizer.js leaks WASM heap buffers when an exception occurs after _malloc but
before the normal cleanup path. Update the snapshot block around
copy_analyzer_snapshot, the Float32Array view creation, and the downstream
processing so inPtr and outPtr are always freed in a finally-style cleanup, even
when the catch returns false; alternatively, reuse persistent buffers for the
panel lifetime if that better fits the surrounding code.
Comment on lines
+84
to
+92
| // Compute bass energy (first ~5% of FFT bins = low frequencies) | ||
| // For time-domain, look at low-frequency content via downsampled RMS | ||
| let sum = 0; | ||
| const bassBins = Math.min(128, n); | ||
| for (let i = 0; i < bassBins; i++) { | ||
| sum += Math.abs(outView[i]); | ||
| } | ||
| bassEnergy = sum / bassBins; | ||
| smoothedBass = smoothedBass * 0.8 + bassEnergy * 0.2; |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Compute bass from low-frequency content, not the first samples.
outView is time-domain data, so the first 128 samples are not “FFT bins” or low frequencies. This makes circular mode respond to an arbitrary slice of the buffer instead of bass energy.
Proposed fix
- // Compute bass energy (first ~5% of FFT bins = low frequencies)
- // For time-domain, look at low-frequency content via downsampled RMS
- let sum = 0;
- const bassBins = Math.min(128, n);
- for (let i = 0; i < bassBins; i++) {
- sum += Math.abs(outView[i]);
- }
- bassEnergy = sum / bassBins;
+ // Approximate low-frequency energy with a simple one-pole low-pass over the full buffer.
+ let low = 0;
+ let lowSquareSum = 0;
+ const lowPassAlpha = 0.08;
+ for (let i = 0; i < n; i++) {
+ low += lowPassAlpha * (outView[i] - low);
+ lowSquareSum += low * low;
+ }
+ bassEnergy = Math.sqrt(lowSquareSum / n);
smoothedBass = smoothedBass * 0.8 + bassEnergy * 0.2;📝 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
| // Compute bass energy (first ~5% of FFT bins = low frequencies) | |
| // For time-domain, look at low-frequency content via downsampled RMS | |
| let sum = 0; | |
| const bassBins = Math.min(128, n); | |
| for (let i = 0; i < bassBins; i++) { | |
| sum += Math.abs(outView[i]); | |
| } | |
| bassEnergy = sum / bassBins; | |
| smoothedBass = smoothedBass * 0.8 + bassEnergy * 0.2; | |
| // Approximate low-frequency energy with a simple one-pole low-pass over the full buffer. | |
| let low = 0; | |
| let lowSquareSum = 0; | |
| const lowPassAlpha = 0.08; | |
| for (let i = 0; i < n; i++) { | |
| low += lowPassAlpha * (outView[i] - low); | |
| lowSquareSum += low * low; | |
| } | |
| bassEnergy = Math.sqrt(lowSquareSum / n); | |
| smoothedBass = smoothedBass * 0.8 + bassEnergy * 0.2; |
🤖 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 `@web/visualizer.js` around lines 84 - 92, The bass-energy calculation in the
visualizer is using the first 128 values from outView as if they were FFT bins,
but outView is time-domain data, so this should be changed to derive bass from
actual low-frequency content instead of an arbitrary buffer slice. Update the
logic in the bass-energy section of visualizer.js to use the appropriate
frequency-domain source or a proper low-frequency aggregation path, and keep the
smoothing via smoothedBass unchanged once bassEnergy is computed correctly.
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
1 participant
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.
Real-Time Audio Visualization Panel has been implemented with the following components:
#379
1.
web/visualizer.js- JavaScript Visualization ModulerequestAnimationFrameloop, HiDPI-aware Canvas rendering viadevicePixelRatio, and automatically enables/disables the C++ analyzer capture when the panel is opened/closed to avoid unnecessary CPU usage.2.
src/main.cpp- C++ Bridge Functions (3 new Emscripten exports)enable_analyzer(int enabled)- Toggles theAnalyzerCapturecomponent on/offget_analyzer_sequence()- Returns the current snapshot sequence counter (for change detection)copy_analyzer_snapshot()- Copies 2048 pre/post-chain audio samples into JS-accessible WASM heap buffers3.
web/shell.html- Integrationvisualizer.jsin the Emscripten shell pageArchitecture
The panel connects to the existing
AnalyzerCaptureinfrastructure in the C++ audio engine, which captures 2048-sample ring buffers of both pre-chain (input) and post-chain (output) audio. The JS module polls the sequence counter each frame and copies snapshot data viaModule.ccallwith temporary WASM heap allocations, then renders using the HTML5 Canvas 2D API.Summary by CodeRabbit