Repository navigation
Use sensor API on multi device test - #14859
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the multi-device live streaming test to use the RealSense sensor API (open/start callbacks) instead of per-device rs.pipeline() streaming, aiming to reduce intermittent failures seen with the pipeline approach.
Changes:
- Replaced pipeline-based streaming/collection with
device.query_sensors()+sensor.open()/sensor.start(callback)and time-based collection. - Updated aggregation/logging to report total frame counts (rather than framesets).
- Removed now-unused helper functions for pipeline setup/stabilization/frame collection.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for stream_type, stream_index, w, h, fmt, fps in stream_configs: | ||
| for idx, sensor in enumerate(sensors): | ||
| for p in sensor.get_stream_profiles(): | ||
| if not p.is_video_stream_profile(): | ||
| continue | ||
| vp = p.as_video_stream_profile() | ||
| idx_match = stream_index < 0 or vp.stream_index() == stream_index | ||
| if (vp.stream_type() == stream_type and vp.format() == fmt | ||
| and vp.width() == w and vp.height() == h | ||
| and vp.fps() == fps and idx_match): | ||
| profiles_by_idx[idx].append(p) | ||
| break | ||
| else: | ||
| continue | ||
| break | ||
|
|
There was a problem hiding this comment.
The stream profile selection silently skips any (stream_type, stream_index, w, h, fmt, fps) entry that isn’t found on a device (no error is raised/logged). This can make the test pass while actually streaming fewer streams than requested, and it can also mask a stream_index mismatch. Consider validating that every requested stream_config is matched exactly once per device and failing the test (or at least logging a fatal error) when a requested profile can’t be located.
There was a problem hiding this comment.
Fixed — added log.f() if no matching profile is found for a requested stream config.
| sensors[idx].open(profiles) | ||
| sensors[idx].start(cb) | ||
| active_sensors.append(sensors[idx]) |
There was a problem hiding this comment.
If sensor.open(profiles) succeeds but sensor.start(cb) raises, the sensor won’t be added to active_sensors and therefore won’t be stopped/closed in the finally block. This can leak an opened sensor and affect subsequent tests/devices. Track sensors as soon as they’re opened (or use a try/except around start to close on failure).
| sensors[idx].open(profiles) | |
| sensors[idx].start(cb) | |
| active_sensors.append(sensors[idx]) | |
| sensor = sensors[idx] | |
| sensor.open(profiles) | |
| try: | |
| sensor.start(cb) | |
| except Exception: | |
| # Ensure we don't leak an opened sensor if start fails | |
| sensor.close() | |
| raise | |
| active_sensors.append(sensor) |
There was a problem hiding this comment.
Fixed — sensor is appended to active_sensors right after open(), before start(). If start() fails, the finally block will still close() it.
| log.i(f"Stabilizing for {STABILIZATION_TIME_SEC} seconds...") | ||
| time.sleep(STABILIZATION_TIME_SEC) | ||
|
|
||
| # Reset counters so stabilization frames don't count | ||
| for ctr, cnt in zip(all_counters, all_counts): | ||
| ctr.clear() | ||
| cnt.clear() | ||
|
|
There was a problem hiding this comment.
Counters are cleared while streaming callbacks are still running (callbacks likely execute on a background thread). Mutating shared dict/list state concurrently can lead to lost counts or inconsistent counter sequences, increasing flakiness. Consider gating counts with a thread-safe flag/epoch, or protecting counter updates + reset with a lock, or restarting the sensors after stabilization instead of clearing shared structures mid-stream.
There was a problem hiding this comment.
Fixed — replaced clear() with a counting bool flag. Callbacks skip frames when counting is False (during stabilization). Flag is set to True only when we're ready to collect.
| log.i(f"Streaming for {duration_sec} seconds...") | ||
| time.sleep(duration_sec) | ||
| finally: | ||
| for pipe in pipes: | ||
| for s in active_sensors: | ||
| try: | ||
| pipe.stop() | ||
| s.stop() | ||
| s.close() | ||
| except Exception as e: | ||
| log.d(f"Failed to stop pipeline during cleanup: {e}") | ||
| log.d(f"Cleanup error: {e}") | ||
|
|
||
| # Feed into existing analysis | ||
| all_framesets = [sum(cnt.values()) for cnt in all_counts] | ||
| return aggregate_results(all_counters, all_framesets, all_counts, device_info, duration_sec) |
There was a problem hiding this comment.
The code reports/uses actual_duration but currently passes duration_sec into aggregate_results without measuring the real streaming duration (setup + stabilization overhead and scheduling can skew it). If duration is used for reporting/thresholds, measure it with time.time() around the streaming window and pass the measured duration instead of the requested duration.
There was a problem hiding this comment.
Fixed — now measuring actual duration with time.time() around the streaming sleep and passing actual_duration to aggregate_results.
| for s in active_sensors: | ||
| try: | ||
| pipe.stop() | ||
| s.stop() | ||
| s.close() | ||
| except Exception as e: |
There was a problem hiding this comment.
Cleanup calls s.stop() and s.close() in the same try block; if stop() raises, close() will be skipped and the sensor may remain open. Consider ensuring close() is attempted even when stop() fails (e.g., separate try blocks or a finally).
There was a problem hiding this comment.
Fixed — stop() and close() now have separate try blocks so close() always runs even if stop() fails.
|
|
||
| pytestmark = [ | ||
| pytest.mark.device("D400*"), | ||
| pytest.mark.device_exclude("D401*"), |
There was a problem hiding this comment.
Please remove the * if possible
| pipes, cfgs, device_info = setup_pipelines(devs, stream_configs) | ||
|
|
||
| device_info = [] | ||
| all_counters = [] |
There was a problem hiding this comment.
please assign more explicit names to all_counters and all_counts.
There was a problem hiding this comment.
renamed, let me know if you have better names in mind
Managed to get passing runs with this change. Failures were seen sometimes but passed on retry