Fix timed out Camera FPS config test#1845
Conversation
📝 WalkthroughWalkthroughCamera pool size test coverage is split into two on-device tests with updated CMake registration, and reconnect monitoring now changes how crash-dump handled state is reset after success. ChangesCamera pool sizes test coverage
Device reconnection crash-dump handling
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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: 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/src/ondevice_tests/pipeline/node/camera_fps_config_test.cpp`:
- Line 184: The condition using a magic number (200) in the check
if(outQueuesCounter[idx] <= queueSize + 200) should be documented or replaced
with a named constant; either add a short inline comment explaining that 200
represents ~6.67 seconds at 30fps as the recovery verification window, or
refactor to a descriptive constant like kRecoveryFrameThreshold (used as
queueSize + kRecoveryFrameThreshold) so the intent of outQueuesCounter, idx and
queueSize is clear.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2c76d27f-84eb-4804-9695-c0e39b43ed89
📒 Files selected for processing (1)
tests/src/ondevice_tests/pipeline/node/camera_fps_config_test.cpp
📜 Review details
🔇 Additional comments (1)
tests/src/ondevice_tests/pipeline/node/camera_fps_config_test.cpp (1)
177-191: LGTM!
| if(outQueuesCounter[idx] <= queueSize + 200) { | ||
| gotEnoughFrames = false; | ||
| } | ||
| } | ||
| if(gotEnoughFrames) { | ||
| break; | ||
| } |
There was a problem hiding this comment.
I think this might defeat the purpuse of the test CC: @jakgra
I would rather reduce the time to block for 30s if total runtime is a problem
There was a problem hiding this comment.
The current test implementation asserts only that 200 frames were successfully sent, not asserting sustained streaming for the full 30s window (while those 30s of streaming do happen). The early break preserves that assertion and removes dead wait on the success path, not influencing the test checks in any way.
I would rather reduce the time to block for 30s if total runtime is a problem
Can go with this, but would have to update the current assertion as well to actually achieve the mentioned purpose of the test.
aljazkonec1
left a comment
aljazkonec1
left a comment
There was a problem hiding this comment.
w.r.t. the 200 frame limit: the code does not reflect the spirit of the test. Instead of validating running for 30 seconds, it just checks for >200 recieved frames so I agree with what @aljazdu proposed to adapt.
IMO the entire test would need to be rewritten but lets make that a separate PR just because of higher prio items.
Before merge, just test out splitting the test into two files, one with the first TEST_CASE and half of the queue sizes form second TEST_CASE and another test file with the other half of the queue sizes. This should stop the timeout from hitting without the 200 frame limit.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/src/ondevice_tests/pipeline/node/camera_fps_config_test_part_2.cpp`:
- Around line 18-129: The Camera pool sizes test logic is duplicated here and
has already drifted from the original test body, so consolidate the shared
behavior into one helper used by both `camera_fps_config_test_part_2.cpp` and
`camera_fps_config_test.cpp`. Refactor the main body around `TEST_CASE("Camera
pool sizes")` into a reusable function or header-level helper that accepts the
override queue list and any target-specific parameters, then have each test file
call that shared helper so future changes stay in sync.
- Around line 45-56: The queue-size fallback logic in this companion camera FPS
test is unreachable because the local override list never uses the sentinel
value that would select the default path. Simplify the test by removing the dead
`autoCalibrationOnStartup`/`defaultQueueSize` computation and the
`overrideQueueSize != -1` branch in the `Camera` setup, and keep only the direct
`overrideQueueSize` assignment used by this split test.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4fcd5cc5-84bf-4c62-af46-654a5fb32fd2
📒 Files selected for processing (4)
src/device/DeviceBase.cpptests/CMakeLists.txttests/src/ondevice_tests/pipeline/node/camera_fps_config_test.cpptests/src/ondevice_tests/pipeline/node/camera_fps_config_test_part_2.cpp
📜 Review details
🧰 Additional context used
🪛 Cppcheck (2.21.0)
tests/src/ondevice_tests/pipeline/node/camera_fps_config_test_part_2.cpp
[error] 33-33: There is an unknown macro here somewhere. Configuration is required. If DEPTHAI_NLOHMANN_DEFINE_TYPE_INTRUSIVE is a macro then please configure it.
(unknownMacro)
🔇 Additional comments (3)
tests/src/ondevice_tests/pipeline/node/camera_fps_config_test.cpp (1)
81-81: LGTM!tests/CMakeLists.txt (1)
849-851: 📐 Maintainability & Code QualityNo label drift.
camera_fps_config_part_2_testuses the same labels andDEPTHAI_AUTOCALIBRATION=OFFenvironment ascamera_fps_config_test.src/device/DeviceBase.cpp (1)
1529-1530: 🎯 Functional CorrectnessKeep the buffered crash flag aligned with pending dump state
hasCrashed()tracks the current crash-dump state, so clearingcrashedhere when no dump is pending avoids leaving a stale crash indicator after recovery.> Likely an incorrect or invalid review comment.
| TEST_CASE("Camera pool sizes") { | ||
| auto firstDevice = dai::Device::getFirstAvailableDevice(); | ||
| auto isRvc4 = std::get<1>(firstDevice).platform == X_LINK_RVC4; | ||
| for(const int overrideQueueSize : (isRvc4 ? std::vector<int>{50, 4, 5} : std::vector<int>{3, 4, 5})) { | ||
| std::cout << "Testing num frames = " << overrideQueueSize << "\n" << std::flush; | ||
| dai::Pipeline pipeline; | ||
| std::map<dai::CameraBoardSocket, std::vector<std::tuple<int, int, float>>> streamsRvc4{ | ||
| // Has to be (for now): | ||
| // - without FpsRegulator (different fps per same sensor)(different fps on different sensors also doesn't work right now) | ||
| // - without ManipResizer so size should be supported by ISP directly | ||
| {dai::CameraBoardSocket::CAM_A, {{640, 480, 30.0f}, {1920, 1440, 30.0f}}}, | ||
| {dai::CameraBoardSocket::CAM_B, {{640, 400, 30.0f}, {1280, 800, 30.0f}}}, | ||
| {dai::CameraBoardSocket::CAM_C, {{640, 400, 30.0f}, {1280, 800, 30.0f}}}, | ||
| }; | ||
| // RVC2 is more RAM bound so use smaller sizes for the test | ||
| std::map<dai::CameraBoardSocket, std::vector<std::tuple<int, int, float>>> streamsRvc2{ | ||
| // Has to be (for now): | ||
| // - not a size supported directly by ISP as then isp is passed trough and the isp pool size value is used not the outputs pool size | ||
| {dai::CameraBoardSocket::CAM_A, {{300, 300, 30.0f}}}, | ||
| {dai::CameraBoardSocket::CAM_B, {{300, 300, 30.0f}, {200, 200, 30.0f}}}, | ||
| {dai::CameraBoardSocket::CAM_C, {{200, 200, 30.0f}}}, | ||
| }; | ||
| auto streams = isRvc4 ? streamsRvc4 : streamsRvc2; | ||
| std::vector<std::shared_ptr<dai::MessageQueue>> outQueues; | ||
| std::vector<int> outQueuesCounter; | ||
| std::vector<std::shared_ptr<dai::node::Camera>> cameras; | ||
| auto script = pipeline.create<dai::node::Script>(); | ||
| // If startup AutoCalibration is enabled globally, default pool count is increased by 3. | ||
| const char* autoCalibrationEnv = std::getenv("DEPTHAI_AUTOCALIBRATION"); | ||
| const bool autoCalibrationOnStartup = | ||
| autoCalibrationEnv != nullptr && (std::string(autoCalibrationEnv) == "ON_START" || std::string(autoCalibrationEnv) == "CONTINUOUS"); | ||
| const int defaultQueueSizeBase = isRvc4 ? 7 : 3; | ||
| const int defaultQueueSize = defaultQueueSizeBase + (autoCalibrationOnStartup ? 3 : 0); | ||
| int queueSize = overrideQueueSize == -1 ? defaultQueueSize : overrideQueueSize; | ||
| for(const auto& [socket, resolutions] : streams) { | ||
| auto camera = pipeline.create<dai::node::Camera>()->build(socket); | ||
| camera->properties.maxSizePoolOutputs = 1 * 1024 * 1024 * 1024; // 1G size limit to only test num frames limitation | ||
| if(overrideQueueSize != -1) { | ||
| camera->properties.numFramesPoolOutputs = overrideQueueSize; | ||
| } | ||
| for(const auto& resolution : resolutions) { | ||
| std::string theKey = std::to_string(outQueues.size()); | ||
| std::string inputName = "in" + theKey; | ||
| std::string outputName = "out" + theKey; | ||
| camera->requestOutput({std::get<0>(resolution), std::get<1>(resolution)}, std::nullopt, dai::ImgResizeMode::CROP, std::get<2>(resolution)) | ||
| ->link(script->inputs[inputName]); | ||
| script->inputs[inputName].setBlocking(false); | ||
| script->inputs[inputName].setMaxSize(1000); | ||
| outQueues.push_back(script->outputs[outputName].createOutputQueue()); | ||
| outQueuesCounter.push_back(0); | ||
| } | ||
| cameras.push_back(camera); | ||
| } | ||
| int timeToBlock = 20; | ||
| std::string scriptContent = isRvc4 ? R"( | ||
| from depthai import BenchmarkReport)" | ||
| : ""; | ||
| scriptContent += R"( | ||
| import time | ||
|
|
||
| all_frames=[] | ||
| max_id = )" + std::to_string(outQueues.size() - 1) | ||
| + R"( | ||
| start_time = time.time() | ||
| while time.time() - start_time < )" | ||
| + std::to_string(timeToBlock) + R"(: | ||
| for idx in range(max_id + 1): | ||
| the_key = str(idx) | ||
| frame = node.inputs["in" + the_key].tryGet() | ||
| if frame is not None: | ||
| all_frames.append(frame) | ||
| out = BenchmarkReport() | ||
| node.outputs["out" + the_key].send(out) | ||
| all_frames = [] | ||
| while True: | ||
| for idx in range(max_id + 1): | ||
| the_key = str(idx) | ||
| frame = node.inputs["in" + the_key].tryGet() | ||
| if frame is not None: | ||
| out = BenchmarkReport() | ||
| node.outputs["out" + the_key].send(out) | ||
| )"; | ||
| script->setScript(scriptContent); | ||
| pipeline.start(); | ||
| auto startTime = std::chrono::steady_clock::now(); | ||
| // Keep frames in script node and check Camera node stops sending frames after buffer limit is hit | ||
| while(std::chrono::duration<double>(std::chrono::steady_clock::now() - startTime).count() < timeToBlock - 5) { | ||
| for(int idx = 0; idx < outQueues.size(); ++idx) { | ||
| auto frame = outQueues[idx]->tryGet(); | ||
| if(frame) { | ||
| ++outQueuesCounter[idx]; | ||
| } | ||
| } | ||
| } | ||
| std::cout << "Got the first part frames\n" << std::flush; | ||
| for(const auto& count : outQueuesCounter) { | ||
| REQUIRE(count == queueSize); | ||
| } | ||
| // Check stream still works after script node unblocks the Camera node | ||
| while(std::chrono::duration<double>(std::chrono::steady_clock::now() - startTime).count() < timeToBlock + 30) { | ||
| for(int idx = 0; idx < outQueues.size(); ++idx) { | ||
| auto frame = outQueues[idx]->tryGet(); | ||
| if(frame) { | ||
| ++outQueuesCounter[idx]; | ||
| } | ||
| } | ||
| } | ||
| for(const auto& count : outQueuesCounter) { | ||
| REQUIRE(count > queueSize + 200); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy lift
Near-total duplication with the original Camera pool sizes test.
This file is a copy of the Camera pool sizes body in camera_fps_config_test.cpp with only the override-list parameters differing. They are already diverging (the original dropped the early-break/gotEnoughFrames logic while this copy retains the queueSize + 200 tail check), which is exactly the drift duplication invites. Consider extracting the shared body into a common helper (e.g. a header taking the override-list as an argument) so both targets exercise identical logic.
🧰 Tools
🪛 Cppcheck (2.21.0)
[error] 33-33: There is an unknown macro here somewhere. Configuration is required. If DEPTHAI_NLOHMANN_DEFINE_TYPE_INTRUSIVE is a macro then please configure it.
(unknownMacro)
🤖 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 `@tests/src/ondevice_tests/pipeline/node/camera_fps_config_test_part_2.cpp`
around lines 18 - 129, The Camera pool sizes test logic is duplicated here and
has already drifted from the original test body, so consolidate the shared
behavior into one helper used by both `camera_fps_config_test_part_2.cpp` and
`camera_fps_config_test.cpp`. Refactor the main body around `TEST_CASE("Camera
pool sizes")` into a reusable function or header-level helper that accepts the
override queue list and any target-specific parameters, then have each test file
call that shared helper so future changes stay in sync.
| // If startup AutoCalibration is enabled globally, default pool count is increased by 3. | ||
| const char* autoCalibrationEnv = std::getenv("DEPTHAI_AUTOCALIBRATION"); | ||
| const bool autoCalibrationOnStartup = | ||
| autoCalibrationEnv != nullptr && (std::string(autoCalibrationEnv) == "ON_START" || std::string(autoCalibrationEnv) == "CONTINUOUS"); | ||
| const int defaultQueueSizeBase = isRvc4 ? 7 : 3; | ||
| const int defaultQueueSize = defaultQueueSizeBase + (autoCalibrationOnStartup ? 3 : 0); | ||
| int queueSize = overrideQueueSize == -1 ? defaultQueueSize : overrideQueueSize; | ||
| for(const auto& [socket, resolutions] : streams) { | ||
| auto camera = pipeline.create<dai::node::Camera>()->build(socket); | ||
| camera->properties.maxSizePoolOutputs = 1 * 1024 * 1024 * 1024; // 1G size limit to only test num frames limitation | ||
| if(overrideQueueSize != -1) { | ||
| camera->properties.numFramesPoolOutputs = overrideQueueSize; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Dead default-queue-size / autocalibration logic in this split.
In this companion test the override list (line 21) never contains -1, so queueSize always equals overrideQueueSize and the defaultQueueSize/autoCalibrationOnStartup computation plus the overrideQueueSize != -1 guard are unreachable. This was carried over verbatim from the original test where -1 exercised the default path. Consider dropping it here to avoid confusion (the CMake DEPTHAI_AUTOCALIBRATION=OFF already makes the env read moot).
♻️ Proposed simplification
- auto script = pipeline.create<dai::node::Script>();
- // If startup AutoCalibration is enabled globally, default pool count is increased by 3.
- const char* autoCalibrationEnv = std::getenv("DEPTHAI_AUTOCALIBRATION");
- const bool autoCalibrationOnStartup =
- autoCalibrationEnv != nullptr && (std::string(autoCalibrationEnv) == "ON_START" || std::string(autoCalibrationEnv) == "CONTINUOUS");
- const int defaultQueueSizeBase = isRvc4 ? 7 : 3;
- const int defaultQueueSize = defaultQueueSizeBase + (autoCalibrationOnStartup ? 3 : 0);
- int queueSize = overrideQueueSize == -1 ? defaultQueueSize : overrideQueueSize;
+ auto script = pipeline.create<dai::node::Script>();
+ int queueSize = overrideQueueSize;
for(const auto& [socket, resolutions] : streams) {
auto camera = pipeline.create<dai::node::Camera>()->build(socket);
camera->properties.maxSizePoolOutputs = 1 * 1024 * 1024 * 1024; // 1G size limit to only test num frames limitation
- if(overrideQueueSize != -1) {
- camera->properties.numFramesPoolOutputs = overrideQueueSize;
- }
+ camera->properties.numFramesPoolOutputs = overrideQueueSize;📝 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.
| // If startup AutoCalibration is enabled globally, default pool count is increased by 3. | |
| const char* autoCalibrationEnv = std::getenv("DEPTHAI_AUTOCALIBRATION"); | |
| const bool autoCalibrationOnStartup = | |
| autoCalibrationEnv != nullptr && (std::string(autoCalibrationEnv) == "ON_START" || std::string(autoCalibrationEnv) == "CONTINUOUS"); | |
| const int defaultQueueSizeBase = isRvc4 ? 7 : 3; | |
| const int defaultQueueSize = defaultQueueSizeBase + (autoCalibrationOnStartup ? 3 : 0); | |
| int queueSize = overrideQueueSize == -1 ? defaultQueueSize : overrideQueueSize; | |
| for(const auto& [socket, resolutions] : streams) { | |
| auto camera = pipeline.create<dai::node::Camera>()->build(socket); | |
| camera->properties.maxSizePoolOutputs = 1 * 1024 * 1024 * 1024; // 1G size limit to only test num frames limitation | |
| if(overrideQueueSize != -1) { | |
| camera->properties.numFramesPoolOutputs = overrideQueueSize; | |
| int queueSize = overrideQueueSize; | |
| for(const auto& [socket, resolutions] : streams) { | |
| auto camera = pipeline.create<dai::node::Camera>()->build(socket); | |
| camera->properties.maxSizePoolOutputs = 1 * 1024 * 1024 * 1024; // 1G size limit to only test num frames limitation | |
| camera->properties.numFramesPoolOutputs = overrideQueueSize; |
🤖 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 `@tests/src/ondevice_tests/pipeline/node/camera_fps_config_test_part_2.cpp`
around lines 45 - 56, The queue-size fallback logic in this companion camera FPS
test is unreachable because the local override list never uses the sentinel
value that would select the default path. Simplify the test by removing the dead
`autoCalibrationOnStartup`/`defaultQueueSize` computation and the
`overrideQueueSize != -1` branch in the `Camera` setup, and keep only the direct
`overrideQueueSize` assignment used by this split test.
|
Can merge once POE and Crashdump tests pass |
3 participants
Purpose
Camera FPS config test often timed out for RVC2 POE devices. This fix adds early stopping when enough frames are collected.
Specification
None / not applicable
Dependencies & Potential Impact
None / not applicable
Deployment Plan
None / not applicable
Testing & Validation
None / not applicable
AI Usage
Assisted-by: /
Submitted code was reviewed by a human: YES
The author is taking the responsibility for the contribution: YES
Summary by CodeRabbit
camera_fps_config_part_2_test) to verify camera output queue/frame pool limits across platforms.DEPTHAI_AUTOCALIBRATION=OFF.