Fix memory leaks, races, crashes; add CI build (Windows x64 + Linux x64)#1
Open
stytim wants to merge 2 commits into
Open
Fix memory leaks, races, crashes; add CI build (Windows x64 + Linux x64)#1stytim wants to merge 2 commits into
stytim wants to merge 2 commits into
Conversation
Ports the hardening RealSense-ToolTracker received this year (PRs #2 and #3) to the Azure Kinect codebase, adapted for the k4a SDK, plus CI workflows. Memory leaks / RAII: - AHATFrame is RAII: pDepth -> std::vector<uint16_t>, m_CurrentFrame -> std::unique_ptr; ProcessFrame takes the frame by reference; all manual new/delete removed (fixes a per-frame leak when ProcessFrame returned false). - Replace raw new[] ToolResultContainer with std::vector; drop dead tool_solutions allocation in UnionSegmentation. - IRToolTracker destructor joins both worker threads and frees the pending frame (fixes use-after-free on owner destruction). - Release the depth image on the IR-image-null capture path (per-frame leak). Threading / races: - m_bShouldStop / m_bIsCurrentlyTracking / m_bIsCurrentlyCalibrating and the ViewerWindow udp/multi/csv flags are std::atomic; ImGui checkboxes use a local bool + load/store. - Pass IRTrackedTool by reference through TrackTool/MatchPointsKabsch so the per-sphere Kalman filters persist across frames. - m_ToolsMutex guards m_Tools, the index map, cur_transform and m_lTrackedTimestamp; GetToolTransform deep-copies under the lock; UnionSegmentation publishes all results atomically. - StartTracking marks tracking active before spawning the thread (closes a use-after-free window vs. AddTool/RemoveTool); RemoveAllTools stops tracking before clearing m_Tools. - Split the IR preview into a tracking-thread working buffer and a mutex-published buffer; GetProcessedFrame returns a deep clone under the frame mutex; processStreams publishes trackingFrame under the same mutex and guards m_IRToolTracker for both AddFrame and GetProcessedFrame. Crash / lifecycle hardening: - Gate Calibrate Tool on a selected device or recording and block re-entry while a session runs; join the streaming thread before reassigning the handle (Calibrate + Start Tracking). - Wrap the k4a capture loop in try/catch and set Terminated=true on every failure/EOF path so a mid-stream unplug no longer aborts a worker thread. - shutdown() nulls the k4a device/playback handles after closing and is guarded; null device after every close site so the always-on shutdown() cannot double-close. ViewerWindow gains a destructor that joins all workers. Calibration validation: - Reject results with <3 or >6 spheres or any non-finite coordinate; guard empty processedFrames, varying per-frame sphere counts, empty distanceToLine, and divide-by-zero -> NaN in averaging; surface a "calibration unsuccessful" popup. Input validation: - Bounds-check ROMParser marker count and data block; validate tool JSON; clamp numTools and per-tool sphere counts; clamp UDP ports; validate UDP datagram length before memcpy; reference-count socket init/deinit. Performance / cleanup: - Quantize per-radius map keys (SphereRadiusKey, std::map<int,...>) so UI and calibration radii share a cache entry; clamp 1000/frequency in the UDP/CSV worker threads; poll 50 ms before recv in the UDP receive thread. - Delete the unused FrameQueue and FlipTransformRightLeft; clean up the smoothing-toggle macros; seed the Kalman filter position from the first measurement. CI: - Add .github/workflows/build.yml and release.yml for windows-x86_64 and linux-x86_64. Windows uses the vendored extern/k4a + OpenCV (choco) and an NSIS installer; Linux installs the Azure Kinect SDK from the Microsoft apt feed and produces a .deb. Link k4a::k4arecord on Linux so the .mkv playback symbols resolve. Preserves Azure Kinect specifics: 16-bit IR (CV_16UC1, threshold 3000), the regionGrowing depth-sampling path, and the .mkv playback path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…tively) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR ports a set of hardening fixes into the Azure Kinect ToolTracker, focusing on eliminating leaks and unsafe lifecycles in the tracking pipeline while also adding CI workflows for Windows/Linux builds and releases.
Changes:
- Reworked frame ownership and per-frame allocations to use RAII (
std::unique_ptr,std::vector) and added defensive validation for tool definitions / calibration outputs. - Tightened threading behavior (atomics + mutexes) around shared state (tool definitions, published frames, sockets) to reduce races and shutdown crashes.
- Added GitHub Actions CI for building on Windows x86_64 and Linux x86_64 plus a tagged release workflow that produces installers/packages.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
src/ViewerWindow.cpp |
Adds defensive JSON parsing, tool/port clamps, socket init refcounting, and threading guards for UI/UDP/CSV interactions. |
src/IRToolTracking.cpp |
Hardens device/playback lifecycle and stream loop behavior; improves teardown idempotence and error handling. |
src/IRToolTrack.cpp |
Converts frame/results to RAII containers, adds locking for shared tool state, and improves calibration robustness. |
include/ViewerWindow.h |
Switches control flags to atomics, adds mutexes/helpers, and makes shutdown run from the destructor. |
include/ROMParser.h |
Adds bounds checks when parsing ROM marker data. |
include/IRToolTracking.h |
Removes an unused frame-queue implementation and related includes. |
include/IRToolTrack.h |
Updates APIs for RAII frames, adds atomics/mutexes, and introduces working/published frame split. |
include/IRStructs.h |
Adds SphereRadiusKey() bucketing and converts depth storage to std::vector<uint16_t>. |
include/IRKalmanFilter.h |
Seeds Kalman state from the first measurement for more stable startup behavior. |
CMakeLists.txt |
Links k4a::k4arecord on Linux when available to support .mkv playback symbols. |
.github/workflows/build.yml |
Adds matrix CI builds for Ubuntu 22.04 and Windows 2022 with platform-specific dependencies. |
.github/workflows/release.yml |
Adds tagged release packaging for Linux .deb and Windows NSIS installer, uploading artifacts to GitHub Releases. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
44
to
50
| result = k4a_playback_get_calibration(playback, &calibration); | ||
| if (result != K4A_RESULT_SUCCEEDED) | ||
| { | ||
| std::cerr << "Failed to get calibration" << std::endl; | ||
| Terminated = true; | ||
| return; | ||
| } |
Comment on lines
64
to
+68
| if (K4A_RESULT_SUCCEEDED != k4a_device_open(index, &device)) | ||
| { | ||
| std::cout << "Failed to open device" << std::endl; | ||
| k4a_device_close(device); | ||
| device = NULL; |
Comment on lines
22
to
26
| k4a_device_open(i, &device); | ||
| char serial_number[256]; | ||
| size_t serial_number_length = sizeof(serial_number); | ||
| k4a_device_get_serialnum(device, serial_number, &serial_number_length); | ||
| k4a_device_close(device); |
Comment on lines
+430
to
431
| std::copy(tool_transform.begin(), tool_transform.begin() + 3, data.position); | ||
| std::copy(tool_transform.begin() + 3, tool_transform.end(), data.quaternion); |
Comment on lines
+106
to
+109
| std::cout << "Loaded tool definition: " << tool.toolName << std::endl; | ||
| tools.push_back(std::move(tool)); | ||
| numTools += 1; | ||
| } |
Comment on lines
127
to
133
| if (K4A_RESULT_SUCCEEDED != k4a_device_start_cameras(device, &config)) | ||
| { | ||
| std::cout << "Failed to start device" << std::endl; | ||
| Terminated = true; | ||
| return; | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Ports the hardening RealSense-ToolTracker received this year (PRs #2 and #3) to the Azure Kinect codebase, adapted for the k4a SDK, and adds CI.
Memory leaks / RAII
AHATFrameRAII (std::vector<uint16_t>pDepth,std::unique_ptrcurrent frame); removes the per-frame leak whenProcessFramereturned false and a use-after-free on shutdown.std::vectortool results instead ofnew[]; releases the depth image on the IR-null capture path (per-frame k4a leak).Threading / races
std::atomicflags;m_ToolsMutexguardingm_Tools/cur_transform;GetToolTransformdeep-copies under lock;UnionSegmentationpublishes atomically; Kalman state now persists (by-reference tools);StartTrackingflag-before-spawn; working/published IR preview split.Crash / lifecycle (PR #3)
shutdown()idempotent (handles nulled at every close site) +~ViewerWindow().Validation
numTools/sphere/port clamps; UDP datagram-length check; refcounted socket init.CI
build.yml+release.ymlfor windows-x86_64 (vendoredextern/k4a+ OpenCV → NSIS) and linux-x86_64 (Azure Kinect SDK from Microsoft apt feed →.deb). Linksk4a::k4arecordon Linux for.mkvplayback.Preserves Azure Kinect specifics: 16-bit IR (
CV_16UC1, threshold 3000), theregionGrowingdepth path, and the.mkvplayback path.Verification
All translation units syntax-check clean locally (clang
-fsyntax-only) and the diff was reviewed via adversarial multi-agent passes. The Linux CI path (k4a apt feed / EULA preseed /find_package(k4a)config) is the main thing this PR run validates — the Windows path uses the in-repo vendored SDK and is higher-confidence.