Use native segmentation parser and mask#304
Conversation
|
Warning Review limit reached
Next review available in: 32 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR replaces the local segmentation-mask path with ChangesNative SegmentationMask migration
CI and dependency version bump
YOLOExtendedParser stride configuration
SnapData documentation update
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
depthai_nodes/node/utils/message_remapping.py (1)
62-78: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winUse a uint8 border sentinel here
cv2.warpPerspectiveturnsborderValue=-1into0foruint8masks, so remapped border pixels are labeled as class 0 instead of the unassigned sentinel used elsewhere (255). SetborderValue=255or make it dtype-aware.🤖 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 `@depthai_nodes/node/utils/message_remapping.py` around lines 62 - 78, The remapping logic in remap_segmentation_mask_array uses a borderValue that does not preserve the unassigned class for uint8 masks, so border pixels get remapped to class 0 instead of the sentinel used elsewhere. Update the cv2.warpPerspective call to use a uint8-safe sentinel such as 255, or make the borderValue depend on the segmentation_mask dtype, while keeping the existing transformation and interpolation behavior unchanged.depthai_nodes/node/utils/util_constants.py (1)
12-23: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winChange
UNASSIGNED_MASK_LABELto255in mask remapping.depthai_nodes/node/utils/message_remapping.pystill uses this constant asborderValueforcv2.warpPerspective, and-1gets clipped to0foruint8masks, not255. That turns padded/unassigned pixels into class0instead of background.🤖 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 `@depthai_nodes/node/utils/util_constants.py` around lines 12 - 23, The mask remapping background sentinel is wrong: UNASSIGNED_MASK_LABEL should be set to 255 so padded pixels stay as background instead of being clipped to 0 for uint8 masks. Update the constant in util_constants and keep message_remapping’s cv2.warpPerspective borderValue usage aligned with that sentinel so unassigned mask areas remain correctly marked.
🤖 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.
Inline comments:
In @.github/workflows/e2e_tests.yaml:
- Around line 114-115: The `exec hil_runner` command in the workflow is
interpolating `github.ref_name` directly into a shell string, which can allow
template/shell injection on the runner. Move `github.ref_name` and the other
`${{ }}` values used in `--docker-run-args` into step-level `env:` variables,
then reference them as shell variables inside the `run:` command. Keep the fix
localized to this workflow step so the `exec hil_runner` invocation no longer
embeds attacker-influenced values directly.
In `@depthai_nodes/node/parsers/fastsam.py`:
- Around line 380-382: The empty-detection fallback in merge_masks handling is
using an all-255 mask, which gets treated as active pixels and overwrites the
output instead of preserving the background/unassigned sentinel. Update the
no-results branch in fastsam.py around the results_masks initialization so the
fallback is zero-filled before calling merge_masks(), keeping the inactive path
truly inactive.
In `@depthai_nodes/node/parsers/segmentation.py`:
- Around line 47-58: The SegmentationParser currently skips the
`background_class` remap whenever `classes_in_one_layer` is true, so class 0
remains unchanged in that combination. Update the parsing logic in
`SegmentationParser` so the background remap is still applied when
`background_class` is enabled, even if `classes_in_one_layer` is set, or
explicitly validate/document that this flag combination is unsupported. Use the
`SegmentationParser` initializer and the class-remapping branch in its parse
flow to locate the change.
In `@depthai_nodes/node/utils/message_remapping.py`:
- Around line 81-92: remap_segmentation_mask currently rebuilds a new
dai.SegmentationMask but only transfers the CV mask, so labels metadata is lost.
Update remap_segmentation_mask in message_remapping.py to preserve labels the
same way copy_message.py does: read labels from the input segmentation_mask when
available, and set them on new_mask after setCvMask, using the same guarded
getLabels/setLabels pattern as the existing message-copy logic.
---
Outside diff comments:
In `@depthai_nodes/node/utils/message_remapping.py`:
- Around line 62-78: The remapping logic in remap_segmentation_mask_array uses a
borderValue that does not preserve the unassigned class for uint8 masks, so
border pixels get remapped to class 0 instead of the sentinel used elsewhere.
Update the cv2.warpPerspective call to use a uint8-safe sentinel such as 255, or
make the borderValue depend on the segmentation_mask dtype, while keeping the
existing transformation and interpolation behavior unchanged.
In `@depthai_nodes/node/utils/util_constants.py`:
- Around line 12-23: The mask remapping background sentinel is wrong:
UNASSIGNED_MASK_LABEL should be set to 255 so padded pixels stay as background
instead of being clipped to 0 for uint8 masks. Update the constant in
util_constants and keep message_remapping’s cv2.warpPerspective borderValue
usage aligned with that sentinel so unassigned mask areas remain correctly
marked.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cb782501-6076-47ad-931e-5f93cb42ebc7
📒 Files selected for processing (23)
.github/workflows/e2e_tests.yaml.github/workflows/stability_tests.yamldepthai_nodes/message/README.mddepthai_nodes/message/__init__.pydepthai_nodes/message/creators/segmentation.pydepthai_nodes/message/segmentation.pydepthai_nodes/message/utils/copy_message.pydepthai_nodes/node/README.mddepthai_nodes/node/apply_colormap.pydepthai_nodes/node/parser_generator.pydepthai_nodes/node/parsers/fastsam.pydepthai_nodes/node/parsers/segmentation.pydepthai_nodes/node/parsers/utils/fastsam.pydepthai_nodes/node/parsers/yolo.pydepthai_nodes/node/utils/message_remapping.pydepthai_nodes/node/utils/util_constants.pyrequirements.txttests/stability_tests/check_messages.pytests/unittests/test_creators/test_segmentation.pytests/unittests/test_messages/test_copy_message.pytests/unittests/test_messages/test_segmentation_msg.pytests/unittests/test_nodes/test_host_nodes/test_apply_colormap_node.pytests/utils/messages/creators/arrays.py
💤 Files with no reviewable changes (4)
- depthai_nodes/message/init.py
- depthai_nodes/message/segmentation.py
- tests/unittests/test_messages/test_segmentation_msg.py
- depthai_nodes/message/README.md
klemen1999
left a comment
klemen1999
left a comment
There was a problem hiding this comment.
Few comments but overall looks good
|
TODO: Before merge we need next DepthAI version released. This means that we need to wait with merge and update the requirements.txt |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@tests/stability_tests/run_parser_test.py`:
- Around line 93-96: The stability test currently forces SegmentationParser to
use the host-side path, so it never validates the native
dai.node.SegmentationParser flow. Update the test around parser_generator.build
in run_parser_test.py to add a separate SegmentationParser case or parameterize
hostOnly so both host-only and native segmentation parsing are exercised, while
keeping the existing parser_name-based logic for the other parsers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ba14b24d-a601-46b7-846e-fc0328c0b379
📒 Files selected for processing (3)
depthai_nodes/node/parsers/fastsam.pytests/stability_tests/check_messages.pytests/stability_tests/run_parser_test.py
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/stability_tests/check_messages.py
- depthai_nodes/node/parsers/fastsam.py
|
Got it. Pushed the follow-up fixes and CI is green now. I fixed the stability failures from the native SegmentationMask migration, addressed the review comments around getLabels/setLabels, background_class handling, and setNNArchiveHead(head), and updated the parser mock to match the new ParserGenerator API usage. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #304 +/- ##
==========================================
- Coverage 48.69% 48.34% -0.35%
==========================================
Files 100 99 -1
Lines 6430 6410 -20
==========================================
- Hits 3131 3099 -32
- Misses 3299 3311 +12 ☔ View full report in Codecov by Harness. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In @.github/workflows/e2e_tests.yaml:
- Around line 114-115: Update both hil_runner command call sites around the
visible --docker-run-args invocations to avoid direct interpolation of
github.ref_name into shell command strings. Define the branch name through the
workflow step-level env configuration, then reference the quoted shell variable
in each command while preserving the existing BRANCH value behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d06aafb0-40c3-4834-bde1-f0fa3fe6acbc
📒 Files selected for processing (3)
.github/workflows/e2e_tests.yaml.github/workflows/stability_tests.yamlrequirements.txt
🚧 Files skipped from review as they are similar to previous changes (1)
- requirements.txt
3 participants
Purpose
Use native DepthAI segmentation components in
depthai-nodes.This PR updates the segmentation flow to use native
dai.SegmentationMaskand nativedai.node.SegmentationParserwhere applicable, while preserving the existing host-sidedepthai-nodesparser forhostOnly=True.It also aligns host parser background semantics with the native parser and fixes visualization of binary segmentation masks where class
0is a valid foreground class and255is background.Additionally, this PR adds the missing
setStrides(...)setter toYOLOExtendedParser.Specification
depthai_nodes.message.segmentation.SegmentationMaskmessage.copy_message(dai.SegmentationMask)support.ParserGeneratorto use nativedai.node.SegmentationParserwhenparser_name == "SegmentationParser"andhostOnly=False.depthai_nodes.node.parsers.segmentation.SegmentationParserforhostOnly=True.background_classsupport to the hostSegmentationParser.background_class=Truemaps class0to255.classes_in_one_layer=Trueignoresbackground_class.ApplyColormaphandling for nativedai.SegmentationMask:255background maps to visual background index0.0maps to a visible foreground index.YOLOExtendedParser.setStrides(...)for manually setting YOLO output head strides.Dependencies & Potential Impact
This depends on a DepthAI version exposing native
dai.SegmentationMaskanddai.node.SegmentationParser.Potential impact is limited to segmentation parsing, segmentation mask copying, segmentation visualization paths, and the new YOLO parser strides setter API. The existing host segmentation parser path remains available through
hostOnly=True.Direct Visualizer rendering of binary
dai.SegmentationMaskvalues[0, 255]may not visibly color class0; theApplyColormap -> ImgFrameOverlaypath handles this correctly.Deployment Plan
No special deployment steps required.
Rollback: revert this PR.
Testing & Validation
tests/unittests/test_messages/test_copy_message.pytests/unittests/test_nodes/test_host_nodes/test_apply_colormap_node.pySegmentationParser.run()directly with synthetic NN output.0 -> 255whenbackground_class=Trueclasses_in_one_layer=Trueignoresbackground_class[0, 255]masks.ApplyColormap -> ImgFrameOverlaycorrectly visualizes binary masks where foreground class id is0.YOLOExtendedParser.setStrides(...)locally:python -m compileallpassed for touched parser files.Note: For
luxonis/mediapipe-selfie-segmentation:256x144, a dual-parser MRE feeding the sameNeuralNetworkoutput to both parsers showed nativedai.node.SegmentationParseremitting only[0], while the hostdepthai-nodesparser emitted[0, 255]for the same sequence numbers and mask shape. Since both parsers consumed the same NN output, this appears isolated to the native parser/model metadata path rather than the host parser changes in this PR.AI Usage
Assisted-by: ChatGPT
Submitted code was reviewed by a human: YES
The author is taking the responsibility for the contribution: YES
Summary by CodeRabbit
dai.SegmentationMasksupport across parsing/remapping and ApplyColormap processing.background_classoption to segmentation outputs.setStridessupport for extended YOLO parsing.SnapData(replacing the previous segmentation-mask documentation).uint8, with consistent handling for empty and unassigned pixels.dai.SegmentationMask.3.8.0in workflows and dependency constraints.