fix grayscale segmentation regression + RGB masks recoloring issue #5266
Conversation
WalkthroughThe changes introduce a new function Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 1
🧹 Outside diff range and nitpick comments (5)
app/packages/looker/src/lookers/utils.ts (1)
3-7: LGTM! Consider adding JSDoc documentation.The implementation is clean and efficient. Consider adding JSDoc documentation to describe the parameters and return value, especially since this is a utility function that will be used across the codebase.
+/** + * Checks if a frame number exists within any of the buffer ranges + * @param buffers - Array of buffer ranges [start, end] + * @param frameNumber - Frame number to check + * @returns boolean indicating if the frame exists in any buffer range + */ export const hasFrame = (buffers: Buffers, frameNumber: number) => {app/packages/looker/src/lookers/utils.test.ts (1)
5-19: Consider adding more edge cases to the test suite.The current test coverage is good but could be enhanced by adding these scenarios:
- Empty buffers array
- Single-frame buffer (where start equals end)
- Adjacent buffers (e.g., [1,3], [4,6])
describe("looker utilities", () => { + it("handles empty buffers", () => { + const EMPTY_BUFFERS: Buffers = []; + expect(hasFrame(EMPTY_BUFFERS, 1)).toBe(false); + }); + + it("handles single-frame buffers", () => { + const SINGLE_FRAME: Buffers = [[5, 5]]; + expect(hasFrame(SINGLE_FRAME, 5)).toBe(true); + expect(hasFrame(SINGLE_FRAME, 4)).toBe(false); + }); + it("determines frame availability given a buffer list", () => {app/packages/looker/src/worker/canvas-decoder.test.ts (2)
17-25: Consider adding more alpha value edge casesThe test effectively checks for non-255 alpha values, but consider adding tests for alpha = 0 and other boundary values.
it("should handle various alpha values", () => { const alphaTests = [ [100, 100, 100, 0], // fully transparent [100, 100, 100, 1], // nearly transparent [100, 100, 100, 254], // nearly opaque ]; for (const pixel of alphaTests) { const data = createData([pixel, ...Array(99).fill([100, 100, 100, 255])]); expect(isGrayscale(data)).toBe(false); } });
27-35: Consider adding more color channel variation testsWhile the test effectively checks for non-grayscale pixels, consider adding tests for variations in red and blue channels as well.
it("should detect various non-grayscale patterns", () => { const colorTests = [ [101, 100, 100, 255], // red variation [100, 100, 101, 255], // blue variation [0, 255, 0, 255], // extreme difference ]; for (const pixel of colorTests) { const data = createData([pixel, ...Array(99).fill([100, 100, 100, 255])]); expect(isGrayscale(data)).toBe(false); } });app/packages/looker/src/worker/canvas-decoder.ts (1)
41-48: Consider preserving original imageDataThe current implementation modifies the original imageData. Consider creating a new buffer instead.
if (channels === 1) { // get rid of the G, B, and A channels, new buffer will be 1/4 the size const data = new Uint8ClampedArray(width * height); for (let i = 0; i < data.length; i++) { data[i] = imageData.data[i * 4]; } - imageData.data.set(data); + return { + buffer: data.buffer, + channels, + arrayType: "Uint8ClampedArray", + shape: [height, width], + } as OverlayMask; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/packages/looker/src/lookers/utils.test.ts(1 hunks)app/packages/looker/src/lookers/utils.ts(1 hunks)app/packages/looker/src/lookers/video.ts(2 hunks)app/packages/looker/src/worker/canvas-decoder.test.ts(1 hunks)app/packages/looker/src/worker/canvas-decoder.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
app/packages/looker/src/lookers/utils.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/lookers/utils.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/canvas-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/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/lookers/video.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 (4)
app/packages/looker/src/lookers/video.ts (1)
398-398: LGTM! Clean refactor of hasFrame method.
The change simplifies the implementation by delegating to the new utility function while maintaining the same behavior. This improves code maintainability and reduces duplication.
app/packages/looker/src/worker/canvas-decoder.test.ts (2)
4-8: LGTM: Helper function is well-implemented
The createData utility function is well-typed and correctly converts pixel tuples to the required Uint8ClampedArray format.
11-15: LGTM: Good test coverage for basic grayscale case
The test effectively verifies the basic grayscale detection with sufficient sample size.
app/packages/looker/src/worker/canvas-decoder.ts (1)
51-54: LGTM: Return object structure is correct
The OverlayMask object is properly constructed with all required fields.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
app/packages/looker/src/worker/canvas-decoder.ts (2)
3-8: Enhance function documentationWhile the current documentation explains the sampling strategy well, consider adding:
@paramtag describing thedataparameter@returnstag explaining the boolean return value@exampleshowing typical usage/** * Checks if the given pixel data is grayscale by sampling a subset of pixels. * The function will check at least 500 pixels or 1% of all pixels, whichever is larger. * If the image is grayscale, the R, G, and B channels will be equal for all sampled pixels, * and the alpha channel will always be 255. + * + * @param data - Uint8ClampedArray containing RGBA pixel data + * @returns boolean - true if the image is grayscale, false otherwise + * @example + * const imageData = ctx.getImageData(0, 0, width, height); + * const isGray = isGrayscale(imageData.data); */
39-48: Improve memory management and error handlingThe grayscale optimization is good, but consider these improvements:
- Use
TypedArray.transfer()for better memory efficiency- Add error handling for buffer allocation
const channels = isGrayscale(imageData.data) ? 1 : 4; if (channels === 1) { - // get rid of the G, B, and A channels, new buffer will be 1/4 the size - const data = new Uint8ClampedArray(width * height); + try { + // Preallocate with correct size + const data = new Uint8ClampedArray(new ArrayBuffer(width * height)); + + // Use typed array methods for better performance + for (let i = 0; i < data.length; i++) { + data[i] = imageData.data[i * 4]; + } + + // Transfer the buffer to avoid memory copy + imageData.data = new Uint8ClampedArray(data.buffer); + } catch (error) { + console.error("Failed to optimize grayscale buffer:", error); + // Fallback to RGBA + return { + buffer: imageData.data.buffer, + channels: 4, + arrayType: "Uint8ClampedArray", + shape: [height, width], + } as OverlayMask; + } - for (let i = 0; i < data.length; i++) { - data[i] = imageData.data[i * 4]; - } - imageData.data.set(data); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/packages/looker/src/worker/canvas-decoder.test.ts(1 hunks)app/packages/looker/src/worker/canvas-decoder.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/packages/looker/src/worker/canvas-decoder.test.ts
🧰 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 (1)
app/packages/looker/src/worker/canvas-decoder.ts (1)
51-54: Verify type compatibility and memory management
Let's verify the impact of these changes on the codebase.
✅ Verification successful
Type compatibility and memory management are properly handled
Based on the verification results:
- The
OverlayMaskinterface innumpy.tscorrectly defines the expected type structure withbuffer: ArrayBuffer, matching the implementation. - Memory management is properly handled:
- The buffer is part of the
ImageDataobject which is managed by the browser - Canvas cleanup is implemented in overlay classes via
cleanup()methods - Video and bitmap resources are properly released through
close()andrelease()methods
- The buffer is part of the
The changes are type-safe and follow the established memory management patterns in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check OverlayMask type usage and potential memory issues
# Find all uses of OverlayMask type
echo "Checking OverlayMask usage..."
ast-grep --pattern 'type $_ = OverlayMask'
# Look for memory management patterns
echo "Checking memory management patterns..."
rg -l 'imageData\.data\.buffer' --type ts
# Check for existing memory leak reports
echo "Checking for memory leak reports..."
rg -i 'memory\s+leak.*canvas' --type ts
Length of output: 416
Script:
#!/bin/bash
# Let's check more thoroughly
# Find type definitions and interfaces related to canvas data
echo "Checking type definitions..."
rg "interface.*Mask" --type ts -A 5
rg "type.*Mask\s*=" --type ts -A 5
# Check buffer usage patterns in the codebase
echo "Checking buffer usage..."
rg "\.buffer" --type ts -A 2
# Look for canvas cleanup or disposal patterns
echo "Checking cleanup patterns..."
rg "dispose|cleanup|release|free" --type ts -A 3
# Check for ArrayBuffer or TypedArray related patterns
echo "Checking array buffer patterns..."
rg "Uint8ClampedArray|ArrayBuffer" --type ts -A 2
Length of output: 51601
sashankaryal
force-pushed
the
fix/grayscale-segmentations
branch
from
5307d57 to
d83c00a
Compare
sashankaryal
changed the title
What changes are proposed in this pull request?
Use a heuristic to detect if mask is grayscale. This is unfortunately a limitation of using canvas decoding method (introduced by #5089), which always outputs an RGBA bitmap.
Also fixes recoloring issue with RGB mask targets
How is this patch tested? If it is not, please explain why.
Locally. Use following code:
Release Notes
Is this a user-facing change that should be mentioned in the release notes?
notes for FiftyOne users.
(Details in 1-2 sentences. You can just refer to another PR with a description
if this PR is part of a larger change.)
What areas of FiftyOne does this PR affect?
fiftyonePython library changesSummary by CodeRabbit
New Features
Bug Fixes
Tests