heatmap targets memory optimization #5340
Conversation
WalkthroughThe pull request introduces significant changes to canvas and image decoding functionality across multiple files. The primary modifications focus on enhancing the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (3)
👮 Files not reviewed due to content moderation or server errors (3)
🧰 Additional context used📓 Path-based instructions (3)app/packages/looker/src/worker/disk-overlay-decoder.test.ts (1)Pattern app/packages/looker/src/worker/disk-overlay-decoder.ts (1)Pattern app/packages/looker/src/worker/canvas-decoder.ts (1)Pattern ⏰ Context from checks skipped due to timeout of 90000ms (5)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
app/packages/looker/src/worker/canvas-decoder.ts (2)
44-50: Clarify the documentation for in-place modification.
The JSDoc mentions “sets the buffer in place,” but the function returns a new sliced buffer. Consider clarifying this detail to avoid confusion.
Line range hint
70-121: Functionality looks good; consider a fallback for non-OffscreenCanvas contexts.
The code properly handles grayscale and heatmap cases. You may also consider a fallback path if OffscreenCanvas isn’t supported or the usage environment is unknown.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/packages/looker/src/worker/canvas-decoder.ts(4 hunks)app/packages/looker/src/worker/disk-overlay-decoder.test.ts(5 hunks)app/packages/looker/src/worker/disk-overlay-decoder.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
app/packages/looker/src/worker/disk-overlay-decoder.test.ts (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/looker/src/worker/disk-overlay-decoder.ts (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/looker/src/worker/canvas-decoder.ts (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
🔇 Additional comments (12)
app/packages/looker/src/worker/canvas-decoder.ts (3)
1-1: No issues with the new import statement.
Looks good to import the necessary constant here.
4-13: Ensure proper fallback or validation for OffscreenCanvas availability.
IfOffscreenCanvasis unavailable in the environment,canvasAndCtxwill remainundefined, but usage later forces a non-null assertion. This can lead to runtime errors in older or unsupported browsers.
51-68: Verify stride assumptions to prevent out-of-bounds reads.
Ifstride*totalPixelsexceeds the buffer length, the loop could read beyond the array. Confirm all usage ensures safe bounds.✅ Verification successful
Let me gather more information about the test cases and usage.
Let me check the actual usage in canvas-decoder.ts to see how stride is controlled.
Stride usage is safe and well-controlled
The verification shows that
recastBufferToMonoChannelis used safely:
The function is only called in two controlled scenarios in
decodeWithCanvas:
- When
channels === 1(grayscale PNG), with a fixed stride of 4 for RGBA data- When
cls === HEATMAP && channels > 1, with the actual channel count as strideThe test cases confirm safe usage with various strides (1, 3, 4) and validate correct output
The input buffer is always from
ImageData.datawhich is guaranteed to be properly sized for the image dimensions, ensuringstride * totalPixelsnever exceeds the buffer length🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Searches for all calls to recastBufferToMonoChannel to see if stride is always controlled rg -A 3 "recastBufferToMonoChannel"Length of output: 3594
Script:
#!/bin/bash # Get the test cases to understand how stride is used cat app/packages/looker/src/worker/disk-overlay-decoder.test.tsLength of output: 8739
Script:
#!/bin/bash # Get the full canvas-decoder.ts file to see how stride is used cat app/packages/looker/src/worker/canvas-decoder.tsLength of output: 3895
app/packages/looker/src/worker/disk-overlay-decoder.ts (3)
2-7: Imports look appropriate for label class constants.
These added imports are consistent with usage in the file.
11-11: Importing recastBufferToMonoChannel is acceptable.
The new import aligns with the added decoding logic incanvas-decoder.
122-122: Updated decodeWithCanvas call is correct.
PassingclstodecodeWithCanvasaligns with the function’s updated signature.app/packages/looker/src/worker/disk-overlay-decoder.test.ts (6)
2-2: Added imports match new overlay classes.
Bringing inDETECTIONS,HEATMAP, andSEGMENTATIONis consistent with the tested logic.
7-7: Imports decodeWithCanvas and recastBufferToMonoChannel for testing.
Including the new function for deeper test coverage is a good practice.
19-28: Mocking approach looks reasonable.
Usingvi.importActualensures the main logic remains intact while selectively mockingdecodeWithCanvas.
95-95: Ensures decodeWithCanvas is invoked with SEGMENTATION.
Confirming the correct class parameter in the test is appropriate.
131-131: Ensures decodeWithCanvas is invoked with HEATMAP.
Validates that the heatmap label type is handled properly.
213-286: Comprehensive tests for recastBufferToMonoChannel.
These tests cover various strides and buffer shapes, making the solution robust.
sashankaryal
force-pushed
the
optimization/heatmap
branch
from
b9bea14 to
c68f7f3
Compare
What changes are proposed in this pull request?
When png or jpg heatmaps are decoded, in addition to the overlay image buffer (later converted into GPU optimized ImageBitmap), we also store targets buffers for the tooltip. This targets buffer need not be w x h x 4, and can be of shape w x h.
This optimization should reduce the memory footprint by a factor of 4 when dealing with heatmaps.
How is this patch tested? If it is not, please explain why.
Unit testing, local testing.
Release Notes
Is this a user-facing change that should be mentioned in the release notes?
notes for FiftyOne users.
What areas of FiftyOne does this PR affect?
fiftyonePython library changesSummary by CodeRabbit
New Features
Bug Fixes
Tests