Upgrade DELTACAST to native operators #1162
Conversation
laurent-radoux
force-pushed
the
upgrade-deltacast-integration
branch
3 times, most recently
from
93bb48a to
b988722
Compare
tbirdso
left a comment
tbirdso
left a comment
There was a problem hiding this comment.
Hi @laurent-radoux , would you please add a short description to this PR to help us review?
laurent-radoux
force-pushed
the
upgrade-deltacast-integration
branch
2 times, most recently
from
5ae9840 to
ebe58f6
Compare
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additional Comments (3)
-
operators/deltacast_videomaster/python/videomaster_pydoc.hpp, line 45 (link)syntax: typo: 'baords' should be 'boards'
-
operators/deltacast_videomaster/python/videomaster_pydoc.hpp, line 94 (link)syntax: typo: 'baords' should be 'boards'
-
operators/deltacast_videomaster/python/videomaster_pydoc.hpp, line 129 (link)logic: copy-paste error: header guard comment says 'AJA_SOURCE' but should say 'VIDEOMASTER_SOURCE' or similar
45 files reviewed, 36 comments
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This review covers only the most recent changes since the last review, not the entire PR.
The latest changes address critical compilation and documentation issues identified in previous reviews. In videomaster_source.hpp, the removal of using declarations from the header has been completed, but this creates a compilation blocker because method signatures in the class declaration (lines 41-46) still reference unqualified Holoscan types like OperatorSpec, InputContext, OutputContext, and ExecutionContext. The declarations must either be fully qualified (e.g., holoscan::OperatorSpec) or the using declarations must be restored to the header. In videomaster_transmitter.cpp, critical safety improvements were made: buffer overflow validation now prevents copying tensors larger than the destination buffer, and the error-handling logic in configure_board_for_overlay and configure_stream_for_overlay was refactored to return immediately on failures instead of masking errors. Documentation and metadata fixes across Python and C++ variants ensure version requirements (3.0.0 minimum) and tags accurately reflect the non-AI video display purpose. The main.cpp refactoring improves robustness by using weakly_canonical() to handle symlink invocations gracefully.
Important Files Changed
| Filename | Score | Overview |
|---|---|---|
operators/deltacast_videomaster/videomaster_source.hpp |
1/5 | Removed using declarations from header but method signatures still use unqualified types—compilation will fail |
applications/deltacast_receiver/python/CMakeLists.txt |
2/5 | Moved find_package(holoscan) outside BUILD_TESTING block, but GXF_LIB_DIR referenced in test without corresponding find_package(GXF) |
operators/deltacast_videomaster/videomaster_transmitter.cpp |
5/5 | Added buffer overflow protection, fixed memcpy size bug, and refactored error handling to return early on failures |
operators/deltacast_videomaster/videomaster_base.cpp |
4/5 | Added comprehensive CUDA free error checking in cleanup path with appropriate best-effort logging |
applications/deltacast_receiver/cpp/main.cpp |
5/5 | Improved getopt loop structure, reordered operator creation for readability, and switched to weakly_canonical for symlink safety |
operators/deltacast_videomaster/videomaster_source.cpp |
5/5 | Added CUDA error checking for memcpy operations and explicit using declarations in implementation file |
operators/deltacast_videomaster/videomaster_base.hpp |
5/5 | Removed GXF dependency and replaced sleep macro with inline function for better type safety |
operators/deltacast_videomaster/videomaster_transmitter.hpp |
5/5 | Fixed documentation (transmitter vs receiver) and initialized member variables at declaration |
applications/deltacast_receiver/README.md |
5/5 | Documented both C++ and Python run instructions for the receiver application |
operators/deltacast_videomaster/python/videomaster_pydoc.hpp |
3/5 | Fixed typos ("baords", "a overlay") but header guard still references incorrect file name |
applications/deltacast_receiver/cpp/metadata.json |
5/5 | Corrected minimum_required_version to 3.0.0 and removed inappropriate "Healthcare AI" tag |
applications/deltacast_receiver/python/metadata.json |
5/5 | Updated version requirements and tags to match C++ variant, added trailing newline |
applications/deltacast_receiver/python/deltacast_receiver.py |
4/5 | Replaced exit() with sys.exit(1) for proper process termination |
Confidence score: 1/5
- This PR is not safe to merge due to a critical compilation error that will break the build.
- Score reflects a blocking issue in
videomaster_source.hppwhere unqualified Holoscan types in method signatures will fail to compile after removal ofusingdeclarations from the header. The Python CMakeLists.txt also references undefined variables (GXF_LIB_DIR) that could cause test configuration issues. - Pay close attention to
operators/deltacast_videomaster/videomaster_source.hpp(must restoreusingdeclarations or fully qualify types) andapplications/deltacast_receiver/python/CMakeLists.txt(must addfind_package(GXF)or remove GXF_LIB_DIR reference).
Additional Comments (2)
-
operators/deltacast_videomaster/python/videomaster_pydoc.hpp, line 31 (link)syntax: doc comment describes transmitter functionality ("stream video FROM") but should describe receiver/capture ("get video stream FROM"). The description matches VideoMasterSourceOp which receives input, not transmits output.
-
operators/deltacast_videomaster/python/videomaster_pydoc.hpp, line 79 (link)syntax: "stream video FROM" should be "stream video TO" - transmitter sends video to the card, not from it
13 files reviewed, 5 comments
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughRemoved the GXF-based deltacast_videomaster extension; migrated VideoMaster functionality into native operators; added a new deltacast_receiver application (C++ and Python); removed explicit pool allocator args across operators/apps; updated CMake, metadata, tests, container device mounts, and added Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Pre-merge checks✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
gxf_extensions/CMakeLists.txt (1)
19-21: Incomplete removal: stale references to deltacast_videomaster remain across the codebase.The extension was removed from
gxf_extensions/CMakeLists.txt, but dependent registrations and references must also be cleaned up:
operators/CMakeLists.txt:24– removeadd_holohub_operator(deltacast_videomaster)applications/CMakeLists.txt:38, 42– removeOPERATORS deltacast_videomasterdependenciesapplications/CMakeLists.txt:58– remove fromOPTIONALlistapplications/orsi/CMakeLists.txt:36– remove operator appendapplications/endoscopy_tool_tracking/cpp/README.md:26, 58– update/remove build instructionsoperators/deltacast_videomaster/python/videomaster.cpp (1)
128-137: Wrong default types in Python bindings (strings instead of integers).Defaults like
"0"sand"60"sarestd::string, but the constructor expectsuint32_t. This will break default-arg construction.- "board"_a = "0"s, - "input"_a = "0"s, - "width"_a = "0"s, - "height"_a = "0"s, + "board"_a = 0u, + "input"_a = 0u, + "width"_a = 1920u, + "height"_a = 1080u, "progressive"_a = true, - "framerate"_a = "60"s, + "framerate"_a = 60u, "name"_a = "videomaster_source"s,- "board"_a = "0"s, - "output"_a = "0"s, - "width"_a = "0"s, - "height"_a = "0"s, + "board"_a = 0u, + "output"_a = 0u, + "width"_a = 1920u, + "height"_a = 1080u, "progressive"_a = true, - "framerate"_a = "60"s, + "framerate"_a = 60u, "enable_overlay"_a = false,Also applies to: 157-167
operators/deltacast_videomaster/videomaster_base.cpp (1)
204-243: Guard optional before dereference in configure_stream.
get_video_format(stream_handle())returns an optional but is dereferenced without checking. This can crash.- if (!_video_information->get_video_format(stream_handle())->progressive) { + auto opt_vf = _video_information->get_video_format(stream_handle()); + if (!opt_vf.has_value()) { + HOLOSCAN_LOG_ERROR("Failed to query video format"); + return false; + } + if (!opt_vf->progressive) { success_b = holoscan_log_on_error(Deltacast::Helper::ApiSuccess{ VHD_SetStreamProperty(*stream_handle(), VHD_CORE_SP_FIELD_MERGE, TRUE) }, "Failed to set field merge property"); if (!success_b) { return false; } } - const auto &id_to_stream_type = _is_input ? id_to_rx_stream_type : id_to_tx_stream_type; - _video_format = _video_information->get_video_format(stream_handle()).value(); + const auto &id_to_stream_type = _is_input ? id_to_rx_stream_type : id_to_tx_stream_type; + _video_format = *opt_vf;
♻️ Duplicate comments (9)
applications/deltacast_receiver/README.md (1)
29-35: Python run instructions have been added.The past review comment about missing Python variant is now addressed - the README includes run instructions for both C++ (lines 21-27) and Python (lines 29-35) applications.
applications/deltacast_receiver/python/deltacast_receiver.yaml (1)
46-46: Consider adding a trailing newline.While not critical, it's conventional to end text files with a newline character. This aligns with the past review comment.
operators/deltacast_videomaster/videomaster_transmitter.cpp (2)
151-153: Simplify double‑negation for “stream not started” check.Use a direct empty‑format comparison; clearer and avoids confusion.
- } else if (!(_video_master_base->video_format() != - Deltacast::Helper::VideoFormat{})) { // stream not started yet + } else if (_video_master_base->video_format() == Deltacast::Helper::VideoFormat{}) { + // stream not started yet
242-247: Good: size guard before copy resolves prior risk.The explicit
tensor->nbytes() > buffer_sizecheck prevents overruns flagged earlier.operators/deltacast_videomaster/videomaster_base.hpp (1)
33-35: Inline sleep helper: good change.
Replaces macro with a typesafe inline function as previously suggested.operators/deltacast_videomaster/videomaster_source.cpp (2)
130-136: Simplify TIMEOUT handling and avoid redundant conditions.
if (api_result != VHDERR_NOERROR && api_result != VHDERR_TIMEOUT) { ... }followed byif (api_result != VHDERR_NOERROR && api_result == VHDERR_TIMEOUT)is redundant. Use a clearif/else if.- if (api_result != VHDERR_NOERROR && api_result != VHDERR_TIMEOUT) { - throw std::runtime_error("Failed to wait for incoming slot"); - } - if (api_result != VHDERR_NOERROR && api_result == VHDERR_TIMEOUT) { - HOLOSCAN_LOG_INFO("Timeout"); - return; - } + if (api_result == VHDERR_TIMEOUT) { + HOLOSCAN_LOG_INFO("Timeout"); + return; + } else if (api_result != VHDERR_NOERROR) { + throw std::runtime_error("Failed to wait for incoming slot"); + }
90-93: Make stream-start condition readable.
!(_video_master_base->video_format() != VideoFormat{})is hard to parse. Invert the logic.-} else if (!(_video_master_base->video_format() != - Deltacast::Helper::VideoFormat{})) { // start stream +} else if (_video_master_base->video_format() == + Deltacast::Helper::VideoFormat{}) { // start streamoperators/deltacast_videomaster/videomaster_base.cpp (2)
283-323: Clean up partially allocated buffers on failure paths.On allocation errors (
cudaHostAlloc,cudaMalloc,posix_memalign), the function returnsfalsewithout freeing previously allocated buffers.- if (cuda_error != cudaSuccess) { + if (cuda_error != cudaSuccess) { HOLOSCAN_LOG_ERROR("CUDA host allocation failed: {}", cudaGetErrorString(cuda_error)); - return false; + free_buffers(); + return false; } ... - if (cuda_error != cudaSuccess) { + if (cuda_error != cudaSuccess) { HOLOSCAN_LOG_ERROR("CUDA device allocation failed: {}", cudaGetErrorString(cuda_error)); - return false; + free_buffers(); + return false; } ... - if (posix_memalign((void**)&_system_buffers[slot_index][buffer_type_index], + if (posix_memalign((void**)&_system_buffers[slot_index][buffer_type_index], 4096, buffer_sizes[buffer_type_index])) { HOLOSCAN_LOG_ERROR("posix_memalign failed for system buffers"); - return false; + free_buffers(); + return false; }
75-100: Avoid partially initialized objects on CUDA init failure.Constructor logs and returns on CUDA errors, leaving the object usable but in an undefined state. Prefer throwing or recording a fatal init error and preventing further use.
- if (cuda_error != cudaSuccess) { - HOLOSCAN_LOG_ERROR("Failed to set CUDA device: {}", - cudaGetErrorString(cuda_error)); - _is_igpu = false; // Default to discrete GPU behavior - return; - } + if (cuda_error != cudaSuccess) { + throw std::runtime_error(std::string("Failed to set CUDA device: ") + + cudaGetErrorString(cuda_error)); + } ... - if (cuda_error != cudaSuccess) { - HOLOSCAN_LOG_ERROR("Failed to get CUDA device properties: {}", - cudaGetErrorString(cuda_error)); - _is_igpu = false; // Default to discrete GPU behavior - return; - } + if (cuda_error != cudaSuccess) { + throw std::runtime_error(std::string("Failed to get CUDA device properties: ") + + cudaGetErrorString(cuda_error)); + }
🧹 Nitpick comments (16)
operators/deltacast_videomaster/videomaster_transmitter.cpp (3)
20-24: Remove unused GXF resource includes.
allocator.hppandcuda_stream_pool.hpparen't referenced; drop them to speed up builds and avoid implying a GXF resource dependency.
38-42: Drop unusedid_to_genlock_sourceor use it.The map is defined but never referenced. Remove to reduce clutter.
249-255: Consider async copies to avoid blocking the request thread.
cudaMemcpyis synchronous on the default stream; this can stall compute. If feasible, introduce a CUDA stream resource and usecudaMemcpyAsync+ sync at the hand‑off point.- cudaError_t cuda_result = cudaMemcpy( - buffer, tensor->data(), tensor->nbytes(), - (_use_rdma ? cudaMemcpyDeviceToDevice : cudaMemcpyDeviceToHost)); + // if a cudaStream_t 'stream' is available: + cudaError_t cuda_result = cudaMemcpyAsync( + buffer, tensor->data(), tensor->nbytes(), + (_use_rdma ? cudaMemcpyDeviceToDevice : cudaMemcpyDeviceToHost), + stream); + // Ensure completion before queueing the slot (or use events): + cudaError_t sync_result = cudaStreamSynchronize(stream); + if (sync_result != cudaSuccess) { /* log & return */ }If you prefer to keep it simple for now, keep
cudaMemcpybut revisit for throughput later.operators/deltacast_videomaster/videomaster_base.hpp (3)
33-35: Avoid global namespace pollution forsleep_ms.Place the helper inside your namespace to prevent collisions in headers.
-inline void sleep_ms(uint32_t value) { - std::this_thread::sleep_for(std::chrono::milliseconds(value)); -} +namespace holoscan::ops { +inline void sleep_ms(uint32_t value) { + std::this_thread::sleep_for(std::chrono::milliseconds(value)); +} +} // namespace holoscan::opsThen drop the duplicate
namespace holoscan::ops {opener at Line 37 or adjust placement accordingly.
76-76: Return plainbool(notconst bool).
conston a value return has no effect; simplify signature.- const bool is_igpu() const { return _is_igpu; } + bool is_igpu() const { return _is_igpu; }
47-56: Consider [[nodiscard]] on configuration methods.Encourage callers to check results; most places already do.
- bool configure_board(); + [[nodiscard]] bool configure_board(); - bool open_stream(); + [[nodiscard]] bool open_stream(); - bool configure_stream(); + [[nodiscard]] bool configure_stream(); - bool init_buffers(); + [[nodiscard]] bool init_buffers(); - bool start_stream(); + [[nodiscard]] bool start_stream();operators/deltacast_videomaster/README.md (2)
36-39: Tighten parameter descriptions (clarity/terminology).
- “interleaved” -> “interlaced”
- Clarify overlay wording
-| `progressive` | bool | interleaved or progressive | true | -| `enable_overlay` | bool | Is overlay is add by card or not | false | +| `progressive` | bool | Interlaced (`false`) or progressive (`true`) | true | +| `enable_overlay` | bool | Whether the board/keyer composes overlay on TX | false |
48-59: Table formatting: avoid code fences/strikethrough for feature flags.Inside code blocks,
~~won’t render. Prefer plain text like “RDMA: off/on” and drop code fences for the table.-| deltacast_transmitter | DELTA-12G-elp-key 11 | TX0 (SDI) / ~~RDMA~~ | PASSED | +| deltacast_transmitter | DELTA-12G-elp-key 11 | TX0 (SDI) / RDMA: off | PASSED | -... similar rows ...Also consider moving the table out of any fenced block so Markdown renders properly.
applications/CMakeLists.txt (1)
41-43: Keep application list strictly alphabetical.Place
deltacast_receiverbeforedeltacast_transmitter.As per coding guidelines.
-add_holohub_application(deltacast_transmitter DEPENDS - OPERATORS deltacast_videomaster - ) - -add_holohub_application(deltacast_receiver DEPENDS +add_holohub_application(deltacast_receiver DEPENDS OPERATORS deltacast_videomaster ) +add_holohub_application(deltacast_transmitter DEPENDS + OPERATORS deltacast_videomaster + )applications/deltacast_receiver/python/CMakeLists.txt (1)
45-45: Add trailing newline at end of file.Most CMake files in the repository end with a newline character for consistency.
Apply this diff:
install( FILES deltacast_receiver.yaml DESTINATION bin/deltacast_receiver/python ) +applications/deltacast_receiver/python/metadata.json (1)
39-39: Add trailing newline at end of file.For consistency with other JSON metadata files in the repository.
Apply this diff:
} } +applications/deltacast_receiver/python/deltacast_receiver.py (1)
121-123: Consider extracting error message to constant.Ruff suggests avoiding long messages directly in exception raises for better maintainability.
Apply this diff:
+ CONFIG_NOT_FOUND_MSG = "Configuration file {config} does not exist at expected location. Use --config to specify the correct path." + # Ensure the configuration file exists if not os.path.exists(args.config): - raise FileNotFoundError( - f"Configuration file {args.config} does not exist at expected location. Use --config to specify the correct path." - ) + raise FileNotFoundError(CONFIG_NOT_FOUND_MSG.format(config=args.config))operators/deltacast_videomaster/videomaster_source.cpp (2)
121-123: Prefer centralized shutdown over direct VHD call.Call
_video_master_base->stop_stream()instead of rawVHD_StopStreamfor consistent cleanup.- VHD_StopStream(*_video_master_base->stream_handle()); + _video_master_base->stop_stream();
82-83: Remove unused variable.
bool success_b = true;is unused.- bool success_b = true;operators/deltacast_videomaster/videomaster_base.cpp (2)
102-119: Stop the stream before resetting the handle.
stop_stream()should callVHD_StopStreamwhen a stream is active.- if (_stream_handle) { - _stream_handle.reset(); - } + if (_stream_handle) { + (void)holoscan_log_on_error( + Deltacast::Helper::ApiSuccess{VHD_StopStream(*_stream_handle)}, + "Failed to stop stream"); + _stream_handle.reset(); + }
477-494: Use logical-and assignment for readability.Bitwise
&evaluates all sides; prefer&=with booleans (or&&=in C++23) to convey intent.- success_b = success_b & holoscan_log_on_error(Deltacast::Helper::ApiSuccess{ ... }); + success_b &= holoscan_log_on_error(Deltacast::Helper::ApiSuccess{ ... });Repeat for the next two assignments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (45)
.gitignore(1 hunks)applications/CMakeLists.txt(1 hunks)applications/deltacast_receiver/CMakeLists.txt(1 hunks)applications/deltacast_receiver/README.md(1 hunks)applications/deltacast_receiver/cpp/CMakeLists.txt(1 hunks)applications/deltacast_receiver/cpp/deltacast_receiver.yaml(1 hunks)applications/deltacast_receiver/cpp/main.cpp(1 hunks)applications/deltacast_receiver/cpp/metadata.json(1 hunks)applications/deltacast_receiver/python/CMakeLists.txt(1 hunks)applications/deltacast_receiver/python/deltacast_receiver.py(1 hunks)applications/deltacast_receiver/python/deltacast_receiver.yaml(1 hunks)applications/deltacast_receiver/python/metadata.json(1 hunks)applications/deltacast_transmitter/cpp/deltacast_transmitter.yaml(1 hunks)applications/deltacast_transmitter/cpp/main.cpp(1 hunks)applications/deltacast_transmitter/cpp/metadata.json(1 hunks)applications/deltacast_transmitter/python/deltacast_transmitter.py(1 hunks)applications/deltacast_transmitter/python/deltacast_transmitter.yaml(1 hunks)applications/deltacast_transmitter/python/metadata.json(1 hunks)applications/endoscopy_tool_tracking/cpp/README.md(1 hunks)applications/endoscopy_tool_tracking/cpp/endoscopy_tool_tracking.yaml(0 hunks)applications/endoscopy_tool_tracking/cpp/main.cpp(1 hunks)gxf_extensions/CMakeLists.txt(1 hunks)gxf_extensions/deltacast_videomaster/CMakeLists.txt(0 hunks)gxf_extensions/deltacast_videomaster/README.md(0 hunks)gxf_extensions/deltacast_videomaster/metadata.json(0 hunks)gxf_extensions/deltacast_videomaster/videomaster_base.hpp(0 hunks)gxf_extensions/deltacast_videomaster/videomaster_ext.cpp(0 hunks)gxf_extensions/deltacast_videomaster/videomaster_source.cpp(0 hunks)gxf_extensions/deltacast_videomaster/videomaster_source.hpp(0 hunks)gxf_extensions/deltacast_videomaster/videomaster_transmitter.cpp(0 hunks)gxf_extensions/deltacast_videomaster/videomaster_transmitter.hpp(0 hunks)operators/CMakeLists.txt(1 hunks)operators/deltacast_videomaster/CMakeLists.txt(2 hunks)operators/deltacast_videomaster/README.md(1 hunks)operators/deltacast_videomaster/api_helper(1 hunks)operators/deltacast_videomaster/metadata.json(2 hunks)operators/deltacast_videomaster/python/videomaster.cpp(5 hunks)operators/deltacast_videomaster/python/videomaster_pydoc.hpp(4 hunks)operators/deltacast_videomaster/videomaster_base.cpp(11 hunks)operators/deltacast_videomaster/videomaster_base.hpp(1 hunks)operators/deltacast_videomaster/videomaster_source.cpp(3 hunks)operators/deltacast_videomaster/videomaster_source.hpp(2 hunks)operators/deltacast_videomaster/videomaster_transmitter.cpp(3 hunks)operators/deltacast_videomaster/videomaster_transmitter.hpp(3 hunks)utilities/cli/container.py(1 hunks)
💤 Files with no reviewable changes (10)
- gxf_extensions/deltacast_videomaster/metadata.json
- gxf_extensions/deltacast_videomaster/README.md
- gxf_extensions/deltacast_videomaster/videomaster_source.hpp
- gxf_extensions/deltacast_videomaster/videomaster_ext.cpp
- gxf_extensions/deltacast_videomaster/CMakeLists.txt
- gxf_extensions/deltacast_videomaster/videomaster_transmitter.hpp
- gxf_extensions/deltacast_videomaster/videomaster_source.cpp
- gxf_extensions/deltacast_videomaster/videomaster_transmitter.cpp
- applications/endoscopy_tool_tracking/cpp/endoscopy_tool_tracking.yaml
- gxf_extensions/deltacast_videomaster/videomaster_base.hpp
🧰 Additional context used
📓 Path-based instructions (4)
applications/CMakeLists.txt
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Register applications in applications/CMakeLists.txt using add_holohub_application(...) and optional DEPENDS OPERATORS
Files:
applications/CMakeLists.txt
**/metadata.json
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/metadata.json: In metadata.json, within one of {application, operator, tutorial, benchmark, workflow, gxf_extension}, ensure there is a tags array and treat the first element as the category
The category (first tag) in metadata.json must be one of the approved categories: Benchmarking, Camera, Computer Vision and Perception, Converter, Deployment, Development, Extended Reality, Healthcare AI, Image Processing, Inference, Interoperability, Medical Imaging, Natural Language and Conversational AI, Networking and Distributed Computing, Optimization, Quantum Computing, Rendering, Robotics, Scheduler, Signal Processing, Streaming, Threading, Video, Video Capture, Visualization, XR
Files:
applications/deltacast_receiver/python/metadata.jsonoperators/deltacast_videomaster/metadata.jsonapplications/deltacast_transmitter/cpp/metadata.jsonapplications/deltacast_receiver/cpp/metadata.jsonapplications/deltacast_transmitter/python/metadata.json
operators/CMakeLists.txt
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Register operators in operators/CMakeLists.txt using add_holohub_operator(...) and optional DEPENDS EXTENSIONS
Files:
operators/CMakeLists.txt
gxf_extensions/CMakeLists.txt
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Register extensions in gxf_extensions/CMakeLists.txt using add_holohub_extension(...)
Files:
gxf_extensions/CMakeLists.txt
🧠 Learnings (6)
📚 Learning: 2025-10-22T16:16:16.226Z
Learnt from: CR
PR: nvidia-holoscan/holohub#0
File: CONTRIBUTING.md:0-0
Timestamp: 2025-10-22T16:16:16.226Z
Learning: Applies to applications/CMakeLists.txt : Register applications in applications/CMakeLists.txt using add_holohub_application(...) and optional DEPENDS OPERATORS
Applied to files:
applications/CMakeLists.txtoperators/CMakeLists.txt
📚 Learning: 2025-10-22T16:16:16.226Z
Learnt from: CR
PR: nvidia-holoscan/holohub#0
File: CONTRIBUTING.md:0-0
Timestamp: 2025-10-22T16:16:16.226Z
Learning: Applies to holohub/applications/*/CMakeLists.txt : Each application must provide a CMakeLists.txt for build system integration
Applied to files:
applications/CMakeLists.txt
📚 Learning: 2025-10-22T16:16:16.226Z
Learnt from: CR
PR: nvidia-holoscan/holohub#0
File: CONTRIBUTING.md:0-0
Timestamp: 2025-10-22T16:16:16.226Z
Learning: Applies to workflow/CMakeLists.txt : Register workflows in workflow/CMakeLists.txt using add_holohub_application(...) and optional DEPENDS OPERATORS
Applied to files:
applications/CMakeLists.txtoperators/CMakeLists.txt
📚 Learning: 2025-10-22T16:16:16.226Z
Learnt from: CR
PR: nvidia-holoscan/holohub#0
File: CONTRIBUTING.md:0-0
Timestamp: 2025-10-22T16:16:16.226Z
Learning: Applies to operators/CMakeLists.txt : Register operators in operators/CMakeLists.txt using add_holohub_operator(...) and optional DEPENDS EXTENSIONS
Applied to files:
applications/CMakeLists.txtoperators/CMakeLists.txtgxf_extensions/CMakeLists.txt
📚 Learning: 2025-10-22T16:16:16.226Z
Learnt from: CR
PR: nvidia-holoscan/holohub#0
File: CONTRIBUTING.md:0-0
Timestamp: 2025-10-22T16:16:16.226Z
Learning: Applies to gxf_extensions/CMakeLists.txt : Register extensions in gxf_extensions/CMakeLists.txt using add_holohub_extension(...)
Applied to files:
gxf_extensions/CMakeLists.txt
📚 Learning: 2025-10-22T16:16:16.226Z
Learnt from: CR
PR: nvidia-holoscan/holohub#0
File: CONTRIBUTING.md:0-0
Timestamp: 2025-10-22T16:16:16.226Z
Learning: Applies to holohub/gxf_extensions/*/CMakeLists.txt : Each GXF extension must provide a CMakeLists.txt for build system integration
Applied to files:
gxf_extensions/CMakeLists.txt
🧬 Code graph analysis (9)
applications/deltacast_receiver/cpp/main.cpp (1)
applications/adv_networking_bench/cpp/main.cpp (1)
config_path(130-130)
applications/deltacast_receiver/python/deltacast_receiver.py (1)
applications/deltacast_receiver/cpp/main.cpp (3)
holoscan(28-72)main(109-129)main(109-109)
operators/deltacast_videomaster/videomaster_transmitter.hpp (1)
operators/deltacast_videomaster/videomaster_source.hpp (2)
spec(41-41)op_input(45-46)
operators/deltacast_videomaster/videomaster_source.cpp (3)
operators/deltacast_videomaster/videomaster_transmitter.cpp (8)
initialize(85-90)initialize(85-85)start(92-127)start(92-92)compute(129-267)compute(129-130)stop(269-272)stop(269-269)operators/deltacast_videomaster/videomaster_source.hpp (2)
op_input(45-46)buffer(50-51)operators/deltacast_videomaster/videomaster_base.hpp (1)
result(52-53)
operators/deltacast_videomaster/videomaster_base.cpp (1)
operators/deltacast_videomaster/videomaster_base.hpp (15)
VideoMasterBase(41-42)_is_igpu(76-76)_board_handle(58-58)_video_format(74-74)_video_format(75-75)_video_information(62-64)_video_information(65-67)_gpu_buffers(68-68)_gpu_buffers(69-69)_system_buffers(70-70)_system_buffers(71-71)_slot_handles(72-72)_slot_handles(73-73)result(52-53)state(56-56)
operators/deltacast_videomaster/videomaster_base.hpp (1)
gxf_extensions/deltacast_videomaster/videomaster_base.hpp (1)
VideoMasterBase(35-83)
applications/deltacast_transmitter/python/deltacast_transmitter.py (2)
applications/endoscopy_tool_tracking/cpp/main.cpp (1)
holoscan(76-320)applications/deltacast_transmitter/cpp/main.cpp (1)
holoscan(33-56)
operators/deltacast_videomaster/videomaster_source.hpp (1)
operators/deltacast_videomaster/videomaster_transmitter.hpp (2)
spec(41-41)op_input(44-45)
operators/deltacast_videomaster/videomaster_transmitter.cpp (4)
operators/deltacast_videomaster/videomaster_source.cpp (8)
initialize(63-68)initialize(63-63)start(70-78)start(70-70)compute(80-157)compute(80-81)stop(159-162)stop(159-159)operators/deltacast_videomaster/videomaster_base.hpp (2)
sleep_ms(33-35)sleep_ms(33-33)operators/deltacast_videomaster/videomaster_source.hpp (2)
op_input(45-46)buffer(50-51)operators/deltacast_videomaster/videomaster_transmitter.hpp (1)
op_input(44-45)
🪛 Clang (14.0.6)
applications/deltacast_receiver/cpp/main.cpp
[error] 18-18: 'holoscan/holoscan.hpp' file not found
(clang-diagnostic-error)
operators/deltacast_videomaster/videomaster_base.hpp
[error] 19-19: 'array' file not found
(clang-diagnostic-error)
🪛 LanguageTool
applications/deltacast_receiver/README.md
[grammar] ~11-~11: Use a hyphen to join words.
Context: ...tructions See instructions from the top level README on how to build this applic...
(QB_NEW_EN_HYPHEN)
[grammar] ~13-~13: Use a hyphen to join words.
Context: ...with the following command, from the top level Holohub source directory: ```bash...
(QB_NEW_EN_HYPHEN)
🪛 Ruff (0.14.1)
applications/deltacast_receiver/python/deltacast_receiver.py
121-123: Avoid specifying long messages outside the exception class
(TRY003)
134-134: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (38)
.gitignore (1)
34-34: Appropriate addition to ignore VS Code workspace settings.Adding
.vscode/to the development environment section is a standard practice—the folder contains user-specific IDE settings, workspace configurations, and launch profiles that should not be version-controlled.operators/deltacast_videomaster/metadata.json (2)
22-24: Clarify the tested_versions reduction and version compatibility gap.The tested_versions has been significantly narrowed from
["0.5.0","2.9.0","3.0.0"]to only["3.6.0"], while theminimum_required_versionremains"0.5.0"(line 21). This creates a potential documentation inconsistency—the operator claims to support versions from 0.5.0 onward, but is only tested on 3.6.0.Please clarify the following:
- Is the tested_versions reduction intentional? If so, was this due to the native operator migration requiring a minimum version bump?
- If the native operator requires 3.6.0+, should
minimum_required_versionbe updated to reflect the actual minimum?- Have you verified compatibility with the older Holoscan SDK versions (0.5.0–3.0.0), or should these be removed from
minimum_required_version?
30-32: Metadata structure is correct.The
tagsarray is properly structured with"Camera"as the first element, which is in the approved categories list. Therequirementschange fromgxf_extensionsto an empty object aligns well with the GXF-to-native operator migration.utilities/cli/container.py (1)
220-222: LGTM! Deltacast x370 device support added correctly.The implementation follows the exact same pattern as the existing x380 SDI device handling above, correctly checking for device existence before mounting.
applications/deltacast_transmitter/python/deltacast_transmitter.py (2)
21-21: LGTM! Import correctly updated for native operator.The removal of
UnboundedAllocatoris appropriate sinceVideoMasterTransmitterOpas a native operator no longer requires an external pool allocator. The remaining imports (BlockMemoryPool,MemoryStorageType) are actively used in the code.
68-79: LGTM! Native operator correctly initialized without pool allocator.The
VideoMasterTransmitterOpinitialization correctly omits the pool parameter, consistent with the conversion to a native operator that manages its own memory internally. This matches the C++ implementation pattern shown in the relevant code snippets.applications/endoscopy_tool_tracking/cpp/main.cpp (2)
122-130: LGTM! Consistent pool allocator removal from VideoMasterSourceOp.The pool allocator argument has been correctly removed from
VideoMasterSourceOpwhile preserving all functional parameters (rdma, board, input, width, height, progressive, framerate). This is consistent with the native operator migration and the changes indeltacast_transmitter/cpp/main.cpp.
262-271: LGTM! Overlay transmitter correctly updated.The pool allocator argument has been properly removed from the
VideoMasterTransmitterOpoverlayer instance while maintaining all necessary configuration arguments includingenable_overlay. This change is consistent with the native operator migration applied throughout the VideoMaster components.applications/deltacast_transmitter/cpp/main.cpp (1)
50-52: The original review comment is reviewing a file outside this PR's scope.
applications/deltacast_transmitter/cpp/main.cppis not modified in this PR (0 matches in git diff). The PR actually modifies:
applications/deltacast_receiver/cpp/main.cpp(which correctly uses the new pattern without pool allocator)- Operator implementations in
operators/deltacast_videomaster/(videomaster_source.cpp, videomaster_transmitter.cpp, etc.)The original review comment references lines 50-52 of a file that hasn't changed. This review is invalid and should be disregarded. Focus verification on the actual modified files in the PR.
Likely an incorrect or invalid review comment.
applications/deltacast_transmitter/python/deltacast_transmitter.yaml (1)
37-37: Confirm & document framerate reduction 60 → 30Framerate in the transmitter YAML(s) is now 30 — confirm this is intentional and add the rationale to the PR description (e.g., performance, hardware limit, operator change). Reconcile operator defaults/docs and verify downstream consumers.
- Evidence found in repo: applications/deltacast_transmitter/python/deltacast_transmitter.yaml (framerate: 30) and applications/deltacast_transmitter/cpp/deltacast_transmitter.yaml (framerate: 30); receiver configs also use 30 (applications/deltacast_receiver/{cpp,python}).
- Operator note: operators/deltacast_videomaster source/docs still show default framerate = 60 — either update the operator/docs or document that the YAML intentionally overrides the operator default.
- Action: add a short rationale to the PR, confirm there are no downstream breakages (receivers/encoders/replayers), and update docs/operator defaults if this is meant to be the new standard.
applications/deltacast_transmitter/cpp/metadata.json (3)
22-26: Metadata update looks good.The tested versions for holoscan_sdk are correctly ordered and aligned with the PR's upgrade to native operators. The versions follow semantic versioning conventions.
30-34: VideoMaster SDK version updates are appropriately ordered.The tested versions are monotonically increasing and consistent with the version bump scope in the PR.
41-41: Tags comply with coding guidelines.The first tag "Streaming" is an approved category, and the additional tags provide useful context about the application. As per coding guidelines.
operators/deltacast_videomaster/api_helper (1)
1-1: Verify the submodule commit exists in the upstream repository and review for compatibility.The submodule reference itself is valid (commit hash format is correct and properly registered as mode 160000 in git), and the update aligns with the PR's GXF-to-native operator migration context for DELTACAST. However, due to sandbox limitations, the commit cannot be verified to exist in the upstream submodule repository, and the changes it contains cannot be inspected.
Manually verify:
- The commit
7b5539582c73f2cc06a6059a519fbfdfb554896fexists in the api_helper submodule repository- The changes in that commit are compatible with your native operator implementation
- The .gitmodules configuration (if added in this PR) correctly points to the api_helper repository URL
applications/deltacast_transmitter/python/metadata.json (1)
20-21: LGTM!The tested version updates for both Holoscan SDK (3.6.0) and VideoMaster SDK (6.32.0) appropriately reflect the upgraded dependencies in this PR.
Also applies to: 28-29
applications/deltacast_transmitter/cpp/deltacast_transmitter.yaml (1)
37-37: Verify the framerate change is intentional.The framerate has been changed from 25 to 30. Ensure this change aligns with your video format requirements (PAL vs NTSC standard). Note that the deltacast_receiver application also uses 30 fps, so this change may be for consistency.
applications/endoscopy_tool_tracking/cpp/README.md (1)
26-26: LGTM!The updated build instruction provides clearer guidance for building with the Deltacast VideoMaster operator, which aligns with the build system changes in this PR.
applications/deltacast_receiver/CMakeLists.txt (1)
16-19: LGTM!The CMake configuration is appropriately structured as a wrapper project with
LANGUAGES NONE, delegating to the C++ subdirectory.applications/deltacast_receiver/cpp/metadata.json (1)
35-35: LGTM! Category tag compliant.The tags array correctly uses "Video" as the first element (category), which is in the approved categories list. The tags accurately describe this video receiver application.
As per coding guidelines.
applications/deltacast_receiver/cpp/main.cpp (3)
28-72: LGTM!The pipeline composition is well-structured. Operator creation order (source → drop_alpha → format_converter → visualizer) matches the pipeline flow, making the code easy to follow. The memory pool configurations appropriately use the calculated
source_block_sizeand RDMA-dependent block counts.
76-107: LGTM!The argument parsing logic is correct and uses standard getopt_long patterns. The while loop condition
while ((c = getopt_long(...)) != -1 && c != '?')is idiomatic C/C++ and properly handles both end-of-arguments (-1) and errors ('?').
121-123: Good use of weakly_canonical() for safety.The code correctly uses
weakly_canonical()instead ofcanonical(), which addresses the past review concern about potentialfilesystem_errorexceptions. This is the safer choice when the path might not exist.applications/CMakeLists.txt (1)
41-43: Registration looks correct.
add_holohub_application(... DEPENDS OPERATORS deltacast_videomaster)is the right pattern.Based on learnings.
operators/CMakeLists.txt (1)
24-24: LGTM! Correct migration from GXF extension to native operator.The removal of
DEPENDS EXTENSIONS deltacast_videomastercorrectly reflects the transition of the deltacast_videomaster operator from a GXF extension to a native Holoscan operator.operators/deltacast_videomaster/python/videomaster_pydoc.hpp (4)
45-45: Good fix! Typo corrected.Corrected "baords" → "boards" in the RX channel documentation.
94-94: Good fix! Typo corrected.Corrected "baords" → "boards" in the TX channel documentation.
104-104: Good fix! Grammar corrected.Fixed article usage from "a overlay" → "an overlay".
129-129: Good fix! Header guard corrected.Updated the endif guard macro from
PYHOLOSCAN_OPERATORS_AJA_SOURCE_PYDOC_HPPtoPYHOLOSCAN_OPERATORS_VIDEOMASTER_SOURCE_PYDOC_HPPto match the actual file purpose.applications/deltacast_receiver/python/CMakeLists.txt (1)
17-17: LGTM! Correct placement of find_package.The
find_package(holoscan ...)is correctly placed outside theBUILD_TESTINGblock, ensuring it's available for both test and install targets.applications/deltacast_receiver/cpp/deltacast_receiver.yaml (1)
1-46: LGTM! Well-structured configuration.The YAML configuration provides sensible defaults for the Deltacast receiver pipeline with clear sections for video capture, format conversion, and visualization.
applications/deltacast_receiver/python/metadata.json (2)
16-18: LGTM! Version requirements are consistent.The minimum required Holoscan SDK version (3.0.0) is appropriately lower than the tested version (3.6.0).
31-31: LGTM! Category tag is valid.The first tag "Video" is from the approved category list per coding guidelines.
operators/deltacast_videomaster/CMakeLists.txt (2)
18-19: LGTM! C++17 standard set appropriately.Setting C++17 standard is consistent with modern CMake practices and Holoscan requirements.
53-62: LGTM! Correctly changed to PUBLIC linkage.Changing library dependencies from
PRIVATEtoPUBLICis appropriate for the transition to a native Holoscan operator, ensuring that dependent applications inherit the necessary link dependencies.applications/deltacast_receiver/cpp/CMakeLists.txt (1)
15-40: LGTM! Well-structured CMake configuration.The CMake file correctly:
- Requires minimum CMake 3.20
- Finds the Holoscan package
- Creates the executable with necessary library dependencies
- Ensures the YAML configuration is copied before building the executable via the custom target and dependency
applications/deltacast_receiver/python/deltacast_receiver.py (2)
134-136: LGTM! Acceptable error handling for top-level main.Catching broad
Exceptionis acceptable here for top-level error handling in the application entry point, and the use ofsys.exit(1)is correct.
91-95: LGTM! Pipeline flow matches C++ implementation.The operator connections correctly mirror the C++ implementation: source → drop_alpha_channel_converter → format_converter → holoviz.
operators/deltacast_videomaster/videomaster_transmitter.hpp (1)
33-36: Docstring correction looks good.Accurately describes a transmitter that sends video to the board.
thomasdethier
force-pushed
the
upgrade-deltacast-integration
branch
from
5608e12 to
92a59dc
Compare
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This review covers only the changes made since the last review, not the entire PR. The latest changes address code quality issues across the Deltacast operator integration: (1) metadata files now correctly specify minimum SDK version 3.0.0 (aligning with tested version 3.6.0) and remove misleading "Healthcare AI" tags, (2) the Python receiver application imports sys for proper sys.exit(1) usage, (3) source headers eliminate GXF dependencies and redundant includes to complete the migration to native Holoscan operators, (4) member variables are now initialized at declaration to prevent undefined behavior, (5) documentation improvements include fixing docstring typos ("boards" not "baords") and correcting a transmitter's docstring that previously described it as a receiver, and (6) CUDA error checking added to buffer cleanup operations for improved robustness. These changes complete the architectural shift away from GXF-based operators and bring the Deltacast integration in line with modern C++ practices and project conventions.
Important Files Changed
| Filename | Score | Overview |
|---|---|---|
| applications/deltacast_receiver/cpp/metadata.json | 5/5 | Updated minimum SDK version from 0.5.0 to 3.0.0 and removed misleading "Healthcare AI" tag |
| applications/deltacast_receiver/python/metadata.json | 5/5 | Updated minimum SDK version to 3.0.0, replaced healthcare tags with "Display", added trailing newline |
| applications/deltacast_receiver/python/CMakeLists.txt | 2.5/5 | Moved find_package(holoscan) outside BUILD_TESTING block; potential issue with GXF_LIB_DIR scope |
| operators/deltacast_videomaster/videomaster_transmitter.hpp | 5/5 | Corrected docstring from "get" to "transmit" and initialized member variables at declaration |
| operators/deltacast_videomaster/videomaster_base.cpp | 2/5 | Added CUDA error checking to cleanup; constructor early-return issue persists |
| operators/deltacast_videomaster/videomaster_base.hpp | 5/5 | Removed GXF include dependency, converted macro to inline function for type safety |
| operators/deltacast_videomaster/videomaster_source.hpp | 5/5 | Removed redundant includes and namespace pollution, initialized member variables at declaration |
| applications/deltacast_receiver/python/deltacast_receiver.py | 4/5 | Added sys import for proper sys.exit(1) usage instead of built-in exit(1) |
| operators/deltacast_videomaster/python/videomaster_pydoc.hpp | 5/5 | Fixed three documentation typos: "baords" → "boards", "a overlay" → "an overlay", header guard comment |
| operators/deltacast_videomaster/videomaster_source.cpp | 5/5 | Added using declarations for readability and CUDA error checking for memcpy operations with proper exception handling |
| applications/deltacast_receiver/README.md | 5/5 | Added documentation for Python variant run instructions to match repository patterns |
Confidence score: 3/5
- This PR requires careful review of the CMakeLists and constructor error handling patterns before merging
- Score reflects two unresolved issues: (1)
find_packageplacement in Python CMakeLists may cause GXF_LIB_DIR to be undefined when testing is disabled, breaking PYTHONPATH setup, and (2) VideoMasterBase constructor continues execution after CUDA initialization failures (lines 84-85, 93-95 in videomaster_base.cpp), leaving the object in a partially constructed state that will cause undefined behavior in subsequent method calls—this should throw an exception rather than return early - Pay close attention to applications/deltacast_receiver/python/CMakeLists.txt and operators/deltacast_videomaster/videomaster_base.cpp constructor error handling
Additional Comments (1)
-
operators/deltacast_videomaster/python/videomaster_pydoc.hpp, line 130 (link)style: trailing empty line - most source files in the repo end at the last line without an extra newline
13 files reviewed, 2 comments
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (9)
applications/deltacast_receiver/python/deltacast_receiver.py (2)
44-45: Verify the buffer size calculation.The calculation
width * height * 4 * 4allocates 4× the standard size for an RGBA buffer (which requireswidth * height * 4bytes). The C++ implementation inapplications/deltacast_receiver/cpp/main.cpp(line 33) uses the same formula, suggesting this may be intentional across both implementations.However, without documentation explaining the extra factor of 4, it's unclear whether this is:
- Intentional buffering headroom for the DELTACAST hardware
- A calculation error replicated in both versions
Please verify with the DELTACAST SDK documentation or add a comment explaining the rationale.
54-65: Remove unsupported pool parameter from VideoMasterSourceOp.The Python binding for
VideoMasterSourceOpdoes not accept apoolparameter. According to the binding definition inoperators/deltacast_videomaster/python/videomaster.cpp, only these parameters are exposed:fragment,rdma,board,input,width,height,progressive,framerate, andname.Additionally, the C++ implementation in
applications/deltacast_receiver/cpp/main.cpp(lines 36-44) does not pass a pool argument to the source operator, confirming this parameter should not be used.Passing this unsupported parameter will cause a runtime error.
Apply this diff to remove the unsupported parameter:
source = VideoMasterSourceOp( self, name="deltacast_source", - pool=UnboundedAllocator(self, name="source_pool"), rdma=use_rdma, board=deltacast_kwargs.get("board", 0), input=deltacast_kwargs.get("input", 0), width=width, height=height, progressive=deltacast_kwargs.get("progressive", True), framerate=deltacast_kwargs.get("framerate", 30), )operators/deltacast_videomaster/videomaster_transmitter.hpp (1)
25-27: Remove redundant include.
operator.hppalready includesoperator_spec.hpp.-#include "holoscan/core/operator.hpp" -#include "holoscan/core/operator_spec.hpp" +#include "holoscan/core/operator.hpp"operators/deltacast_videomaster/videomaster_source.hpp (1)
49-52: Fix buffer lifetime: carry slot handle and re-queue on downstream release.
transmit_buffer_datamust accept theHANDLEand install a release callback; otherwise the slot is re-queued while downstream still reads from the buffer, risking corruption.Apply this diff:
- void transmit_buffer_data(void* buffer, uint32_t buffer_size, - OutputContext& op_output, ExecutionContext& context); + void transmit_buffer_data(HANDLE slot_handle, + void* buffer, + size_t buffer_size, + OutputContext& op_output, + ExecutionContext& context);operators/deltacast_videomaster/videomaster_source.cpp (1)
150-157: Defer slot re-queue until downstream release; add release callback and checkwrapMemory.Re-queuing right after emit risks HW overwriting memory still in use downstream. Pass
slot_handle, install a release callback, and re-queue on buffer release. Also handle errors by returning the slot.- HOLOSCAN_LOG_DEBUG("Transmit slot buffer {} - size: {} bytes", - (void*)buffer, buffer_size); - transmit_buffer_data(buffer, buffer_size, op_output, context); - - HOLOSCAN_LOG_DEBUG("Queue slot"); - VHD_QueueInSlot(slot_handle); - _slot_count++; + HOLOSCAN_LOG_DEBUG("Transmit slot buffer {} - size: {} bytes", + (void*)buffer, buffer_size); + transmit_buffer_data(slot_handle, buffer, static_cast<size_t>(buffer_size), op_output, context); + _slot_count++;-void VideoMasterSourceOp::transmit_buffer_data(void* buffer, uint32_t buffer_size, - OutputContext& op_output, - ExecutionContext& context) { +namespace { +struct SlotReleaseCtx { + HANDLE slot{}; + VideoMasterBase* base{}; +}; +static void ReleaseSlot(void* arg) { + auto* ctx = static_cast<SlotReleaseCtx*>(arg); + if (!ctx) return; + (void)ctx->base->holoscan_log_on_error( + Deltacast::Helper::ApiSuccess{VHD_QueueInSlot(ctx->slot)}, + "Failed to queue input slot on release"); + delete ctx; +} +} // namespace + +void VideoMasterSourceOp::transmit_buffer_data(HANDLE slot_handle, + void* buffer, + size_t buffer_size, + OutputContext& op_output, + ExecutionContext& context) { if (!_use_rdma) { @@ - if (cuda_status != cudaSuccess) { - throw std::runtime_error(std::string("cudaMemcpy failed: ") + - cudaGetErrorString(cuda_status)); - } + if (cuda_status != cudaSuccess) { + // Return slot to avoid stalling pipeline + (void)_video_master_base->holoscan_log_on_error( + Deltacast::Helper::ApiSuccess{VHD_QueueInSlot(slot_handle)}, + "Failed to queue input slot after cudaMemcpy error"); + throw std::runtime_error(std::string("cudaMemcpy failed: ") + + cudaGetErrorString(cuda_status)); + } buffer = gpu_buffer; } auto video_output = nvidia::gxf::Entity::New(context.context()); if (!video_output) { - throw std::runtime_error("Failed to allocate video output; terminating."); + (void)_video_master_base->holoscan_log_on_error( + Deltacast::Helper::ApiSuccess{VHD_QueueInSlot(slot_handle)}, + "Failed to queue input slot after entity alloc failure"); + throw std::runtime_error("Failed to allocate video output; terminating."); } @@ auto video_buffer = video_output.value().add<nvidia::gxf::VideoBuffer>(); if (!video_buffer) { - throw std::runtime_error("Failed to allocate video buffer; terminating."); + (void)_video_master_base->holoscan_log_on_error( + Deltacast::Helper::ApiSuccess{VHD_QueueInSlot(slot_handle)}, + "Failed to queue input slot after video buffer alloc failure"); + throw std::runtime_error("Failed to allocate video buffer; terminating."); } @@ - auto storage_type = _video_master_base->is_igpu() + auto storage_type = _video_master_base->is_igpu() ? nvidia::gxf::MemoryStorageType::kHost : nvidia::gxf::MemoryStorageType::kDevice; - video_buffer.value()->wrapMemory(info, buffer_size, storage_type, buffer, nullptr); + // Re-queue the slot when downstream releases this buffer + auto* ctx = new SlotReleaseCtx{slot_handle, _video_master_base.get()}; + auto wrap_result = video_buffer.value()->wrapMemory( + info, buffer_size, storage_type, buffer, &ReleaseSlot, static_cast<void*>(ctx)); + if (!wrap_result) { + delete ctx; + (void)_video_master_base->holoscan_log_on_error( + Deltacast::Helper::ApiSuccess{VHD_QueueInSlot(slot_handle)}, + "Failed to queue input slot after wrapMemory failure"); + throw std::runtime_error("wrapMemory failed"); + }Also applies to: 164-213
operators/deltacast_videomaster/videomaster_transmitter.cpp (3)
151-152: Simplify double negation logic for clarity.The condition
!(_video_master_base->video_format() != Deltacast::Helper::VideoFormat{})uses confusing double negation to check whether the stream has not been started (i.e., video format is still at its default value).Apply this diff to improve readability:
- } else if (!(_video_master_base->video_format() != - Deltacast::Helper::VideoFormat{})) { // stream not started yet + } else if (_video_master_base->video_format() == + Deltacast::Helper::VideoFormat{}) { // stream not started yetOr extract to a named helper method for even better clarity:
bool is_stream_configured() const { return _video_master_base->video_format() != Deltacast::Helper::VideoFormat{}; }
153-155: Fix log format to avoid awkward output.The format string
"{}p"combined with"progressive"/"interlaced"produces output like "progressivep" or "interlacedp".Apply this diff to fix the format:
- HOLOSCAN_LOG_INFO("Configuring overlay mode: {}x{} {}p @{}Hz", + HOLOSCAN_LOG_INFO("Configuring overlay mode: {}x{} {} @ {} Hz", _width, _height, - _progressive ? "progressive" : "interlaced", _framerate); + _progressive ? "p" : "i", _framerate);
286-320: Validate_channel_indexbefore map lookups to prevent exceptions.Multiple
.at()calls on the keyer mapping tables (lines 290, 299, 308, 316) will throwstd::out_of_rangeif_channel_indexis outside the valid range (0-3 for keyer maps). Consider adding explicit validation at the start of the method.Apply this diff to add validation:
bool VideoMasterTransmitterOp::configure_board_for_overlay() { HOLOSCAN_LOG_INFO("Configuring board for overlay with keyer on channel {}", _channel_index); + + // Validate channel index for keyer operations (valid range: 0-3) + if (_channel_index > 3) { + HOLOSCAN_LOG_ERROR("Invalid channel_index {} for keyer configuration (valid range: 0-3)", + _channel_index); + return false; + } if (!_video_master_base->video_information()->configure_sync(Verification script to check if there are any bounds checks elsewhere:
#!/bin/bash # Search for _channel_index validation in the codebase rg -nP --type=cpp '_channel_index\s*[<>=]' operators/deltacast_videomaster -C3operators/deltacast_videomaster/videomaster_base.hpp (1)
81-81: Initialize_channel_typeto prevent undefined behavior.The member
_channel_typeis declared without an in-class initializer and is not included in the constructor's member initializer list (seevideomaster_base.cpp:75-78). It is only assigned later inconfigure_board(), leaving it uninitialized immediately after construction.Apply this diff to add a safe default value:
- VHD_CHANNELTYPE _channel_type; + VHD_CHANNELTYPE _channel_type{}; // Zero-initialize to prevent undefined behaviorOr initialize it in the constructor's member initializer list if a specific default value from the VideoMaster API is more appropriate.
🧹 Nitpick comments (11)
applications/deltacast_receiver/README.md (1)
11-11: Fix hyphenation in compound adjectives.Lines 11 and 13 use "top level" which should be hyphenated as "top-level" when used as a compound adjective modifying a noun.
- See instructions from the top level README on how to build this application. + See instructions from the top-level README on how to build this application.- This can be done with the following command, from the top level Holohub source directory: + This can be done with the following command, from the top-level Holohub source directory:Also applies to: 13-13
applications/deltacast_receiver/python/CMakeLists.txt (1)
45-45: Add newline at end of file.CMake files in this repository conventionally end with a newline. Please add a blank line after the closing parenthesis.
applications/deltacast_receiver/python/deltacast_receiver.py (2)
121-123: Consider shortening the exception message.Static analysis suggests avoiding long messages in exception instantiation. While the current message is helpful, you could either:
- Shorten it to just the essential information
- Define a custom exception class if this pattern appears frequently
Based on learnings: As per coding guidelines (Ruff TRY003)
134-134: Consider catching specific exceptions.The broad
except Exceptioncatches all exceptions, which can mask unexpected errors. For better error handling, consider catching specific expected exceptions (e.g.,FileNotFoundError,ValueError,RuntimeError) separately.However, for a top-level main function in a demo application, catching all exceptions to provide user-friendly error messages is a common and acceptable pattern.
Based on learnings: As per coding guidelines (Ruff BLE001)
operators/deltacast_videomaster/videomaster_source.hpp (1)
27-28: Reduce header dependencies (forward declare base, include in .cpp).Saves rebuild time and avoids coupling.
-#include "videomaster_base.hpp" +// forward declaration to avoid heavy header in public interface +class VideoMasterBase;Ensure
videomaster_source.cppincludesvideomaster_base.hpp.applications/deltacast_receiver/cpp/main.cpp (2)
34-36: Right-size pool blocks; current value is likely 4× oversized.
width * height * 4 * 4assumes 16 B/px. For RGBA8 it should bewidth * height * 4. Oversizing wastes memory.- uint64_t source_block_size = width * height * 4 * 4; + // RGBA8 default; adjust if config requests float32 + uint64_t source_block_size = width * height * 4;If the format converter outputs float32 RGBA, keep
* 16and add a comment. Please confirm the intended output format in your YAML.
89-96: Help should exit with status 0.Printing usage is a successful path; main currently returns 1.
Options:
- Make
parse_argumentsreturn a tri-state (ok/usage/error).- Or call
std::exit(0)in the-hbranch.operators/deltacast_videomaster/videomaster_transmitter.hpp (1)
28-29: Reduce header coupling by forward-declaringVideoMasterBase.Move
#include "videomaster_base.hpp"to the .cpp and forward-declare here.-#include "videomaster_base.hpp" +class VideoMasterBase;operators/deltacast_videomaster/videomaster_source.cpp (2)
82-83: Simplify conditions and remove unused variable.
- Remove unused
success_b.- Replace double-negation equality with a direct check.
- Simplify timeout handling.
- bool success_b = true; + // no-op @@ - } else if (!(_video_master_base->video_format() != - Deltacast::Helper::VideoFormat{})) { // start stream + } else if (_video_master_base->video_format() == Deltacast::Helper::VideoFormat{}) { // start stream @@ - VHD_StopStream(*_video_master_base->stream_handle()); throw std::runtime_error("Input signal does not match configuration");- if (api_result != VHDERR_NOERROR && api_result == VHDERR_TIMEOUT) { + if (api_result == VHDERR_TIMEOUT) { HOLOSCAN_LOG_INFO("Timeout"); return; }Also applies to: 90-101, 130-136
20-26: Remove unused includes.
allocator.hppandcuda_stream_pool.hppare not used here.-#include "holoscan/core/resources/gxf/allocator.hpp" -#include "holoscan/core/resources/gxf/cuda_stream_pool.hpp"Also applies to: 32-34
operators/deltacast_videomaster/videomaster_base.cpp (1)
477-494: Improve boolean accumulation readability.Use
&=or explicit booleans instead of bitwise&onbool.- success_b = success_b & holoscan_log_on_error(Deltacast::Helper::ApiSuccess{ + success_b &= holoscan_log_on_error(Deltacast::Helper::ApiSuccess{ @@ - success_b = success_b & holoscan_log_on_error(Deltacast::Helper::ApiSuccess{ + success_b &= holoscan_log_on_error(Deltacast::Helper::ApiSuccess{ @@ - success_b = success_b & holoscan_log_on_error(Deltacast::Helper::ApiSuccess{ + success_b &= holoscan_log_on_error(Deltacast::Helper::ApiSuccess{
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
applications/deltacast_receiver/README.md(1 hunks)applications/deltacast_receiver/cpp/main.cpp(1 hunks)applications/deltacast_receiver/cpp/metadata.json(1 hunks)applications/deltacast_receiver/python/CMakeLists.txt(1 hunks)applications/deltacast_receiver/python/deltacast_receiver.py(1 hunks)applications/deltacast_receiver/python/metadata.json(1 hunks)operators/deltacast_videomaster/python/videomaster_pydoc.hpp(4 hunks)operators/deltacast_videomaster/videomaster_base.cpp(11 hunks)operators/deltacast_videomaster/videomaster_base.hpp(1 hunks)operators/deltacast_videomaster/videomaster_source.cpp(3 hunks)operators/deltacast_videomaster/videomaster_source.hpp(2 hunks)operators/deltacast_videomaster/videomaster_transmitter.cpp(3 hunks)operators/deltacast_videomaster/videomaster_transmitter.hpp(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- applications/deltacast_receiver/cpp/metadata.json
🚧 Files skipped from review as they are similar to previous changes (2)
- operators/deltacast_videomaster/python/videomaster_pydoc.hpp
- applications/deltacast_receiver/python/metadata.json
🧰 Additional context used
🧬 Code graph analysis (7)
operators/deltacast_videomaster/videomaster_source.cpp (3)
operators/deltacast_videomaster/videomaster_transmitter.cpp (8)
initialize(85-90)initialize(85-85)start(92-127)start(92-92)compute(129-267)compute(129-130)stop(269-272)stop(269-269)operators/deltacast_videomaster/videomaster_source.hpp (2)
op_input(45-46)buffer(50-51)operators/deltacast_videomaster/videomaster_base.hpp (1)
result(52-53)
applications/deltacast_receiver/python/deltacast_receiver.py (1)
applications/deltacast_receiver/cpp/main.cpp (3)
holoscan(28-71)main(108-128)main(108-108)
operators/deltacast_videomaster/videomaster_source.hpp (1)
operators/deltacast_videomaster/videomaster_transmitter.hpp (2)
spec(41-41)op_input(44-45)
applications/deltacast_receiver/cpp/main.cpp (1)
applications/adv_networking_bench/cpp/main.cpp (1)
config_path(130-130)
operators/deltacast_videomaster/videomaster_base.cpp (2)
operators/deltacast_videomaster/videomaster_base.hpp (15)
VideoMasterBase(41-42)_is_igpu(76-76)_board_handle(58-58)_video_format(74-74)_video_format(75-75)_video_information(62-64)_video_information(65-67)_gpu_buffers(68-68)_gpu_buffers(69-69)_system_buffers(70-70)_system_buffers(71-71)_slot_handles(72-72)_slot_handles(73-73)result(52-53)state(56-56)applications/matlab_gpu_coder/matlab_beamform/main.cpp (1)
cudaFree(98-108)
operators/deltacast_videomaster/videomaster_transmitter.hpp (1)
operators/deltacast_videomaster/videomaster_source.hpp (2)
spec(41-41)op_input(45-46)
operators/deltacast_videomaster/videomaster_transmitter.cpp (3)
operators/deltacast_videomaster/videomaster_source.cpp (8)
initialize(63-68)initialize(63-63)start(70-78)start(70-70)compute(80-157)compute(80-81)stop(159-162)stop(159-159)operators/deltacast_videomaster/videomaster_base.hpp (2)
sleep_ms(33-35)sleep_ms(33-33)operators/deltacast_videomaster/videomaster_transmitter.hpp (1)
op_input(44-45)
🪛 Clang (14.0.6)
applications/deltacast_receiver/cpp/main.cpp
[error] 18-18: 'holoscan/holoscan.hpp' file not found
(clang-diagnostic-error)
operators/deltacast_videomaster/videomaster_base.hpp
[error] 19-19: 'array' file not found
(clang-diagnostic-error)
🪛 LanguageTool
applications/deltacast_receiver/README.md
[grammar] ~11-~11: Use a hyphen to join words.
Context: ...tructions See instructions from the top level README on how to build this applic...
(QB_NEW_EN_HYPHEN)
[grammar] ~13-~13: Use a hyphen to join words.
Context: ...with the following command, from the top level Holohub source directory: ```bash...
(QB_NEW_EN_HYPHEN)
🪛 Ruff (0.14.1)
applications/deltacast_receiver/python/deltacast_receiver.py
121-123: Avoid specifying long messages outside the exception class
(TRY003)
134-134: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (4)
applications/deltacast_receiver/README.md (1)
29-35: Good: Python variant documentation is now included.This addresses the previous review comment about missing Python documentation. The README now properly documents both C++ and Python run instructions alongside each other, consistent with the sibling
deltacast_transmitterapplication.applications/deltacast_receiver/cpp/main.cpp (1)
18-18: The review comment is incorrect; no changes are needed.The CMake configuration for
deltacast_receiveris correctly set up and follows the standard pattern used throughout the codebase. Thefind_package(holoscan 1.0 REQUIRED CONFIG ...)call successfully locates the Holoscan SDK, and thetarget_link_libraries()linking toholoscan::coreautomatically propagates all necessary include directories to the target via CMake's transitive dependencies mechanism. No explicittarget_include_directoriesis required or used for this purpose. The static analysis warning is likely a tool limitation or false positive, as the identical configuration pattern works across 70+ applications in the codebase without issues.Likely an incorrect or invalid review comment.
operators/deltacast_videomaster/videomaster_base.cpp (1)
97-100: Incorrect technical premise; review comment should be disregarded.The review's claim that "pinned host memory is incompatible with GPUDirect RDMA" is incorrect. On Tegra / some integrated-GPU platforms you often must use pinned host memory (cudaHostAlloc) or allocate host-mapped memory and then obtain the device pointer via cudaHostGetDevicePointer; the device must report capability to use host pointers for registered memory (cudaDevAttrCanUseHostPointerForRegisteredMem). Pinned host memory is often the required allocation method for iGPU RDMA, not forbidden.
The actual concern is that the code (lines 287–289) uses
cudaHostAllocon iGPU without validating whether the device supports host pointer registration for RDMA. The code should querycudaDevAttrCanUseHostPointerForRegisteredMemto determine if RDMA can be safely enabled, not disable it unconditionally. Alternatively, allocate device memory for the video buffer on iGPU instead of host memory.The proposed fix is overly conservative and based on a false technical premise.
Likely an incorrect or invalid review comment.
operators/deltacast_videomaster/videomaster_transmitter.cpp (1)
243-256: Good defensive validation before memory operations.The size validation at lines 243-247 and the subsequent CUDA error checking properly address the concern raised in past reviews about potential buffer overflow. The implementation correctly validates that
tensor->nbytes()does not exceedbuffer_sizebefore copying.
|
Hi @laurent-radoux @thomasdethier, I’d like to help move this PR forward so we can merge it as soon as possible and get this valuable update to the DELTACAST operators. Could you please review and address the comments from the AI reviewer? Please feel free to just mark them resolve or ask for our feedback if they are not relevant or can be skipped. Also, would it be possible for you to enable “Allow edits by maintainers” so we can help resolve the remaining issues along the way? Thanks! |
I'll be available to review the rest of the issues in 2 weeks. |
|
Due to a GitHub bug, “Allow edits by maintainers” cannot be set : https://github.com/orgs/community/discussions/5634 |
There was a problem hiding this comment.
Pull request overview
This PR converts the DELTACAST VideoMaster operators from GXF-based extensions to native Holoscan operators and introduces a new standalone receiver application for simplified video display.
Key Changes:
- Migrated VideoMaster operators from GXF codelets to native Holoscan operators
- Removed GXF extension dependencies and centralized VideoMaster logic into a shared
VideoMasterBaseclass - Added dedicated
deltacast_receiverapplication (C++ and Python) for video capture and visualization - Standardized framerate to 30 fps across applications
- Updated device detection to include
delta-x370SDI devices
Reviewed changes
Copilot reviewed 44 out of 45 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| operators/deltacast_videomaster/*.{hpp,cpp} | Converted from GXF operators to native operators with shared VideoMasterBase class |
| gxf_extensions/deltacast_videomaster/* | Removed GXF extension files as operators are now native |
| applications/deltacast_receiver/* | New standalone receiver application for video display |
| applications/deltacast_transmitter/* | Updated to use native operators and remove GXF extension loading |
| applications/endoscopy_tool_tracking/cpp/* | Updated to use native operators and remove pool allocator parameters |
| utilities/cli/container.py | Added support for delta-x370 device mounting |
| operators/deltacast_videomaster/metadata.json | Updated version and removed GXF extension requirements |
thomasdethier
left a comment
thomasdethier
left a comment
There was a problem hiding this comment.
FIxed most issues of review
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
operators/deltacast_videomaster/CMakeLists.txt (1)
40-61: Useadd_holohub_operator()instead ofadd_library()for operator build integration.The coding guidelines require that operator CMakeLists.txt files use
add_holohub_operator()for proper build system integration. This file currently usesadd_library()directly, which bypasses the standard holohub operator build configuration.Based on coding guidelines, this is the required pattern for operators.
Refactor to use the holohub operator build macro:
-add_library(videomaster SHARED - videomaster_base.hpp - videomaster_base.cpp - videomaster_source.hpp - videomaster_source.cpp - videomaster_transmitter.hpp - videomaster_transmitter.cpp - ) -add_library(holoscan::videomaster ALIAS videomaster) - -target_include_directories(videomaster INTERFACE ${CMAKE_CURRENT_SOURCE_DIR}) -target_compile_definitions(videomaster INTERFACE DELTACAST_VIDEOMASTER) -target_link_libraries(videomaster -PUBLIC - VideoMasterAPIHelper - VideoMasterHD::Core - CUDA::cudart - CUDA::cuda_driver - GXF::multimedia - GXF::std - holoscan::core -) +add_holohub_operator(videomaster + SOURCES + videomaster_base.hpp + videomaster_base.cpp + videomaster_source.hpp + videomaster_source.cpp + videomaster_transmitter.hpp + videomaster_transmitter.cpp + LIBRARIES + VideoMasterAPIHelper + VideoMasterHD::Core + CUDA::cudart + CUDA::cuda_driver + GXF::multimedia + GXF::std + holoscan::core +)Note: Verify the exact syntax and available parameters for
add_holohub_operator()in the holohub CMake modules, as you may need to adjust compile definitions and interface directories accordingly.
♻️ Duplicate comments (3)
operators/deltacast_videomaster/CMakeLists.txt (1)
30-35: Past review comment: Addapi_helperto.gitignoreor removeSOURCE_DIR.This issue was previously flagged: FetchContent with
SOURCE_DIRin the source tree will pollute version control unless properly ignored.As noted in the past review, either:
- Add
operators/deltacast_videomaster/api_helper/to.gitignore, or- Remove the
SOURCE_DIRline to use FetchContent's default build-directory behavior (recommended)operators/deltacast_videomaster/videomaster_transmitter.cpp (2)
285-319: Validate_channel_indexbefore using.at()on maps.The calls to
.at(_channel_index)on lines 289, 298, and 307 will throwstd::out_of_rangeif_channel_indexis invalid. Surface a clear error message instead of allowing an exception to propagate.Add validation before the first
.at()call:bool VideoMasterTransmitterOp::configure_board_for_overlay() { HOLOSCAN_LOG_INFO("Configuring board for overlay with keyer on channel {}", _channel_index); + // Validate channel index before using with maps + if (_channel_index >= 4) { // Maps support channels 0-3 + HOLOSCAN_LOG_ERROR("Invalid channel_index {} for overlay configuration (must be 0-3)", + _channel_index); + return false; + } + if (!_video_master_base->video_information()->configure_sync( _video_master_base->board_handle(), _channel_index)) { return false; }Apply similar validation for
id_to_keyer_video_output.at(_channel_index)at line 315.
151-152: Simplify double-negation logic.The condition
!(_video_master_base->video_format() != Deltacast::Helper::VideoFormat{})uses confusing double negation. The comment "stream not started yet" indicates you're checking for equality.Apply this diff:
- } else if (!(_video_master_base->video_format() != - Deltacast::Helper::VideoFormat{})) { // stream not started yet + } else if (_video_master_base->video_format() == + Deltacast::Helper::VideoFormat{}) { // stream not started yet
🧹 Nitpick comments (1)
applications/deltacast_receiver/python/deltacast_receiver.py (1)
97-135: LGTM with optional style improvements.The parse_config() and main() functions are well-structured with proper error handling. The broad Exception catch in main() is appropriate for top-level error handling, and sys.exit(1) is used correctly.
If you want to address the static analysis hints, consider these optional improvements:
- For the TRY003 hint (long error message), define a custom exception:
+class ConfigurationError(Exception): + """Raised when configuration file is not found.""" + pass + def parse_config(): ... if not os.path.exists(args.config): - raise FileNotFoundError( - f"Configuration file {args.config} does not exist at expected location. Use --config to specify the correct path." - ) + raise ConfigurationError(f"Configuration file not found: {args.config}")
- The broad Exception catch in main() is acceptable practice for a top-level handler, so no change is needed there.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
applications/deltacast_receiver/python/CMakeLists.txt(1 hunks)applications/deltacast_receiver/python/deltacast_receiver.py(1 hunks)applications/deltacast_transmitter/python/CMakeLists.txt(1 hunks)operators/deltacast_videomaster/CMakeLists.txt(2 hunks)operators/deltacast_videomaster/videomaster_base.hpp(1 hunks)operators/deltacast_videomaster/videomaster_transmitter.cpp(3 hunks)operators/deltacast_videomaster/videomaster_transmitter.hpp(3 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
{applications,workflows}/**/CMakeLists.txt
📄 CodeRabbit inference engine (CONTRIBUTING.md)
CMakeLists.txt must include build system integration using add_holohub_application() for applications and workflows
Files:
applications/deltacast_transmitter/python/CMakeLists.txtapplications/deltacast_receiver/python/CMakeLists.txt
applications/**/CMakeLists.txt
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Applications should include a testing section in CMakeLists.txt for functional testing using CTest
Files:
applications/deltacast_transmitter/python/CMakeLists.txtapplications/deltacast_receiver/python/CMakeLists.txt
operators/**/*.{py,cpp,hpp}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Use TitleCase + 'Op' suffix for operator class names
Files:
operators/deltacast_videomaster/videomaster_transmitter.hppoperators/deltacast_videomaster/videomaster_base.hppoperators/deltacast_videomaster/videomaster_transmitter.cpp
{operators,applications,workflows,gxf_extensions}/**/*.{py,cpp,hpp}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
{operators,applications,workflows,gxf_extensions}/**/*.{py,cpp,hpp}: All code must adhere to Holoscan SDK coding standards including proper error handling and validation
Use descriptive English naming for functions, variables, and components; minimize acronyms and brand names
Include inline comments for complex logic in code
Files:
operators/deltacast_videomaster/videomaster_transmitter.hppoperators/deltacast_videomaster/videomaster_base.hppapplications/deltacast_receiver/python/deltacast_receiver.pyoperators/deltacast_videomaster/videomaster_transmitter.cpp
{operators,applications,workflows,gxf_extensions}/**/*.{py,cpp,hpp,cmake}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
All code must compile and build successfully on target platforms before submission
Files:
operators/deltacast_videomaster/videomaster_transmitter.hppoperators/deltacast_videomaster/videomaster_base.hppapplications/deltacast_receiver/python/deltacast_receiver.pyoperators/deltacast_videomaster/videomaster_transmitter.cpp
**/*.py
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Python code must pass linting checks via ./holohub lint
Files:
applications/deltacast_receiver/python/deltacast_receiver.py
operators/**/CMakeLists.txt
📄 CodeRabbit inference engine (CONTRIBUTING.md)
CMakeLists.txt must include build system integration using add_holohub_operator() for operators
Files:
operators/deltacast_videomaster/CMakeLists.txt
🧠 Learnings (13)
📓 Common learnings
Learnt from: CR
Repo: nvidia-holoscan/holohub PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T16:28:06.281Z
Learning: Applies to **CMakeLists.txt : Use CMake target dependencies: add DEPENDS EXTENSIONS for operators wrapping GXF extensions, add DEPENDS OPERATORS for applications/workflows
Learnt from: CR
Repo: nvidia-holoscan/holohub PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T16:28:06.280Z
Learning: Applies to {operators,applications,workflows,gxf_extensions}/**/*.{py,cpp,hpp} : All code must adhere to Holoscan SDK coding standards including proper error handling and validation
📚 Learning: 2025-11-24T16:28:06.280Z
Learnt from: CR
Repo: nvidia-holoscan/holohub PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T16:28:06.280Z
Learning: Applies to {applications,workflows}/**/CMakeLists.txt : CMakeLists.txt must include build system integration using add_holohub_application() for applications and workflows
Applied to files:
applications/deltacast_transmitter/python/CMakeLists.txtapplications/deltacast_receiver/python/CMakeLists.txtoperators/deltacast_videomaster/CMakeLists.txt
📚 Learning: 2025-11-24T16:28:06.280Z
Learnt from: CR
Repo: nvidia-holoscan/holohub PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T16:28:06.280Z
Learning: Applies to operators/**/CMakeLists.txt : CMakeLists.txt must include build system integration using add_holohub_operator() for operators
Applied to files:
applications/deltacast_transmitter/python/CMakeLists.txtoperators/deltacast_videomaster/videomaster_transmitter.hppoperators/deltacast_videomaster/videomaster_base.hppoperators/deltacast_videomaster/CMakeLists.txt
📚 Learning: 2025-11-24T16:28:06.280Z
Learnt from: CR
Repo: nvidia-holoscan/holohub PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T16:28:06.280Z
Learning: Applies to gxf_extensions/**/CMakeLists.txt : CMakeLists.txt must include build system integration using add_holohub_extension() for GXF extensions
Applied to files:
applications/deltacast_transmitter/python/CMakeLists.txtapplications/deltacast_receiver/python/CMakeLists.txtoperators/deltacast_videomaster/videomaster_base.hppoperators/deltacast_videomaster/CMakeLists.txt
📚 Learning: 2025-10-22T16:53:45.393Z
Learnt from: cdinea
Repo: nvidia-holoscan/holohub PR: 1170
File: operators/video_streaming/streaming_client_enhanced/python/CMakeLists.txt:16-24
Timestamp: 2025-10-22T16:53:45.393Z
Learning: The pybind11_add_holohub_module CMake macro in cmake/pybind11_add_holohub_module.cmake encapsulates all pybind11 setup internally, including finding pybind11, linking against holoscan::pybind11 conditionally, and linking the C++ operator target. Operator Python bindings in holohub should only call this macro without additional pybind11 setup.
Applied to files:
applications/deltacast_transmitter/python/CMakeLists.txtoperators/deltacast_videomaster/CMakeLists.txt
📚 Learning: 2025-11-24T16:28:06.280Z
Learnt from: CR
Repo: nvidia-holoscan/holohub PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T16:28:06.280Z
Learning: Applies to applications/**/CMakeLists.txt : Applications should include a testing section in CMakeLists.txt for functional testing using CTest
Applied to files:
applications/deltacast_transmitter/python/CMakeLists.txtapplications/deltacast_receiver/python/CMakeLists.txt
📚 Learning: 2025-11-24T16:28:06.280Z
Learnt from: CR
Repo: nvidia-holoscan/holohub PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T16:28:06.280Z
Learning: Applies to operators/**/ : Operators should follow the directory structure with metadata.json, README.md, implementation files, test files, and CMakeLists.txt
Applied to files:
applications/deltacast_receiver/python/CMakeLists.txt
📚 Learning: 2025-11-24T16:28:06.281Z
Learnt from: CR
Repo: nvidia-holoscan/holohub PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T16:28:06.281Z
Learning: Applies to applications/**/ : Applications should follow the directory structure with metadata.json, README.md, implementation files, and CMakeLists.txt
Applied to files:
applications/deltacast_receiver/python/CMakeLists.txt
📚 Learning: 2025-11-24T16:28:06.281Z
Learnt from: CR
Repo: nvidia-holoscan/holohub PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T16:28:06.281Z
Learning: Applies to workflows/**/ : Workflows should follow the directory structure with metadata.json, README.md, implementation files, and CMakeLists.txt
Applied to files:
applications/deltacast_receiver/python/CMakeLists.txt
📚 Learning: 2025-11-24T16:28:06.280Z
Learnt from: CR
Repo: nvidia-holoscan/holohub PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T16:28:06.280Z
Learning: Applies to {operators,applications,workflows,gxf_extensions}/**/*.{py,cpp,hpp} : All code must adhere to Holoscan SDK coding standards including proper error handling and validation
Applied to files:
applications/deltacast_receiver/python/CMakeLists.txtoperators/deltacast_videomaster/videomaster_transmitter.hppoperators/deltacast_videomaster/videomaster_base.hpp
📚 Learning: 2025-11-24T16:28:06.281Z
Learnt from: CR
Repo: nvidia-holoscan/holohub PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T16:28:06.281Z
Learning: Applies to **CMakeLists.txt : Use CMake target dependencies: add DEPENDS EXTENSIONS for operators wrapping GXF extensions, add DEPENDS OPERATORS for applications/workflows
Applied to files:
operators/deltacast_videomaster/videomaster_base.hppoperators/deltacast_videomaster/CMakeLists.txt
📚 Learning: 2025-11-24T16:28:06.280Z
Learnt from: CR
Repo: nvidia-holoscan/holohub PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T16:28:06.280Z
Learning: Applies to {operators,applications,workflows,gxf_extensions}/**/*.{py,cpp,hpp} : Include inline comments for complex logic in code
Applied to files:
operators/deltacast_videomaster/videomaster_base.hpp
📚 Learning: 2025-10-22T23:47:42.896Z
Learnt from: cdinea
Repo: nvidia-holoscan/holohub PR: 1170
File: applications/video_streaming/video_streaming_server/cpp/CMakeLists.txt:109-111
Timestamp: 2025-10-22T23:47:42.896Z
Learning: In the video streaming server application (applications/video_streaming/video_streaming_server/cpp/CMakeLists.txt), bundling libcudart.so.12 from the NGC operator binaries is intentional to ensure consistency with NGC binaries, even though the target links to CUDA::cudart.
Applied to files:
operators/deltacast_videomaster/CMakeLists.txt
🧬 Code graph analysis (2)
operators/deltacast_videomaster/videomaster_transmitter.hpp (1)
operators/deltacast_videomaster/videomaster_source.hpp (2)
spec(41-41)op_input(45-46)
applications/deltacast_receiver/python/deltacast_receiver.py (1)
applications/deltacast_receiver/cpp/main.cpp (3)
holoscan(28-71)main(108-128)main(108-108)
🪛 Cppcheck (2.18.0)
operators/deltacast_videomaster/videomaster_transmitter.cpp
[error] 36-36: There is an unknown macro here somewhere. Configuration is required. If HOLOSCAN_OPERATOR_FORWARD_ARGS is a macro then please configure it.
(unknownMacro)
🪛 Ruff (0.14.8)
applications/deltacast_receiver/python/deltacast_receiver.py
120-122: Avoid specifying long messages outside the exception class
(TRY003)
133-133: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (10)
operators/deltacast_videomaster/videomaster_transmitter.hpp (1)
25-65: LGTM: Clean migration to native Holoscan operator.The header correctly migrates from GXFOperator to holoscan::Operator, uses the appropriate forward args macro, declares lifecycle methods, and includes the new videomaster_base.hpp dependency. Private members are properly initialized with defaults.
operators/deltacast_videomaster/videomaster_base.hpp (1)
33-97: LGTM: Well-designed base class for VideoMaster hardware abstraction.The VideoMasterBase class provides a clean interface for managing DELTACAST hardware lifecycle, buffer management, and configuration. The inline
sleep_mshelper is acceptable for this use case. Member initialization is complete (e.g.,_channel_typeat line 81 has a safe default).operators/deltacast_videomaster/videomaster_transmitter.cpp (1)
242-246: Good addition: Tensor size validation prevents buffer overflow.The validation ensures
tensor->nbytes()does not exceedbuffer_sizebefore the memcpy operation, preventing potential memory corruption.applications/deltacast_receiver/python/deltacast_receiver.py (5)
16-24: LGTM!Imports are well-organized and include all necessary modules for the DeltaCast receiver application.
27-31: LGTM!Initialization follows standard Application pattern correctly.
54-64: LGTM!The VideoMasterSourceOp instantiation correctly omits the unsupported
poolparameter and passes all required arguments matching the operator's binding.
67-94: LGTM!The format converters, visualizer, and pipeline connections correctly match the C++ implementation. The data flow (source → drop_alpha_channel_converter → format_converter → holoviz) is properly established.
44-45: Clarify the source_block_size calculation.The calculation
width * height * 4 * 4allocates 4× the expected size for RGBA data. For 1920×1080, this yields ~33MB instead of the expected ~8MB. While the C++ implementation uses the identical calculation, there are no comments explaining the extra factor of 4.Confirm whether this oversized allocation is required by the DELTACAST hardware or VideoMasterSourceOp, or clarify the intent with an inline comment.
applications/deltacast_receiver/python/CMakeLists.txt (2)
33-42: LGTM!Install rules correctly package the Python application and configuration file for distribution.
1-42: Useadd_holohub_application()for proper build system integration at the parent level.The deltacast_receiver application should follow the standard pattern used by other multi-variant applications (e.g., holoscan_ros2, nvidia_nim). The parent
applications/deltacast_receiver/CMakeLists.txtshould calladd_holohub_application()for each implementation variant (cpp and python) instead of manually usingadd_subdirectory()and individual CMakeLists.txt files for testing/installation. This ensures consistent build system integration across all Holohub applications.⛔ Skipped due to learnings
Learnt from: CR Repo: nvidia-holoscan/holohub PR: 0 File: CONTRIBUTING.md:0-0 Timestamp: 2025-11-24T16:28:06.280Z Learning: Applies to {applications,workflows}/**/CMakeLists.txt : CMakeLists.txt must include build system integration using add_holohub_application() for applications and workflowsLearnt from: CR Repo: nvidia-holoscan/holohub PR: 0 File: CONTRIBUTING.md:0-0 Timestamp: 2025-11-24T16:28:06.281Z Learning: Applies to applications/**/ : Applications should follow the directory structure with metadata.json, README.md, implementation files, and CMakeLists.txtLearnt from: CR Repo: nvidia-holoscan/holohub PR: 0 File: CONTRIBUTING.md:0-0 Timestamp: 2025-11-24T16:28:06.280Z Learning: Applies to applications/**/CMakeLists.txt : Applications should include a testing section in CMakeLists.txt for functional testing using CTestLearnt from: CR Repo: nvidia-holoscan/holohub PR: 0 File: CONTRIBUTING.md:0-0 Timestamp: 2025-11-24T16:28:06.280Z Learning: Applies to operators/**/CMakeLists.txt : CMakeLists.txt must include build system integration using add_holohub_operator() for operatorsLearnt from: CR Repo: nvidia-holoscan/holohub PR: 0 File: CONTRIBUTING.md:0-0 Timestamp: 2025-11-24T16:28:06.280Z Learning: Applies to gxf_extensions/**/CMakeLists.txt : CMakeLists.txt must include build system integration using add_holohub_extension() for GXF extensionsLearnt from: CR Repo: nvidia-holoscan/holohub PR: 0 File: CONTRIBUTING.md:0-0 Timestamp: 2025-11-24T16:28:06.280Z Learning: Applies to {operators,applications,workflows,gxf_extensions}/**/*.{py,cpp,hpp} : All code must adhere to Holoscan SDK coding standards including proper error handling and validationLearnt from: CR Repo: nvidia-holoscan/holohub PR: 0 File: CONTRIBUTING.md:0-0 Timestamp: 2025-11-24T16:28:06.281Z Learning: Applies to {operators,applications,workflows,gxf_extensions}/**/*.{py,cpp,hpp,cmake} : All code must compile and build successfully on target platforms before submission
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
operators/deltacast_videomaster/videomaster_source.cpp (1)
204-214: CheckwrapMemoryreturn value.The return value of
wrapMemoryis not checked. If it fails, the code continues to emit potentially invalid video data.nvidia::gxf::VideoBufferInfo info{ format->width, format->height, video_type.value, color_planes, nvidia::gxf::SurfaceLayout::GXF_SURFACE_LAYOUT_PITCH_LINEAR}; auto storage_type = _video_master_base->is_igpu() ? nvidia::gxf::MemoryStorageType::kHost : nvidia::gxf::MemoryStorageType::kDevice; - video_buffer.value()->wrapMemory(info, buffer_size, storage_type, buffer, nullptr); + auto wrap_result = video_buffer.value()->wrapMemory(info, buffer_size, storage_type, buffer, nullptr); + if (!wrap_result) { + throw std::runtime_error("Failed to wrap memory for video buffer"); + } auto result = nvidia::gxf::Entity(std::move(video_output.value())); op_output.emit(result, "signal");operators/deltacast_videomaster/videomaster_transmitter.cpp (1)
289-296: Validate_channel_indexbefore accessing keyer maps.The
.at()calls onid_to_rx_keyer_input,id_to_tx_keyer_input, andid_to_keyer_video_outputwill throwstd::out_of_rangeif_channel_indexexceeds 3 (the maximum key in these maps). Consider adding explicit validation with a clearer error message.bool VideoMasterTransmitterOp::configure_board_for_overlay() { HOLOSCAN_LOG_INFO("Configuring board for overlay with keyer on channel {}", _channel_index); + // Validate channel index for keyer maps (only channels 0-3 are supported) + if (_channel_index > 3) { + HOLOSCAN_LOG_ERROR("Channel index {} not supported for overlay mode (max: 3)", _channel_index); + return false; + } + if (!_video_master_base->video_information()->configure_sync( _video_master_base->board_handle(), _channel_index)) { return false; }
🧹 Nitpick comments (4)
applications/deltacast_receiver/python/deltacast_receiver.py (1)
127-135: Consider logging the exception traceback for debugging.The broad exception handler is acceptable for a CLI entry point, but consider logging the full traceback at debug level to aid troubleshooting while keeping the user-facing message clean.
def main(): try: args = parse_config() app = DeltacastReceiverApp() app.config(args.config) app.run() except Exception as e: + import traceback + traceback.print_exc() # Or use logging.debug for production print(f"Error: {e}") sys.exit(1)applications/deltacast_receiver/cpp/main.cpp (1)
117-123: Use idiomatic filesystem path operations.Minor style improvement: prefer
empty()for string emptiness check and/=for path concatenation.- if (config_name != "") { + if (!config_name.empty()) { app->config(config_name); } else { auto config_path = std::filesystem::weakly_canonical(argv[0]).parent_path(); - config_path += "/deltacast_receiver.yaml"; + config_path /= "deltacast_receiver.yaml"; app->config(config_path); }operators/deltacast_videomaster/videomaster_source.cpp (1)
80-82: Remove unused variablesuccess_b.The variable
success_bis declared but never used in thecompute()method.void VideoMasterSourceOp::compute(InputContext& op_input, OutputContext& op_output, ExecutionContext& context) { - bool success_b = true; - if (!_video_master_base->signal_present()) {operators/deltacast_videomaster/videomaster_base.cpp (1)
37-73: Consider wrapping constant maps in anonymous namespace.The constant maps (
id_to_rx_stream_type,id_to_tx_stream_type, etc.) have external linkage by default. For consistency withvideomaster_transmitter.cpp(which wraps similar maps in an anonymous namespace), consider adding internal linkage to prevent potential ODR issues.namespace holoscan::ops { +namespace { + const std::unordered_map<uint32_t, VHD_STREAMTYPE> id_to_rx_stream_type = { // ... existing content ... }; // ... other maps ... const std::unordered_map<uint32_t, VHD_CORE_BOARDPROPERTY> id_to_firmware_loopback_prop = { {0, VHD_CORE_BP_FIRMWARE_LOOPBACK_0}, {1, VHD_CORE_BP_FIRMWARE_LOOPBACK_1} }; +} // anonymous namespace + VideoMasterBase::VideoMasterBase(bool is_input, uint32_t board_index,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
applications/deltacast_receiver/cpp/main.cpp(1 hunks)applications/deltacast_receiver/python/CMakeLists.txt(1 hunks)applications/deltacast_receiver/python/deltacast_receiver.py(1 hunks)applications/deltacast_transmitter/cpp/main.cpp(2 hunks)applications/deltacast_transmitter/python/deltacast_transmitter.py(2 hunks)operators/deltacast_videomaster/videomaster_base.cpp(11 hunks)operators/deltacast_videomaster/videomaster_source.cpp(3 hunks)operators/deltacast_videomaster/videomaster_transmitter.cpp(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- applications/deltacast_receiver/python/CMakeLists.txt
- applications/deltacast_transmitter/cpp/main.cpp
- applications/deltacast_transmitter/python/deltacast_transmitter.py
🧰 Additional context used
📓 Path-based instructions (4)
{operators,applications,workflows,gxf_extensions}/**/*.{py,cpp,hpp}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
{operators,applications,workflows,gxf_extensions}/**/*.{py,cpp,hpp}: All code must adhere to Holoscan SDK coding standards including proper error handling and validation
Use descriptive English naming for functions, variables, and components; minimize acronyms and brand names
Include inline comments for complex logic in code
Files:
applications/deltacast_receiver/cpp/main.cppapplications/deltacast_receiver/python/deltacast_receiver.pyoperators/deltacast_videomaster/videomaster_source.cppoperators/deltacast_videomaster/videomaster_base.cppoperators/deltacast_videomaster/videomaster_transmitter.cpp
{operators,applications,workflows,gxf_extensions}/**/*.{py,cpp,hpp,cmake}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
All code must compile and build successfully on target platforms before submission
Files:
applications/deltacast_receiver/cpp/main.cppapplications/deltacast_receiver/python/deltacast_receiver.pyoperators/deltacast_videomaster/videomaster_source.cppoperators/deltacast_videomaster/videomaster_base.cppoperators/deltacast_videomaster/videomaster_transmitter.cpp
**/*.py
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Python code must pass linting checks via ./holohub lint
Files:
applications/deltacast_receiver/python/deltacast_receiver.py
operators/**/*.{py,cpp,hpp}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Use TitleCase + 'Op' suffix for operator class names
Files:
operators/deltacast_videomaster/videomaster_source.cppoperators/deltacast_videomaster/videomaster_base.cppoperators/deltacast_videomaster/videomaster_transmitter.cpp
🧠 Learnings (5)
📚 Learning: 2025-10-22T16:33:55.411Z
Learnt from: cdinea
Repo: nvidia-holoscan/holohub PR: 1170
File: applications/video_streaming/video_streaming_client/python/streaming_client_demo_replayer.yaml:27-36
Timestamp: 2025-10-22T16:33:55.411Z
Learning: In the video_streaming bidirectional client applications (applications/video_streaming/video_streaming_client), the pipeline has two separate data paths: (1) Outgoing: source → format_converter → streaming_client INPUT (sends to server), and (2) Incoming: streaming_client OUTPUT → holoviz (receives from server). The format_converter prepares data for transmission and does NOT feed directly into holoviz visualization.
Applied to files:
applications/deltacast_receiver/cpp/main.cpp
📚 Learning: 2025-10-22T23:47:42.896Z
Learnt from: cdinea
Repo: nvidia-holoscan/holohub PR: 1170
File: applications/video_streaming/video_streaming_server/cpp/CMakeLists.txt:109-111
Timestamp: 2025-10-22T23:47:42.896Z
Learning: In the video streaming server application (applications/video_streaming/video_streaming_server/cpp/CMakeLists.txt), bundling libcudart.so.12 from the NGC operator binaries is intentional to ensure consistency with NGC binaries, even though the target links to CUDA::cudart.
Applied to files:
operators/deltacast_videomaster/videomaster_source.cpp
📚 Learning: 2025-11-17T06:32:45.908Z
Learnt from: AndreasHeumann
Repo: nvidia-holoscan/holohub PR: 1220
File: applications/pipeline_visualization/cpp/create_tensor.cpp:64-87
Timestamp: 2025-11-17T06:32:45.908Z
Learning: In CUDA, when `cudaMemcpyAsync` is used with pageable (non-pinned) host memory, the operation is synchronous with respect to the host thread because the runtime must prevent memory modification during transfer. Only pinned (page-locked) memory allows truly asynchronous behavior. Therefore, no explicit `cudaStreamSynchronize` is required after `cudaMemcpyAsync` when the destination is pageable host memory.
Applied to files:
operators/deltacast_videomaster/videomaster_source.cpp
📚 Learning: 2025-11-24T16:28:06.280Z
Learnt from: CR
Repo: nvidia-holoscan/holohub PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T16:28:06.280Z
Learning: Applies to gxf_extensions/**/CMakeLists.txt : CMakeLists.txt must include build system integration using add_holohub_extension() for GXF extensions
Applied to files:
operators/deltacast_videomaster/videomaster_base.cpp
📚 Learning: 2025-10-22T16:21:10.281Z
Learnt from: cdinea
Repo: nvidia-holoscan/holohub PR: 1170
File: applications/video_streaming/video_streaming_client/cpp/streaming_client_demo.yaml:42-49
Timestamp: 2025-10-22T16:21:10.281Z
Learning: V4L2VideoCaptureOp in Holoscan SDK performs internal conversion from the camera's native pixel format (specified by the pixel_format parameter) to RGBA8888 output, regardless of the input format (e.g., YUYV, MJPEG). When using V4L2 as a source, downstream operators should expect RGBA8888 (4-channel) output format.
Applied to files:
operators/deltacast_videomaster/videomaster_base.cpp
🧬 Code graph analysis (4)
applications/deltacast_receiver/cpp/main.cpp (1)
applications/deltacast_transmitter/cpp/main.cpp (3)
holoscan(33-56)parse_arguments(63-87)parse_arguments(63-63)
applications/deltacast_receiver/python/deltacast_receiver.py (2)
applications/deltacast_receiver/cpp/main.cpp (2)
main(108-128)main(108-108)applications/deltacast_transmitter/python/deltacast_transmitter.py (1)
main(132-140)
operators/deltacast_videomaster/videomaster_base.cpp (1)
operators/deltacast_videomaster/videomaster_base.hpp (15)
VideoMasterBase(41-42)_is_igpu(76-76)_board_handle(58-58)_video_format(74-74)_video_format(75-75)_video_information(62-64)_video_information(65-67)_gpu_buffers(68-68)_gpu_buffers(69-69)_system_buffers(70-70)_system_buffers(71-71)_slot_handles(72-72)_slot_handles(73-73)result(52-53)state(56-56)
operators/deltacast_videomaster/videomaster_transmitter.cpp (3)
operators/deltacast_videomaster/videomaster_source.cpp (8)
initialize(63-68)initialize(63-63)start(70-78)start(70-70)compute(80-158)compute(80-81)stop(160-163)stop(160-160)operators/deltacast_videomaster/videomaster_base.hpp (2)
sleep_ms(33-35)sleep_ms(33-33)operators/deltacast_videomaster/videomaster_source.hpp (2)
op_input(45-46)buffer(50-51)
🪛 Cppcheck (2.18.0)
operators/deltacast_videomaster/videomaster_source.cpp
[error] 37-37: There is an unknown macro here somewhere. Configuration is required. If HOLOSCAN_OPERATOR_FORWARD_ARGS is a macro then please configure it.
(unknownMacro)
operators/deltacast_videomaster/videomaster_transmitter.cpp
[error] 36-36: There is an unknown macro here somewhere. Configuration is required. If HOLOSCAN_OPERATOR_FORWARD_ARGS is a macro then please configure it.
(unknownMacro)
🪛 Ruff (0.14.8)
applications/deltacast_receiver/python/deltacast_receiver.py
120-122: Avoid specifying long messages outside the exception class
(TRY003)
133-133: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (8)
applications/deltacast_receiver/python/deltacast_receiver.py (2)
27-64: LGTM! Pipeline setup matches C++ implementation.The operator creation and configuration align well with the C++ counterpart in
applications/deltacast_receiver/cpp/main.cpp. The block size calculation (width * height * 4) correctly accounts for RGBA format (4 bytes per pixel), and the block count logic (3 for RDMA, 4 otherwise) matches the C++ implementation.
66-94: Pipeline wiring looks correct.The flow
source -> drop_alpha_channel_converter -> format_converter -> holovizwith the{("", "receivers")}port mapping for HolovizOp matches the C++ implementation exactly.applications/deltacast_receiver/cpp/main.cpp (1)
25-71: LGTM! Well-structured application class.The
compose()method properly configures the VideoMaster source operator, format converters, and HolovizOp with appropriate memory pools. The pipeline flow matches the Python counterpart.operators/deltacast_videomaster/videomaster_transmitter.cpp (2)
38-64: Good: Constants properly wrapped in anonymous namespace.The constant maps are now correctly placed in an anonymous namespace, preventing ODR violations and namespace pollution. This addresses a previous review concern.
133-270: LGTM! Robust error handling in compute method.The compute method includes proper validation:
- Null slot handle check (lines 222-226)
- Tensor size vs buffer size validation (lines 246-250)
- CUDA memcpy error handling (lines 255-258)
These address previous review concerns about memory safety.
operators/deltacast_videomaster/videomaster_base.cpp (3)
100-103: Good: RDMA properly disabled on integrated GPUs.This addresses the previous critical review about GPUDirect RDMA requiring device memory. The warning message clearly informs users about the fallback behavior.
380-426: Good: CUDA free operations now include error checking.The
free_buffers()method now properly checks return values fromcudaFreeHostandcudaFree, logging errors when they occur. This addresses a previous review comment about unchecked CUDA free operations.
428-454: LGTM! Clean error handling patterns.The
start_stream()andholoscan_log_on_error()methods follow consistent patterns with appropriate logging and error propagation.
Greptile SummaryThis PR successfully migrates DELTACAST VideoMaster integration from legacy GXF extensions to native Holoscan operators. Major Changes:
Testing: Architecture: Confidence Score: 5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant App as Application
participant Source as VideoMasterSourceOp
participant Base as VideoMasterBase
participant SDK as VideoMaster SDK
participant CUDA as CUDA Runtime
participant FC as FormatConverter
participant Viz as Holoviz
App->>Source: initialize()
Source->>Base: create VideoMasterBase
Base->>CUDA: check iGPU/dGPU
App->>Source: start()
Source->>Base: configure_board()
Source->>Base: open_stream()
loop Every Frame
App->>Source: compute()
Source->>Base: signal_present()
alt First Frame
Source->>Base: configure_stream()
Source->>Base: init_buffers()
Base->>CUDA: allocate GPU/system buffers
Source->>Base: start_stream()
end
Source->>SDK: VHD_WaitSlotFilled()
SDK-->>Source: slot_handle with data
Source->>SDK: VHD_GetSlotBuffer()
alt Non-RDMA Mode
Source->>CUDA: cudaMemcpy(system->GPU)
end
Source->>Source: wrap in GXF Entity
Source->>FC: emit video buffer
FC->>Viz: format and display
Source->>SDK: VHD_QueueInSlot()
end
App->>Source: stop()
Source->>Base: stop_stream()
Base->>SDK: VHD_StopStream()
Base->>CUDA: free buffers
|
There was a problem hiding this comment.
Additional Comments (12)
-
applications/deltacast_receiver/cpp/deltacast_receiver.yaml, line 46 (link)style: missing newline at end of file
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
-
operators/deltacast_videomaster/python/videomaster.cpp, line 175 (link)style: missing newline at end of file
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
-
operators/deltacast_videomaster/CMakeLists.txt, line 27-28 (link)logic: minimum required Holoscan version "0.5" conflicts with version "3.6.0" referenced in other parts of the PR. Should this be updated to match the tested version? Is Holoscan SDK 0.5 actually the minimum supported version, or should this align with the 3.6.0 version mentioned in other files?
-
operators/deltacast_videomaster/README.md, line 38 (link)syntax: grammar: 'overlay is add by card' should be 'overlay is added by the card' or 'overlay is provided by the card'
-
operators/deltacast_videomaster/videomaster_base.hpp, line 33-35 (link)style: Global utility function in header pollutes namespace. Move to implementation file or place in anonymous namespace.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
-
operators/deltacast_videomaster/videomaster_base.hpp, line 22 (link)style:
#include <optional>is unused in this header - can be removed -
operators/deltacast_videomaster/videomaster_base.hpp, line 62-76 (link)style: Non-const getters returning references to private members break encapsulation. Consider returning const references or providing specific setter methods for controlled access. Are these non-const getters intentionally exposing private state for modification by client code?
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
-
operators/deltacast_videomaster/videomaster_base.cpp, line 24 (link)logic: Remove GXF dependency - this include contradicts the PR goal of moving away from GXF to native operators
-
operators/deltacast_videomaster/videomaster_base.cpp, line 28 (link)logic: GXF multimedia header should be removed - this migration is moving away from GXF dependencies
-
operators/deltacast_videomaster/videomaster_source.cpp, line 28 (link)logic: This GXF include contradicts the PR's goal of avoiding GXF dependencies. The operator should use native Holoscan video buffer types instead. Should this operator use GXF video types, or are there native Holoscan alternatives that would better align with the goal of eliminating GXF dependencies?
-
operators/deltacast_videomaster/videomaster_source.cpp, line 82 (link)style:
success_bvariable is declared but never used - can be removedNote: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
-
operators/deltacast_videomaster/videomaster_source.cpp, line 131 (link)syntax: Missing space after
ifkeyword
45 files reviewed, 12 comments
…oject Signed-off-by: Laurent Radoux <[email protected]> Signed-off-by: td <[email protected]>
Signed-off-by: Laurent Radoux <[email protected]> Signed-off-by: td <[email protected]>
Signed-off-by: Laurent Radoux <[email protected]> Signed-off-by: td <[email protected]>
Signed-off-by: Laurent Radoux <[email protected]> Signed-off-by: td <[email protected]>
Signed-off-by: Laurent Radoux <[email protected]> Signed-off-by: td <[email protected]>
Signed-off-by: Laurent Radoux <[email protected]> Signed-off-by: td <[email protected]>
Signed-off-by: Laurent Radoux <[email protected]> Signed-off-by: td <[email protected]>
Signed-off-by: Laurent Radoux <[email protected]> Signed-off-by: td <[email protected]>
Signed-off-by: Laurent Radoux <[email protected]> Signed-off-by: td <[email protected]>
Signed-off-by: Laurent Radoux <[email protected]> Signed-off-by: td <[email protected]>
Signed-off-by: Laurent Radoux <[email protected]> Signed-off-by: td <[email protected]>
Co-authored-by: Copilot <[email protected]> Signed-off-by: Thomas Dethier <[email protected]> Signed-off-by: Laurent Radoux <[email protected]> Signed-off-by: td <[email protected]>
Signed-off-by: Laurent Radoux <[email protected]> Signed-off-by: td <[email protected]>
Signed-off-by: td <[email protected]>
Signed-off-by: td <[email protected]>
… Deltacast integration Signed-off-by: td <[email protected]>
Signed-off-by: td <[email protected]>
… with sleep function Signed-off-by: td <[email protected]>
Signed-off-by: td <[email protected]>
Signed-off-by: td <[email protected]>
Signed-off-by: td <[email protected]>
thomasdethier
force-pushed
the
upgrade-deltacast-integration
branch
from
95a50d2 to
b851be8
Compare
|
Too many files changed for review. |
|
Too many files changed for review. |
Hi @bhashemian, PR is ready to be merged. All checks have passed, branch is up to date with main. |
5 participants
This PR reworks the integration of DELTACAST input and output devices to avoid being based on GXF operators.
Adds a new standalone "receiver" demo application specifically for DELTACAST.
Sequence Diagram
sequenceDiagram participant User participant DeltacastReceiverApp participant VideoMasterSourceOp participant VideoMasterBase participant FormatConverterOp participant HolovizOp participant DeltacastHW as DELTACAST Hardware User->>DeltacastReceiverApp: Run application DeltacastReceiverApp->>DeltacastReceiverApp: Parse command line arguments DeltacastReceiverApp->>DeltacastReceiverApp: Load YAML configuration DeltacastReceiverApp->>DeltacastReceiverApp: compose() DeltacastReceiverApp->>VideoMasterSourceOp: Create operator DeltacastReceiverApp->>FormatConverterOp: Create format converter (drop alpha) DeltacastReceiverApp->>FormatConverterOp: Create format converter (RGBA) DeltacastReceiverApp->>HolovizOp: Create visualizer DeltacastReceiverApp->>DeltacastReceiverApp: Connect pipeline flow DeltacastReceiverApp->>VideoMasterSourceOp: start() VideoMasterSourceOp->>VideoMasterBase: Create VideoMasterBase instance VideoMasterBase->>VideoMasterBase: Detect GPU type (iGPU/dGPU) VideoMasterSourceOp->>VideoMasterBase: configure_board() VideoMasterBase->>DeltacastHW: Query board properties VideoMasterSourceOp->>VideoMasterBase: open_stream() VideoMasterBase->>DeltacastHW: Open RX stream loop Compute Cycle VideoMasterSourceOp->>VideoMasterSourceOp: compute() VideoMasterSourceOp->>VideoMasterBase: Check signal_present() alt First frame or signal change VideoMasterSourceOp->>VideoMasterBase: configure_stream() VideoMasterBase->>DeltacastHW: Configure video format VideoMasterSourceOp->>VideoMasterBase: init_buffers() VideoMasterBase->>VideoMasterBase: Allocate GPU/system buffers VideoMasterBase->>DeltacastHW: Create and queue slots VideoMasterSourceOp->>VideoMasterBase: start_stream() end VideoMasterSourceOp->>VideoMasterBase: Wait for incoming slot VideoMasterBase->>DeltacastHW: VHD_WaitSlotFilled() DeltacastHW-->>VideoMasterBase: Slot ready with video data alt Non-RDMA mode VideoMasterBase->>VideoMasterBase: Copy system buffer to GPU end VideoMasterSourceOp->>VideoMasterSourceOp: transmit_buffer_data() VideoMasterSourceOp->>FormatConverterOp: Emit video buffer FormatConverterOp->>FormatConverterOp: Drop alpha channel & resize FormatConverterOp->>FormatConverterOp: Convert to RGBA FormatConverterOp->>HolovizOp: Send formatted frame HolovizOp->>HolovizOp: Render to display VideoMasterSourceOp->>VideoMasterBase: Queue slot for reuse end User->>DeltacastReceiverApp: Stop application DeltacastReceiverApp->>VideoMasterSourceOp: stop() VideoMasterSourceOp->>VideoMasterBase: stop_stream() VideoMasterBase->>DeltacastHW: Stop stream VideoMasterBase->>VideoMasterBase: Free buffers VideoMasterBase->>VideoMasterBase: Close handlesSummary by CodeRabbit
New Features
Improvements
Refactor
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.