Skip to content

Bugfix: avoid querying device after depthNode pipeline stops - #1961

Open
MaticTonin wants to merge 2 commits into
developfrom
bugfix/depthNode-autostop
Open

Bugfix: avoid querying device after depthNode pipeline stops#1961
MaticTonin wants to merge 2 commits into
developfrom
bugfix/depthNode-autostop

Conversation

@MaticTonin

@MaticTonin MaticTonin commented Aug 25, 2026

Copy link
Copy Markdown
Collaborator

Summary
Fixes a segfault in the Depth AUTO backend on-device tests on RVC2/network devices.
The test previously queried getConnectedCameraFeatures() after startPipelineAndRequireFirstFrames() returned. That helper stops the pipeline before returning, which closes the device RPC client. The subsequent capability query dereferenced the closed client and caused a SIGSEGV.
Capture the platform and ToF capability while the device is live, then use those cached values for post-stop backend assertions.

Tested with:

cmake --build build --target depth_node_test -j 4
DEPTHAI_PLATFORM=rvc2 ./build/tests/depth_node_test \
  'Depth: AUTO selects backend by platform and sensors' --reporter compact

https://github.com/luxonis/depthai-core/actions/runs/32837049490/job/97770930315#step:3:4404

Summary by CodeRabbit

  • Tests
    • Improved validation of automatic depth-camera backend selection across standard, user-camera, and camera-reuse scenarios.
    • Test checks now use the active platform and precomputed time-of-flight selection state for more consistent results.
    • Expanded coverage helps ensure reliable depth processing across supported camera configurations and device platforms.

@coderabbitai

coderabbitai Bot commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: b203232e-9d37-4041-bc3b-31421f964289

📥 Commits

Reviewing files that changed from the base of the PR and between 9837c7d and b563988.

📒 Files selected for processing (1)
  • tests/src/ondevice_tests/pipeline/node/depth_node_test.cpp

Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review.

📜 Recent review details
🔇 Additional comments (1)
tests/src/ondevice_tests/pipeline/node/depth_node_test.cpp (1)

140-163: LGTM!

Also applies to: 379-380, 414-414, 444-452, 510-512, 546-546, 559-559, 639-639, 652-652


📝 Walkthrough

Walkthrough

Depth AUTO backend tests now receive the platform and precomputed RVC2 ToF-sensor state. User-camera, AUTO-selection, rebuild, and camera-reuse test paths pass these values to backend validation.

Changes

Depth AUTO validation

Layer / File(s) Summary
Update AUTO backend validation contract
tests/src/ondevice_tests/pipeline/node/depth_node_test.cpp
requireDepthAutoBackend now accepts explicit platform and ToF-sensor inputs.
Pass precomputed selection state
tests/src/ondevice_tests/pipeline/node/depth_node_test.cpp
User-camera, AUTO-selection, rebuild, and camera-reuse tests compute and pass platform and ToF-sensor state.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to b5639

The test now captures device capabilities while the device is live, avoiding the post-stop query that caused the crash; no actionable merge-blocking risk remains after normal checks and review.

Suggested reviewers: cenekalbl, viacheslavtelelyukhin

Poem

A rabbit checks the depth path bright
AUTO hops with state in sight
ToF choices arrive prepared
Each camera test is now compared
Ears up, the backend matches right

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 1 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly describes the main change: it prevents device queries after the depthNode pipeline stops. It is concise and specific.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch bugfix/depthNode-autostop

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@MaticTonin MaticTonin changed the title fix(tests): avoid querying device after depth pipeline stops Bugfix: avoid querying device after depthNode pipeline stops Aug 25, 2026
bool userPreviewStream) {
PipelineStopGuard guard(pipeline);
const auto platform = device->getPlatform();
const auto autoUsesTof = platform == Platform::RVC2 && deviceReportsTofSensor(device);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why we require RVC2 platform? You check the platform later.
I suggest rename it to hasTofSensor and just place there deviceReportsTofSensor(device).

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed and remived the RVC2 platform check yea, since its duplicated

@MaticTonin MaticTonin added the testable Trigger PR testing, latest python build, and Core CI/CD label Aug 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

testable Trigger PR testing, latest python build, and Core CI/CD

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants