minZ feature integration into libRS viewer by alexkunin-gh · Pull Request #15027 · realsenseai/librealsense

alexkunin-gh · 2026-05-06T07:41:03Z

minZ feature integration into libRS viewer

aikido-pr-checks · 2026-05-06T07:41:46Z

+                    }
+                    else if( !res_values.empty() )
+                    {
+                        auto& res = res_values[ui.selected_res_id];


Accessing res_values[ui.selected_res_id] without validating ui.selected_res_id may index out-of-bounds. Check that ui.selected_res_id is within [0, res_values.size()) before using it.

Details

✨ AI Reasoning
A newly added availability predicate captures this and reads res_values[ui.selected_res_id] after only checking res_values.empty(). If ui.selected_res_id is out-of-range (negative or >= res_values.size()) this will index past the vector bounds and can cause a segmentation fault. The code elsewhere sets selected_res_id but it can be modified elsewhere (user/UI) or be uninitialized at times; accessing without validating the index increases crash risk.

🔧 How do I fix it?
Add null checks before dereferencing pointers, validate array bounds before access, avoid using pointers after free/delete, don't write to string literals, and prefer smart pointers in modern C++.

_{Reply @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.}
_{Reply @AikidoSec ignore: [REASON] to ignore this issue.}
_{More info}

Nir-Az

generated by rs-agentic

Branch naming: akunin/minz uses a username prefix, which violates the convention documented in .github/copilot-instructions.md (rule: short descriptive name, no username prefix). Please rename to something like minz-viewer-integration.

Nir-Az · 2026-05-06T07:45:14Z

                        }
                        catch (...)
                        {
+                            if( !pb_available ) ImGui::EndDisabled();


generated by rs-agentic

Potential double EndDisabled() in the catch block — ImGui stack imbalance.

The normal code path already calls ImGui::EndDisabled() inside the if( !pb_available ) block a few lines above (around line 3083). If an exception is thrown after that call but still inside the try (e.g. from ImGui::PopStyleColor(5) or ImGui::PopFont()), the catch block here will call EndDisabled() a second time, leaving ImGui's internal disabled-state stack unbalanced and causing hard-to-debug rendering glitches.

Consider tracking whether EndDisabled has already been called with a local flag, or use an RAII guard:

struct DisabledGuard { bool active; DisabledGuard( bool a ) : active(a) { if (active) ImGui::BeginDisabled(true); } ~DisabledGuard() { if (active) ImGui::EndDisabled(); } }; DisabledGuard dg( !pb_available );

This eliminates both the explicit BeginDisabled/EndDisabled calls and the duplicate-call hazard.

Nir-Az · 2026-05-06T07:45:14Z

+    {
+        _depth_mm_buf.resize( w * h );
+        for( int i = 0; i < w * h; ++i )
+            _depth_mm_buf[i] = static_cast< uint16_t >( raw[i] * scale_mm );


generated by rs-agentic

Silent uint16_t overflow when scale_mm > 1.0.

raw[i] * scale_mm is computed in float and then truncated to uint16_t with no bounds check. When depth_units > 0.001 (e.g. depth_units = 0.01 → scale_mm = 10), any pixel value above 6553 will wrap around silently, corrupting the depth map passed to DepthRangeImprover.

Add a clamp before the cast:

_depth_mm_buf[i] = static_cast<uint16_t>( std::min( raw[i] * scale_mm, static_cast<float>( std::numeric_limits<uint16_t>::max() ) ) );

Nir-Az · 2026-05-06T07:45:14Z

+
+    // get_data() returns const void* even for newly-allocated frames;
+    // exclusive ownership from allocate_video_frame above makes the write safe.
+    auto * dst = reinterpret_cast< uint16_t * >( const_cast< void * >( new_frame.get_data() ) );


generated by rs-agentic

const_cast to write into a freshly-allocated frame is a fragile API-contract violation.

rs2::frame::get_data() returns const void*. Stripping const via const_cast relies on an undocumented internal guarantee that the buffer is actually mutable. While the comment is correct that exclusive ownership makes it currently safe, a future rs2 change (e.g., marking the buffer read-only in debug/ASAN builds, or pooled-frame reuse policy) could cause a silent write to a shared buffer or a protection fault.

A cleaner approach: build the converted values into _out_buf (already done) and then memcpy from _out_buf.data() using the existing std::memcpy path — which is already in place for the is_1mm branch. Alternatively, request a writable-frame accessor from the rs2 API owners.

Nir-Az · 2026-05-06T07:45:14Z

+            {
+                // Safe to capture this: the lambda lives in model which lives in post_processing,
+                // a member of this subdevice_model — so the lambda cannot outlive its owner.
+                model->available = [this]()


generated by rs-agentic

Raw this capture in a long-lived lambda stored in a shared_ptr.

The comment documents the current lifetime guarantee, but model is stored in a shared_ptr<processing_block_model> inside post_processing. If that shared_ptr is ever copied or moved outside its current owner (e.g. into a callback, cache, or async task in a future refactor), this will dangle without any compile-time or runtime warning.

A more robust pattern — if subdevice_model inherits from std::enable_shared_from_this — is:

auto weak_self = weak_from_this(); model->available = [weak_self]() { auto self = weak_self.lock(); if( !self ) return false; // use self-> instead of this-> ... };

If enable_shared_from_this is not available here, at minimum add a static_assert or comment referencing the specific invariant that must hold.

Copilot

Pull request overview

This PR integrates an optional “MinZ” depth-improvement processing block into the RealSense Viewer, intended for Jetson/aarch64 builds that have the external rs-enhanced-depth package installed.

Changes:

Adds an aarch64-only CMake option (BUILD_WITH_MINZ) to locate and link rs-enhanced-depth headers/libs and enable BUILD_WITH_MINZ compilation.
Introduces a viewer-side MinZ adapter (min_z_depth_improver) and an rs2::filter wrapper (minz_filter) and registers it in the per-sensor post-processing chain.
Extends the post-processing UI model to support “availability” predicates and disabled-tooltips, and uses them to gate MinZ based on active streams/resolution.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
tools/realsense-viewer/CMakeLists.txt	Adds aarch64-only MinZ build option, finds headers/libs under `MINZ_ROOT`, and links the external library.
common/CMakeLists.txt	Adds MinZ adapter/filter sources to the viewer common source list.
common/min-z-depth-improver.h	Declares a viewer-side adapter around the external MinZ/DepthRangeImprover implementation.
common/min-z-depth-improver.cpp	Implements frameset handling, lazy init from calibration, and depth replacement in framesets.
common/minz-filter.h	Adds an `rs2::filter` wrapper to insert MinZ into the existing post-processing chain.
common/subdevice-model.cpp	Registers the MinZ processing block and provides UI availability gating/tooltips.
common/processing-block-model.h	Adds optional availability predicate + tooltip to the processing block UI model.
common/device-model.cpp	Disables the processing-block toggle in the UI when `processing_block_model::is_available()` is false and shows tooltip when hovered.

+
+    try
+    {
+        _impl = std::make_unique< rs_depth::DepthRangeImprover >( cal );


+    int w = ir_left.get_width();
+    int h = ir_left.get_height();
+    if( ! _impl || w != _init_width || h != _init_height )
+        if( ! init( ir_left, ir_right ) )
+            return f;
+
+    auto new_depth = run( fs, ir_left, ir_right, depth, src );
+    if( ! new_depth )


+                    // Resolution check — VGA (640x480) minimum
+                    if( ui.is_multiple_resolutions )
+                    {
+                        // Per-stream resolutions: check depth and IR independently
+                        auto check = [&]( rs2_stream stream ) {
+                            auto it = ui.selected_stream_to_res.find( stream );
+                            if( it == ui.selected_stream_to_res.end() ) return false;
+                            return it->second.first >= 640 && it->second.second >= 480;
+                        };
+                        if( !check( RS2_STREAM_DEPTH ) || !check( RS2_STREAM_INFRARED ) )
+                            return false;
+                    }
+                    else if( !res_values.empty() )
+                    {
+                        auto& res = res_values[ui.selected_res_id];
+                        if( res.first < 640 || res.second < 480 )
+                            return false;
+                    }
+
+                    bool depth = false, ir1 = false, ir2 = false;
+                    for( auto& p : profiles )
+                    {
+                        auto it = stream_enabled.find( p.unique_id() );
+                        if( it == stream_enabled.end() || !it->second ) continue;
+                        if( p.stream_type() == RS2_STREAM_DEPTH ) depth = true;
+                        else if( p.stream_type() == RS2_STREAM_INFRARED && p.stream_index() == 1 ) ir1 = true;
+                        else if( p.stream_type() == RS2_STREAM_INFRARED && p.stream_index() == 2 ) ir2 = true;
+                    }
+                    return depth && ir1 && ir2;
+                };
+                model->unavailable_tooltip = "Depth, IR Left/Right streams have to be enabled at VGA or higher resolution";


@@ -3075,11 +3078,20 @@ namespace rs2
                                }
                            }

+                            if( !pb_available )
+                            {
+                                ImGui::EndDisabled();
+                                if( ImGui::IsItemHovered( ImGuiHoveredFlags_AllowWhenDisabled )
+                                    && !pb->unavailable_tooltip.empty() )
+                                    RsImGui::CustomTooltip( "%s", pb->unavailable_tooltip.c_str() );
+                            }
+
                            ImGui::PopStyleColor(5);
                            ImGui::PopFont();
                        }
                        catch (...)
                        {
+                            if( !pb_available ) ImGui::EndDisabled();
                            ImGui::PopStyleColor(5);


Nir-Az · 2026-05-06T08:34:34Z

 set(RS_VIEWER_LIBS ${GTK3_LIBRARIES} Threads::Threads realsense2-gl)

+if(CMAKE_SYSTEM_PROCESSOR MATCHES "aarch64")
+    option(BUILD_WITH_MINZ "Enable MinZ depth improvement (Jetson only, requires rs-enhanced-depth deb)" OFF)


LRS Cmake flags are grouped in https://github.com/realsenseai/librealsense/blob/development/CMake/lrs_options.cmake
Please align

Nir-Az · 2026-05-06T08:37:07Z


 set(RS_VIEWER_LIBS ${GTK3_LIBRARIES} Threads::Threads realsense2-gl)

+if(CMAKE_SYSTEM_PROCESSOR MATCHES "aarch64")


Can we take all of this section and place it in a seperate .cmake file that we import and use here?

yes but then it'll be different design vs OpenVino and the rest

Nir-Az · 2026-05-06T17:09:17Z

@alexkunin-gh how do we test this works?
Should we deploy the library on libci machines?
If so, who will update it?
Lets speak

aikido-pr-checks · 2026-05-06T18:40:59Z

+            throw new InvalidOperationException($"Could not find a sensor matching {extension}");
+        }
+
+        /// <summary>Returns the first depth sensor on the device.</summary>


Multiple one-line sensor helper methods (e.g., FirstDepthSensor, FirstColorSensor, FirstMotionSensor, FirstFisheyeSensor) duplicate the same thin wrapper pattern. Consider consolidating to a single parameterized method or remove redundant wrappers.

Details

✨ AI Reasoning
The change adds several one-line wrapper methods that all follow the same pattern: each returns First() or a typed wrapper of that call. These methods are introduced together in the same file and are highly similar; this is a case of trivial repetition of identical logic across multiple small methods. Consolidation (for example, exposing a single method with an Extension parameter or using a single typed factory) could reduce maintenance surface. The duplication was introduced by this PR and appears in the same file as the additions.

🔧 How do I fix it?
Delete extra code. Extract repeated code sequences into reusable functions or methods. Use loops or data structures to eliminate repetitive patterns.

_{Reply @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.}
_{Reply @AikidoSec ignore: [REASON] to ignore this issue.}
_{More info}

aikido-pr-checks · 2026-05-06T19:32:41Z

+
+public:
+    minz_filter()
+        : rs2::filter( [this]( rs2::frame f, rs2::frame_source & src )


Lambda in minz_filter ctor captures 'this' and dereferences member _improver later; if the callback is invoked after minz_filter is destroyed (or before construction completes), the captured 'this' will be dangling and dereferencing it can segfault. Avoid capturing 'this' into a callback stored in base class or ensure the callback cannot be invoked beyond the object's lifetime.

Details

✨ AI Reasoning
A constructor passes a lambda that captures 'this' to the base rs2::filter constructor. The lambda later dereferences member _improver via that captured this. If the stored callback is invoked after the minz_filter object has been destroyed (or before the derived ctor completed), the captured this becomes dangling and dereferencing _improver will cause a segmentation fault. The issue was introduced by adding minz_filter with a lambda capture of 'this' in the base initializer. This is a classic use-after-free/dangling pointer pattern that can crash at runtime.

🔧 How do I fix it?
Add null checks before dereferencing pointers, validate array bounds before access, avoid using pointers after free/delete, don't write to string literals, and prefer smart pointers in modern C++.

_{Reply @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.}
_{Reply @AikidoSec ignore: [REASON] to ignore this issue.}
_{More info}

alexkunin-gh requested review from Nir-Az and Copilot May 6, 2026 07:41

aikido-pr-checks Bot reviewed May 6, 2026

View reviewed changes

Copilot started reviewing on behalf of alexkunin-gh May 6, 2026 07:41 View session

Nir-Az reviewed May 6, 2026

View reviewed changes

Copilot AI reviewed May 6, 2026

View reviewed changes

Nir-Az reviewed May 6, 2026

View reviewed changes

Comment thread tools/realsense-viewer/CMakeLists.txt Outdated

Nir-Az reviewed May 6, 2026

View reviewed changes

Comment thread tools/realsense-viewer/CMakeLists.txt

Nir-Az reviewed May 6, 2026

View reviewed changes

Comment thread CMake/lrs_options.cmake

aikido-pr-checks Bot reviewed May 6, 2026

View reviewed changes

alexkunin-gh force-pushed the akunin/minz branch from 52fdbb1 to 660c392 Compare May 6, 2026 19:32

aikido-pr-checks Bot reviewed May 6, 2026

View reviewed changes

alexkunin-gh added 4 commits May 6, 2026 22:34

minz

8b41ba1

disable minz for non relavant cameras and resolutions

4ee1fc9

code review changes

c205b20

code review changes

d231278

alexkunin-gh force-pushed the akunin/minz branch from 660c392 to d231278 Compare May 6, 2026 19:35

aikido fix

adcb225


		set(RS_VIEWER_LIBS ${GTK3_LIBRARIES} Threads::Threads realsense2-gl)

		if(CMAKE_SYSTEM_PROCESSOR MATCHES "aarch64")

Conversation

alexkunin-gh commented May 6, 2026

Uh oh!

aikido-pr-checks Bot May 6, 2026

Choose a reason for hiding this comment

Uh oh!

Nir-Az left a comment

Choose a reason for hiding this comment

Uh oh!

Nir-Az May 6, 2026

Choose a reason for hiding this comment

Uh oh!

Nir-Az May 6, 2026

Choose a reason for hiding this comment

Uh oh!

Nir-Az May 6, 2026

Choose a reason for hiding this comment

Uh oh!

Nir-Az May 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Nir-Az May 6, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Nir-Az May 6, 2026

Choose a reason for hiding this comment

Uh oh!

alexkunin-gh May 6, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Nir-Az commented May 6, 2026

Uh oh!

aikido-pr-checks Bot May 6, 2026

Choose a reason for hiding this comment

Uh oh!

aikido-pr-checks Bot May 6, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants