Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds an SDL-backed PreviewStream and integrates it into the OAK camera plugin: CMake now fetches and bundles SDL2, OakCamera pipeline creation is refactored to return a local dai::Pipeline and accept a color resolution, a new PreviewStream class is added, and a new --preview CLI flag enables live preview rendering. Changes
Sequence DiagramsequenceDiagram
participant User
participant OakCamera
participant Pipeline
participant PreviewStream
participant Queue as DataOutputQueue
participant SDL as SDL Renderer
User->>OakCamera: init (config.preview = true)
OakCamera->>OakCamera: create_pipeline(color_resolution)
OakCamera->>PreviewStream: PreviewStream::create(name, Pipeline, resolution)
PreviewStream->>Pipeline: add ColorCamera (CAM_A) & XLinkOut
PreviewStream->>SDL: SDL_CreateWindow / SDL_CreateRenderer
PreviewStream-->>OakCamera: return PreviewStream
OakCamera->>Pipeline: start pipeline on device
OakCamera->>PreviewStream: setOutputQueue(queue)
loop per-frame
OakCamera->>PreviewStream: update()
PreviewStream->>Queue: getImgFrame(timeout)
Queue-->>PreviewStream: ImgFrame
PreviewStream->>SDL: SDL_UpdateTexture(frame data)
SDL->>SDL: SDL_RenderCopy & SDL_RenderPresent
PreviewStream->>PreviewStream: poll SDL events (quit / esc / q)
end
User->>PreviewStream: press q / esc / close window
PreviewStream->>SDL: cleanup texture, renderer, window
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Comment |
shaosu-nvidia
force-pushed
the
ssx/camera_recording_preview
branch
from
23be2d5 to
419d8ef
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/plugins/oak/CMakeLists.txt`:
- Around line 4-8: The top-of-file comment block in CMakeLists.txt has malformed
separator lines (e.g., "#== == == ..." with extra spaces); fix by normalizing
those header comment separators to a single consistent, readable pattern (for
example a continuous sequence of "=" or "-" characters) and remove the stray
spaces so the block around the "OAK Camera Plugin" header is uniformly
formatted; update the three affected lines in the header comment to match the
chosen consistent separator style.
- Around line 35-46: The CMakeLists currently pins SDL2 via the SDL2_VERSION
variable set to "2.30.11" (used by FetchContent_Declare sdl2 and the URL)
without explanation; either update SDL2_VERSION to "2.32.10" to use the latest
stable release or add a short comment above the SDL2_VERSION declaration
explaining why 2.30.11 is required (e.g., compatibility, API/ABI constraints, or
CI/test matrix). Ensure the change touches the SDL2_VERSION definition and the
FetchContent_Declare URL reference so they remain consistent.
In `@src/plugins/oak/core/oak_camera.cpp`:
- Around line 56-57: If config.preview is true but
PreviewStream::try_create(kPreviewStreamName, pipeline, color_resolution)
returns nullptr, add a log message indicating preview creation failed so users
know their --preview flag had no effect; check the result assigned to m_preview
and when it is null call an appropriate logger (e.g., warn/error) with context
including kPreviewStreamName and that SDL/preview initialization failed to help
debugging.
In `@src/plugins/oak/core/preview_stream.cpp`:
- Around line 107-110: PreviewStream::setOutputQueue currently dereferences
m_impl without validation; add a defensive null check to avoid a crash when
called on a default-constructed PreviewStream by verifying m_impl is non-null
before using it and returning or logging/throwing if it is null. Specifically,
in PreviewStream::setOutputQueue, check if m_impl (the internal implementation
pointer) is null, and only call m_impl->queue = std::move(queue) when it's
valid; if m_impl is null, handle gracefully (e.g., return early or report an
error via existing logging/error mechanisms).
- Around line 24-33: The Impl destructor currently calls SDL_Quit()
unconditionally which can tear down SDL for other users; update the cleanup in
~Impl() to avoid globally quitting SDL — either call
SDL_QuitSubSystem(SDL_INIT_VIDEO) instead of SDL_Quit() to only shut down the
video subsystem, or implement a simple reference-counted init/teardown for the
video subsystem (increment on Init call and decrement in ~Impl(), calling
SDL_QuitSubSystem(SDL_INIT_VIDEO) when the count reaches zero); modify the
~Impl() body (the destructor of Impl in PreviewStream) to use
SDL_QuitSubSystem(SDL_INIT_VIDEO) or the ref-counting mechanism and remove the
unconditional SDL_Quit() call.
- Around line 112-116: PreviewStream::update currently calls
m_impl->queue->tryGet<dai::ImgFrame>() without ensuring m_impl->queue is set,
which can crash if update() runs before setOutputQueue(); add a defensive null
check for m_impl->queue at the start of PreviewStream::update and return early
if it's null. Reference the m_impl->queue pointer and the setOutputQueue setter
when adding the guard so future callers won't dereference a null queue before
calling tryGet.
- Around line 145-152: The update loop in PreviewStream consumes quit/key events
but doesn't inform callers; change PreviewStream::update() to return a bool
(true = continue, false = quit) by updating the declaration in
preview_stream.hpp and its implementation in preview_stream.cpp so that when
SDL_QUIT or keydown for SDLK_ESCAPE/SDLK_q is seen the method returns false
instead of early-returning void; then update callers (e.g., oak_camera.cpp) to
check the returned bool and initiate shutdown/stop the preview when false.
Ensure all call sites are updated to handle the new bool return and recompile.
In `@src/plugins/oak/main.cpp`:
- Line 6: Remove the unused include "core/preview_stream.hpp" from main.cpp:
inspect the top of the file where `#include` "core/preview_stream.hpp" is present
and delete it if PreviewStream is not referenced; verify that OakCamera (class
OakCamera) provides the preview functionality and that no direct use or forward
declaration of PreviewStream exists in main.cpp before removing; run a build to
confirm there are no missing symbols and only re-add the include if a direct
dependency or forward declaration for PreviewStream is discovered.
- Around line 109-110: The help text incorrectly says "via OpenCV window";
update the usage/help output in main.cpp (the block that prints "\nPreview:\n"
and the line containing "--preview Show live color camera preview via
OpenCV window") to reference SDL2 instead (e.g., "Show live color camera preview
via SDL2 window" or similar). Also ensure any adjacent help wording in the same
printing routine or function (main / usage printing) is consistent with
preview_stream.cpp's SDL2-based implementation.
In `@src/plugins/oak/README.md`:
- Around line 25-27: Update the src/plugins/oak/README.md to document the new
CLI flag --preview: add an entry for --preview in the Configuration/options
table describing its behavior, list any SDL2 system dependencies required to
enable preview mode (e.g., libsdl2-dev or equivalent), and add a short example
command line showing how to build/run with --preview (alongside the existing
cmake examples). Ensure the text clearly notes that preview requires SDL2 and
any platform-specific notes so users know to install the dependency before using
--preview.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
src/plugins/oak/CMakeLists.txtsrc/plugins/oak/README.mdsrc/plugins/oak/core/oak_camera.cppsrc/plugins/oak/core/oak_camera.hppsrc/plugins/oak/core/preview_stream.cppsrc/plugins/oak/core/preview_stream.hppsrc/plugins/oak/main.cpp
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/plugins/oak/core/preview_stream.cpp`:
- Around line 73-98: After SDL_Init succeeds, ensure we clean up SDL state and
any partially-created resources on early returns: when SDL_CreateWindow or
SDL_CreateRenderer fails, call SDL_DestroyRenderer(impl->renderer) if non-null,
SDL_DestroyWindow(impl->window) if non-null, then call
SDL_QuitSubSystem(SDL_INIT_VIDEO) (or SDL_Quit() as appropriate) before
returning nullptr; you can encapsulate this in a small cleanup helper (e.g.,
CleanupPreview(Impl* impl)) and invoke it in the failure branches that follow
SDL_CreateWindow and SDL_CreateRenderer to ensure no SDL subsystem or resources
are leaked.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
src/plugins/oak/CMakeLists.txtsrc/plugins/oak/README.mdsrc/plugins/oak/core/oak_camera.cppsrc/plugins/oak/core/oak_camera.hppsrc/plugins/oak/core/preview_stream.cppsrc/plugins/oak/core/preview_stream.hppsrc/plugins/oak/main.cpp
Collaborator
|
@shaosu-nvidia do we have OSRB ticket for it uses SDLv2 for window and rendering. It's https://github.com/libsdl-org/SDL/blob/main/LICENSE.txt It may not be self served and will have lead time to land. |
Contributor
Author
|
Thanks @jiwenc-nv for checking this. It seems zlib is a even more permissive license than MIT so theoretically should get some auto approval. But yeah I will definitely start the OSRB ticket before landing the code. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/plugins/oak/CMakeLists.txt`:
- Around line 41-44: The FetchContent_Declare call for target "sdl2" currently
downloads SDL2 via URL using SDL2_VERSION without checksum; compute the SHA256
of the exact SDL2-${SDL2_VERSION}.tar.gz you intend to use (e.g., download and
run sha256sum) and add a URL_HASH = "SHA256=<hex>" entry to the same
FetchContent_Declare block so FetchContent_Declare(sdl2 ...) includes URL_HASH
and pins artifact integrity; keep the symbol names FetchContent_Declare, sdl2,
SDL2_VERSION, and URL_HASH when making the change.
- Around line 45-46: The install step assumes a concrete target named SDL2
exists while the link uses the alias SDL2::SDL2; update the CMakeLists to detect
and install the actual concrete target created by
FetchContent_MakeAvailable(sdl2) by checking TARGET existence and installing
either SDL2 or SDL2-static accordingly (e.g., if(TARGET SDL2) install(TARGETS
SDL2 ...) elseif(TARGET SDL2-static) install(TARGETS SDL2-static ...) endif),
while leaving the existing link usage of SDL2::SDL2 unchanged so both shared and
static SDL2 builds are handled safely.
Comment on lines
+41
to
+44
| FetchContent_Declare( | ||
| sdl2 | ||
| URL "https://github.com/libsdl-org/SDL/releases/download/release-${SDL2_VERSION}/SDL2-${SDL2_VERSION}.tar.gz" | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and read the relevant section
if [ -f "src/plugins/oak/CMakeLists.txt" ]; then
echo "=== File exists. Reading lines 35-50 for context ==="
cat -n src/plugins/oak/CMakeLists.txt | sed -n '35,50p'
else
echo "File not found at src/plugins/oak/CMakeLists.txt"
# Try to find CMakeLists.txt files in the repo
find . -name "CMakeLists.txt" -type f 2>/dev/null | head -20
fiRepository: NVIDIA/IsaacTeleop
Length of output: 805
🏁 Script executed:
# Search for SDL2_VERSION definition and FetchContent usage
echo "=== Searching for SDL2_VERSION definition ==="
rg "SDL2_VERSION" --type cmake -B 2 -A 2 | head -30
echo ""
echo "=== Searching for FetchContent_Declare blocks ==="
rg "FetchContent_Declare" --type cmake -A 3 | head -50Repository: NVIDIA/IsaacTeleop
Length of output: 3651
🏁 Script executed:
# Check if any FetchContent declarations use URL_HASH
echo "=== Checking URL_HASH usage in FetchContent_Declare blocks ==="
rg "URL_HASH" --type cmake -B 2 -A 2 | head -40Repository: NVIDIA/IsaacTeleop
Length of output: 124
🌐 Web query:
SDL2 2.32.10 release checksums official github libsdl-org
💡 Result:
For SDL2 2.32.10 (tag release-2.32.10), libsdl-org does not publish official SHA256/MD5 “checksums” on the GitHub release page—the official integrity mechanism they provide for the upstream release artifacts is detached signature files (.sig). [1][2]
Official release artifacts for 2.32.10 (with signatures) are listed here: SDL2-2.32.10.tar.gz + SDL2-2.32.10.tar.gz.sig, and SDL2-2.32.10.zip + SDL2-2.32.10.zip.sig (plus platform binaries). [2]
Sources: [1] [2]
Add URL_HASH for SDL2 FetchContent to protect artifact integrity.
Line 43 fetches a remote tarball without checksum verification. Add URL_HASH to pin the artifact's integrity. Note that SDL2 doesn't publish official SHA256 checksums; the hash must be computed manually from the downloaded artifact and pinned in the configuration.
Implementation approach
FetchContent_Declare(
sdl2
URL "https://github.com/libsdl-org/SDL/releases/download/release-${SDL2_VERSION}/SDL2-${SDL2_VERSION}.tar.gz"
+ URL_HASH SHA256=<computed_hash>
)To obtain the hash: download SDL2-2.32.10.tar.gz, compute its SHA256 (sha256sum SDL2-2.32.10.tar.gz), and pin the value. SDL2 provides detached GPG signatures on the release page for additional verification if needed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/plugins/oak/CMakeLists.txt` around lines 41 - 44, The
FetchContent_Declare call for target "sdl2" currently downloads SDL2 via URL
using SDL2_VERSION without checksum; compute the SHA256 of the exact
SDL2-${SDL2_VERSION}.tar.gz you intend to use (e.g., download and run sha256sum)
and add a URL_HASH = "SHA256=<hex>" entry to the same FetchContent_Declare block
so FetchContent_Declare(sdl2 ...) includes URL_HASH and pins artifact integrity;
keep the symbol names FetchContent_Declare, sdl2, SDL2_VERSION, and URL_HASH
when making the change.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
src/plugins/oak/core/preview_stream.cpp (3)
99-102: 🧹 Nitpick | 🔵 TrivialMissing null check on
m_impl.If
setOutputQueueis called on a default-constructedPreviewStream, this will crash. While the factory pattern makes this unlikely, a defensive check improves robustness.Proposed defensive check
void PreviewStream::setOutputQueue(std::shared_ptr<dai::DataOutputQueue> queue) { + if (!m_impl) + return; m_impl->queue = std::move(queue); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/plugins/oak/core/preview_stream.cpp` around lines 99 - 102, The setOutputQueue implementation uses m_impl without validating it, which can crash if PreviewStream was default-constructed; update PreviewStream::setOutputQueue to defensively check m_impl before use (e.g., if m_impl is null, either initialize it via m_impl = std::make_unique<Impl>() or return/log an error) and then assign m_impl->queue = std::move(queue); reference the PreviewStream class and its m_impl member when making this change.
74-90:⚠️ Potential issue | 🟡 MinorSDL resources leak if window/renderer creation throws.
After
SDL_Initsucceeds, ifSDL_CreateWindoworSDL_CreateRendererfails, the exception is thrown without cleaning up the initialized SDL subsystem. WhenSDL_CreateRendererfails, the already-created window also leaks.Proposed fix with proper cleanup
if (SDL_Init(SDL_INIT_VIDEO) != 0) throw std::runtime_error(std::string("Preview: SDL_Init failed: ") + SDL_GetError()); auto impl = std::make_unique<Impl>(); impl->window = SDL_CreateWindow( name.c_str(), SDL_WINDOWPOS_CENTERED, SDL_WINDOWPOS_CENTERED, preview_w, preview_h, SDL_WINDOW_SHOWN); if (!impl->window) + { + SDL_QuitSubSystem(SDL_INIT_VIDEO); throw std::runtime_error(std::string("Preview: SDL_CreateWindow failed: ") + SDL_GetError()); + } impl->renderer = SDL_CreateRenderer(impl->window, -1, SDL_RENDERER_ACCELERATED | SDL_RENDERER_PRESENTVSYNC); if (!impl->renderer) impl->renderer = SDL_CreateRenderer(impl->window, -1, 0); if (!impl->renderer) + { + SDL_DestroyWindow(impl->window); + SDL_QuitSubSystem(SDL_INIT_VIDEO); throw std::runtime_error(std::string("Preview: SDL_CreateRenderer failed: ") + SDL_GetError()); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/plugins/oak/core/preview_stream.cpp` around lines 74 - 90, The code leaks SDL resources when SDL_CreateWindow or SDL_CreateRenderer fails: after SDL_Init succeeds you must ensure SDL_DestroyRenderer, SDL_DestroyWindow and SDL_Quit are called on error. Modify the preview constructor around SDL_Init/impl creation so that if SDL_CreateWindow fails you call SDL_Quit before throwing, and if SDL_CreateRenderer fails you destroy the created window (SDL_DestroyWindow) and call SDL_Quit before throwing; alternatively wrap the window/renderer in RAII helpers or use a unique_ptr with a custom deleter for Impl->window and Impl->renderer to guarantee cleanup on exceptions. Ensure references to SDL_Init, SDL_CreateWindow, SDL_CreateRenderer, impl->window and impl->renderer are handled accordingly.
104-139:⚠️ Potential issue | 🟡 MinorPreview window won't respond to close/quit events.
The
update()method renders frames but doesn't poll SDL events. Users cannot close the preview window using Escape, Q, or the window's close button — they must use Ctrl+C in the terminal. Consider adding event handling.Proposed fix - add event polling
void PreviewStream::update() { if (!m_impl->queue) throw std::runtime_error("Preview: Output queue not set"); auto frame = m_impl->queue->tryGet<dai::ImgFrame>(); if (!frame) return; // ... rendering code ... SDL_RenderPresent(m_impl->renderer); + + SDL_Event event; + while (SDL_PollEvent(&event)) + { + if (event.type == SDL_QUIT || + (event.type == SDL_KEYDOWN && + (event.key.keysym.sym == SDLK_ESCAPE || event.key.keysym.sym == SDLK_q))) + { + // Raise SIGINT to trigger the existing shutdown path + raise(SIGINT); + return; + } + } }This raises
SIGINTto use the existing signal-based shutdown inmain.cpp.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/plugins/oak/core/preview_stream.cpp` around lines 104 - 139, The preview window never polls SDL events so it can't handle close/quit actions; modify PreviewStream::update to run an SDL_Event loop (SDL_PollEvent) each frame, check for SDL_QUIT and SDL_KEYDOWN with SDLK_ESCAPE or SDLK_q, and on those events call raise(SIGINT) (add `#include` <csignal> at top) to trigger the existing signal-based shutdown; keep rendering logic intact and only add the event loop before/after updating the texture so m_impl->renderer and m_impl->texture are still valid.src/plugins/oak/CMakeLists.txt (1)
41-44:⚠️ Potential issue | 🟠 MajorAdd
URL_HASHfor SDL2 FetchContent to ensure artifact integrity.The SDL2 tarball is fetched without checksum verification. Adding
URL_HASHprotects against corrupted downloads or supply chain attacks.Implementation approach
FetchContent_Declare( sdl2 URL "https://github.com/libsdl-org/SDL/releases/download/release-${SDL2_VERSION}/SDL2-${SDL2_VERSION}.tar.gz" + URL_HASH SHA256=<computed_hash> )Compute the hash: download SDL2-2.32.10.tar.gz and run
sha256sum SDL2-2.32.10.tar.gz, then pin the value.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/plugins/oak/CMakeLists.txt` around lines 41 - 44, The FetchContent declaration for sdl2 currently omits checksum verification; update the FetchContent_Declare(sdl2 ...) block to include a URL_HASH entry with the SHA256 checksum for the SDL2 tarball so CMake verifies integrity on download; compute the hash (e.g., download SDL2-${SDL2_VERSION}.tar.gz and run sha256sum) and add the resulting value as URL_HASH SHA256=<checksum> to the sdl2 FetchContent_Declare call.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/plugins/oak/CMakeLists.txt`:
- Around line 69-74: The elseif branch installing the SDL2-static ARCHIVE is
dead code given SDL_SHARED ON and SDL_STATIC OFF (set via
SDL_SHARED/SDL_STATIC), so either remove the elseif( TARGET SDL2-static )
install branch to simplify the logic, or keep it as an explicit fallback but add
a comment clarifying it’s optional and that installing static libraries as
ARCHIVE will not get RPATH handling; update the install block around the
SDL2/SDL2-static checks (targets SDL2 and SDL2-static) accordingly so the code
and comments accurately reflect the intended SDL_SHARED/SDL_STATIC
configuration.
---
Duplicate comments:
In `@src/plugins/oak/CMakeLists.txt`:
- Around line 41-44: The FetchContent declaration for sdl2 currently omits
checksum verification; update the FetchContent_Declare(sdl2 ...) block to
include a URL_HASH entry with the SHA256 checksum for the SDL2 tarball so CMake
verifies integrity on download; compute the hash (e.g., download
SDL2-${SDL2_VERSION}.tar.gz and run sha256sum) and add the resulting value as
URL_HASH SHA256=<checksum> to the sdl2 FetchContent_Declare call.
In `@src/plugins/oak/core/preview_stream.cpp`:
- Around line 99-102: The setOutputQueue implementation uses m_impl without
validating it, which can crash if PreviewStream was default-constructed; update
PreviewStream::setOutputQueue to defensively check m_impl before use (e.g., if
m_impl is null, either initialize it via m_impl = std::make_unique<Impl>() or
return/log an error) and then assign m_impl->queue = std::move(queue); reference
the PreviewStream class and its m_impl member when making this change.
- Around line 74-90: The code leaks SDL resources when SDL_CreateWindow or
SDL_CreateRenderer fails: after SDL_Init succeeds you must ensure
SDL_DestroyRenderer, SDL_DestroyWindow and SDL_Quit are called on error. Modify
the preview constructor around SDL_Init/impl creation so that if
SDL_CreateWindow fails you call SDL_Quit before throwing, and if
SDL_CreateRenderer fails you destroy the created window (SDL_DestroyWindow) and
call SDL_Quit before throwing; alternatively wrap the window/renderer in RAII
helpers or use a unique_ptr with a custom deleter for Impl->window and
Impl->renderer to guarantee cleanup on exceptions. Ensure references to
SDL_Init, SDL_CreateWindow, SDL_CreateRenderer, impl->window and impl->renderer
are handled accordingly.
- Around line 104-139: The preview window never polls SDL events so it can't
handle close/quit actions; modify PreviewStream::update to run an SDL_Event loop
(SDL_PollEvent) each frame, check for SDL_QUIT and SDL_KEYDOWN with SDLK_ESCAPE
or SDLK_q, and on those events call raise(SIGINT) (add `#include` <csignal> at
top) to trigger the existing signal-based shutdown; keep rendering logic intact
and only add the event loop before/after updating the texture so
m_impl->renderer and m_impl->texture are still valid.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
src/plugins/oak/CMakeLists.txtsrc/plugins/oak/README.mdsrc/plugins/oak/core/oak_camera.cppsrc/plugins/oak/core/preview_stream.cppsrc/plugins/oak/core/preview_stream.hppsrc/plugins/oak/main.cpp
shaosu-nvidia
force-pushed
the
ssx/camera_recording_preview
branch
from
eb75c80 to
e8797e4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/plugins/oak/main.cpp (1)
113-115:⚠️ Potential issue | 🟡 MinorFix camera names in the CLI example.
Line 115 uses
LeftMono/RightMono, but parsing expectsMonoLeft/MonoRight(seeparse_camera_name). This example currently suggests invalid values.Suggested patch
- << " --add-stream=camera=Color,output=./color.h264 --add-stream=camera=LeftMono,output=./left.h264 --add-stream=camera=RightMono,output=./right.h264\n"; + << " --add-stream=camera=Color,output=./color.h264 --add-stream=camera=MonoLeft,output=./left.h264 --add-stream=camera=MonoRight,output=./right.h264\n";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/plugins/oak/main.cpp` around lines 113 - 115, The CLI example prints invalid camera names; update the example string that uses program_name so the camera identifiers match parse_camera_name's expected values (MonoLeft and MonoRight) instead of LeftMono/RightMono; locate the formatted output lines around program_name in main.cpp and change "LeftMono" to "MonoLeft" and "RightMono" to "MonoRight" so the example matches the parse_camera_name camera name parsing.
♻️ Duplicate comments (2)
src/plugins/oak/CMakeLists.txt (1)
41-44:⚠️ Potential issue | 🟠 MajorAdd
URL_HASHfor the SDL2 tarball fetch.Line 43 downloads a remote archive without integrity pinning. Please add
URL_HASH SHA256=...in the sameFetchContent_Declareblock.Suggested patch
FetchContent_Declare( sdl2 URL "https://github.com/libsdl-org/SDL/releases/download/release-${SDL2_VERSION}/SDL2-${SDL2_VERSION}.tar.gz" + URL_HASH SHA256=<computed_sha256> )#!/bin/bash set -euo pipefail ver="2.32.10" url="https://github.com/libsdl-org/SDL/releases/download/release-${ver}/SDL2-${ver}.tar.gz" tmp="$(mktemp)" trap 'rm -f "$tmp"' EXIT curl -fsSL "$url" -o "$tmp" sha256sum "$tmp"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/plugins/oak/CMakeLists.txt` around lines 41 - 44, The FetchContent_Declare block for sdl2 currently fetches the SDL2 tarball via URL without integrity verification; update the FetchContent_Declare call (the block that declares sdl2 and uses SDL2_VERSION and URL "https://github.com/libsdl-org/SDL/releases/download/release-${SDL2_VERSION}/SDL2-${SDL2_VERSION}.tar.gz") to include an URL_HASH entry with the SHA256 checksum (e.g., URL_HASH SHA256=<calculated-hash>) so CMake will verify the downloaded archive integrity.src/plugins/oak/core/preview_stream.cpp (1)
104-139:⚠️ Potential issue | 🟠 MajorPump SDL events in
update()and propagate quit intent.
PreviewStream::update()renders frames but never processes SDL events. That can make the preview window non-responsive to close/keyboard actions. Please poll events each tick and return/propagate a quit signal to the caller.#!/bin/bash set -euo pipefail rg -n -C2 'SDL_PollEvent|SDL_QUIT|SDLK_ESCAPE|SDLK_q' src/plugins/oak/core/preview_stream.cpp src/plugins/oak/core/oak_camera.cpp🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/plugins/oak/core/preview_stream.cpp` around lines 104 - 139, PreviewStream::update() must poll SDL events each tick using SDL_PollEvent and detect SDL_QUIT and keydown events for SDLK_ESCAPE and SDLK_q; when such a quit intent is seen return a quit signal instead of silently ignoring it—change the signature of PreviewStream::update() to return bool (true = quit requested, false = continue), call SDL_PollEvent in the method (checking event.type == SDL_QUIT and event.type == SDL_KEYDOWN with event.key.keysym.sym == SDLK_ESCAPE or SDLK_q), and update all callers to handle the returned quit flag (or alternately set a dedicated member like m_impl->quit_requested if you prefer a stateful approach); retain all current rendering steps and ensure SDL_PollEvent is invoked before or after SDL_RenderPresent().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/plugins/oak/core/oak_camera.cpp`:
- Around line 46-50: The code unconditionally throws if CAM_A is missing but
only needs CAM_A when a color stream or preview is used; modify the CAM_A
existence check in oak_camera.cpp so it only runs when config.preview ||
has_stream(streams, core::StreamType_Color). In practice, wrap the
sensors.find(CAM_A) existence check and the subsequent
color_sensor_name/color_resolution assignment inside an if (config.preview ||
has_stream(streams, core::StreamType_Color)) block (or equivalent early-return
guard), using the existing symbols sensors, color_sensor_name, color_resolution,
config.preview, has_stream, streams, and core::StreamType_Color to locate and
change the logic.
---
Outside diff comments:
In `@src/plugins/oak/main.cpp`:
- Around line 113-115: The CLI example prints invalid camera names; update the
example string that uses program_name so the camera identifiers match
parse_camera_name's expected values (MonoLeft and MonoRight) instead of
LeftMono/RightMono; locate the formatted output lines around program_name in
main.cpp and change "LeftMono" to "MonoLeft" and "RightMono" to "MonoRight" so
the example matches the parse_camera_name camera name parsing.
---
Duplicate comments:
In `@src/plugins/oak/CMakeLists.txt`:
- Around line 41-44: The FetchContent_Declare block for sdl2 currently fetches
the SDL2 tarball via URL without integrity verification; update the
FetchContent_Declare call (the block that declares sdl2 and uses SDL2_VERSION
and URL
"https://github.com/libsdl-org/SDL/releases/download/release-${SDL2_VERSION}/SDL2-${SDL2_VERSION}.tar.gz")
to include an URL_HASH entry with the SHA256 checksum (e.g., URL_HASH
SHA256=<calculated-hash>) so CMake will verify the downloaded archive integrity.
In `@src/plugins/oak/core/preview_stream.cpp`:
- Around line 104-139: PreviewStream::update() must poll SDL events each tick
using SDL_PollEvent and detect SDL_QUIT and keydown events for SDLK_ESCAPE and
SDLK_q; when such a quit intent is seen return a quit signal instead of silently
ignoring it—change the signature of PreviewStream::update() to return bool (true
= quit requested, false = continue), call SDL_PollEvent in the method (checking
event.type == SDL_QUIT and event.type == SDL_KEYDOWN with event.key.keysym.sym
== SDLK_ESCAPE or SDLK_q), and update all callers to handle the returned quit
flag (or alternately set a dedicated member like m_impl->quit_requested if you
prefer a stateful approach); retain all current rendering steps and ensure
SDL_PollEvent is invoked before or after SDL_RenderPresent().
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
src/plugins/oak/CMakeLists.txtsrc/plugins/oak/README.mdsrc/plugins/oak/core/oak_camera.cppsrc/plugins/oak/core/preview_stream.cppsrc/plugins/oak/core/preview_stream.hppsrc/plugins/oak/main.cpp
shaosu-nvidia
force-pushed
the
ssx/camera_recording_preview
branch
2 times, most recently
from
d36f2d9 to
ded82fd
Compare
Collaborator
The OSRB ticket looks good. Feel feel to work with Qingsi on getting the rest of the PR reviewed and merged. Thanks. |
shaosu-nvidia
force-pushed
the
ssx/camera_recording_preview
branch
from
ded82fd to
0eceac4
Compare
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
2 participants
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.
it uses SDLv2 for window and rendering. It's https://github.com/libsdl-org/SDL/blob/main/LICENSE.txt. The OSRB ticket can be found: https://nvbugspro.nvidia.com/bug/5944021
By default it uses the color sensor from oak camera for preview. so if only mono left and mono right are required, it will open the color for preview.
it's using --preview to control
Summary by CodeRabbit
New Features
Documentation
Chores