"fix" jpg segmentations #5406
"fix" jpg segmentations #5406
Conversation
WalkthroughThe pull request enhances the Changes
Sequence DiagramsequenceDiagram
participant Caller
participant decodeWithCanvas
participant Canvas
Caller->>decodeWithCanvas: blob, cls, field, coloring
decodeWithCanvas->>Canvas: Create canvas context
alt Segmentation Case
decodeWithCanvas->>decodeWithCanvas: Retrieve mask targets
decodeWithCanvas->>Canvas: Process mask targets
end
Canvas-->>decodeWithCanvas: Decoded image data
decodeWithCanvas-->>Caller: Return processed image
Possibly related PRs
Suggested reviewers
Poem
Finishing Touches
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 (1)
requirements/test.txt (1)
1-1: Consider using a version range for open3d.While pinning to version 0.18.0 ensures consistency, it might miss important security updates. Consider using a version range (e.g.,
open3d>=0.18.0,<0.19.0) to allow patch updates while maintaining compatibility.-open3d==0.18.0 +open3d>=0.18.0,<0.19.0
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/packages/looker/src/worker/canvas-decoder.ts(3 hunks)app/packages/looker/src/worker/disk-overlay-decoder.test.ts(2 hunks)app/packages/looker/src/worker/disk-overlay-decoder.ts(2 hunks)requirements/test.txt(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
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/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/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.
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: e2e / test-e2e
- GitHub Check: test / test-python (ubuntu-latest-m, 3.11)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.10)
- GitHub Check: lint / eslint
- GitHub Check: build / build
- GitHub Check: test / test-app
- GitHub Check: build
🔇 Additional comments (5)
app/packages/looker/src/worker/disk-overlay-decoder.ts (1)
117-122: LGTM! The function call has been correctly updated.The
decodeWithCanvasfunction call has been properly updated to include the new parametersfieldandcoloring, maintaining consistency with the function's new signature.app/packages/looker/src/worker/canvas-decoder.ts (2)
72-77: LGTM! Function signature has been properly updated.The function signature has been enhanced to include the necessary parameters for handling segmentation cases.
131-154: Consider handling JPG artifacts more robustly.While the implementation correctly handles RGB mask targets for segmentation, the comment about JPG artifacts suggests a potential issue where slight color changes can cause masks to be rendered as transparent. This could affect the reliability of the segmentation rendering.
Let's verify if there are any JPG segmentation files in the codebase that might be affected:
Consider adding a color tolerance threshold for JPG segmentations to handle compression artifacts more gracefully. For example:
if (cls === SEGMENTATION) { let maskTargets = coloring.maskTargets[field]; if (!maskTargets) { maskTargets = coloring.defaultMaskTargets; } const isRgbMaskTargets_ = isRgbMaskTargets(maskTargets); + // Add color tolerance for JPG compression artifacts + const colorTolerance = blob.type.includes('jpg') ? 5 : 0; + if (!isRgbMaskTargets_ && channels > 1) { // recast to mono channel because we don't need the other channels targetsBuffer = recastBufferToMonoChannel(✅ Verification successful
The JPG artifact handling suggestion is valid but not critical.
The codebase supports JPG images and acknowledges compression artifacts in comments, but lacks any mitigation strategy. The suggested color tolerance threshold would indeed improve the robustness of segmentation mask rendering for JPG images.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for JPG segmentation files rg -l --type-add 'seg:*.{jpg,jpeg}' --type seg 'segmentation|mask' .Length of output: 70
Script:
#!/bin/bash # Search for JPG/JPEG handling in segmentation context rg -A 5 -B 5 "jpg|jpeg" app/packages/looker/src/ # Look for blob type handling ast-grep --pattern 'blob.type' # Search for any existing color tolerance or artifact handling rg -A 3 "tolerance|artifact|threshold" app/packages/looker/src/Length of output: 1866
app/packages/looker/src/worker/disk-overlay-decoder.test.ts (2)
95-100: LGTM! Test expectations properly updated.The test expectations have been correctly updated to match the new function signature, including the additional parameters
fieldandCOLORING.
136-141: LGTM! HEATMAP test case properly updated.The test expectations for the HEATMAP case have been correctly updated to include the new parameters, maintaining consistency with the function signature changes.
sashankaryal
force-pushed
the
fix/segmentation-jpg
branch
from
0b2b292 to
21da464
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/packages/looker/src/worker/canvas-decoder.ts (1)
150-154: Consider propagating JPG artifact warning to caller.The comment about potential JPG artifacts with RGB mask targets is important information that should be propagated to the caller, possibly through a warning flag in the return object.
return { buffer: targetsBuffer, channels, arrayType: "Uint8ClampedArray", - shape: [height, width], + shape: [height, width], + warnings: isRgbMaskTargets_ && blob.type === "image/jpeg" + ? ["Potential artifacts in JPG segmentation with RGB mask targets"] + : [], } as OverlayMask;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/packages/looker/src/worker/canvas-decoder.ts(3 hunks)app/packages/looker/src/worker/disk-overlay-decoder.test.ts(2 hunks)app/packages/looker/src/worker/disk-overlay-decoder.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- app/packages/looker/src/worker/disk-overlay-decoder.ts
- app/packages/looker/src/worker/disk-overlay-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.
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: lint / eslint
- GitHub Check: test / test-app
- GitHub Check: e2e / test-e2e
- GitHub Check: test / test-python (ubuntu-latest-m, 3.10)
- GitHub Check: build / build
🔇 Additional comments (3)
app/packages/looker/src/worker/canvas-decoder.ts (3)
1-2: LGTM! Necessary imports added for segmentation support.The new imports for
SEGMENTATIONandColoringare correctly added to support the enhanced functionality.
72-77: LGTM! Function signature updated appropriately.The addition of
fieldandcoloringparameters provides the necessary context for handling different types of mask targets in segmentation processing.
137-148: Add validation for maskTargets structure.The code should validate the structure of
maskTargetsbefore using it withisRgbMaskTargetsto prevent runtime errors.
| let maskTargets = coloring.maskTargets[field]; | ||
| if (!maskTargets) { | ||
| maskTargets = coloring.defaultMaskTargets; | ||
| } |
There was a problem hiding this comment.
Add type checking for maskTargets access.
The access to coloring.maskTargets[field] should include type checking to ensure maskTargets exists before accessing it.
- let maskTargets = coloring.maskTargets[field];
- if (!maskTargets) {
+ let maskTargets = coloring.maskTargets?.[field];
+ if (maskTargets === undefined) {📝 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.
| let maskTargets = coloring.maskTargets[field]; | |
| if (!maskTargets) { | |
| maskTargets = coloring.defaultMaskTargets; | |
| } | |
| let maskTargets = coloring.maskTargets?.[field]; | |
| if (maskTargets === undefined) { | |
| maskTargets = coloring.defaultMaskTargets; | |
| } |
What changes are proposed in this pull request?
"fixes" jpg segmentations. It's fixed in a sense that something is rendered now as opposed to nothing; it's not fixed in a sense that using jpgs for segmentations might not be the wisest choice.
There are two possibilities of JPG masks:
JPG segmentation mask + int mask targets: more of a valid real use case when customer is willing to trade off specificity offered by PNG for size and speed of JPG. The pixel values are usually slightly altered along contrast lines because of banding, so they will be rendered as background in the app. The intensity of the artifacts depends on JPG compression levels.
JPG segmentation mask + RGB mask targets: while this will be technically supported, customers should expect major artifacts here. This is because JPG compression often involves some sort of color space conversion and it's impossible to map RGB mask targets to pixel values after color space conversion. These masks often might appear as mostly dark (background color).
How is this patch tested? If it is not, please explain why.
(Details)
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
Refactor