Support argus camera for camera_viz - #833
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds an optional Jetson Argus camera backend with C++/CUDA capture, YUV-to-RGBA conversion, pybind11 bindings, and Python Estimated code review effort: 5 (Critical) | ~120 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 3
🤖 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 `@examples/camera_viz/argus/argus_camera.cpp`:
- Around line 783-799: Prevent published device buffers from being reused while
the consumer still holds them: update the latest() buffer handoff and
pick_write_index() selection to track the index currently leased by the consumer
and exclude it from writes until release, or implement an equivalent FrameView
lease/refcount lifecycle. Preserve existing publication behavior while ensuring
producer reuse waits for or skips any buffer still in use.
- Around line 731-753: Update the CU_EGL_FRAME_TYPE_PITCH handling around
launch_yuv420_pitch_to_rgba so planar U/V strides are not derived from
frame.pitch / 2. Read each plane’s pitch from the available buffer metadata and
pass those per-plane values, or explicitly reject planar pitch frames when
metadata is unavailable; preserve the existing non-planar path.
- Around line 671-684: Update the acquire call in the frame acquisition flow
around cuEGLStreamConsumerAcquireFrame to pass config_.acquire_timeout_ms
converted from milliseconds to microseconds. Treat both CUDA_ERROR_TIMEOUT and
CUDA_ERROR_LAUNCH_TIMEOUT as retryable timeout results that return false, while
preserving existing handling for other CUDA errors.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 8c1004b5-6560-4cd1-a0a7-d91a0c7813c4
📒 Files selected for processing (16)
examples/camera_viz/README.mdexamples/camera_viz/argus/CMakeLists.txtexamples/camera_viz/argus/__init__.pyexamples/camera_viz/argus/argus_bindings.cppexamples/camera_viz/argus/argus_camera.cppexamples/camera_viz/argus/argus_camera.hppexamples/camera_viz/argus/build.shexamples/camera_viz/argus/yuv_to_rgba.cuexamples/camera_viz/argus/yuv_to_rgba.cuhexamples/camera_viz/camera_viz.shexamples/camera_viz/configs/argus_shw5g.yamlexamples/camera_viz/scripts/_install_deps.shexamples/camera_viz/sources/__init__.pyexamples/camera_viz/sources/_helpers.pyexamples/camera_viz/sources/argus.pyexamples/camera_viz/tests/test_paired_frame_source_race.py
| CUresult result = cuEGLStreamConsumerAcquireFrame( | ||
| &stream.connection, &out.resource, &out.stream, config_.acquire_timeout_ms); | ||
| if (result == CUDA_ERROR_TIMEOUT) | ||
| { | ||
| return false; | ||
| } | ||
| if (result != CUDA_SUCCESS) | ||
| { | ||
| if (!running_.load()) | ||
| { | ||
| return false; | ||
| } | ||
| check_cuda(result, "cuEGLStreamConsumerAcquireFrame"); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the file and inspect the relevant slice with line numbers.
ast-grep outline examples/camera_viz/argus/argus_camera.cpp --view expanded || true
sed -n '640,710p' examples/camera_viz/argus/argus_camera.cpp
# Find the config definition and any timeout-related helpers/usages.
rg -n "acquire_timeout_ms|CUDA_ERROR_LAUNCH_TIMEOUT|CUDA_ERROR_TIMEOUT|cuEGLStreamConsumerAcquireFrame|CUDA_EGL_INFINITE_TIMEOUT" -S .Repository: NVIDIA/IsaacTeleop
Length of output: 5645
🌐 Web query:
CUDA EGLStreamConsumerAcquireFrame timeout microseconds CUDA_ERROR_LAUNCH_TIMEOUT documentation
💡 Result:
In the CUDA EGLStream API, the function cuEGLStreamConsumerAcquireFrame (and its runtime equivalent cudaEGLStreamConsumerAcquireFrame) takes a timeout parameter specified in microseconds (usec) [1][2]. This parameter defines the maximum time the consumer will wait for a new frame to be acquired from the EGLStream [2]. Regarding CUDA_ERROR_LAUNCH_TIMEOUT: 1. Usage and Context: While the function documentation highlights the timeout parameter, CUDA_ERROR_LAUNCH_TIMEOUT is explicitly cited as a possible return value for operations involving EGLStream connections [2][3]. Specifically, this error can occur if a consumer has not returned a frame to the EGLStream or if an acquisition attempt times out [2][4]. 2. Error Handling: When CUDA_ERROR_LAUNCH_TIMEOUT is returned, it indicates that the system failed to acquire or release a frame within the expected time window [2][3]. NVIDIA documentation suggests that in such cases, the application can safely retry the operation [2][5]. 3. Timeout Semantics: If the timeout parameter is set to CUDA_EGL_INFINITE_TIMEOUT, the acquisition call will wait indefinitely for a frame [2][4]. If a standard timeout value is set and the operation expires, the CUDA consumer may attempt to acquire an older frame if the EGL_SUPPORT_REUSE_NV flag is set (which is enabled by default) [2][4]. For developers, it is important to note that the timeout mechanism is not designed to be shorter than the producer's frame rate interval [6]. Setting a timeout value smaller than the expected time between frames may lead to frequent CUDA_ERROR_LAUNCH_TIMEOUT results or unwanted acquisition of stale frames [6][2].
Citations:
- 1: https://docs.nvidia.com/cuda/cuda-runtime-api/group__CUDART__EGL.html
- 2: https://docs.nvidia.com/cuda/cuda-driver-api/group__CUDA__EGL.html
- 3: https://github.com/jokla/ros_gmsl_driver/blob/master/include/drive-t186ref-linux/include/cudaEGL.h
- 4: https://docs.nvidia.com/cuda/archive/12.9.2/cuda-driver-api/group__CUDA__EGL.html
- 5: https://docs.nvidia.com/cuda/archive/13.0.1/cuda-driver-api/group__CUDA__EGL.html
- 6: https://forums.developer.nvidia.com/t/question-about-argus-library-frameconsumer-and-acquiring-frames/52704/9
🌐 Web query:
site:docs.nvidia.com cuEGLStreamConsumerAcquireFrame timeout usec return CUDA_ERROR_LAUNCH_TIMEOUT
💡 Result:
The return value CUDA_ERROR_LAUNCH_TIMEOUT when calling cuEGLStreamConsumerAcquireFrame (or its runtime API equivalent, cudaEGLStreamConsumerAcquireFrame) indicates that the consumer has not successfully acquired a new frame from the EGLStream within the specified timeout duration [1][2][3]. Key details regarding this error and API behavior: 1. Meaning: This error is explicitly documented to occur if the consumer has not returned a frame to the EGL stream before the timeout [1][3]. 2. Recommended Action: If this error is returned, the application is expected to retry the operation [1][2][4]. 3. Timeout Parameter: The timeout parameter is specified in microseconds (usec) [1][2][4]. Setting this parameter to CUDA_EGL_INFINITE_TIMEOUT (defined as 0xFFFFFFFF) causes the call to wait indefinitely [2][4][5]. 4. Frame Reuse: After the timeout occurs, the CUDA consumer may still attempt to acquire an "old" frame if one is available and the EGL_SUPPORT_REUSE_NV flag is set (which is enabled by default) [2][3][4]. If you wish to disable this behavior, you must explicitly set EGL_SUPPORT_REUSE_NV to EGL_FALSE during EGLStream initialization [3][6][4]. For further implementation details, refer to the official NVIDIA CUDA Toolkit documentation under the EGL Interoperability section [1][3][4].
Citations:
- 1: https://docs.nvidia.com/cuda/cuda-runtime-api/group__CUDART__EGL.html
- 2: https://docs.nvidia.com/cuda/archive/13.0.1/cuda-driver-api/group__CUDA__EGL.html
- 3: https://docs.nvidia.com/cuda/cuda-driver-api/group__CUDA__EGL.html
- 4: https://docs.nvidia.com/cuda/developer-preview/13.4/cuda-driver-api/cuda_driver_api/group__CUDA__EGL.html
- 5: https://docs.nvidia.com/cuda/cuda-driver-api/group__CUDA__TYPES.html
- 6: https://docs.nvidia.com/cuda/archive/13.2.2/cuda-driver-api/group__CUDA__EGL.html
Convert the acquire timeout to microseconds and handle CUDA_ERROR_LAUNCH_TIMEOUT as a retryable timeout. cuEGLStreamConsumerAcquireFrame expects a usec timeout, but config_.acquire_timeout_ms is passed through unchanged here, so any finite timeout is 1000× too short. The timeout branch is also checking the wrong error code; a normal timeout can fall into check_cuda() and stop the producer thread.
🤖 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 `@examples/camera_viz/argus/argus_camera.cpp` around lines 671 - 684, Update
the acquire call in the frame acquisition flow around
cuEGLStreamConsumerAcquireFrame to pass config_.acquire_timeout_ms converted
from milliseconds to microseconds. Treat both CUDA_ERROR_TIMEOUT and
CUDA_ERROR_LAUNCH_TIMEOUT as retryable timeout results that return false, while
preserving existing handling for other CUDA errors.
| if (frame.frameType == CU_EGL_FRAME_TYPE_PITCH) | ||
| { | ||
| const auto* y_plane = static_cast<const uint8_t*>(frame.frame.pPitch[0]); | ||
| const auto* uv_or_u_plane = static_cast<const uint8_t*>(frame.frame.pPitch[1]); | ||
| const auto* v_plane = planar ? static_cast<const uint8_t*>(frame.frame.pPitch[2]) : nullptr; | ||
| const int y_pitch = static_cast<int>(frame.pitch); | ||
| const int uv_pitch = static_cast<int>(planar ? frame.pitch / 2 : frame.pitch); | ||
| const int v_pitch = static_cast<int>(planar ? frame.pitch / 2 : 0); | ||
| launch_yuv420_pitch_to_rgba(y_plane, | ||
| uv_or_u_plane, | ||
| v_plane, | ||
| y_pitch, | ||
| uv_pitch, | ||
| v_pitch, | ||
| config_.width, | ||
| config_.height, | ||
| dest.ptr, | ||
| static_cast<int>(dest.pitch), | ||
| layout, | ||
| full_range, | ||
| reinterpret_cast<cudaStream_t>(convert_stream_)); | ||
| return pending_textures; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does CUeglFrame pitch apply to chroma planes for YUV420 planar CU_EGL_FRAME_TYPE_PITCH frames on Jetson?
💡 Result:
For CUeglFrame structures of type CU_EGL_FRAME_TYPE_PITCH used on NVIDIA Jetson platforms, the pitch field in the CUeglFrame structure strictly refers only to the pitch of the first plane (typically the Y plane for YUV formats) [1][2][3]. It does not apply to subsequent chroma planes [1][2]. Authoritative NVIDIA documentation and developer guidance specify that for multiplanar EGL frames, only the pitch of the first plane is to be considered by the application [1][2]. The pitch values, if any, associated with additional planes (such as U or V in YUV420) within the CUeglFrame structure cannot be relied upon and are often incorrect [2][4]. To obtain the correct pitch for secondary planes (chroma planes) on Jetson, you should use platform-specific APIs such as NvBufferGetParams from the Jetson Multimedia API, which correctly retrieves the pitch for each individual plane [1][2][4]. Relying on calculations based solely on the primary plane's pitch or the values populated in the CUeglFrame structure for secondary planes will generally lead to incorrect memory access and data corruption (e.g., visual artifacts/striping) [2][4].
Citations:
- 1: https://forums.developer.nvidia.com/t/pitch-of-second-frame/175667
- 2: https://forums.developer.nvidia.com/t/cuda-mapping-egl-frames-from-encoder-returns-incorrect-pitch-for-uv-planes/242472/5
- 3: https://docs.nvidia.com/cuda/cuda-driver-api/group__CUDA__EGL.html
- 4: https://forums.developer.nvidia.com/t/cuda-mapping-egl-frames-from-encoder-returns-incorrect-pitch-for-uv-planes/242472/12
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant function and any nearby pitch handling.
file="examples/camera_viz/argus/argus_camera.cpp"
echo "=== Context around lines 700-780 ==="
sed -n '700,780p' "$file" | cat -n
echo
echo "=== Search for planar/pitch handling in this file ==="
rg -n "planar|uv_pitch|v_pitch|CU_EGL_FRAME_TYPE_PITCH|pPitch\\[" "$file"Repository: NVIDIA/IsaacTeleop
Length of output: 5532
Don’t derive planar chroma pitch from frame.pitch / 2. CUeglFrame::pitch only covers the first plane; for planar YUV420 frames, U/V can have different strides. This can misindex chroma and read past the buffer. Fetch per-plane pitches from the buffer metadata or reject planar CU_EGL_FRAME_TYPE_PITCH frames.
🤖 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 `@examples/camera_viz/argus/argus_camera.cpp` around lines 731 - 753, Update
the CU_EGL_FRAME_TYPE_PITCH handling around launch_yuv420_pitch_to_rgba so
planar U/V strides are not derived from frame.pitch / 2. Read each plane’s pitch
from the available buffer metadata and pass those per-plane values, or
explicitly reject planar pitch frames when metadata is unavailable; preserve the
existing non-planar path.
| void ArgusCamera::publish(uint32_t write_idx, uint64_t timestamp_ns) | ||
| { | ||
| std::lock_guard<std::mutex> guard(publish_mutex_); | ||
| publish_idx_ = static_cast<int>(write_idx); | ||
| published_timestamp_ns_ = timestamp_ns; | ||
| ++published_sequence_; | ||
| } | ||
|
|
||
| uint32_t ArgusCamera::pick_write_index() const | ||
| { | ||
| std::lock_guard<std::mutex> guard(publish_mutex_); | ||
| if (publish_idx_ < 0) | ||
| { | ||
| return 0; | ||
| } | ||
| return static_cast<uint32_t>((publish_idx_ + 1) % 3); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Published buffers can be overwritten while the Python consumer still reads them.
latest() (Line 286-310) hands raw device pointers out of buffers_[eye][publish_idx_], and sources/argus.py keeps using them (CUDA view on stream 0) after publish_mutex_ is released. pick_write_index() only avoids the currently published index, so after two more publishes the producer wraps back onto the buffer the consumer is still reading — an unsynchronized producer/consumer race on device memory, visible as torn frames under any consumer stall.
Either track the index handed out by latest() and exclude it from pick_write_index(), or give FrameView an explicit lease (release call / refcount) that the producer honours before reusing a slot.
🤖 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 `@examples/camera_viz/argus/argus_camera.cpp` around lines 783 - 799, Prevent
published device buffers from being reused while the consumer still holds them:
update the latest() buffer handoff and pick_write_index() selection to track the
index currently leased by the consumer and exclude it from writes until release,
or implement an equivalent FrameView lease/refcount lifecycle. Preserve existing
publication behavior while ensuring producer reuse waits for or skips any buffer
still in use.
| // Argus YUV420 -> RGBA8 conversion for CUDA EGLStream consumers. | ||
|
|
||
| #include "yuv_to_rgba.cuh" | ||
|
|
There was a problem hiding this comment.
it's a bit funny what we till need to hand roll rgh/yuv conversion... maybe we can move this to viz lib in the future.
^ just an agent review. ignore if already addressed/irrelevent. Let's also add argus to the public docs too. Otherwise LGTM |
Signed-off-by: Teng Wei <tengw@nvidia.com>
3 participants
Description
Implement argus camera driver support in the camera_viz example
Fixes #(issue)
Type of change
Testing
Tested with camera that supports argus driver
Checklist
SKIP=check-copyright-year pre-commit run --all-filesgit commit -s) per the DCOSummary by CodeRabbit
New Features
--with-argussetup support and improved setup guidance.Bug Fixes
Tests