performance: use offscreen canvas to decode on-disk masks #5089
Conversation
WalkthroughThe changes in this pull request introduce enhancements to the Changes
Suggested reviewers
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
|
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (3)
app/packages/looker/src/worker/canvas-decoder.ts (2)
23-25: LGTM with minor suggestionThe channel calculation logic is correct, but could be more explicit.
Optional improvement for clarity:
- const numChannels = - overlayData.channels ?? - overlayData.data.length / (overlayData.width * overlayData.height); + const pixelCount = overlayData.width * overlayData.height; + const numChannels = overlayData.channels ?? (overlayData.data.length / pixelCount);
27-33: Improve type safetyThe return value uses type assertion which could be avoided with proper typing.
Consider this improvement:
- return { + const result: OverlayMask = { buffer: overlayData.data.buffer, channels: numChannels, arrayType: overlayData.data.constructor.name as OverlayMask["arrayType"], - shape: [overlayData.height, overlayData.width], + shape: [overlayData.height, overlayData.width] as [number, number], - } as OverlayMask; + }; + return result;app/packages/looker/src/worker/index.ts (1)
144-152: Consider using the URL API for parsing and handling URLs.The current implementation manually splits the URL string and inspects query parameters. Utilizing the built-in
URLAPI can improve readability and robustness, especially when dealing with complex URLs.Apply this diff to refactor the code:
-const urlTokens = overlayImageUrl.split("?"); - -let baseUrl = overlayImageUrl; - -// remove query params if not local URL -if (!urlTokens.at(1)?.startsWith("filepath=")) { - baseUrl = overlayImageUrl.split("?")[0]; -} - -const fileExtension = baseUrl.split(".").pop(); +const overlayUrl = new URL(overlayImageUrl); + +// Remove query params if not local URL +if (!overlayUrl.searchParams.has("filepath")) { + overlayUrl.search = ""; +} + +const fileExtension = overlayUrl.pathname.split(".").pop();
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (3)
app/package.jsonis excluded by!**/*.jsonapp/packages/looker/package.jsonis excluded by!**/*.jsonapp/yarn.lockis excluded by!**/yarn.lock,!**/*.lock,!**/*.lock
📒 Files selected for processing (3)
app/packages/looker/src/numpy.ts(2 hunks)app/packages/looker/src/worker/canvas-decoder.ts(1 hunks)app/packages/looker/src/worker/index.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
app/packages/looker/src/numpy.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.
app/packages/looker/src/worker/index.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 (3)
app/packages/looker/src/worker/canvas-decoder.ts (1)
1-33: Verify performance improvements
While the implementation looks solid, the claimed 2x performance improvement should be verified with benchmarks.
Consider:
- Adding performance benchmarks
- Alternative testing approaches:
- Integration tests with a real browser environment
- Performance comparison tests between old and new implementations
- Edge case testing with various image formats
Run this script to find existing benchmark/test files:
app/packages/looker/src/numpy.ts (1)
12-12: LGTM! Addition of Uint8ClampedArray aligns with canvas implementation.
The addition of Uint8ClampedArray to ARRAY_TYPES is necessary for supporting the new canvas-based mask decoding implementation. This array type is specifically designed for handling image data with automatic value clamping between 0 and 255.
app/packages/looker/src/worker/index.ts (1)
31-31: Import 'decodeWithCanvas' correctly integrated.
The import statement for decodeWithCanvas from canvas-decoder.ts is appropriately added and necessary for the new image decoding functionality.
|
Love it! |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
app/packages/looker/src/worker/canvas-decoder.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
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 (5)
app/packages/looker/src/worker/canvas-decoder.ts (5)
3-6: Enhance documentation and add input validation
The past review comment about adding input validation and documentation is still valid.
Let's check browser compatibility:
#!/bin/bash
# Description: Check browser compatibility for OffscreenCanvas
# Expected: Find any polyfills or fallbacks for browsers not supporting OffscreenCanvas
# Search for browser compatibility checks or polyfills
rg -g '!node_modules' -g '*.{ts,tsx,js,jsx}' 'OffscreenCanvas'
# Search for any existing fallback mechanisms
rg -g '!node_modules' -g '*.{ts,tsx,js,jsx}' 'createImageBitmap'7-9: Add error handling and verify performance claims
The past review comment about adding error handling for bitmap creation is still valid.
Let's verify the performance claims:
#!/bin/bash
# Description: Find and analyze the previous implementation to verify 2x performance improvement
# Expected: Locate the old implementation using fast-png or jpeg-js
# Search for removed or replaced image decoding implementations
rg -g '!node_modules' -B 10 -A 10 'fast-png|jpeg-js'
# Search for any performance benchmarks or tests
rg -g '!node_modules' -g '*.{ts,tsx,js,jsx}' 'benchmark|performance.now'11-12: Add context optimization and error handling
The past review comment about adding context type assertion and error handling is still valid.
30-36: Improve type safety and resource management
Replace type assertion with runtime type checking:
- } as OverlayMask;
+ } satisfies OverlayMask;Let's check for consistent resource cleanup:
#!/bin/bash
# Description: Check for consistent resource cleanup patterns
# Expected: Find similar image processing code to ensure resources are properly cleaned up
# Search for bitmap cleanup patterns
rg -g '!node_modules' -g '*.{ts,tsx,js,jsx}' 'close\(\)|dispose\(\)'
# Search for canvas cleanup patterns
rg -g '!node_modules' -g '*.{ts,tsx,js,jsx}' 'canvas\.width\s*=\s*0|canvas = null'14-16:
Add error handling and optimize memory usage
Add error handling for drawing operations and image data extraction:
- ctx.drawImage(imageBitmap, 0, 0);
- const imageData = ctx.getImageData(0, 0, width, height);
+ try {
+ ctx.drawImage(imageBitmap, 0, 0);
+ const imageData = ctx.getImageData(0, 0, width, height);
+ } catch (error) {
+ throw new Error(`Failed to process image: ${error.message}`);
+ }Let's check for memory leaks:
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
app/packages/looker/src/worker/index.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
app/packages/looker/src/worker/index.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 (3)
app/packages/looker/src/worker/index.ts (3)
31-31: LGTM: Clean import of canvas decoder
The import of decodeWithCanvas aligns with the PR's performance improvement objectives.
162-170: LGTM: Comprehensive MIME type handling
Good implementation with:
- Support for multiple image formats
- Safe fallback to 'application/octet-stream'
- Clean mapping structure
171-174:
Add error handling for canvas decoding operations
The canvas decoding implementation lacks error handling for potential failures.
Additionally, verify the buffer size calculation:
#!/bin/bash
# Check for potential buffer overflow issues in image processing
ast-grep --pattern 'new ArrayBuffer($size)' Also applies to: 179-179
| * Decodes a given image source into an OverlayMask using an OffscreenCanvas | ||
| */ | ||
| export const decodeWithCanvas = async (blob: ImageBitmapSource) => { | ||
| const imageBitmap = await createImageBitmap(blob); |
There was a problem hiding this comment.
I wonder if we could use ImageBitmap in the Overlay classes instead of ImageData. It seems like that could be a performance improvement.
Just thinking out loud. Nothing actionable in this PR
There was a problem hiding this comment.
I like the idea. Let's explore in another thread.
What changes are proposed in this pull request?
This approach of using an off-screen canvas to decode mask images provides a significant performance boost (almost 2x based on benchmarking) in decoding mask paths compared to a naive javascript approach that the 3pp libraries (fast-png and jpeg-js) were using. Canvas also automatically handles edge-case scenarios in decoding images like multi-palette pngs and non-8-bit images.
Benchmarking script:
canvas-lean.zip
To start benchmarking app:
npx http-serverin root directory. This is to host images.yarn && yarn start.How is this patch tested? If it is not, please explain why.
Chose to not write unit tests because canvas in vitest would have to be mocked and that defeats the purpose. Tested locally.
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
Uint8ClampedArrayin the array types.Improvements
Bug Fixes