Performance: Optimize spectrogram canvas rendering loops#271
Conversation
- Changed `drawSpectrogramToCanvas` to pre-calculate mapping arrays and process data linearly. - Re-ordered loops for cache locality. - Inlined calculations to avoid function call overhead per pixel.
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
📝 WalkthroughWalkthroughThis PR optimizes spectrogram rendering in ChangesCanvas Spectrogram Rendering Optimization
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Review Summary by QodoOptimize spectrogram canvas rendering with cache-friendly loop structure
WalkthroughsDescription• Optimized spectrogram canvas rendering by swapping loop order for sequential buffer writes • Pre-computed time scale mappings into Int32Array to eliminate repeated calculations • Inlined normalizeMelForDisplay constants and clamping logic into tight inner loop • Achieved ~30% performance improvement (6.8ms to 4.7ms per 1k frames) Diagramflowchart LR
A["Original: X-outer Y-inner<br/>Non-sequential writes"] -->|"Swap loops"| B["Y-outer X-inner<br/>Sequential idx++"]
C["Function calls per pixel<br/>normalizeMelForDisplay"] -->|"Inline constants"| D["Direct arithmetic<br/>MEL_MIN/MEL_RANGE"]
E["Repeated calculations<br/>timeScale per pixel"] -->|"Pre-compute"| F["Int32Array tMap<br/>One-time setup"]
B --> G["30% faster rendering<br/>Better cache locality"]
D --> G
F --> G
File Changes1. src/components/LayeredBufferVisualizer.tsx
|
Code Review by Qodo
1. Per-draw tMap allocation
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The new
tMapbehavior changes semantics from skipping columns whent >= timeStepsto duplicating the last timestep across the remaining width; if that wasn’t intentional, consider preserving the earlybreakor explicitly documenting this visual change. tMapand theInt32Arrayare allocated on every render; if this method runs frequently, you could cache and reuse a buffer sized to the currentwidthto avoid per-frame allocations and GC overhead.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `tMap` behavior changes semantics from skipping columns when `t >= timeSteps` to duplicating the last timestep across the remaining width; if that wasn’t intentional, consider preserving the early `break` or explicitly documenting this visual change.
- `tMap` and the `Int32Array` are allocated on every render; if this method runs frequently, you could cache and reuse a buffer sized to the current `width` to avoid per-frame allocations and GC overhead.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request optimizes the log-mel spectrogram rendering in LayeredBufferVisualizer.tsx by restructuring the loops to ensure sequential writes to the ImageData buffer, which improves cache locality. Key changes include pre-computing time scale mappings and inlining normalization logic. Feedback suggests importing shared constants to prevent maintenance issues and further optimizing the inner loop by replacing division with multiplication.
| const MEL_MIN = -11.0; | ||
| const MEL_RANGE = 11.0; |
There was a problem hiding this comment.
Hardcoding these values creates a maintenance risk and potential for desync if the scaling logic in mel-display.ts is updated. It is recommended to import MEL_DISPLAY_MIN_DB and MEL_DISPLAY_DB_RANGE from ../lib/audio/mel-display and use them here. Modern JavaScript engines will optimize these imported constants just as effectively as local ones.
| const m = Math.floor((height - 1 - y) * freqScale); | ||
| if (m >= melBins) continue; | ||
| // Inline normalization and clamping | ||
| let lutIdx = (((val - MEL_MIN) / MEL_RANGE) * 255) | 0; |
There was a problem hiding this comment.
In this tight inner loop, replacing the division with a multiplication by a pre-calculated constant can improve performance. Since MEL_RANGE is a constant, the expression (255 / MEL_RANGE) will be constant-folded by the JIT compiler, resulting in a single multiplication per pixel instead of a division and a multiplication.
| let lutIdx = (((val - MEL_MIN) / MEL_RANGE) * 255) | 0; | |
| let lutIdx = ((val - MEL_MIN) * (255 / MEL_RANGE)) | 0; |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/components/LayeredBufferVisualizer.tsx (1)
350-355: ⚡ Quick win
tMapis reallocated on every spectrogram draw — cache it likecachedSpecImgData.
new Int32Array(width)is created at ~10 fps, producing short-lived typed-array allocations (4–8 KB at HiDPI) that the GC must collect. The file already has an established pattern of caching and conditionally growing pre-allocated buffers (cachedSpecImgData,waveformReadBuf).tMapshould follow the same pattern.♻️ Proposed fix — cache tMap at component scope
Add at component scope alongside the other cached buffers (~line 116):
+ let cachedTMap: Int32Array | null = null; + let cachedTMapWidth = 0;Then inside
drawSpectrogramToCanvas:- // Pre-compute time scale mapping - const tMap = new Int32Array(width); - for (let x = 0; x < width; x++) { - const t = (x * timeScale) | 0; - tMap[x] = t >= timeSteps ? timeSteps - 1 : t; - } + // Pre-compute time scale mapping — reuse buffer if width is unchanged + if (!cachedTMap || cachedTMapWidth !== width) { + cachedTMap = new Int32Array(width); + cachedTMapWidth = width; + } + const tMap = cachedTMap; + for (let x = 0; x < width; x++) { + const t = (x * timeScale) | 0; + tMap[x] = t >= timeSteps ? timeSteps - 1 : t; + }🤖 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/components/LayeredBufferVisualizer.tsx` around lines 350 - 355, The tMap Int32Array is being reallocated every draw in drawSpectrogramToCanvas; allocate a cached tMap at component scope (like cachedSpecImgData and waveformReadBuf), reuse it across draws, and only (re)create a larger Int32Array when width increases; inside drawSpectrogramToCanvas update/resize the cached tMap and fill its entries instead of doing new Int32Array(width) on every frame to avoid GC churn.
🤖 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 `@src/components/LayeredBufferVisualizer.tsx`:
- Around line 350-355: The tMap Int32Array is being reallocated every draw in
drawSpectrogramToCanvas; allocate a cached tMap at component scope (like
cachedSpecImgData and waveformReadBuf), reuse it across draws, and only
(re)create a larger Int32Array when width increases; inside
drawSpectrogramToCanvas update/resize the cached tMap and fill its entries
instead of doing new Int32Array(width) on every frame to avoid GC churn.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9b48efab-5faa-48e1-95af-009bdaa1245f
📒 Files selected for processing (2)
.jules/bolt.mdsrc/components/LayeredBufferVisualizer.tsx
1 participant
What changed
drawSpectrogramToCanvasmethod inLayeredBufferVisualizer.data[idx++]) into the 1DImageDatabuffer instead of striding.timeScaleinteger mappings into anInt32Arraybefore the inner loops.normalizeMelForDisplaydirectly inside the tight inner loop.Why it was needed
normalizeMelForDisplay) per pixel.Impact
How to verify
PR created automatically by Jules for task 1922297626600177194 started by @ysdede
Summary by Sourcery
Optimize spectrogram canvas rendering in LayeredBufferVisualizer to reduce main-thread CPU usage during visualization.
Enhancements:
Chores:
Summary by CodeRabbit
Refactor
Documentation