[Binyaminp]

Summary

• Fix rs-align-advanced crash with "No stream found to align with Depth" on GMSL devices whose default pipeline only activates the depth stream
• Fix motion stream (ACCEL/GYRO) being incorrectly selected as alignment target on devices with IMU, causing a null pointer crash inside the align processing block

Root Causes

Motion stream selection (find_stream_to_align):
The function iterated all non-depth streams without filtering out motion streams. On devices with an IMU (e.g. D430 USB), ACCEL/GYRO streams were selected as the alignment target. Passing a motion frame to the align processing block caused a null pointer error when it tried to retrieve a video stream profile from a motion profile.

GMSL depth-only default profile:
On D430 GMSL, pipe.start() with default settings only activates the depth stream. find_stream_to_align found no valid target and threw immediately, preventing the tool from opening.

Changes

• find_stream_to_align: added sp.is<rs2::video_stream_profile>() check to exclude motion streams from alignment target selection
• main: added fallback when no non-depth video stream is found in the default profile — stops the pipeline and restarts with depth + IR explicitly enabled

[sysrsbuild-gh-agentic]

Automated review by rs-agentic bot.

The two fixes are clearly targeted and address real crash scenarios on GMSL and IMU-equipped devices. A few observations inline below. No test coverage exists for the new fallback path — see comments.

[sysrsbuild-gh-agentic]

[Enhancement] — auto-generated by rs-agentic bot

align_to is declared without an initializer, leaving it with an indeterminate value (rs2_stream is an enum/int). While all code paths that reach the subsequent rs2::align align(align_to); line must have assigned it (throw or assign), best practice is to initialize it explicitly:

rs2_stream align_to = RS2_STREAM_ANY;

This eliminates any theoretical UB if the control flow is ever changed.

[sysrsbuild-gh-agentic]

change approved — align_to is now correctly initialized to RS2_STREAM_ANY in this commit. The explicit initialization eliminates any theoretical UB and also makes the downstream guard (if (align_to == RS2_STREAM_ANY) throw ...) semantically clearer. ✅

auto-generated by rs-agentic bot

[sysrsbuild-gh-agentic]

[BUG] — auto-generated by rs-agentic bot

The catch block is overly broad. find_stream_to_align throws two distinct std::runtime_error messages:

1. "No Depth stream available" — the device has no depth sensor at all.
2. "No stream found to align with Depth" — depth is present but no suitable target stream was found.

This fallback should only trigger for case (2). Currently, if a device genuinely lacks a depth sensor, the code will swallow that error, call pipe.stop(), restart with RS2_STREAM_DEPTH forced on, and then throw again from the second find_stream_to_align call — giving a misleading error path.

Consider checking the exception message, or (better) add a separate helper that inspects stream profiles before throwing, so the caller can branch without relying on exception text matching.

[sysrsbuild-gh-agentic]

change approved — the overly-broad catch block concern is now moot. The new approach proactively inspects the profile via has_non_depth_video() before ever calling find_stream_to_align, so no exception-driven control flow is needed for the GMSL fallback path. This is the correct design: the function now only throws on genuine error conditions (truly no depth sensor, or a profile with depth but no alignable video stream), not as a normal flow signal. ✅

auto-generated by rs-agentic bot

[sysrsbuild-gh-agentic]

[Enhancement] — auto-generated by rs-agentic bot

Stream index 1 is hardcoded here:

cfg.enable_stream(RS2_STREAM_INFRARED, 1);

This works for D4xx single-IMU configurations but may silently fail on devices with different IR sensor indexing. Consider adding a comment explaining why index 1 is the correct fallback, or querying the device's available streams to pick the first available IR index dynamically.

Also, depth_scale is computed from profile.get_device() before the restart. After pipe.start(cfg) the device is the same so the scale is unchanged — but this implicit assumption is not obvious to future readers. A brief comment would help.

[sysrsbuild-gh-agentic]

change approved — The hardcoded cfg.enable_stream(RS2_STREAM_INFRARED, 1) concern is fully resolved in this commit. The new configure_alignable_streams() function queries the device's sensors and stream profiles dynamically, enabling the first available non-depth video stream by type+index. No stream index is hardcoded. ✅

auto-generated by rs-agentic bot

[sysrsbuild-gh-agentic]

[Enhancement] — auto-generated by rs-agentic bot

The sp.is<rs2::video_stream_profile>() guard is the right fix and is minimal. One additional edge case to be aware of: RS2_STREAM_POSE (T2xx tracking devices) produces a motion_stream_profile, not a video_stream_profile, so it is also correctly excluded by this check. If in the future a pose/fisheye combo device is supported, the fisheye stream will have a video_stream_profile and will be picked up correctly — no further changes needed.

[sysrsbuild-gh-agentic]

[BUG] — auto-generated by rs-agentic bot

Reviewing commit 9c61f64 ("fix comment"): the align_to initialization and the try/catch replacement are both approved (see thread replies above). One new issue found in the hot-reconnect path that was not part of the original commits:

The has_non_depth_video pre-check was correctly added before the initial pipe.start(), but the same guard is missing inside the profile_changed handler in the main frame loop:

// ~line 82 in rs-align-advanced.cpp
if (profile_changed(pipe.get_active_profile().get_streams(), profile.get_streams()))
{
    profile = pipe.get_active_profile();
    align_to = find_stream_to_align(profile.get_streams()); // ← no has_non_depth_video check here
    align = rs2::align(align_to);
    depth_scale = get_depth_scale(profile.get_device());
}

If a GMSL device is disconnected and reconnected (hot-plug), pipe.wait_for_frames() may resume with a depth-only default profile. find_stream_to_align will then throw "No stream found to align with Depth" — propagating uncaught through the frame loop, hitting the top-level catch (const std::exception&), and terminating the application. This is the exact crash scenario this PR was meant to fix at startup, but it can still happen at runtime after a reconnect.

Suggested fix — extract the fallback logic into a shared helper and call it from both sites:

auto ensure_non_depth_video = [&]() {
    if (!has_non_depth_video(profile.get_streams()))
    {
        pipe.stop();
        rs2::config cfg;
        cfg.enable_stream(RS2_STREAM_DEPTH);
        cfg.enable_stream(RS2_STREAM_INFRARED, 1);
        profile = pipe.start(cfg);
    }
};

// After initial pipe.start():
ensure_non_depth_video();

// Inside the profile_changed branch:
if (profile_changed(...))
{
    profile = pipe.get_active_profile();
    ensure_non_depth_video();
    align_to = find_stream_to_align(profile.get_streams());
    align = rs2::align(align_to);
    depth_scale = get_depth_scale(profile.get_device());
}

This ensures consistent behaviour on both initial startup and hot-reconnect.

[ashrafk93]

change approved — The ensure_non_depth_video lambda is now correctly called at both the initial startup site and inside the profile_changed handler in the frame loop. This directly addresses the hot-reconnect crash scenario described in the previous review. The implementation captures pipe and profile by reference so the lambda correctly updates the shared profile variable when it restarts the pipeline. ✅

auto-generated by rs-agentic bot

[ashrafk93]

[Enhancement] — auto-generated by rs-agentic bot

subprocess.run(['udevadm', 'settle'], timeout=10) is not guarded against two failure modes:

1. FileNotFoundError — if udevadm is not present on the test host (non-systemd or container environments), the call will throw and propagate an unexpected exception that masks the original RuntimeError.
2. subprocess.TimeoutExpired — if udevadm takes longer than 10 seconds the exception will also propagate uncaught.

Consider wrapping in a try/except or using check=False with a log warning:

try:
    subprocess.run(['udevadm', 'settle'], timeout=10, check=False)
except (FileNotFoundError, subprocess.TimeoutExpired):
    log.warning("udevadm settle failed or not available; continuing")

[ashrafk93]

[Enhancement] — auto-generated by rs-agentic bot

The 0xff/0xff unavailability check and the signed whole_number_part conversion are both correct. One minor note: the docstring comment added above (lines 51-53) describes the byte ordering as decimal_part at even indices and whole_number_part at odd indices. It would be worth confirming (with a brief inline comment here) that the 0xff 0xff sentinel is checked after both bytes are fully read, which is the case — the check happens in the else branch (odd index), ensuring both bytes are known before the comparison. This is already correct; just consider adding a one-line comment # both bytes known at this point for future readers.

[ashrafk93]

change approved — The ensure_non_depth_video lambda is now correctly called at both the initial startup site (line 63) and inside the profile_changed handler (line 86), directly addressing the hot-reconnect crash scenario flagged in the previous review. The lambda captures pipe and profile by reference and correctly updates the shared profile variable when it restarts the pipeline. ✅

---

One new issue found in this commit — see inline comment below.

auto-generated by rs-agentic bot

[ashrafk93]

[BUG] — auto-generated by rs-agentic bot

The sp.is<rs2::video_stream_profile>() guard is correct for the align_to selection path, but it introduces a regression in the else branch below:

if (profile_stream != RS2_STREAM_DEPTH && sp.is<rs2::video_stream_profile>())
{
    // align_to selection ...
}
else
{
    depth_stream_found = true;  // ← now also triggered by motion streams
}

Before this PR the else exclusively caught RS2_STREAM_DEPTH streams. After this change, the else is also triggered by any stream that is not depth AND not video (i.e. ACCEL/GYRO motion streams). On a D430 USB with IMU, ACCEL/GYRO frames will set depth_stream_found = true regardless of whether a real depth stream is present.

In practice this is masked because depth is always present alongside IMU, but it produces the wrong error message on a theoretical depth-less device: it would throw "No stream found to align with Depth" instead of "No Depth stream available".

Suggested fix — decouple the two checks:

for (rs2::stream_profile sp : streams)
{
    rs2_stream profile_stream = sp.stream_type();
    if (profile_stream == RS2_STREAM_DEPTH)
    {
        depth_stream_found = true;
    }
    else if (sp.is<rs2::video_stream_profile>())
    {
        if (!color_stream_found)
            align_to = profile_stream;
        if (profile_stream == RS2_STREAM_COLOR)
            color_stream_found = true;
    }
    // motion streams (ACCEL/GYRO/POSE) are silently skipped
}

This restores the original semantics for depth_stream_found while keeping the motion-stream exclusion for align_to.

[sysrsbuild-gh-agentic]

change approved — The depth_stream_found regression is fixed. The loop in find_stream_to_align now has fully decoupled branches: RS2_STREAM_DEPTH exclusively sets depth_stream_found, sp.is<rs2::video_stream_profile>() exclusively drives the align_to selection, and motion streams (ACCEL/GYRO/POSE) fall through silently. ACCEL/GYRO can no longer set depth_stream_found = true. ✅

auto-generated by rs-agentic bot

[ashrafk93]

[Enhancement] — auto-generated by rs-agentic bot

depth_scale (computed at line 47 from profile.get_device() before this lambda runs) is not refreshed inside the lambda after pipe.start(cfg) restarts the pipeline. If ensure_non_depth_video() triggers the restart at startup, the depth_scale value used throughout the frame loop is derived from the pre-restart profile.

In practice the device object and its depth scale are identical before and after the restart (same physical device, different stream config), so this is not a runtime crash. However, the assumption is silent. Consider either:

1. Moving depth_scale initialization to after ensure_non_depth_video(), or
2. Adding a one-line comment: // depth_scale is unaffected — same device, only stream config changes

For consistency, note that the profile_changed handler correctly calls depth_scale = get_depth_scale(profile.get_device()) after ensure_non_depth_video() — the startup path should mirror that order.

[sysrsbuild-gh-agentic]

change approved — The depth_scale ordering concern is resolved. The new startup path calls pipe.start(cfg) exactly once (using cfg.resolve() to probe up-front without streaming), and depth_scale is computed from the returned profile after that single start. No pre-restart stale value is possible. The hot-reconnect path already correctly recomputes depth_scale after pipe.start(reconnect_cfg). ✅

auto-generated by rs-agentic bot

[Nir-Az]

Does it also don't work with D455? which has IMU?

[sysrsbuild-gh-agentic]

Yes, the fix is needed for D455 as well. D455 has an integrated IMU (ACCEL + GYRO streams). Before this PR, find_stream_to_align would iterate all non-depth streams and could select ACCEL or GYRO as the alignment target, since it only checked profile_stream != RS2_STREAM_DEPTH. The sp.is<rs2::video_stream_profile>() guard added in this PR explicitly excludes motion stream profiles (ACCEL, GYRO, POSE), so on D455 the function correctly selects COLOR or IR as the alignment target. The D455 default pipeline includes color, so streams_alignable() returns true and no fallback config is needed — but the motion-stream exclusion in find_stream_to_align is required regardless.

auto-generated by rs-agentic bot

[Nir-Az]

Seems weird to do start, condition later stop and later start again, can't we make it simpler?
We can query streams info before streaming is needed, maybe even default profiles

If it's not so simple we can exclude devices with no color stream

[sysrsbuild-gh-agentic]

The new commit (2eea11f — "redesigned configuration and fixed comment") directly addresses this concern. The startup path now uses cfg.resolve(pipe) to probe the default profile without starting the pipeline, so pipe.start(cfg) is called exactly once at startup regardless of whether the device needs an explicit stream config. The start/stop/restart sequence only occurs in the profile_changed hot-reconnect branch — which is inherently a reconnect scenario that requires a restart anyway.

auto-generated by rs-agentic bot

[sysrsbuild-gh-agentic]

Automated review by rs-agentic bot.

Reviewing commit 2eea11fe ("redesigned configuration and fixed comment"). The cfg.resolve() probe-first approach cleanly solves Nir-Az's start/stop/start concern at startup. Three new issues noted inline below.

[sysrsbuild-gh-agentic]

[BUG] — auto-generated by rs-agentic bot

configure_alignable_streams(profile.get_device(), ...) is called after pipe.stop() on line 85. The profile object was obtained from pipe.get_active_profile() (line 80) while the pipeline was still running. rs2::device uses shared-ownership semantics so the handle stays alive, but calling profile.get_device() on a profile captured before pipe.stop() is fragile — if the SDK ever releases the internal device ref on stop, this becomes a use-after-release.

Safer to extract the device before stopping:

auto dev = profile.get_device();  // capture before stop
pipe.stop();
rs2::config reconnect_cfg;
configure_alignable_streams(dev, reconnect_cfg);
profile = pipe.start(reconnect_cfg);

This also makes the intent explicit: the device object is carried across the restart.

[sysrsbuild-gh-agentic]

change approved — The pipe.stop() / configure_alignable_streams(profile.get_device(), ...) sequence that triggered this concern has been removed entirely in this commit. The fragile device-after-stop reference no longer exists. ✅

auto-generated by rs-agentic bot

[sysrsbuild-gh-agentic]

[Enhancement] — auto-generated by rs-agentic bot

configure_alignable_streams picks the first non-depth video stream encountered while iterating dev.query_sensors(), without any preference for RS2_STREAM_COLOR. find_stream_to_align (the consumer of whatever this function enables) explicitly prefers color over IR. If sensor iteration order places an IR sensor before a color sensor, configure_alignable_streams will enable IR even when color is available and would be preferred by find_stream_to_align.

Consider mirroring the same priority — do a first pass for color, fall back to any other video stream if color is not found:

rs2_stream preferred = RS2_STREAM_ANY;
for (auto& sensor : dev.query_sensors())
{
    for (auto& sp : sensor.get_stream_profiles())
    {
        if (!depth_found && sp.stream_type() == RS2_STREAM_DEPTH)
        {
            cfg.enable_stream(RS2_STREAM_DEPTH, sp.stream_index());
            depth_found = true;
        }
        else if (sp.stream_type() == RS2_STREAM_COLOR && sp.is<rs2::video_stream_profile>())
        {
            preferred = RS2_STREAM_COLOR;  // always prefer color
            video_found = true;
        }
        else if (!video_found && sp.is<rs2::video_stream_profile>())
        {
            preferred = sp.stream_type();  // fallback IR or other
            video_found = true;
        }
    }
    if (depth_found && video_found && preferred == RS2_STREAM_COLOR) break;
}
if (video_found) cfg.enable_stream(preferred);

[sysrsbuild-gh-agentic]

[Guidelines] — auto-generated by rs-agentic bot

streams_alignable reimplements a subset of the stream-inspection logic that already exists in find_stream_to_align. Both functions iterate the same vector<rs2::stream_profile> and check for the same depth + non-depth video pair. Per the project coding guidelines: "Never add a second copy of logic that can be merged with an existing one — consolidate first, don't layer."

One way to consolidate without breaking find_stream_to_align's throwing contract:

// Rename find_stream_to_align return: RS2_STREAM_ANY means "no alignable non-depth stream found"
// Callers that need a predicate just check the return value:
inline bool streams_alignable(const std::vector<rs2::stream_profile>& streams)
{
    rs2_stream s = find_stream_to_align_nothrow(streams);  // shared impl, no throw
    return s != RS2_STREAM_ANY;
}

Alternatively, have find_stream_to_align accept an optional bool* has_depth out-param so the pre-check and the selection are done in one pass. The current duplication is low-risk given the small size, but it violates the guideline and creates a maintenance risk if the filtering logic ever changes.

[sysrsbuild-gh-agentic]

[Missing test] — auto-generated by rs-agentic bot

No unit or integration test covers the streams_alignable() / configure_alignable_streams() fallback path — the primary new logic introduced by this PR. Without test coverage, future refactors could silently regress both the GMSL depth-only fallback and the hot-reconnect path.

At minimum, add a test that:

1. Constructs a std::vector<rs2::stream_profile> containing only a depth stream profile and verifies streams_alignable() returns false.
2. Constructs one with depth + IR and verifies streams_alignable() returns true.
3. Verifies find_stream_to_align does not select ACCEL/GYRO profiles (motion-stream regression guard).

See .github/skills/testing.md for the test framework used in this repo.

[Nir-Az]

? not expected

[Binyaminp]

this is a case that pipeline is already running, profile changed to a profile without depth/align-to stream, need to stop pipeline, reconfigure and restart.

[Binyaminp]

removed profile change case

[sysrsbuild-gh-agentic]

[BUG] — auto-generated by rs-agentic bot

The profile_changed hot-reconnect handler has been removed entirely, but the SDK contract for wait_for_frames() explicitly allows the pipeline to replace the active device after a disconnect/error. The original code even had a comment explaining this:

"rs2::pipeline::wait_for_frames() can replace the device it uses in case of device error or disconnection. Since rs2::align is aligning depth to some other stream, we need to make sure that the stream was not changed after the call to wait_for_frames();"

Without this guard:

1. If a device disconnects and reconnects during the session, wait_for_frames() may return frames from a new device/profile.
2. The align object (constructed once before the loop with the initial align_to) will be stale — it is bound to the original stream type and internal profile handles.
3. align.process(frameset) on a frameset from a mismatched profile can crash with a null dereference or produce silently incorrect output.

If the reconnect path was intentionally removed (e.g. the device is guaranteed stable), please add a comment documenting that assumption. Otherwise, the original profile_changed guard (or an equivalent) should be retained.

auto-generated by rs-agentic bot

[sysrsbuild-gh-agentic]

Automated review by rs-agentic bot.

Reviewing latest commit 56393945 (synchronize push — finalizes the profile_changed cleanup following the Nir-Az / Binyaminp discussion). The resolve-first startup approach via cfg.resolve() is clean and the overall structure of the change looks good. One open BUG from the previous review (commit 0581309b) remains unresolved — see inline comment.

[sysrsbuild-gh-agentic]

[BUG] — auto-generated by rs-agentic bot

The hot-reconnect concern raised in the review of commit 0581309b is still unresolved in this commit. The profile_changed guard has been fully and intentionally removed, but the SDK contract for wait_for_frames() still permits the pipeline to silently replace the active device after a disconnection or error (the original code even had an explicit comment documenting this).

Without the guard, after a mid-session device disconnect/reconnect:

1. frameset is from the new device/profile.
2. align was constructed once before the loop with the original align_to type — it is now stale.
3. align.process(frameset) can crash (null dereference on an unexpected stream type) or produce silently wrong output if the reconnected device presents different stream types.

This is the exact class of crash the PR is fixing at startup — removing the guard re-exposes it at runtime.

Minimum ask — if the intentional removal stands (e.g. the device is known to be stable in the target deployments), please add a comment documenting the assumption so future maintainers know the omission is deliberate:

// NOTE: Hot-reconnect / profile-change handling is intentionally omitted.
// The pipeline may transparently replace the device after a disconnection, but
// this example targets fixed-profile devices where that scenario does not occur.
rs2::frameset frameset = pipe.wait_for_frames();

If reconnect robustness is required, the original guard — or a lightweight equivalent using pipe.get_active_profile() — should be reinstated.