Add ST2110 source operator for SMPTE ST 2110-20 video ingestion#1346
Add ST2110 source operator for SMPTE ST 2110-20 video ingestion#1346Ollie-cj wants to merge 5 commits intonvidia-holoscan:mainfrom
Conversation
Greptile SummaryThis PR adds a new ST2110 source operator for receiving SMPTE ST 2110-20 uncompressed video streams over IP networks, along with a demo application. The operator uses standard Linux UDP sockets with CUDA pinned memory for efficient video ingestion on both x86_64 and aarch64 platforms. Key Features:
Implementation Quality:
Minor Issue Found:
Confidence Score: 4/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[ST 2110-20 Multicast Stream] -->|UDP Packets| B[Linux Socket<br/>recvmmsg batch receive]
B --> C[RTP/ST2110 Header Parsing<br/>Extract line number, offset, timestamp]
C --> D{Triple-Buffer<br/>CUDA Pinned Memory}
D -->|Buffer 1| E[Assembling]
D -->|Buffer 2| F[Complete]
D -->|Buffer 3| G[Complete]
F --> H[cudaMemcpyAsync H2D]
G --> H
H --> I[GPU Raw Buffer<br/>YCbCr-4:2:2-10bit]
I --> J[raw_output port<br/>Always emitted]
I --> K{RGBA enabled?}
I --> L{NV12 enabled?}
K -->|Yes| M[CUDA Conversion Kernel<br/>BT.709 color space]
L -->|Yes| N[CUDA Conversion Kernel<br/>4:2:0 subsampling]
M --> O[rgba_output port<br/>RGBA 8-bit]
N --> P[nv12_output port<br/>NV12 8-bit]
O --> Q[Holoviz Display]
P --> R[Video Encoder]
Last reviewed commit: 5f62c29 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| } | ||
| }; | ||
|
|
||
| PYBIND11_MODULE(_st2110_source, m) { |
There was a problem hiding this comment.
syntax: Missing __init__.py file prevents Python module import. The pybind11 module _st2110_source cannot be imported without a companion __init__.py that re-exports the operator.
Create operators/st2110_source/python/__init__.py:
"""
SPDX-FileCopyrightText: Copyright (c) 2025 XRlabs. All rights reserved.
SPDX-License-Identifier: Apache-2.0
"""
try:
from ._st2110_source import ST2110SourceOp
except ImportError as e:
pybind11_hsdk_err = 'unknown base type "holoscan::'
if pybind11_hsdk_err not in str(e):
raise e
# Add SDK version compatibility note
import sys
sys.stderr.write(
"\nError importing ST2110SourceOp: pybind11 binding incompatible with installed Holoscan SDK version.\n"
"This typically indicates an ABI mismatch between the SDK used to build the operator and the installed SDK.\n"
"Rebuild the operator against the installed Holoscan SDK.\n\n"
)
raise e
__all__ = ["ST2110SourceOp"]Without this file, the import at applications/st2110_test/st2110_test_app.py:23 (from holohub.st2110_source import ST2110SourceOp) will fail with ModuleNotFoundError.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughIntroduces the ST2110 Source operator for receiving SMPTE ST 2110-20 video streams over UDP multicast with CUDA-accelerated frame assembly and color conversion (YCbCr to RGBA/NV12), accompanied by a demonstration application and comprehensive documentation. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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
🤖 Fix all issues with AI agents
In `@applications/st2110_test/CMakeLists.txt`:
- Around line 1-19: Replace the current raw install() usage with the project's
application macro: call add_holohub_application(<name> DEPENDS OPERATORS
<operator_list>) to register this Python-only app and declare operator
dependencies; remove executable permissions from non-executable files in the
install step so only the Python script gets OWNER/ GROUP/ WORLD execute flags;
and add a CTest-based testing section (enable_testing() and add_test(...) or
add_holohub_application test helper if available) to run the functional tests
for st2110_test_app.py. Ensure you update references to st2110_test_app.py,
st2110_test_config.yaml and README.md in the new install macro or install rules
so permissions and destinations are correct.
In `@applications/st2110_test/run_nv12_only.sh`:
- Around line 19-21: The command substitutions for uid are unquoted and may be
subject to word splitting; update all occurrences of $(id -u) in the script
(e.g., the -v volume mount and the -e XDG_RUNTIME_DIR assignment in
run_nv12_only.sh) to use quoted command substitution ("$(id -u)") so the
expanded value is treated as a single token.
In `@applications/st2110_test/run.sh`:
- Around line 74-80: The docker run arguments in run.sh use unquoted variable
expansions (e.g. ${HOLOHUB_ROOT}, $(id -u), $DISPLAY, $XDG_RUNTIME_DIR,
$PYTHONPATH, $LD_LIBRARY_PATH) which can break when values contain spaces or
special chars; update each occurrence to use double quotes around the expansion
(for example -v "${HOLOHUB_ROOT}:/workspace", -v "/run/user/$(id
-u):/run/user/$(id -u)", and -e "DISPLAY=$DISPLAY", -e
"XDG_RUNTIME_DIR=/run/user/$(id -u)", -e "PYTHONPATH=$PYTHONPATH", -e
"LD_LIBRARY_PATH=$LD_LIBRARY_PATH") so the values are preserved as single
arguments and not split by the shell.
In `@applications/st2110_test/st2110_test_app.py`:
- Around line 165-167: The user-facing print hardcodes "st2110_test_config.yaml"
instead of showing the chosen config; update the message that prints the
configuration filename to use the parsed argument variable (args.config) so the
displayed filename reflects the actual config passed in; locate the print
call(s) near the startup/messages block (the three prints) in st2110_test_app.py
and replace the hardcoded string with the args.config value while keeping the
other lines unchanged.
In `@operators/st2110_source/metadata.json`:
- Line 25: The first entry in the "tags" array of the metadata.json is a
category and must be one of the approved categories; replace "Video Ingestion"
with an approved category such as "Video Capture" (or "Networking and
Distributed Computing" / "Streaming" if you prefer a different emphasis) so the
tags array becomes e.g. ["Video Capture", "Network", "ST 2110", "Sockets",
"Broadcast"]; update the "tags" value in operators/st2110_source/metadata.json
accordingly.
In `@operators/st2110_source/st2110_kernels.cu`:
- Around line 91-97: The kernel currently only copies pkt_len/4 uint32 words and
drops trailing bytes when pkt_len % 4 != 0; update the kernel (the simple
reorder kernel operating on in_pkt, out, pkt_idx, pkt_len and using
threadIdx.x/blockDim.x) to copy the remaining (pkt_len & 3) bytes too: after the
4-byte-word loop compute size_t tail_offset = (pkt_len / 4) * 4 and have one
thread (e.g., threadIdx.x == 0) copy the remaining bytes using uint8_t pointers
from static_cast<const uint8_t*>(in_pkt) + tail_offset to
static_cast<uint8_t*>(out) + pkt_idx * pkt_len + tail_offset for the range
[tail_offset, pkt_len). Ensure types and pointer arithmetic match the rest of
the kernel.
🧹 Nitpick comments (15)
operators/st2110_source/BUILD.md (1)
107-112: CMake download example is aarch64-only.The example shows an aarch64-specific tarball which won't work on x86_64 systems. Consider adding the x86_64 variant or using a platform-agnostic approach.
Suggested improvement
**Solution**: Install CMake 3.24+ ```bash -# Download and install locally -wget https://github.com/Kitware/CMake/releases/download/v3.28.1/cmake-3.28.1-linux-aarch64.tar.gz -tar -xzf cmake-3.28.1-linux-aarch64.tar.gz -C $HOME/.local/ --strip-components=1 +# Download and install locally (choose appropriate architecture) +# For aarch64: +wget https://github.com/Kitware/CMake/releases/download/v3.28.1/cmake-3.28.1-linux-aarch64.tar.gz +tar -xzf cmake-3.28.1-linux-aarch64.tar.gz -C $HOME/.local/ --strip-components=1 + +# For x86_64: +# wget https://github.com/Kitware/CMake/releases/download/v3.28.1/cmake-3.28.1-linux-x86_64.tar.gz +# tar -xzf cmake-3.28.1-linux-x86_64.tar.gz -C $HOME/.local/ --strip-components=1 + export PATH=$HOME/.local/bin:$PATHapplications/st2110_test/SETUP.md (1)
98-103: Consider adding language specifier to code block.Static analysis flagged this fenced code block as missing a language specifier. For expected output blocks, you can use
textor leave empty with explicit triple backticks.Suggested fix
-``` +```text ✓ All imports successfuloperators/st2110_source/st2110_kernels.cu (2)
324-340: NV12 conversion assumes even width and height.The kernel processes 2x2 blocks and assumes width/height are even. If odd dimensions are passed, the rightmost column or bottom row will be silently skipped. Consider adding validation or documenting this constraint.
Suggested validation
void convert_ycbcr422_10bit_to_nv12(void* out, const void* in, uint32_t width, uint32_t height, cudaStream_t stream) { + // NV12 requires even dimensions for 4:2:0 subsampling + if (width % 2 != 0 || height % 2 != 0) { + // Log warning or handle error - odd dimensions will be truncated + } + // Process 2x2 blocks
101-128: Consider adding CUDA error checking after kernel launches.The host wrappers launch kernels asynchronously but don't check for launch errors. While errors will eventually surface at synchronization points, detecting them early aids debugging.
Example error checking pattern
// After kernel launch cudaError_t err = cudaGetLastError(); if (err != cudaSuccess) { // Log or handle error }applications/st2110_test/README.md (3)
12-26: Duplicate "Quick Start" sections.Lines 12-26 and 40-88 both contain "Quick Start" headings with overlapping content. Consider consolidating into a single section to avoid confusion.
Also applies to: 40-88
228-234: Add language specifier to code block.The example output code block is missing a language specifier.
Suggested fix
-``` +```text [ST2110SourceOp] 1000 packets received, 0 dropped
251-257: Consider using markdown link format for URL.The bare URL at line 257 can be formatted as a proper markdown link for better rendering.
Suggested fix
-- File issues at: https://github.com/nvidia-holoscan/holohub/issues +- File issues at: [HoloHub Issues](https://github.com/nvidia-holoscan/holohub/issues)operators/CMakeLists.txt (1)
44-44: Minor: Alphabetical ordering.The comment at line 16 indicates operators should be listed in alphabetical order.
st2110_sourceshould appear beforestreaming_client(line 40), not afterslang_shader.Suggested placement
add_holohub_operator(realsense_camera) +add_holohub_operator(st2110_source DEPENDS OPERATORS advanced_network) add_holohub_operator(streaming_client) add_holohub_operator(streaming_server) add_subdirectory(orsi) add_holohub_operator(slang_shader) -add_holohub_operator(st2110_source DEPENDS OPERATORS advanced_network) # Unified streaming operatorapplications/st2110_test/st2110_test_config_nv12_only.yaml (1)
28-40: Consider removing the inactive visualizer block or adding a note about the design rationale.Including the inactive
rgba_visualizerblock maintains schema consistency, which is useful if the application unconditionally reads both visualizer configs. If this is intentional for that reason, the existing comment on line 28 is sufficient. Otherwise, removing unused config blocks would reduce clutter.operators/st2110_source/python/st2110_source_pydoc.hpp (1)
21-21: Unnecessary<string>include.The
<string>header appears unused in this file. ThePYDOCmacro uses raw string literals which don't requirestd::string. This also explains the static analysis error (likely a tooling environment issue, but removing the unused include resolves it).Proposed fix
-#include <string> - `#include` "macros.hpp"operators/st2110_source/CMakeLists.txt (2)
30-34:CUDA_ARCHITECTURES "native"limits binary portability.Setting
CUDA_ARCHITECTURESto"native"compiles only for the build machine's GPU. Distributed binaries won't run on systems with different GPU architectures. Consider specifying explicit architectures or using a CMake variable for configurability.Suggested approach
set_target_properties(st2110_source PROPERTIES - # Compile for the architecture of the current GPU - CUDA_ARCHITECTURES "native" + # Compile for common architectures; adjust as needed + CUDA_ARCHITECTURES "${CMAKE_CUDA_ARCHITECTURES}" )This allows users to override via
-DCMAKE_CUDA_ARCHITECTURES=...at configure time, while defaulting to the CMake default behavior.
42-47: Redundant INTERFACE include directory.The
INTERFACEspecification of${CMAKE_CURRENT_SOURCE_DIR}is redundant sincePUBLICalready implies bothPUBLICandINTERFACEvisibility.Proposed fix
target_include_directories(st2110_source PUBLIC ${CMAKE_CURRENT_SOURCE_DIR} - INTERFACE - ${CMAKE_CURRENT_SOURCE_DIR} )applications/st2110_test/st2110_test_app.py (1)
22-29: Hardcoded path in import error message.The error message includes a hardcoded path
/home/thor/Source/holohub/operators/st2110_sourcewhich won't be correct for other users.♻️ Suggested fix
try: from holohub.st2110_source import ST2110SourceOp except ImportError as e: print(f"Error: Could not import ST2110SourceOp: {e}") print("\nMake sure the operator is built:") - print(" cd /home/thor/Source/holohub/operators/st2110_source") - print(" ./build.sh") + print(" ./holohub build st2110_source") sys.exit(1)operators/st2110_source/st2110_source.cpp (2)
34-46: CUDA_TRY logs errors but doesn't halt execution.The macro logs CUDA errors but allows execution to continue. For critical operations like
cudaMalloc(lines 311, 318, 326), a failure should be fatal. Consider adding aCUDA_CHECKvariant that throws on failure for critical paths.♻️ Suggested addition
`#define` CUDA_CHECK(stmt) \ ({ \ cudaError_t _err = CUDA_TRY(stmt); \ if (cudaSuccess != _err) { \ throw std::runtime_error(fmt::format("CUDA call failed: {}", cudaGetErrorString(_err))); \ } \ _err; \ })Then use
CUDA_CHECKfor allocations inallocate_buffers().
203-208: Use integer arithmetic for NV12 frame size calculation.Line 204 uses floating-point multiplication (
* 1.5) which could introduce precision issues for certain resolutions. Prefer integer arithmetic.♻️ Suggested fix
if (enable_nv12_output_.get()) { - nv12_frame_size_ = width_.get() * height_.get() * 1.5; // NV12 = 1.5 bytes/pixel + nv12_frame_size_ = (width_.get() * height_.get() * 3) / 2; // NV12 = 1.5 bytes/pixel HOLOSCAN_LOG_INFO("NV12 output enabled: {} MB per frame", nv12_frame_size_ / (1024*1024));
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
applications/st2110_test/.gitignoreapplications/st2110_test/CMakeLists.txtapplications/st2110_test/README.mdapplications/st2110_test/SETUP.mdapplications/st2110_test/metadata.jsonapplications/st2110_test/run.shapplications/st2110_test/run_nv12_only.shapplications/st2110_test/run_rgba_only.shapplications/st2110_test/st2110_test_app.pyapplications/st2110_test/st2110_test_config.yamlapplications/st2110_test/st2110_test_config_nv12_only.yamlapplications/st2110_test/st2110_test_config_rgba_only.yamloperators/CMakeLists.txtoperators/st2110_source/BUILD.mdoperators/st2110_source/CMakeLists.txtoperators/st2110_source/README.mdoperators/st2110_source/metadata.jsonoperators/st2110_source/python/CMakeLists.txtoperators/st2110_source/python/st2110_source.cppoperators/st2110_source/python/st2110_source_pydoc.hppoperators/st2110_source/st2110_kernels.cuoperators/st2110_source/st2110_kernels.cuhoperators/st2110_source/st2110_source.cppoperators/st2110_source/st2110_source.hpp
🧰 Additional context used
📓 Path-based instructions (7)
**/operators/**/CMakeLists.txt
📄 CodeRabbit inference engine (CONTRIBUTING.md)
CMakeLists.txt for operators must use 'add_holohub_operator()' with optional 'DEPENDS EXTENSIONS' for dependent extensions
Files:
operators/st2110_source/CMakeLists.txtoperators/CMakeLists.txtoperators/st2110_source/python/CMakeLists.txt
**/metadata.json
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
When reviewing PRs that modify or add
metadata.jsonfiles, verify that the category (first tag in thetagsarray) matches 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/st2110_test/metadata.jsonoperators/st2110_source/metadata.json
**/operators/**/*.{py,cpp,hpp}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Class names for operators should use TitleCase with 'Op' suffix (e.g., 'AdaptiveThresholdingOp')
Files:
operators/st2110_source/python/st2110_source_pydoc.hppoperators/st2110_source/python/st2110_source.cppoperators/st2110_source/st2110_source.hppoperators/st2110_source/st2110_source.cpp
**/*.{py,cpp,hpp}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.{py,cpp,hpp}: Code must adhere to Holoscan SDK coding standards including style compliance, descriptive naming, and minimal abbreviations
Code must include appropriate error handling and validation for all operations
All code must pass linting checks using './holohub lint' before submission
All code contributions must use clear, English descriptive names for functions and variables
Code must include inline comments for complex logic and clear documentation of functionality
Files:
operators/st2110_source/python/st2110_source_pydoc.hppapplications/st2110_test/st2110_test_app.pyoperators/st2110_source/python/st2110_source.cppoperators/st2110_source/st2110_source.hppoperators/st2110_source/st2110_source.cpp
**/applications/**/CMakeLists.txt
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/applications/**/CMakeLists.txt: CMakeLists.txt for applications must use 'add_holohub_application()' with optional 'DEPENDS OPERATORS' for dependent operators
Applications should include a testing section in CMakeLists.txt for functional testing using CTest
Files:
applications/st2110_test/CMakeLists.txt
**/operators/**/*.{py,cpp}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Operator filename should match the directory name with appropriate extension (e.g., 'adaptive_thresholding.py')
Files:
operators/st2110_source/python/st2110_source.cppoperators/st2110_source/st2110_source.cpp
**/operators/**/*.cpp
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Operators with C++ implementations must include CMakeLists.txt for build system integration
Files:
operators/st2110_source/python/st2110_source.cppoperators/st2110_source/st2110_source.cpp
🧠 Learnings (28)
📓 Common learnings
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.
📚 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/st2110_test/st2110_test_config.yamlapplications/st2110_test/st2110_test_config_rgba_only.yamlapplications/st2110_test/st2110_test_config_nv12_only.yaml
📚 Learning: 2025-12-18T16:19:45.117Z
Learnt from: CR
Repo: nvidia-holoscan/holohub PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2025-12-18T16:19:45.117Z
Learning: Applies to **/operators/**/CMakeLists.txt : CMakeLists.txt for operators must use 'add_holohub_operator()' with optional 'DEPENDS EXTENSIONS' for dependent extensions
Applied to files:
operators/st2110_source/CMakeLists.txtoperators/CMakeLists.txtoperators/st2110_source/BUILD.mdoperators/st2110_source/python/CMakeLists.txt
📚 Learning: 2025-12-18T16:19:45.117Z
Learnt from: CR
Repo: nvidia-holoscan/holohub PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2025-12-18T16:19:45.117Z
Learning: Applies to **/applications/**/CMakeLists.txt : CMakeLists.txt for applications must use 'add_holohub_application()' with optional 'DEPENDS OPERATORS' for dependent operators
Applied to files:
operators/st2110_source/CMakeLists.txtoperators/CMakeLists.txtapplications/st2110_test/CMakeLists.txtoperators/st2110_source/python/CMakeLists.txt
📚 Learning: 2025-12-18T16:19:45.117Z
Learnt from: CR
Repo: nvidia-holoscan/holohub PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2025-12-18T16:19:45.117Z
Learning: Applies to **/pkg/CMakeLists.txt : CMakeLists.txt for packages must use 'add_holohub_package()' function with optional APPLICATIONS and OPERATORS dependencies
Applied to files:
operators/st2110_source/CMakeLists.txtoperators/CMakeLists.txtoperators/st2110_source/python/CMakeLists.txt
📚 Learning: 2025-12-18T16:19:45.117Z
Learnt from: CR
Repo: nvidia-holoscan/holohub PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2025-12-18T16:19:45.117Z
Learning: Applies to **/operators/**/*.cpp : Operators with C++ implementations must include CMakeLists.txt for build system integration
Applied to files:
operators/st2110_source/CMakeLists.txtoperators/CMakeLists.txtoperators/st2110_source/BUILD.md
📚 Learning: 2025-12-18T16:19:45.117Z
Learnt from: CR
Repo: nvidia-holoscan/holohub PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2025-12-18T16:19:45.117Z
Learning: Applies to **/workflows/**/CMakeLists.txt : CMakeLists.txt for workflows must use 'add_holohub_application()' with optional 'DEPENDS OPERATORS' for dependent operators
Applied to files:
operators/st2110_source/CMakeLists.txtoperators/CMakeLists.txt
📚 Learning: 2025-12-18T16:19:45.117Z
Learnt from: CR
Repo: nvidia-holoscan/holohub PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2025-12-18T16:19:45.117Z
Learning: Applies to **/pkg/**/CMakeLists.txt : CMakeLists.txt for packages must use 'holohub_configure_deb()' with required NAME, DESCRIPTION, VERSION, VENDOR, CONTACT and optional SECTION, PRIORITY parameters
Applied to files:
operators/st2110_source/CMakeLists.txt
📚 Learning: 2026-01-12T16:04:04.046Z
Learnt from: tbirdso
Repo: nvidia-holoscan/holohub PR: 1322
File: applications/bci_visualization/operators/color_buffer_passthrough/CMakeLists.txt:16-46
Timestamp: 2026-01-12T16:04:04.046Z
Learning: Application-specific operators under applications/**/operators/ that are tightly coupled with their application don't require the add_holohub_operator() macro and can use standard CMake add_library() with manual configuration instead.
Applied to files:
operators/st2110_source/CMakeLists.txtoperators/CMakeLists.txtoperators/st2110_source/python/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:
operators/CMakeLists.txtoperators/st2110_source/python/CMakeLists.txtoperators/st2110_source/python/st2110_source.cpp
📚 Learning: 2025-11-26T18:26:56.776Z
Learnt from: cliffburdick
Repo: nvidia-holoscan/holohub PR: 1261
File: operators/advanced_network/advanced_network/managers/dpdk/CMakeLists.txt:43-54
Timestamp: 2025-11-26T18:26:56.776Z
Learning: The advanced_network operator in operators/advanced_network only supports Mellanox NICs, as documented in its requirements. Hardcoded Mellanox-specific libraries (e.g., -lmlx5, -libverbs) in CMakeLists.txt are intentional and appropriate for this operator.
Applied to files:
operators/CMakeLists.txtoperators/st2110_source/BUILD.md
📚 Learning: 2025-10-20T22:19:33.520Z
Learnt from: chengronglai
Repo: nvidia-holoscan/holohub PR: 1136
File: benchmarks/realtime_threads_benchmarking/realtime_thread_benchmark.cpp:245-251
Timestamp: 2025-10-20T22:19:33.520Z
Learning: In the Holoscan framework, operators added via add_operator() can execute without explicitly attaching a scheduling condition - they will still be scheduled and their compute() method will be invoked by the runtime.
Applied to files:
operators/CMakeLists.txt
📚 Learning: 2026-01-14T02:06:54.516Z
Learnt from: tbirdso
Repo: nvidia-holoscan/holohub PR: 1344
File: operators/ucxx_send_receive/metadata.json:1-61
Timestamp: 2026-01-14T02:06:54.516Z
Learning: In operators/**/metadata.json files: Multiple operator metadata entries can be defined in a single metadata.json file using an array structure at the root level (e.g., `[{"operator": {...}}, {"operator": {...}}]`). The HoloHub CLI supports this format for describing multiple projects in one folder.
Applied to files:
operators/CMakeLists.txtapplications/st2110_test/metadata.json
📚 Learning: 2025-12-18T16:19:45.117Z
Learnt from: CR
Repo: nvidia-holoscan/holohub PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2025-12-18T16:19:45.117Z
Learning: Applies to **/(@(operators|applications|workflows|gxf_extensions|tutorials))/**/metadata.json : Every application, operator, workflow, and extension must include a 'metadata.json' file following the appropriate schema
Applied to files:
applications/st2110_test/metadata.jsonoperators/st2110_source/metadata.json
📚 Learning: 2025-12-18T16:19:45.117Z
Learnt from: CR
Repo: nvidia-holoscan/holohub PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2025-12-18T16:19:45.117Z
Learning: Applies to **/(@(operators|applications|workflows|gxf_extensions|tutorials))/**/metadata.json : metadata.json must include 'holoscan_sdk' with 'minimum_required_version' and 'tested_versions' fields
Applied to files:
applications/st2110_test/metadata.jsonoperators/st2110_source/metadata.json
📚 Learning: 2025-12-18T16:19:45.117Z
Learnt from: CR
Repo: nvidia-holoscan/holohub PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2025-12-18T16:19:45.117Z
Learning: Applies to **/(@(operators|applications|workflows|gxf_extensions|tutorials))/**/metadata.json : metadata.json must include 'language' field specifying 'C++', 'Python', or 'GXF'
Applied to files:
applications/st2110_test/metadata.jsonoperators/st2110_source/metadata.json
📚 Learning: 2025-12-18T16:19:45.117Z
Learnt from: CR
Repo: nvidia-holoscan/holohub PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2025-12-18T16:19:45.117Z
Learning: Applies to **/(@(operators|applications|workflows|gxf_extensions|tutorials))/**/metadata.json : metadata.json must include 'name' field matching the class name (TitleCase with 'Op' for operators)
Applied to files:
applications/st2110_test/metadata.jsonoperators/st2110_source/metadata.json
📚 Learning: 2025-12-18T16:19:45.117Z
Learnt from: CR
Repo: nvidia-holoscan/holohub PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2025-12-18T16:19:45.117Z
Learning: Applies to **/(@(operators|applications|workflows|gxf_extensions|tutorials))/**/metadata.json : metadata.json must include 'platforms' array listing supported architectures (e.g., 'x86_64', 'aarch64')
Applied to files:
applications/st2110_test/metadata.json
📚 Learning: 2025-12-18T16:19:45.117Z
Learnt from: CR
Repo: nvidia-holoscan/holohub PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2025-12-18T16:19:45.117Z
Learning: Applies to **/(@(operators|applications|workflows|gxf_extensions|tutorials))/**/metadata.json : metadata.json must include 'version' field in major.minor.patch format
Applied to files:
applications/st2110_test/metadata.json
📚 Learning: 2025-10-30T19:04:41.239Z
Learnt from: bhashemian
Repo: nvidia-holoscan/holohub PR: 1203
File: applications/video_streaming/video_streaming_server/cpp/metadata.json:42-51
Timestamp: 2025-10-30T19:04:41.239Z
Learning: In the video_streaming application structure, umbrella metadata (applications/video_streaming/metadata.json) uses language-qualified mode names like "server_cpp" and "server_python" to distinguish between implementations, while component-specific metadata files (e.g., applications/video_streaming/video_streaming_server/cpp/metadata.json) use simple mode names like "server" since they are already scoped to a specific language implementation by their directory structure.
Applied to files:
applications/st2110_test/metadata.json
📚 Learning: 2025-11-24T16:27:43.600Z
Learnt from: CR
Repo: nvidia-holoscan/holohub PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-24T16:27:43.600Z
Learning: Applies to **/metadata.json : When reviewing PRs that modify or add `metadata.json` files, verify that the **category** (first tag in the `tags` array) matches 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
Applied to files:
applications/st2110_test/metadata.json
📚 Learning: 2025-12-18T16:19:45.117Z
Learnt from: CR
Repo: nvidia-holoscan/holohub PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2025-12-18T16:19:45.117Z
Learning: Applies to **/(@(operators|applications|workflows))/**/metadata.json : Operators, applications, and workflows should use the '<holohub_data_dir>' placeholder in metadata.json 'run' command for referencing data directory
Applied to files:
applications/st2110_test/metadata.json
📚 Learning: 2026-01-14T02:06:54.516Z
Learnt from: tbirdso
Repo: nvidia-holoscan/holohub PR: 1344
File: operators/ucxx_send_receive/metadata.json:1-61
Timestamp: 2026-01-14T02:06:54.516Z
Learning: When reviewing metadata.json under operators/**, accept that the root can be an array of objects for multiple operator entries, e.g. [{"operator": {...}}, {"operator": {...}}]. Ensure tooling and downstream processes parse the root as an array and validate each entry's operator object structure. Maintain this format for folders containing multiple operator definitions.
Applied to files:
operators/st2110_source/metadata.json
📚 Learning: 2025-12-18T16:19:45.117Z
Learnt from: CR
Repo: nvidia-holoscan/holohub PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2025-12-18T16:19:45.117Z
Learning: Applies to **/(@(operators|applications|workflows|gxf_extensions))/**/README.md : Every application and operator must include a comprehensive README.md file with purpose, usage, requirements, examples, and architecture information
Applied to files:
operators/st2110_source/BUILD.mdoperators/st2110_source/README.mdapplications/st2110_test/README.md
📚 Learning: 2025-12-18T16:19:45.117Z
Learnt from: CR
Repo: nvidia-holoscan/holohub PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2025-12-18T16:19:45.117Z
Learning: Applies to **/applications/**/CMakeLists.txt : Applications should include a testing section in CMakeLists.txt for functional testing using CTest
Applied to files:
applications/st2110_test/CMakeLists.txt
📚 Learning: 2025-12-18T16:19:45.117Z
Learnt from: CR
Repo: nvidia-holoscan/holohub PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2025-12-18T16:19:45.117Z
Learning: Applies to **/(@(applications|workflows))/**/CMakeLists.txt : Applications and workflows must include CMakeLists.txt for build system integration
Applied to files:
applications/st2110_test/CMakeLists.txt
📚 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/st2110_source/README.md
📚 Learning: 2026-01-13T23:11:34.601Z
Learnt from: GilShpr
Repo: nvidia-holoscan/holohub PR: 1342
File: operators/gstreamer/gst_src_op.cpp:34-36
Timestamp: 2026-01-13T23:11:34.601Z
Learning: In operator compute() implementations, avoid silently handling empty inputs from input.receive<T>(). The optional from input.receive<T>() should be dereferenced with value() only when data is guaranteed to be available. If receive() returns empty, this indicates a pipeline error and should surface immediately by throwing an exception (e.g., std::bad_optional_access) to avoid masking dropped frames. Ensure code patterns consistently dereference with value() only when input data is known to exist, and allow the exception to propagate for debugging. This guideline applies to all operator implementations under the operators/ directory (e.g., operators/gstreamer/gst_src_op.cpp) and similar CPP files that implement compute().
Applied to files:
operators/st2110_source/python/st2110_source.cppoperators/st2110_source/st2110_source.cpp
🧬 Code graph analysis (2)
operators/st2110_source/python/st2110_source.cpp (2)
operators/st2110_source/st2110_source.cpp (3)
ST2110SourceOp(50-57)setup(59-183)setup(59-59)operators/st2110_source/st2110_source.hpp (2)
ST2110SourceOp(89-89)ST2110SourceOp(90-90)
operators/st2110_source/st2110_source.hpp (1)
operators/st2110_source/st2110_source.cpp (1)
ST2110SourceOp(50-57)
🪛 Clang (14.0.6)
operators/st2110_source/python/st2110_source_pydoc.hpp
[error] 21-21: 'string' file not found
(clang-diagnostic-error)
operators/st2110_source/st2110_source.hpp
[error] 21-21: 'array' file not found
(clang-diagnostic-error)
🪛 Cppcheck (2.19.0)
operators/st2110_source/python/st2110_source.cpp
[error] 87-87: There is an unknown macro here somewhere. Configuration is required. If HOLOSCAN_OPERATOR_FORWARD_ARGS is a macro then please configure it.
(unknownMacro)
operators/st2110_source/st2110_source.cpp
[error] 87-87: There is an unknown macro here somewhere. Configuration is required. If HOLOSCAN_OPERATOR_FORWARD_ARGS is a macro then please configure it.
(unknownMacro)
🪛 markdownlint-cli2 (0.18.1)
applications/st2110_test/SETUP.md
59-59: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
65-65: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
98-98: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
operators/st2110_source/README.md
20-20: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
applications/st2110_test/README.md
40-40: Multiple headings with the same content
(MD024, no-duplicate-heading)
229-229: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
257-257: Bare URL used
(MD034, no-bare-urls)
🪛 Ruff (0.14.11)
applications/st2110_test/st2110_test_app.py
46-46: Unused method argument: op_output
(ARG002)
46-46: Unused method argument: context
(ARG002)
68-69: try-except-pass detected, consider logging the exception
(S110)
68-68: Do not catch blind exception: Exception
(BLE001)
70-70: Do not catch blind exception: Exception
(BLE001)
🪛 Shellcheck (0.11.0)
applications/st2110_test/run_rgba_only.sh
[warning] 19-19: Quote this to prevent word splitting.
(SC2046)
[warning] 19-19: Quote this to prevent word splitting.
(SC2046)
[warning] 21-21: Quote this to prevent word splitting.
(SC2046)
applications/st2110_test/run.sh
[warning] 76-76: Quote this to prevent word splitting.
(SC2046)
[warning] 76-76: Quote this to prevent word splitting.
(SC2046)
[warning] 78-78: Quote this to prevent word splitting.
(SC2046)
applications/st2110_test/run_nv12_only.sh
[warning] 19-19: Quote this to prevent word splitting.
(SC2046)
[warning] 19-19: Quote this to prevent word splitting.
(SC2046)
[warning] 21-21: Quote this to prevent word splitting.
(SC2046)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🔇 Additional comments (33)
applications/st2110_test/.gitignore (1)
1-16: LGTM!Standard and appropriate ignore patterns for a Python test application covering virtual environments, bytecode, IDE configs, and log outputs.
operators/st2110_source/README.md (1)
1-242: LGTM! Comprehensive and well-structured documentation.The README covers all required sections: purpose, architecture, output ports, parameters, requirements, usage examples (Python and C++), building instructions, network configuration, supported formats, troubleshooting, and platform support. This is excellent operator documentation.
The static analysis hint about the code fence at line 20 lacking a language is a false positive — it's an ASCII diagram, not executable code.
operators/st2110_source/BUILD.md (1)
1-166: LGTM! Thorough build documentation.Excellent coverage of prerequisites, build options, environment setup, common issues with solutions, and platform-specific notes.
operators/st2110_source/metadata.json (1)
1-34: Metadata structure is correct.The metadata follows the required schema with proper
name(TitleCase + Op suffix),holoscan_sdkversioning,language, andplatformsfields.applications/st2110_test/SETUP.md (1)
44-70: Documentation inconsistent with PR objectives.The PR description explicitly states: "Implementation uses standard Linux sockets instead of DPDK for portability across Jetson devices." However, this setup guide extensively documents DPDK binding,
dpdk-devbind.py, andvfio-pcidriver configuration.Please clarify: does the operator use Linux sockets or DPDK? If it uses Linux sockets as stated in the PR, this documentation section should be removed or updated to reflect the actual network configuration required.
operators/st2110_source/st2110_kernels.cu (2)
147-209: LGTM - YCbCr to RGBA conversion kernel.The 10-bit big-endian unpacking and BT.709 color space conversion logic appears correct. The normalized coefficient approach differs from the integer form in the header comment but produces equivalent results.
26-67: LGTM - Frame reassembly kernel.The line-based addressing using ST2110 header metadata is correctly implemented. The coalesced 4-byte access pattern with proper remainder handling is well-designed for GPU efficiency.
operators/st2110_source/st2110_kernels.cuh (1)
1-132: LGTM - Well-documented kernel header.The header provides comprehensive documentation for the CUDA kernel interfaces, correctly uses
extern "C"linkage for C compatibility, and applies visibility attributes for symbol export.applications/st2110_test/README.md (1)
1-11: Documentation references DPDK but PR states Linux sockets are used.Similar to SETUP.md, this README describes DPDK configuration while the PR objectives state the implementation uses "standard Linux sockets instead of DPDK." Please reconcile the documentation with the actual implementation.
operators/CMakeLists.txt (1)
44-44: Operator declaration follows holohub conventions.The use of
add_holohub_operator()withDEPENDS OPERATORS advanced_networkcorrectly declares the dependency on the advanced_network operator. Based on coding guidelines, this is the required pattern for operator CMakeLists.txt files.operators/st2110_source/python/CMakeLists.txt (1)
16-21: LGTM!The Python binding configuration correctly uses
pybind11_add_holohub_modulemacro with appropriate parameters. Based on learnings, this macro encapsulates all pybind11 setup internally, so no additional configuration is needed.applications/st2110_test/run_nv12_only.sh (1)
24-25: Inconsistent container image between test scripts.This script uses
holohub:ngc-v3.7.0-cuda13whilerun.shusesnvcr.io/nvidia/clara-holoscan/holoscan:v3.8.0-cuda13. Please verify this is intentional, as it could lead to inconsistent test behavior.applications/st2110_test/metadata.json (1)
1-42: LGTM!The metadata file is well-structured with all required fields. The category "Networking and Distributed Computing" is valid per the approved categories list. As per coding guidelines, the first tag in the tags array correctly identifies the category.
applications/st2110_test/st2110_test_config_rgba_only.yaml (1)
1-54: LGTM!The RGBA-only configuration is well-documented with clear comments indicating the active/inactive state of each visualizer block. The presence of the inactive
nv12_visualizerblock is appropriately explained and maintains consistency with other configuration variants.applications/st2110_test/run.sh (1)
1-19: Well-documented script header.The comprehensive documentation clearly explains the test outputs, prerequisites, and usage, which is excellent for maintainability.
applications/st2110_test/st2110_test_config.yaml (1)
1-57: LGTM! Well-structured configuration with comprehensive inline documentation.The configuration cleanly separates network, video, output, and performance parameters. Comments effectively explain the purpose of each section and the relationship between output toggles and visualizer activation.
operators/st2110_source/python/st2110_source_pydoc.hpp (1)
29-100: Comprehensive and well-structured Python documentation.The documentation clearly covers the operator's purpose, all three output ports with their formats, and all configurable parameters with sensible defaults. The notes section about requirements (multicast support, socket buffers) is helpful for users.
operators/st2110_source/CMakeLists.txt (1)
23-28: Review comment is based on misinterpreting the coding guideline.The
add_holohub_operator()macro is used in the parentoperators/CMakeLists.txt(which already callsadd_holohub_operator(st2110_source DEPENDS OPERATORS advanced_network)), not in individual operator CMakeLists.txt files. All operators in the repository—includingaja_sourceandvolume_renderer—use standard CMakeadd_library()calls in their individual CMakeLists.txt files. Thest2110_source/CMakeLists.txtfollows the correct repository pattern.applications/st2110_test/run_rgba_only.sh (1)
24-24: The hardcoded Docker image tag requires verification for availability and should be parameterized if used across multiple test scripts.The image
holohub:ngc-v3.7.0-cuda13is hardcoded in bothrun_rgba_only.shandrun_nv12_only.shtest scripts. While the tag format suggests an NGC (NVIDIA GPU Cloud) origin, there is no documentation in the repository explaining the image's provenance or how to obtain it. Additionally, the official setup documentation inSETUP.mdandREADME.mdrecommends a Python virtual environment approach withpip install holoscan-cu13rather than Docker containers. Consider:
- Documenting where this image comes from and how to build or pull it
- Adding an environment variable to parameterize the image name if Docker approach is intentional
- Clarifying why Docker is used in test scripts when the primary setup uses venv
applications/st2110_test/st2110_test_app.py (2)
56-71: Exception handling is broad but acceptable for a test utility.The static analysis flags bare
Exceptioncatches. While this is generally discouraged, the current pattern is acceptable for a test/debug utility that probes optional tensor metadata. The innertry-except-pass(lines 68-69) silently swallows errors when tensor names don't exist, which is the intended behavior here.No action required, but consider using more specific exceptions if this code is promoted to production.
74-134: LGTM!Pipeline composition is correct. The ST2110SourceOp feeds all three outputs to their respective monitors, and RGBA/NV12 outputs are additionally routed to Holoviz visualizers for display.
operators/st2110_source/python/st2110_source.cpp (2)
51-94: LGTM!The trampoline class correctly wraps ST2110SourceOp for Python with all parameter defaults matching the C++
setup()method. The pattern follows Holoscan conventions for Python bindings.
96-153: LGTM!The pybind11 module correctly exposes ST2110SourceOp with all parameters and appropriate defaults. The binding follows Holoscan SDK conventions.
operators/st2110_source/st2110_source.hpp (3)
85-98: LGTM!The class declaration follows Holoscan operator conventions with proper lifecycle method overrides. The
HOLOSCAN_OPERATOR_FORWARD_ARGSmacro enables argument forwarding for the operator.
101-114: LGTM!The RTP and ST2110 header structs are correctly defined with
__attribute__((packed))to ensure proper byte alignment for network protocol parsing per RFC 3550 and SMPTE ST 2110-20.
180-186: Verify atomic usage necessity.
assembling_index_usesstd::atomic<int>but the operator appears to run single-threaded (compute() driven by PeriodicCondition). If no concurrent access exists, plainintwould suffice. If this is defensive coding for future multi-threading, consider adding a comment explaining the rationale.operators/st2110_source/st2110_source.cpp (7)
283-337: LGTM with note on error handling.Buffer allocation is well-structured with proper alignment. The use of
CU_MEMHOSTALLOC_WRITECOMBINEDis appropriate for CPU-to-GPU transfers. Note thatcuMemHostAllocfailures throw exceptions (correct), butcudaMallocfailures only log errors (see earlier comment about CUDA_TRY).
339-371: LGTM!Resource cleanup is comprehensive with proper null checks and pointer invalidation. The pattern of not checking cleanup function return values is acceptable during shutdown.
373-441: LGTM!Socket creation is well-implemented with proper error handling, multicast group subscription, and helpful diagnostics for system buffer limits. The non-blocking socket is appropriate for the polling-based packet reception pattern.
837-885: LGTM!Raw frame emission correctly wraps the GPU buffer in a GXF Tensor with appropriate shape and memory management. The no-op deallocator is correct since the operator owns the buffer lifecycle.
887-992: LGTM!RGBA conversion correctly uses CUDA kernel for YCbCr-4:2:2-10bit to RGBA conversion. The additional device-to-device copy to the GXF-allocated tensor is necessary for safe interop with Python operators (prevents race conditions).
1102-1158: LGTM!The compute() method correctly implements the two-phase drain-and-emit pattern. As a source operator with no inputs, there's no input.receive() to validate. The early return when no frame is ready is appropriate behavior that allows continued socket draining on subsequent calls.
616-626: No issue found with offset calculation — it correctly implements ST2110-20 specification.The Offset field in the ST2110-20 RTP payload header is specified in units of pixels (luminance samples for YCbCr), not pixel groups. The code correctly multiplies
line_offsetbybytes_per_pixelto convert the pixel offset to a byte offset within the frame buffer. For 10-bit 4:2:2 sampling (5 bytes per pgroup of 2 pixels, giving 2.5 bytes per pixel), this calculation produces correct byte-aligned results since valid line offsets conform to pgroup boundaries.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| # SPDX-FileCopyrightText: Copyright (c) 2025 XRlabs. All rights reserved. | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| cmake_minimum_required(VERSION 3.24) | ||
| project(st2110_test) | ||
|
|
||
| # This is a Python-only test application | ||
| # No C++ compilation required | ||
|
|
||
| # Install Python application files | ||
| install(FILES | ||
| st2110_test_app.py | ||
| st2110_test_config.yaml | ||
| README.md | ||
| DESTINATION examples/st2110_test | ||
| PERMISSIONS OWNER_READ OWNER_WRITE OWNER_EXECUTE | ||
| GROUP_READ GROUP_EXECUTE | ||
| WORLD_READ WORLD_EXECUTE | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use add_holohub_application() macro and add testing section.
Per HoloHub conventions, application CMakeLists.txt should use add_holohub_application() with DEPENDS OPERATORS for dependent operators. Additionally, applications should include a testing section for functional testing using CTest.
The current install() approach also applies executable permissions to non-executable files (README.md, YAML config).
Suggested structure
# SPDX-FileCopyrightText: Copyright (c) 2025 XRlabs. All rights reserved.
# SPDX-License-Identifier: Apache-2.0
cmake_minimum_required(VERSION 3.24)
-project(st2110_test)
-
-# This is a Python-only test application
-# No C++ compilation required
-
-# Install Python application files
-install(FILES
- st2110_test_app.py
- st2110_test_config.yaml
- README.md
- DESTINATION examples/st2110_test
- PERMISSIONS OWNER_READ OWNER_WRITE OWNER_EXECUTE
- GROUP_READ GROUP_EXECUTE
- WORLD_READ WORLD_EXECUTE
-)
+
+add_holohub_application(st2110_test
+ PYTHON
+ DEPENDS OPERATORS st2110_source
+)
+
+# Testing section
+if(BUILD_TESTING)
+ # Add CTest-based tests here when test infrastructure is available
+endif()As per coding guidelines, applications must use add_holohub_application() and include testing.
🤖 Prompt for AI Agents
In `@applications/st2110_test/CMakeLists.txt` around lines 1 - 19, Replace the
current raw install() usage with the project's application macro: call
add_holohub_application(<name> DEPENDS OPERATORS <operator_list>) to register
this Python-only app and declare operator dependencies; remove executable
permissions from non-executable files in the install step so only the Python
script gets OWNER/ GROUP/ WORLD execute flags; and add a CTest-based testing
section (enable_testing() and add_test(...) or add_holohub_application test
helper if available) to run the functional tests for st2110_test_app.py. Ensure
you update references to st2110_test_app.py, st2110_test_config.yaml and
README.md in the new install macro or install rules so permissions and
destinations are correct.
| -v ${HOLOHUB_ROOT}:/workspace \ | ||
| -v /tmp/.X11-unix:/tmp/.X11-unix \ | ||
| -v /run/user/$(id -u):/run/user/$(id -u) \ | ||
| -e DISPLAY=$DISPLAY \ | ||
| -e XDG_RUNTIME_DIR=/run/user/$(id -u) \ | ||
| -e PYTHONPATH=/opt/nvidia/holoscan/python/lib:/workspace/build/st2110_source/python/lib \ | ||
| -e LD_LIBRARY_PATH=/workspace/build/st2110_source/operators/st2110_source:/opt/nvidia/holoscan/lib \ |
There was a problem hiding this comment.
Quote variables to prevent word splitting.
Several variables in the docker run command should be quoted to prevent issues with paths containing spaces or special characters.
Proposed fix
- -v ${HOLOHUB_ROOT}:/workspace \
+ -v "${HOLOHUB_ROOT}:/workspace" \
-v /tmp/.X11-unix:/tmp/.X11-unix \
- -v /run/user/$(id -u):/run/user/$(id -u) \
- -e DISPLAY=$DISPLAY \
- -e XDG_RUNTIME_DIR=/run/user/$(id -u) \
+ -v "/run/user/$(id -u):/run/user/$(id -u)" \
+ -e DISPLAY="$DISPLAY" \
+ -e XDG_RUNTIME_DIR="/run/user/$(id -u)" \
-e PYTHONPATH=/opt/nvidia/holoscan/python/lib:/workspace/build/st2110_source/python/lib \
-e LD_LIBRARY_PATH=/workspace/build/st2110_source/operators/st2110_source:/opt/nvidia/holoscan/lib \📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| -v ${HOLOHUB_ROOT}:/workspace \ | |
| -v /tmp/.X11-unix:/tmp/.X11-unix \ | |
| -v /run/user/$(id -u):/run/user/$(id -u) \ | |
| -e DISPLAY=$DISPLAY \ | |
| -e XDG_RUNTIME_DIR=/run/user/$(id -u) \ | |
| -e PYTHONPATH=/opt/nvidia/holoscan/python/lib:/workspace/build/st2110_source/python/lib \ | |
| -e LD_LIBRARY_PATH=/workspace/build/st2110_source/operators/st2110_source:/opt/nvidia/holoscan/lib \ | |
| -v "${HOLOHUB_ROOT}:/workspace" \ | |
| -v /tmp/.X11-unix:/tmp/.X11-unix \ | |
| -v "/run/user/$(id -u):/run/user/$(id -u)" \ | |
| -e DISPLAY="$DISPLAY" \ | |
| -e XDG_RUNTIME_DIR="/run/user/$(id -u)" \ | |
| -e PYTHONPATH=/opt/nvidia/holoscan/python/lib:/workspace/build/st2110_source/python/lib \ | |
| -e LD_LIBRARY_PATH=/workspace/build/st2110_source/operators/st2110_source:/opt/nvidia/holoscan/lib \ |
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 76-76: Quote this to prevent word splitting.
(SC2046)
[warning] 76-76: Quote this to prevent word splitting.
(SC2046)
[warning] 78-78: Quote this to prevent word splitting.
(SC2046)
🤖 Prompt for AI Agents
In `@applications/st2110_test/run.sh` around lines 74 - 80, The docker run
arguments in run.sh use unquoted variable expansions (e.g. ${HOLOHUB_ROOT}, $(id
-u), $DISPLAY, $XDG_RUNTIME_DIR, $PYTHONPATH, $LD_LIBRARY_PATH) which can break
when values contain spaces or special chars; update each occurrence to use
double quotes around the expansion (for example -v "${HOLOHUB_ROOT}:/workspace",
-v "/run/user/$(id -u):/run/user/$(id -u)", and -e "DISPLAY=$DISPLAY", -e
"XDG_RUNTIME_DIR=/run/user/$(id -u)", -e "PYTHONPATH=$PYTHONPATH", -e
"LD_LIBRARY_PATH=$LD_LIBRARY_PATH") so the values are preserved as single
arguments and not split by the shell.
| // Check for frame boundary (new timestamp) | ||
| if (recv_buffer->current_timestamp != 0 && timestamp != recv_buffer->current_timestamp) { | ||
| // Mark current frame complete | ||
| recv_buffer->frame_complete = true; |
There was a problem hiding this comment.
logic: Race condition: frame_complete flag is not atomic but accessed from both packet reception path (this line) and frame emission path (find_next_frame_to_emit at line 679). If emit_next_frame() is reading this buffer while it's being marked complete here, it could see inconsistent state.
| recv_buffer->frame_complete = true; | |
| std::atomic_thread_fence(std::memory_order_release); | |
| recv_buffer->frame_complete = true; |
Consider making frame_complete an std::atomic<bool> in the FrameBuffer struct, or add memory barriers to ensure proper synchronization between packet reception and frame emission threads.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@applications/st2110_test/run_nv12_only.sh`:
- Line 24: The container image in run_nv12_only.sh is out of sync: replace the
image string "holohub:ngc-v3.7.0-cuda13" with the same image used by run.sh
("nvcr.io/nvidia/clara-holoscan/holoscan:v3.8.0-cuda13") so the NV12-only runner
uses the identical container; update the image reference wherever
"holohub:ngc-v3.7.0-cuda13" appears in run_nv12_only.sh.
In `@operators/st2110_source/st2110_source.cpp`:
- Around line 310-314: The GPU allocation via
CUDA_TRY(cudaMalloc(&gpu_raw_buffer_, raw_frame_size_)) currently only logs
failures and leaves gpu_raw_buffer_ null, which can cause crashes later (e.g.,
in emit_next_frame() when calling cudaMemcpyAsync). Change the allocation
handling in the block that allocates gpu_raw_buffer_ to check the cudaMalloc
return and on failure throw or return early (consistent with the earlier
cuMemHostAlloc handling): capture the CUDA error/result, log the error including
the result and raw_frame_size_, and then throw an exception or return an error
so gpu_raw_buffer_ is never left null before emit_next_frame() is called. Ensure
emit_next_frame() relies on gpu_raw_buffer_ being valid or checks for null as a
defensive measure.
🧹 Nitpick comments (5)
operators/st2110_source/metadata.json (1)
10-10: Version format should use major.minor.patch.The version field uses "1.0" but the metadata schema requires semantic versioning in
major.minor.patchformat.Suggested fix
- "version": "1.0", + "version": "1.0.0",Based on learnings, metadata.json must include 'version' field in major.minor.patch format.
operators/st2110_source/st2110_kernels.cu (3)
189-211: Color conversion implementation differs from documented formula.The comment (lines 152-155) documents BT.709 integer coefficients, but the code uses a normalized approach. While both can be correct, the coefficients differ:
- Comment:
R = 1.164 * (Y - 64) + 1.793 * (Cr - 512)- Code uses:
R = Y_norm + 1.5748 * Cr_normThe normalized coefficients (1.5748, 0.1873, 0.4681, 1.8556) appear to be for BT.709 full-range output, which is appropriate here. Consider updating the comment to match the actual implementation or add a note explaining the normalization approach.
274-276: Y plane conversion uses truncation instead of rounding.The 10-bit to 8-bit conversion uses simple right-shift (
>> 2) which truncates rather than rounds. For higher fidelity output, consider rounding:// Instead of: y_plane[...] = uint8_t(Y0_top >> 2); // Consider: y_plane[...] = uint8_t((Y0_top + 2) >> 2);This is a minor quality improvement and not strictly required.
Also applies to: 298-300
113-140: Missing CUDA error checking after kernel launches.The host wrappers launch kernels but don't check for launch errors. Asynchronous kernel errors won't be caught until the next synchronous CUDA call, which could make debugging difficult.
Consider adding error checking:
Suggested pattern
st2110_frame_reassembly_kernel<<<num_pkts, threads_per_block, 0, stream>>>( out, in, line_numbers, line_offsets, line_lengths, num_pkts, line_stride); + // Check for launch errors (optional but helpful for debugging) + cudaError_t err = cudaGetLastError(); + if (err != cudaSuccess) { + // Handle error or log + }operators/st2110_source/st2110_source.cpp (1)
210-211: Dead initialization:raw_frame_size_is immediately overwritten.Line 211 sets
raw_frame_size_to a conservative estimate, butconfigure_stream_format()(called on line 193) already sets the correct value at line 484. The line 211 assignment has no effect.Suggested fix
- // Raw frame size calculated after format detection (default: 10-bit 4:2:2 = 2.5 bpp) - raw_frame_size_ = width_.get() * height_.get() * 3; // Conservative: 3 bytes/pixel + // raw_frame_size_ is set by configure_stream_format() above
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
applications/st2110_test/CMakeLists.txtapplications/st2110_test/run.shapplications/st2110_test/run_nv12_only.shapplications/st2110_test/run_rgba_only.shapplications/st2110_test/st2110_test_app.pyoperators/st2110_source/metadata.jsonoperators/st2110_source/st2110_kernels.cuoperators/st2110_source/st2110_source.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- applications/st2110_test/CMakeLists.txt
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{py,cpp,hpp}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.{py,cpp,hpp}: Code must adhere to Holoscan SDK coding standards including style compliance, descriptive naming, and minimal abbreviations
Code must include appropriate error handling and validation for all operations
All code must pass linting checks using './holohub lint' before submission
All code contributions must use clear, English descriptive names for functions and variables
Code must include inline comments for complex logic and clear documentation of functionality
Files:
applications/st2110_test/st2110_test_app.pyoperators/st2110_source/st2110_source.cpp
**/metadata.json
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
When reviewing PRs that modify or add
metadata.jsonfiles, verify that the category (first tag in thetagsarray) matches 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:
operators/st2110_source/metadata.json
**/operators/**/*.{py,cpp,hpp}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Class names for operators should use TitleCase with 'Op' suffix (e.g., 'AdaptiveThresholdingOp')
Files:
operators/st2110_source/st2110_source.cpp
**/operators/**/*.{py,cpp}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Operator filename should match the directory name with appropriate extension (e.g., 'adaptive_thresholding.py')
Files:
operators/st2110_source/st2110_source.cpp
**/operators/**/*.cpp
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Operators with C++ implementations must include CMakeLists.txt for build system integration
Files:
operators/st2110_source/st2110_source.cpp
🧠 Learnings (11)
📚 Learning: 2025-12-18T16:19:45.117Z
Learnt from: CR
Repo: nvidia-holoscan/holohub PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2025-12-18T16:19:45.117Z
Learning: Applies to **/(@(operators|applications|workflows))/**/metadata.json : Operators, applications, and workflows should use the '<holohub_data_dir>' placeholder in metadata.json 'run' command for referencing data directory
Applied to files:
applications/st2110_test/run.sh
📚 Learning: 2026-01-14T02:06:54.516Z
Learnt from: tbirdso
Repo: nvidia-holoscan/holohub PR: 1344
File: operators/ucxx_send_receive/metadata.json:1-61
Timestamp: 2026-01-14T02:06:54.516Z
Learning: When reviewing metadata.json under operators/**, accept that the root can be an array of objects for multiple operator entries, e.g. [{"operator": {...}}, {"operator": {...}}]. Ensure tooling and downstream processes parse the root as an array and validate each entry's operator object structure. Maintain this format for folders containing multiple operator definitions.
Applied to files:
operators/st2110_source/metadata.json
📚 Learning: 2025-12-18T16:19:45.117Z
Learnt from: CR
Repo: nvidia-holoscan/holohub PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2025-12-18T16:19:45.117Z
Learning: Applies to **/(@(operators|applications|workflows|gxf_extensions|tutorials))/**/metadata.json : metadata.json must include 'name' field matching the class name (TitleCase with 'Op' for operators)
Applied to files:
operators/st2110_source/metadata.json
📚 Learning: 2025-12-18T16:19:45.117Z
Learnt from: CR
Repo: nvidia-holoscan/holohub PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2025-12-18T16:19:45.117Z
Learning: Applies to **/(@(operators|applications|workflows|gxf_extensions|tutorials))/**/metadata.json : Every application, operator, workflow, and extension must include a 'metadata.json' file following the appropriate schema
Applied to files:
operators/st2110_source/metadata.json
📚 Learning: 2025-12-18T16:19:45.117Z
Learnt from: CR
Repo: nvidia-holoscan/holohub PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2025-12-18T16:19:45.117Z
Learning: Applies to **/(@(operators|applications|workflows|gxf_extensions|tutorials))/**/metadata.json : metadata.json must include 'holoscan_sdk' with 'minimum_required_version' and 'tested_versions' fields
Applied to files:
operators/st2110_source/metadata.json
📚 Learning: 2025-12-18T16:19:45.117Z
Learnt from: CR
Repo: nvidia-holoscan/holohub PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2025-12-18T16:19:45.117Z
Learning: Applies to **/(@(operators|applications|workflows|gxf_extensions|tutorials))/**/metadata.json : metadata.json must include 'language' field specifying 'C++', 'Python', or 'GXF'
Applied to files:
operators/st2110_source/metadata.json
📚 Learning: 2025-12-18T16:19:45.117Z
Learnt from: CR
Repo: nvidia-holoscan/holohub PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2025-12-18T16:19:45.117Z
Learning: Applies to **/(@(operators|applications|workflows|gxf_extensions|tutorials))/**/metadata.json : metadata.json must include 'version' field in major.minor.patch format
Applied to files:
operators/st2110_source/metadata.json
📚 Learning: 2025-12-18T16:19:45.117Z
Learnt from: CR
Repo: nvidia-holoscan/holohub PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2025-12-18T16:19:45.117Z
Learning: Applies to **/(@(operators|applications|workflows|gxf_extensions|tutorials))/**/metadata.json : metadata.json must include 'platforms' array listing supported architectures (e.g., 'x86_64', 'aarch64')
Applied to files:
operators/st2110_source/metadata.json
📚 Learning: 2025-11-24T16:27:43.600Z
Learnt from: CR
Repo: nvidia-holoscan/holohub PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-24T16:27:43.600Z
Learning: Applies to **/metadata.json : When reviewing PRs that modify or add `metadata.json` files, verify that the **category** (first tag in the `tags` array) matches 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
Applied to files:
operators/st2110_source/metadata.json
📚 Learning: 2025-11-24T16:27:43.600Z
Learnt from: CR
Repo: nvidia-holoscan/holohub PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-24T16:27:43.600Z
Learning: When a PR contains invalid category tags in `metadata.json`, flag the discrepancy and provide constructive feedback: (1) comment on the PR indicating which tags are not recognized, (2) if a similar category exists in the approved list, suggest that category, (3) if a README.md exists in the same directory, suggest an approved category based on README content, and (4) ask the contributor to either use an existing category or justify why a new category should be added
Applied to files:
operators/st2110_source/metadata.json
📚 Learning: 2026-01-13T23:11:34.601Z
Learnt from: GilShpr
Repo: nvidia-holoscan/holohub PR: 1342
File: operators/gstreamer/gst_src_op.cpp:34-36
Timestamp: 2026-01-13T23:11:34.601Z
Learning: In operator compute() implementations, avoid silently handling empty inputs from input.receive<T>(). The optional from input.receive<T>() should be dereferenced with value() only when data is guaranteed to be available. If receive() returns empty, this indicates a pipeline error and should surface immediately by throwing an exception (e.g., std::bad_optional_access) to avoid masking dropped frames. Ensure code patterns consistently dereference with value() only when input data is known to exist, and allow the exception to propagate for debugging. This guideline applies to all operator implementations under the operators/ directory (e.g., operators/gstreamer/gst_src_op.cpp) and similar CPP files that implement compute().
Applied to files:
operators/st2110_source/st2110_source.cpp
🧬 Code graph analysis (2)
applications/st2110_test/st2110_test_app.py (1)
operators/st2110_source/st2110_source.cpp (5)
ST2110SourceOp(50-57)setup(59-183)setup(59-59)compute(1109-1165)compute(1109-1110)
operators/st2110_source/st2110_source.cpp (1)
operators/st2110_source/st2110_kernels.cu (4)
convert_ycbcr422_10bit_to_rgba(317-334)convert_ycbcr422_10bit_to_rgba(317-321)convert_ycbcr422_10bit_to_nv12(336-352)convert_ycbcr422_10bit_to_nv12(336-340)
🪛 Cppcheck (2.19.0)
operators/st2110_source/st2110_source.cpp
[error] 87-87: 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.11)
applications/st2110_test/st2110_test_app.py
45-45: Unused method argument: op_output
(ARG002)
45-45: Unused method argument: context
(ARG002)
67-68: try-except-pass detected, consider logging the exception
(S110)
67-67: Do not catch blind exception: Exception
(BLE001)
69-69: Do not catch blind exception: Exception
(BLE001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🔇 Additional comments (18)
operators/st2110_source/metadata.json (1)
1-34: Metadata structure looks good.The metadata file follows the expected schema with all required fields: name follows TitleCase with 'Op' suffix, category tag "Video Capture" is approved, platforms and SDK versions are properly specified, and hardware requirements are documented.
operators/st2110_source/st2110_kernels.cu (2)
26-67: Frame reassembly kernel looks correct.The kernel properly handles 4-byte aligned copies with remainder byte handling (lines 60-66), and the per-packet block design avoids synchronization issues.
75-109: Simple reorder kernel correctly handles non-aligned payloads.The remainder byte handling (lines 99-108) properly addresses the previously flagged issue about non-4-byte-aligned payload lengths.
operators/st2110_source/st2110_source.cpp (8)
34-46: CUDA_TRY macro logs but doesn't propagate errors.The macro logs CUDA errors but the statement result is only returned, not acted upon. Callers must check the return value if error handling is needed. This is acceptable for fire-and-forget operations but be aware that errors may go unhandled if the return value is ignored.
59-183: Operator setup is well-documented with sensible defaults.Parameters are clearly documented with meaningful defaults. The use of
UnboundedAllocatorfor tensor memory management to prevent race conditions with async Python operators is a good practice.
373-441: Socket creation is well-implemented with good error handling.The socket setup includes proper validation, multicast group membership, and helpful warnings about buffer size limitations with actionable sysctl advice. Error handling correctly cleans up the socket before throwing exceptions.
652-727: Buffer management and frame ordering logic is well-designed.The atomic CAS for buffer claiming and RTP timestamp-based frame ordering with proper wraparound handling (using signed comparison) are correctly implemented. The dropped frame detection and logging is helpful for diagnostics.
744-798: Socket draining uses efficient batch receive with thread_local buffers.The
recvmmsg()batching and thread_local storage for packet buffers (addressing the previous stack allocation concern) is a good implementation. The one-time initialization pattern for the static thread_local structures is efficient.
844-892: Raw frame emission correctly wraps GPU buffer without unnecessary copies.The GXF Entity/Tensor wrapping with a no-op deleter is appropriate since the operator manages buffer lifetime. The shape calculation for YCbCr-4:2:2 10-bit (
width * bytes_per_pixel) correctly handles the packed format.
1109-1165: Compute method cleanly separates network ingestion from frame emission.The two-phase design (drain socket → emit frame) effectively decouples bursty network traffic from steady frame emission. The periodic logging provides useful operational visibility.
616-621: No changes needed—the current offset calculation is correct per SMPTE ST 2110-20.The Sample Row Data Offset field in ST 2110-20 is specified in pixels, not pgroups. The code correctly uses
dest_offset = line_start + (line_offset * bpp)to convert the pixel offset to bytes for buffer placement.Likely an incorrect or invalid review comment.
applications/st2110_test/st2110_test_app.py (4)
21-28: Good import safeguard with actionable error message.The try/except block for importing
ST2110SourceOpwith clear build instructions is excellent UX for developers encountering the operator for the first time.
45-70: OutputMonitor correctly handles varying tensor formats.The broad exception handling (flagged by static analysis) is intentional and appropriate here - different output types may have different tensor structures, and silent fallback is acceptable for a monitoring/diagnostic operator. The unused
op_outputandcontextparameters are required by the Holoscancompute()signature.
73-133: Application pipeline is well-structured with clear documentation.The docstring accurately describes the pipeline topology, and the compose() method correctly wires all three outputs to monitors with optional visualization for RGBA and NV12 formats.
136-199: Main function provides good user experience with validation.The config file validation, descriptive banner, and proper exception handling make this a user-friendly test application. The config path resolution correctly handles both absolute and relative paths.
applications/st2110_test/run_rgba_only.sh (1)
14-25: Docker run script is properly quoted and functional.The quoting issues from the previous review are resolved. The script correctly sets up the Docker environment for ST2110 testing.
One observation: the Docker image tag
holohub:ngc-v3.7.0-cuda13has an unusual CUDA version suffix. Please verify this is the correct image tag for your environment.applications/st2110_test/run_nv12_only.sh (1)
1-25: LGTM on quoting and overall structure.The quoting issues from the previous review have been addressed. The script correctly quotes
$(id -u)substitutions (lines 19, 21) and environment variables (line 20). Theset -eensures the script fails fast on errors.applications/st2110_test/run.sh (1)
1-85: Well-structured script with proper quoting.The script is well-documented with clear prerequisites and comprehensive validation. The quoting issues from the previous review have been addressed (lines 74-78). Good practices observed:
- Prerequisite checks for the built operator and config file
- Colorized output for better UX
- Fully-qualified container image path
- Proper error handling with
set -e
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| -e "XDG_RUNTIME_DIR=/run/user/$(id -u)" \ | ||
| -e PYTHONPATH=/opt/nvidia/holoscan/python/lib:/workspace/build/st2110_source/python/lib \ | ||
| -e LD_LIBRARY_PATH=/workspace/build/st2110_source/operators/st2110_source:/opt/nvidia/holoscan/lib \ | ||
| holohub:ngc-v3.7.0-cuda13 \ |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all docker run commands in ST2110 test runner scripts
echo "=== Searching for docker run commands in ST2110 test scripts ==="
rg -n 'docker run' applications/st2110_test/ -A 15 --type shRepository: nvidia-holoscan/holohub
Length of output: 3822
Align container image with run.sh.
This script uses holohub:ngc-v3.7.0-cuda13 while run.sh uses nvcr.io/nvidia/clara-holoscan/holoscan:v3.8.0-cuda13. Update to match the main test runner script for consistent test execution across all variants.
🤖 Prompt for AI Agents
In `@applications/st2110_test/run_nv12_only.sh` at line 24, The container image in
run_nv12_only.sh is out of sync: replace the image string
"holohub:ngc-v3.7.0-cuda13" with the same image used by run.sh
("nvcr.io/nvidia/clara-holoscan/holoscan:v3.8.0-cuda13") so the NV12-only runner
uses the identical container; update the image reference wherever
"holohub:ngc-v3.7.0-cuda13" appears in run_nv12_only.sh.
| // Allocate GPU raw buffer (always allocated for raw YCbCr-4:2:2-10bit data) | ||
| CUDA_TRY(cudaMalloc(&gpu_raw_buffer_, raw_frame_size_)); | ||
| HOLOSCAN_LOG_INFO("Allocated GPU raw buffer: {} bytes ({} MB)", | ||
| raw_frame_size_, | ||
| raw_frame_size_ / (1024 * 1024)); |
There was a problem hiding this comment.
GPU allocation failures are logged but not handled.
CUDA_TRY(cudaMalloc(...)) logs errors but doesn't throw or return early. If cudaMalloc fails, gpu_raw_buffer_ remains nullptr, leading to crashes in emit_next_frame() when cudaMemcpyAsync is called.
Consider checking the return value and throwing on failure, similar to the cuMemHostAlloc handling above:
Suggested fix
- CUDA_TRY(cudaMalloc(&gpu_raw_buffer_, raw_frame_size_));
+ if (CUDA_TRY(cudaMalloc(&gpu_raw_buffer_, raw_frame_size_)) != cudaSuccess) {
+ throw std::runtime_error("Failed to allocate GPU raw buffer");
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Allocate GPU raw buffer (always allocated for raw YCbCr-4:2:2-10bit data) | |
| CUDA_TRY(cudaMalloc(&gpu_raw_buffer_, raw_frame_size_)); | |
| HOLOSCAN_LOG_INFO("Allocated GPU raw buffer: {} bytes ({} MB)", | |
| raw_frame_size_, | |
| raw_frame_size_ / (1024 * 1024)); | |
| cudaError_t err = cudaMalloc(&gpu_raw_buffer_, raw_frame_size_); | |
| if (err != cudaSuccess) { | |
| throw std::runtime_error("Failed to allocate GPU raw buffer"); | |
| } |
🤖 Prompt for AI Agents
In `@operators/st2110_source/st2110_source.cpp` around lines 310 - 314, The GPU
allocation via CUDA_TRY(cudaMalloc(&gpu_raw_buffer_, raw_frame_size_)) currently
only logs failures and leaves gpu_raw_buffer_ null, which can cause crashes
later (e.g., in emit_next_frame() when calling cudaMemcpyAsync). Change the
allocation handling in the block that allocates gpu_raw_buffer_ to check the
cudaMalloc return and on failure throw or return early (consistent with the
earlier cuMemHostAlloc handling): capture the CUDA error/result, log the error
including the result and raw_frame_size_, and then throw an exception or return
an error so gpu_raw_buffer_ is never left null before emit_next_frame() is
called. Ensure emit_next_frame() relies on gpu_raw_buffer_ being valid or checks
for null as a defensive measure.
Greptile found no issues!From now on, if a review finishes and we haven't found any issues, we will not post anything, but you can confirm that we reviewed your changes in the status check section. This feature can be toggled off in your Code Review Settings by deselecting "Create a status check for each PR". |
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Fix all issues with AI agents
In `@applications/st2110_test/README.md`:
- Line 43: The README contains a duplicate "## Quick Start" heading; change the
second "## Quick Start" (the one around line 43) to a distinct heading such as
"## Setup Guide" or "## Detailed Configuration" and update any adjacent
subheadings or cross-references that assumed the old title so headings remain
unique and navigation (TOC/links) is correct.
- Around line 183-201: The README's "Advanced Network Config" and
"st2110_source" sections document DPDK-specific options (memory_regions,
gpu_direct, header_data_split) which don't apply to the socket implementation;
update those sections to replace DPDK terms with socket-specific guidance:
remove or mark as N/A the DPDK fields (memory_regions, gpu_direct,
header_data_split) and instead document multicast group address and port,
interface selection, socket buffer sizing, and recvmmsg() batch size (batch
processing) for the socket backend, and add a new subsection describing the
triple-buffer architecture with CUDA pinned memory (how buffers are allocated,
how producers/consumers rotate buffers, and any sync semantics) referenced from
the existing "st2110_source" and "advanced_network" headings so readers of the
socket implementation see the correct network and buffer configuration details.
- Around line 170-181: The README's "Check CPU core allocation" block uses
DPDK-specific settings (advanced_network.cfg.master_core and
interfaces[].rx.queues[].cpu_core); remove that section or replace it with
socket-based tuning guidance: describe configuring interrupt/core affinity,
adjusting socket receive buffer sizes, and tuning recvmmsg() batch/timeout
parameters for performance rather than referencing DPDK master_core or per-queue
cpu_core settings. Ensure the replacement references the same logical section
(advanced_network) and clearly labels the tips as socket-based tuning.
- Around line 136-154: Replace the "DPDK Initialization Fails" section with
socket-focused troubleshooting: remove hugepages/sudo DPDK steps and instead
advise verifying multicast group membership (e.g., via ip maddr show), checking
for socket buffer overruns and errors (netstat -su or ss -s), inspecting and
tuning rmem_max/wmem_max via sysctl, validating firewall/iptables/nft rules and
multicast routing, confirming interface multicast subscriptions and VLANs, and
suggesting packet captures (tcpdump) to see multicast traffic; update the
section header and example commands to reflect these socket-specific checks.
- Around line 248-253: Add a language identifier to the shown fenced code block
by changing the opening triple backticks to include "text" (i.e., ```text) so
the log snippet [ST2110SourceOp] 1000 packets received, 0 dropped / Frame rate:
59.8 FPS / Throughput: 2.8 Gbps renders with proper syntax highlighting and
accessibility.
- Around line 3-12: The README currently claims a DPDK backend and DPDK-specific
features (lines referencing "DPDK backend", "via DPDK", "zero-copy HDS mode")
which contradict the PR objective to use standard Linux sockets; update the
README so the ST2110SourceOp description states it uses standard Linux sockets
(not DPDK) for packet reception and clarify that frame assembly uses the
socket-based path, or explicitly note if DPDK is an optional, alternate backend;
ensure references to "DPDK", "zero-copy HDS", and "DPDK backend" are removed or
conditioned as optional to match the implementation and PR objectives.
- Around line 209-236: Update the README's configuration docs to explicitly list
the supported output formats and conversion behavior: under the
st2110_source/pixel_format section document the three outputs—"Raw YCbCr 4:2:2
10-bit" (native ST 2110-20), "RGBA 8-bit", and "NV12 8-bit"—and note that when
converting 10-bit YCbCr→8-bit RGBA or NV12 the CUDA color conversion kernels use
BT.709 tone mapping; also show example YAML values for pixel_format for each
option and add a brief note about batch_size/resolution implications for 4K.
- Around line 1-278: The README currently documents a DPDK-based flow but the PR
implements a socket-based receiver; update the document to reflect the new
architecture by replacing DPDK/vfio/HDS/gpu_direct content with descriptions of
the socket-based pipeline (multicast UDP receive using recvmmsg() batch
processing), RTP header parsing for frame boundary detection, the triple-buffer
design using CUDA pinned memory, and the CUDA kernels performing 10-bit→8-bit
BT.709 conversion; remove DPDK-specific setup/troubleshooting (dpdk-devbind.py,
vfio-pci, hugepages tuning, HDS and zero-copy claims) and instead add platform
rationale (Jetson-family portability, unified memory vs GPUDirect), updated run
instructions (no DPDK sudo binding steps), and a troubleshooting section
covering socket/multicast issues and buffer/kernel tuning, plus an architecture
diagram or pipeline description (receive → RTP parse → buffer → convert →
display).
- Around line 264-270: In the "Related Documentation" list in the README's
Related Documentation section, remove or clarify the
tutorials/high_performance_networking/README.md entry (it's DPDK-focused and
misleading for a socket-based implementation) and either replace it with a
socket-based networking doc or omit it; also verify the
operators/st2110_source/DEVELOPMENT_PLAN.md reference and if that file does not
exist, remove or correct the link (or point to the correct DEVELOPMENT_PLAN
location) and ensure the README instead points to the BUILD.md note that
standard Linux network drivers are sufficient.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
applications/st2110_test/README.mdapplications/st2110_test/metadata.json
🚧 Files skipped from review as they are similar to previous changes (1)
- applications/st2110_test/metadata.json
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-18T16:19:45.117Z
Learnt from: CR
Repo: nvidia-holoscan/holohub PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2025-12-18T16:19:45.117Z
Learning: Applies to **/(@(operators|applications|workflows|gxf_extensions))/**/README.md : Every application and operator must include a comprehensive README.md file with purpose, usage, requirements, examples, and architecture information
Applied to files:
applications/st2110_test/README.md
🪛 markdownlint-cli2 (0.18.1)
applications/st2110_test/README.md
43-43: Multiple headings with the same content
(MD024, no-duplicate-heading)
248-248: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🔇 Additional comments (2)
applications/st2110_test/README.md (2)
38-41: Prerequisites incorrectly require DPDK setup.Lines 40-41 require "DPDK configured and NIC bound to vfio-pci driver" but the PR implementation uses standard Linux sockets with multicast UDP. The vfio-pci driver binding is unnecessary and potentially incompatible with the socket-based implementation.
Update prerequisites to reflect socket-based requirements (e.g., multicast routing, proper network interface configuration, kernel buffer tuning).
⛔ Skipped due to learnings
Learnt from: cliffburdick Repo: nvidia-holoscan/holohub PR: 1261 File: operators/advanced_network/advanced_network/managers/dpdk/CMakeLists.txt:43-54 Timestamp: 2025-11-26T18:26:56.776Z Learning: The advanced_network operator in operators/advanced_network only supports Mellanox NICs, as documented in its requirements. Hardcoded Mellanox-specific libraries (e.g., -lmlx5, -libverbs) in CMakeLists.txt are intentional and appropriate for this operator.Learnt from: cliffburdick Repo: nvidia-holoscan/holohub PR: 1261 File: operators/advanced_network/Dockerfile:140-152 Timestamp: 2025-12-08T16:16:53.561Z Learning: The DPDK 24.11.3 tarball from https://fast.dpdk.org/rel/dpdk-24.11.3.tar.xz extracts to a directory named dpdk-stable-24.11.3 (includes "stable" prefix), so the Dockerfile in operators/advanced_network/Dockerfile correctly uses dpdk-stable-${DPDK_VERSION} when referencing the extracted directory.
62-73: Remove DPDK binding instructions incompatible with socket implementation.These instructions bind the NIC to the vfio-pci driver for DPDK operation. However, the socket-based implementation requires the NIC to remain bound to its standard kernel driver (e.g., mlx5_core for ConnectX NICs) to enable standard socket operations.
Replace with instructions for verifying multicast routing, setting socket buffer sizes, or other socket-specific tuning.
⛔ Skipped due to learnings
Learnt from: cliffburdick Repo: nvidia-holoscan/holohub PR: 1261 File: operators/advanced_network/advanced_network/managers/dpdk/CMakeLists.txt:43-54 Timestamp: 2025-11-26T18:26:56.776Z Learning: The advanced_network operator in operators/advanced_network only supports Mellanox NICs, as documented in its requirements. Hardcoded Mellanox-specific libraries (e.g., -lmlx5, -libverbs) in CMakeLists.txt are intentional and appropriate for this operator.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| # ST2110 Test Application | ||
|
|
||
| Simple test application to verify ST 2110-20 video reception using the ST2110SourceOp operator with DPDK backend. | ||
|
|
||
| ## Purpose | ||
|
|
||
| This application tests the ST2110 source operator by: | ||
|
|
||
| 1. Receiving ST 2110-20 video packets via DPDK | ||
| 2. Assembling packets into video frames using zero-copy HDS mode | ||
| 3. Displaying the video stream using Holoviz | ||
|
|
||
| ## Quick Start | ||
|
|
||
| ```bash | ||
| # 1. Set up virtual environment (first time only) | ||
| python3 -m venv venv | ||
| ./venv/bin/pip install holoscan-cu13 | ||
|
|
||
| # 2. Configure your network (edit st2110_test_config.yaml) | ||
| nano st2110_test_config.yaml # Update PCI address | ||
|
|
||
| # 3. Run the application | ||
| ./run.sh | ||
| ``` | ||
|
|
||
| **See [SETUP.md](SETUP.md) for detailed setup instructions.** | ||
|
|
||
| ## Prerequisites | ||
|
|
||
| ### Hardware | ||
|
|
||
| - NVIDIA Thor AGX (or compatible system) | ||
| - NVIDIA ConnectX-6 or later NIC (or Thor AGX MGBE) | ||
| - ST 2110-20 video source streaming to the network | ||
|
|
||
| ### Software | ||
|
|
||
| - ST2110SourceOp operator built (see `../../operators/st2110_source/BUILD.md`) | ||
| - DPDK configured and NIC bound to vfio-pci driver | ||
| - ST 2110 video source streaming to multicast 239.0.0.1:5004 | ||
|
|
||
| ## Quick Start | ||
|
|
||
| ### 1. Configure Your Network | ||
|
|
||
| Edit `st2110_test_config.yaml` and update: | ||
|
|
||
| ```yaml | ||
| interfaces: | ||
| - name: "st2110_rx" | ||
| address: "0005:03:00.0" # ← Your NIC's PCI address | ||
| ``` | ||
|
|
||
| Find your PCI address: | ||
|
|
||
| ```bash | ||
| lspci | grep -i ethernet | ||
| # Example output: 0005:03:00.0 Ethernet controller: NVIDIA Corporation | ||
| ``` | ||
|
|
||
| ### 2. Bind NIC to DPDK | ||
|
|
||
| ```bash | ||
| # Take interface down | ||
| sudo ifconfig mgbe0_0 down | ||
|
|
||
| # Bind to DPDK | ||
| sudo dpdk-devbind.py --bind=vfio-pci 0005:03:00.0 | ||
|
|
||
| # Verify | ||
| sudo dpdk-devbind.py --status | ||
| ``` | ||
|
|
||
| ### 3. Start Your ST 2110 Source | ||
|
|
||
| Ensure you have a ST 2110-20 video source streaming: | ||
|
|
||
| - **Multicast address**: 239.0.0.1 | ||
| - **UDP port**: 5004 | ||
| - **Format**: YCbCr-4:2:2 10-bit (or RGBA/RGB) | ||
| - **Resolution**: 1920x1080@60fps | ||
|
|
||
| ### 4. Run the Test Application | ||
|
|
||
| ```bash | ||
| # From holohub root directory | ||
| cd applications/st2110_test | ||
|
|
||
| # Run with sudo (required for DPDK) | ||
| sudo python3 st2110_test_app.py | ||
| ``` | ||
|
|
||
| ## Expected Behavior | ||
|
|
||
| When working correctly: | ||
|
|
||
| 1. Console shows "ST2110 Test Application" banner | ||
| 2. Logs indicate DPDK initialization | ||
| 3. Video window opens showing the ST 2110 stream | ||
| 4. Console displays packet reception statistics | ||
| 5. Frame rate should be near 60 FPS | ||
|
|
||
| Press **ESC** or **Q** to quit. | ||
|
|
||
| ## Troubleshooting | ||
|
|
||
| ### No Video Displayed | ||
|
|
||
| **Check ST 2110 source:** | ||
|
|
||
| ```bash | ||
| # Verify multicast traffic is reaching the system | ||
| sudo tcpdump -i mgbe0_0 dst 239.0.0.1 and udp port 5004 | ||
| ``` | ||
|
|
||
| **Check DPDK binding:** | ||
|
|
||
| ```bash | ||
| sudo dpdk-devbind.py --status | ||
| # NIC should show "drv=vfio-pci" and not "Active" | ||
| ``` | ||
|
|
||
| ### "Cannot import ST2110SourceOp" Error | ||
|
|
||
| **Build the operator:** | ||
|
|
||
| ```bash | ||
| cd ../../operators/st2110_source | ||
| mkdir -p build && cd build | ||
| cmake .. -DCMAKE_PREFIX_PATH="/opt/nvidia/holoscan/lib;$HOME/Source/holohub/install/lib/cmake" \ | ||
| -DCMAKE_BUILD_TYPE=Release | ||
| make | ||
| ``` | ||
|
|
||
| ### DPDK Initialization Fails | ||
|
|
||
| **Check huge pages:** | ||
|
|
||
| ```bash | ||
| cat /proc/meminfo | grep Huge | ||
| # Should show: HugePages_Total > 0 | ||
|
|
||
| # Configure if needed: | ||
| sudo sh -c 'echo 2048 > /proc/sys/vm/nr_hugepages' | ||
| ``` | ||
|
|
||
| **Check permissions:** | ||
|
|
||
| ```bash | ||
| # Must run with sudo for DPDK access | ||
| sudo python3 st2110_test_app.py | ||
| ``` | ||
|
|
||
| ### Low Frame Rate or Packet Drops | ||
|
|
||
| **Tune batch_size in config:** | ||
|
|
||
| ```yaml | ||
| st2110_source: | ||
| batch_size: 4320 # Try adjusting this value | ||
| ``` | ||
|
|
||
| Optimal batch_size depends on your video format: | ||
|
|
||
| - 1080p60 RGBA: ~5760 packets/frame | ||
| - 1080p60 NV12: ~3840 packets/frame | ||
| - 4K30 RGBA: ~11520 packets/frame | ||
|
|
||
| **Check CPU core allocation:** | ||
|
|
||
| ```yaml | ||
| advanced_network: | ||
| cfg: | ||
| master_core: 3 # Dedicated core for DPDK master | ||
| interfaces: | ||
| - rx: | ||
| queues: | ||
| - cpu_core: 9 # Dedicated core for RX queue | ||
| ``` | ||
|
|
||
| ## Configuration Details | ||
|
|
||
| ### Advanced Network Config | ||
|
|
||
| The `advanced_network` section configures DPDK: | ||
|
|
||
| - **manager**: "dpdk" for DPDK backend | ||
| - **memory_regions**: HDS split (headers to CPU, payload to GPU) | ||
| - **flows**: Packet steering rules for ST 2110 stream | ||
|
|
||
| ### ST2110 Source Config | ||
|
|
||
| The `st2110_source` section configures the operator: | ||
|
|
||
| - **interface_name**: Must match interface name in advanced_network | ||
| - **width/height**: Video resolution | ||
| - **pixel_format**: Output format (NV12 recommended for NVIDIA) | ||
| - **gpu_direct**: Enable zero-copy to GPU | ||
| - **header_data_split**: Enable HDS mode | ||
|
|
||
| ### Holoviz Config | ||
|
|
||
| The `visualizer` section configures video display: | ||
|
|
||
| - **width/height**: Display window size | ||
| - **framerate**: Target display frame rate | ||
|
|
||
| ## Testing Different Configurations | ||
|
|
||
| ### Test with RGB Format | ||
|
|
||
| ```yaml | ||
| st2110_source: | ||
| pixel_format: "RGB" | ||
| # RGB uses 3 bytes per pixel | ||
| ``` | ||
|
|
||
| ### Test with RGBA Format | ||
|
|
||
| ```yaml | ||
| st2110_source: | ||
| pixel_format: "RGBA" | ||
| # RGBA uses 4 bytes per pixel | ||
| ``` | ||
|
|
||
| ### Test Different Resolutions | ||
|
|
||
| ```yaml | ||
| st2110_source: | ||
| width: 3840 | ||
| height: 2160 | ||
| framerate: 30 | ||
| batch_size: 11520 # Adjust for 4K | ||
| ``` | ||
|
|
||
| ## Performance Monitoring | ||
|
|
||
| The application logs performance metrics: | ||
|
|
||
| - Packets received/dropped | ||
| - Frame rate | ||
| - Bytes received | ||
| - Latency (if available) | ||
|
|
||
| Example output: | ||
|
|
||
| ``` | ||
| [ST2110SourceOp] 1000 packets received, 0 dropped | ||
| [ST2110SourceOp] Frame rate: 59.8 FPS | ||
| [ST2110SourceOp] Throughput: 2.8 Gbps | ||
| ``` | ||
|
|
||
| ## Next Steps | ||
|
|
||
| After verifying basic reception: | ||
|
|
||
| 1. Test with different video formats | ||
| 2. Measure end-to-end latency | ||
| 3. Add downstream processing operators | ||
| 4. Test multi-stream reception | ||
| 5. Optimize for your specific use case | ||
|
|
||
| ## Related Documentation | ||
|
|
||
| - ST2110 Operator: `../../operators/st2110_source/README.md` | ||
| - Build Instructions: `../../operators/st2110_source/BUILD.md` | ||
| - Development Plan: `../../operators/st2110_source/DEVELOPMENT_PLAN.md` | ||
| - Advanced Networking: `../../tutorials/high_performance_networking/README.md` | ||
|
|
||
| ## Support | ||
|
|
||
| For issues: | ||
|
|
||
| - Check logs for error messages | ||
| - Verify DPDK and network configuration | ||
| - Review troubleshooting section above | ||
| - File issues at: <https://github.com/nvidia-holoscan/holohub/issues> |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Documentation requires substantial revision to match socket-based implementation.
The README describes a DPDK-based architecture but the PR implements a socket-based solution. Key architectural details from the PR objectives are missing:
Missing architecture details:
- Multicast UDP reception using Linux sockets with
recvmmsg()batch processing - Triple-buffer architecture with CUDA pinned memory
- RTP header parsing for frame boundary detection
- CUDA kernels for 10-bit → 8-bit color conversion (BT.709)
- Platform support rationale (Jetson-family portability, unified memory vs. GPUDirect)
Incorrect content to remove/revise:
- All DPDK configuration and troubleshooting sections
- vfio-pci driver binding instructions
- HDS (Header Data Split) references
- DPDK-specific performance tuning
- gpu_direct and zero-copy assumptions
Consider restructuring the README to accurately document the socket-based implementation with architecture diagrams showing the receive → RTP parse → buffer → convert → display pipeline.
Based on learnings: comprehensive README requirement for applications.
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
43-43: Multiple headings with the same content
(MD024, no-duplicate-heading)
248-248: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
In `@applications/st2110_test/README.md` around lines 1 - 278, The README
currently documents a DPDK-based flow but the PR implements a socket-based
receiver; update the document to reflect the new architecture by replacing
DPDK/vfio/HDS/gpu_direct content with descriptions of the socket-based pipeline
(multicast UDP receive using recvmmsg() batch processing), RTP header parsing
for frame boundary detection, the triple-buffer design using CUDA pinned memory,
and the CUDA kernels performing 10-bit→8-bit BT.709 conversion; remove
DPDK-specific setup/troubleshooting (dpdk-devbind.py, vfio-pci, hugepages
tuning, HDS and zero-copy claims) and instead add platform rationale
(Jetson-family portability, unified memory vs GPUDirect), updated run
instructions (no DPDK sudo binding steps), and a troubleshooting section
covering socket/multicast issues and buffer/kernel tuning, plus an architecture
diagram or pipeline description (receive → RTP parse → buffer → convert →
display).
| - DPDK configured and NIC bound to vfio-pci driver | ||
| - ST 2110 video source streaming to multicast 239.0.0.1:5004 | ||
|
|
||
| ## Quick Start |
There was a problem hiding this comment.
Remove duplicate "Quick Start" heading.
This creates a second "## Quick Start" section (first at line 13). Consider renaming this to "## Setup Guide" or "## Detailed Configuration" to differentiate it from the initial quick start.
Based on static analysis hints.
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
43-43: Multiple headings with the same content
(MD024, no-duplicate-heading)
🤖 Prompt for AI Agents
In `@applications/st2110_test/README.md` at line 43, The README contains a
duplicate "## Quick Start" heading; change the second "## Quick Start" (the one
around line 43) to a distinct heading such as "## Setup Guide" or "## Detailed
Configuration" and update any adjacent subheadings or cross-references that
assumed the old title so headings remain unique and navigation (TOC/links) is
correct.
| ### DPDK Initialization Fails | ||
|
|
||
| **Check huge pages:** | ||
|
|
||
| ```bash | ||
| cat /proc/meminfo | grep Huge | ||
| # Should show: HugePages_Total > 0 | ||
|
|
||
| # Configure if needed: | ||
| sudo sh -c 'echo 2048 > /proc/sys/vm/nr_hugepages' | ||
| ``` | ||
|
|
||
| **Check permissions:** | ||
|
|
||
| ```bash | ||
| # Must run with sudo for DPDK access | ||
| sudo python3 st2110_test_app.py | ||
| ``` | ||
|
|
There was a problem hiding this comment.
Replace DPDK troubleshooting with socket-specific guidance.
This section troubleshoots DPDK initialization and huge pages, which are not applicable to the socket-based implementation. Replace with socket-relevant troubleshooting such as:
- Verifying multicast group membership (
ip maddr show) - Checking socket buffer overruns (
netstat -su) - Tuning rmem_max/wmem_max kernel parameters
- Verifying firewall rules for multicast traffic
🤖 Prompt for AI Agents
In `@applications/st2110_test/README.md` around lines 136 - 154, Replace the "DPDK
Initialization Fails" section with socket-focused troubleshooting: remove
hugepages/sudo DPDK steps and instead advise verifying multicast group
membership (e.g., via ip maddr show), checking for socket buffer overruns and
errors (netstat -su or ss -s), inspecting and tuning rmem_max/wmem_max via
sysctl, validating firewall/iptables/nft rules and multicast routing, confirming
interface multicast subscriptions and VLANs, and suggesting packet captures
(tcpdump) to see multicast traffic; update the section header and example
commands to reflect these socket-specific checks.
|
|
||
| ### Advanced Network Config | ||
|
|
||
| The `advanced_network` section configures DPDK: | ||
|
|
||
| - **manager**: "dpdk" for DPDK backend | ||
| - **memory_regions**: HDS split (headers to CPU, payload to GPU) | ||
| - **flows**: Packet steering rules for ST 2110 stream | ||
|
|
||
| ### ST2110 Source Config | ||
|
|
||
| The `st2110_source` section configures the operator: | ||
|
|
||
| - **interface_name**: Must match interface name in advanced_network | ||
| - **width/height**: Video resolution | ||
| - **pixel_format**: Output format (NV12 recommended for NVIDIA) | ||
| - **gpu_direct**: Enable zero-copy to GPU | ||
| - **header_data_split**: Enable HDS mode | ||
|
|
There was a problem hiding this comment.
Configuration section describes DPDK features not applicable to socket implementation.
The "Advanced Network Config" and parameters like gpu_direct, header_data_split, and memory_regions are DPDK-specific. The socket-based implementation should document:
- Multicast group address and port
- Interface selection
- Socket buffer configuration
- recvmmsg() batch size (mentioned in PR as "batch processing")
Additionally, the documentation should describe the triple-buffer architecture with CUDA pinned memory mentioned in PR objectives.
🤖 Prompt for AI Agents
In `@applications/st2110_test/README.md` around lines 183 - 201, The README's
"Advanced Network Config" and "st2110_source" sections document DPDK-specific
options (memory_regions, gpu_direct, header_data_split) which don't apply to the
socket implementation; update those sections to replace DPDK terms with
socket-specific guidance: remove or mark as N/A the DPDK fields (memory_regions,
gpu_direct, header_data_split) and instead document multicast group address and
port, interface selection, socket buffer sizing, and recvmmsg() batch size
(batch processing) for the socket backend, and add a new subsection describing
the triple-buffer architecture with CUDA pinned memory (how buffers are
allocated, how producers/consumers rotate buffers, and any sync semantics)
referenced from the existing "st2110_source" and "advanced_network" headings so
readers of the socket implementation see the correct network and buffer
configuration details.
| ## Testing Different Configurations | ||
|
|
||
| ### Test with RGB Format | ||
|
|
||
| ```yaml | ||
| st2110_source: | ||
| pixel_format: "RGB" | ||
| # RGB uses 3 bytes per pixel | ||
| ``` | ||
|
|
||
| ### Test with RGBA Format | ||
|
|
||
| ```yaml | ||
| st2110_source: | ||
| pixel_format: "RGBA" | ||
| # RGBA uses 4 bytes per pixel | ||
| ``` | ||
|
|
||
| ### Test Different Resolutions | ||
|
|
||
| ```yaml | ||
| st2110_source: | ||
| width: 3840 | ||
| height: 2160 | ||
| framerate: 30 | ||
| batch_size: 11520 # Adjust for 4K | ||
| ``` | ||
|
|
There was a problem hiding this comment.
Document all supported output formats from PR objectives.
The PR objectives specify three output types:
- Raw YCbCr 4:2:2 10-bit (native ST 2110-20 format)
- RGBA 8-bit
- NV12 8-bit
The documentation should clarify these outputs and the CUDA color conversion kernels (BT.709) that convert 10-bit → 8-bit formats, as mentioned in the PR objectives.
🤖 Prompt for AI Agents
In `@applications/st2110_test/README.md` around lines 209 - 236, Update the
README's configuration docs to explicitly list the supported output formats and
conversion behavior: under the st2110_source/pixel_format section document the
three outputs—"Raw YCbCr 4:2:2 10-bit" (native ST 2110-20), "RGBA 8-bit", and
"NV12 8-bit"—and note that when converting 10-bit YCbCr→8-bit RGBA or NV12 the
CUDA color conversion kernels use BT.709 tone mapping; also show example YAML
values for pixel_format for each option and add a brief note about
batch_size/resolution implications for 4K.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@applications/CMakeLists.txt`:
- Around line 134-137: Remove "holoviz" from the DEPENDS OPERATORS list in the
add_holohub_application invocation for st2110_test; leave st2110_source as the
operator dependency. Update the call add_holohub_application(st2110_test DEPENDS
OPERATORS st2110_source) so holoviz is no longer listed as an operator
dependency; if visualization is needed, ensure the application links to the
Holoscan SDK operator (holoscan::ops::holoviz) rather than adding a holoviz
HoloHub directory as an operator dependency.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
applications/CMakeLists.txt
🧰 Additional context used
📓 Path-based instructions (1)
**/applications/**/CMakeLists.txt
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/applications/**/CMakeLists.txt: CMakeLists.txt for applications must use 'add_holohub_application()' with optional 'DEPENDS OPERATORS' for dependent operators
Applications should include a testing section in CMakeLists.txt for functional testing using CTest
Files:
applications/CMakeLists.txt
🧠 Learnings (10)
📚 Learning: 2025-12-18T16:19:45.117Z
Learnt from: CR
Repo: nvidia-holoscan/holohub PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2025-12-18T16:19:45.117Z
Learning: Applies to **/applications/**/CMakeLists.txt : CMakeLists.txt for applications must use 'add_holohub_application()' with optional 'DEPENDS OPERATORS' for dependent operators
Applied to files:
applications/CMakeLists.txt
📚 Learning: 2025-12-18T16:19:45.117Z
Learnt from: CR
Repo: nvidia-holoscan/holohub PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2025-12-18T16:19:45.117Z
Learning: Applies to **/pkg/CMakeLists.txt : CMakeLists.txt for packages must use 'add_holohub_package()' function with optional APPLICATIONS and OPERATORS dependencies
Applied to files:
applications/CMakeLists.txt
📚 Learning: 2025-12-18T16:19:45.117Z
Learnt from: CR
Repo: nvidia-holoscan/holohub PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2025-12-18T16:19:45.117Z
Learning: Applies to **/workflows/**/CMakeLists.txt : CMakeLists.txt for workflows must use 'add_holohub_application()' with optional 'DEPENDS OPERATORS' for dependent operators
Applied to files:
applications/CMakeLists.txt
📚 Learning: 2025-12-18T16:19:45.117Z
Learnt from: CR
Repo: nvidia-holoscan/holohub PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2025-12-18T16:19:45.117Z
Learning: Applies to **/operators/**/CMakeLists.txt : CMakeLists.txt for operators must use 'add_holohub_operator()' with optional 'DEPENDS EXTENSIONS' for dependent extensions
Applied to files:
applications/CMakeLists.txt
📚 Learning: 2026-01-12T16:04:04.046Z
Learnt from: tbirdso
Repo: nvidia-holoscan/holohub PR: 1322
File: applications/bci_visualization/operators/color_buffer_passthrough/CMakeLists.txt:16-46
Timestamp: 2026-01-12T16:04:04.046Z
Learning: Application-specific operators under applications/**/operators/ that are tightly coupled with their application don't require the add_holohub_operator() macro and can use standard CMake add_library() with manual configuration instead.
Applied to files:
applications/CMakeLists.txt
📚 Learning: 2025-12-18T16:19:45.117Z
Learnt from: CR
Repo: nvidia-holoscan/holohub PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2025-12-18T16:19:45.117Z
Learning: Applies to **/pkg/**/CMakeLists.txt : CMakeLists.txt for packages must use 'holohub_configure_deb()' with required NAME, DESCRIPTION, VERSION, VENDOR, CONTACT and optional SECTION, PRIORITY parameters
Applied to files:
applications/CMakeLists.txt
📚 Learning: 2025-12-18T16:19:45.117Z
Learnt from: CR
Repo: nvidia-holoscan/holohub PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2025-12-18T16:19:45.117Z
Learning: Applies to **/gxf_extensions/**/CMakeLists.txt : CMakeLists.txt for GXF extensions must use 'add_holohub_extension()' function
Applied to files:
applications/CMakeLists.txt
📚 Learning: 2025-12-18T16:19:45.117Z
Learnt from: CR
Repo: nvidia-holoscan/holohub PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2025-12-18T16:19:45.117Z
Learning: Applies to **/applications/**/CMakeLists.txt : Applications should include a testing section in CMakeLists.txt for functional testing using CTest
Applied to files:
applications/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/CMakeLists.txt
📚 Learning: 2025-12-18T16:19:45.117Z
Learnt from: CR
Repo: nvidia-holoscan/holohub PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2025-12-18T16:19:45.117Z
Learning: Applies to **/(@(applications|workflows))/**/CMakeLists.txt : Applications and workflows must include CMakeLists.txt for build system integration
Applied to files:
applications/CMakeLists.txt
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| add_holohub_application(st2110_test DEPENDS | ||
| OPERATORS st2110_source | ||
| holoviz | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify if holoviz is defined as a HoloHub operator or if it's SDK-provided
# Check if there's a holoviz operator definition in the operators directory
echo "=== Checking for holoviz operator in operators/ ==="
fd -t d "holoviz" operators/
# Check the holoviz subdirectory under applications to understand what it contains
echo "=== Contents of applications/holoviz/ ==="
ls -la applications/holoviz/ 2>/dev/null || echo "Directory not found"
# Search for add_holohub_operator calls for holoviz
echo "=== Searching for holoviz operator registration ==="
rg -n "add_holohub_operator.*holoviz" operators/
# Check if holoviz is used in other applications' CMakeLists.txt
echo "=== Checking holoviz usage in other application CMakeLists.txt ==="
rg -n "holoviz" applications/*/CMakeLists.txt | head -20
# Also check the main applications/CMakeLists.txt for holoviz references
echo "=== Checking holoviz in applications/CMakeLists.txt ==="
rg -n "holoviz" applications/CMakeLists.txtRepository: nvidia-holoscan/holohub
Length of output: 2057
Remove holoviz from the DEPENDS OPERATORS list—it is an application, not a HoloHub operator.
The st2110_source dependency is appropriate for this new application. However, holoviz should not be listed as a dependent operator. holoviz is a HoloHub application directory containing multiple visualization examples, not a HoloHub operator. Applications that require visualization link directly to the Holoscan SDK's holoscan::ops::holoviz operator, not to a HoloHub operator dependency.
Suggested change
add_holohub_application(st2110_test DEPENDS
- OPERATORS st2110_source
- holoviz
- )
+ OPERATORS st2110_source)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| add_holohub_application(st2110_test DEPENDS | |
| OPERATORS st2110_source | |
| holoviz | |
| ) | |
| add_holohub_application(st2110_test DEPENDS | |
| OPERATORS st2110_source) |
🤖 Prompt for AI Agents
In `@applications/CMakeLists.txt` around lines 134 - 137, Remove "holoviz" from
the DEPENDS OPERATORS list in the add_holohub_application invocation for
st2110_test; leave st2110_source as the operator dependency. Update the call
add_holohub_application(st2110_test DEPENDS OPERATORS st2110_source) so holoviz
is no longer listed as an operator dependency; if visualization is needed,
ensure the application links to the Holoscan SDK operator
(holoscan::ops::holoviz) rather than adding a holoviz HoloHub directory as an
operator dependency.
bhashemian
left a comment
bhashemian
left a comment
There was a problem hiding this comment.
Thanks @Ollie-cj and @xrlabs-cj for this interesting PR. I reviewed it briefly and left a few comments. I'd appreciate it if you could address these comments, along with those from AI reviewers, and CI issues. Thanks
| "run": { | ||
| "command": "python3 <holohub_app_source>/st2110_test_app.py", | ||
| "workdir": "holohub_app_bin" | ||
| } |
There was a problem hiding this comment.
Instead of defining different run.sh scripts, please update this run commands with modes, so the user can use the holohub cli to run different modes. Please check an example here:
holohub/applications/colonoscopy_segmentation/metadata.json
Lines 38 to 61 in 124d602
Then the user can call the commands like this:
./holohub run st2110 # the default mode, replacing `run.sh`
./holohub run st2110 rgba # replacing `run_rgba_only.sh`
./holohub run st2110 nv12 # replacing `run_nv12_only.sh`
| @@ -0,0 +1,278 @@ | |||
| # ST2110 Test Application | |||
There was a problem hiding this comment.
Could you please change this title to something more descriptive? Also replacing "test" with "demo" in the directory and file naming since we are using test to refer to our unit and integration tests. Thanks
| ```bash | ||
| # 1. Set up virtual environment (first time only) | ||
| python3 -m venv venv | ||
| ./venv/bin/pip install holoscan-cu13 | ||
|
|
There was a problem hiding this comment.
These are unnecessary as the Holohub CLI takes care of Holoscan SDK dependency. Please try to simplify the documentation for holohub cli, so the user just need to simply call:
./holohub run st2110_test
This will build and run a docker container based on the latest Holoscan SDK, and then build and run the application.
| { | ||
| "name": "NVIDIA Thor AGX with MGBE", | ||
| "description": "Network interface with multicast support" | ||
|
|
There was a problem hiding this comment.
style: Trailing whitespace should be removed
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!
| **Solution**: Install CMake 3.24+ | ||
| ```bash | ||
| # Download and install locally | ||
| wget https://github.com/Kitware/CMake/releases/download/v3.28.1/cmake-3.28.1-linux-aarch64.tar.gz |
There was a problem hiding this comment.
style: Architecture-specific download URL assumes aarch64 but document targets both x86_64and aarch64 platforms. Consider providing both URLs or using architecture detection. Should this provide both x86_64 and aarch64 URLs, or is this intentionally aarch64-focused since Thor AGX is mentioned as primary platform?
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!
| uint32_t current_timestamp; // RTP timestamp of current frame | ||
| uint32_t packets_received; // Packets received for current frame | ||
| uint32_t bytes_received; // Bytes received for current frame | ||
| bool frame_complete; // Frame reassembly complete |
There was a problem hiding this comment.
logic: frame_complete should be atomic since it's accessed from both packet reception (line 592 in .cpp) and frame emission paths without proper synchronization. Race condition could cause inconsistent state when one thread reads while another writes.
Consider changing to std::atomic<bool> frame_complete; to match the pattern used for assembling_index_ at line 183.
| bool frame_complete; // Frame reassembly complete | |
| std::atomic<bool> frame_complete; // Frame reassembly complete |
| ### 4. Bind NIC to DPDK | ||
|
|
||
| ```bash | ||
| # Take interface down | ||
| sudo ifconfig mgbe0_0 down | ||
|
|
||
| # Bind to DPDK (requires vfio-pci driver) | ||
| sudo dpdk-devbind.py --bind=vfio-pci 0005:03:00.0 | ||
|
|
||
| # Verify binding | ||
| sudo dpdk-devbind.py --status | ||
| ``` | ||
|
|
||
| ### 5. Run Application | ||
|
|
||
| **Option A: Using the run script (recommended)** | ||
| ```bash | ||
| ./run.sh | ||
| # The script automatically handles sudo and venv activation | ||
| ``` | ||
|
|
||
| **Option B: Manual run** | ||
| ```bash | ||
| sudo ./venv/bin/python st2110_test_app.py | ||
| ``` | ||
|
|
||
| **Note**: Must run with sudo for DPDK access. |
There was a problem hiding this comment.
logic: Documentation contradiction - the PR description states this operator uses "standard Linux sockets rather than DPDK" but this setup guide extensively covers DPDK binding and configuration. If the ST2110 source uses Linux sockets, users don't need DPDK setup and this will cause confusion. Does the ST2110 source operator actually require DPDK or does it use standard Linux sockets as described in the PR?
| ### DPDK initialization fails | ||
|
|
||
| **Solution**: Check huge pages and permissions | ||
| ```bash | ||
| # Check huge pages | ||
| cat /proc/meminfo | grep Huge | ||
|
|
||
| # Configure if needed | ||
| sudo sh -c 'echo 2048 > /proc/sys/vm/nr_hugepages' | ||
|
|
||
| # Verify vfio-pci driver is loaded | ||
| lsmod | grep vfio | ||
| ``` | ||
|
|
||
| ### NIC binding fails | ||
|
|
||
| **Solution**: Ensure kernel modules are loaded | ||
| ```bash | ||
| # Load vfio-pci | ||
| sudo modprobe vfio-pci | ||
|
|
||
| # Check if NIC is already bound | ||
| sudo dpdk-devbind.py --status | ||
|
|
||
| # If stuck, unbind first | ||
| sudo dpdk-devbind.py --unbind 0005:03:00.0 | ||
| sudo dpdk-devbind.py --bind=vfio-pci 0005:03:00.0 | ||
| ``` |
There was a problem hiding this comment.
logic: DPDK troubleshooting section is inconsistent with the PR description stating the operator uses standard Linux sockets. If DPDK isn't required, this troubleshooting content will mislead users into unnecessary configuration steps.
| // Launch with one block per packet, 128 threads per block | ||
| const int threads_per_block = 128; | ||
| st2110_simple_reorder_kernel<<<num_pkts, threads_per_block, 0, stream>>>( | ||
| out, in, pkt_len, num_pkts); |
There was a problem hiding this comment.
syntax: Missing stream parameter in kernel launch. The st2110_simple_reorder_kernel is launched without the stream parameter while the wrapper function accepts one, causing a mismatch.
| out, in, pkt_len, num_pkts); | |
| out, in, pkt_len, num_pkts); |
Should be:
| out, in, pkt_len, num_pkts); | |
| st2110_simple_reorder_kernel<<<num_pkts, threads_per_block, 0, stream>>>( | |
| out, in, pkt_len, num_pkts); |
| @@ -0,0 +1,278 @@ | |||
| # ST2110 Test Application | |||
|
|
|||
| Simple test application to verify ST 2110-20 video reception using the ST2110SourceOp operator with DPDK backend. | |||
There was a problem hiding this comment.
logic: Documentation claims DPDK backend but PR description states the operator uses "standard Linux sockets rather than DPDK for portability." This fundamental contradiction will mislead users about the actual implementation and required setup steps.
| Simple test application to verify ST 2110-20 video reception using the ST2110SourceOp operator with DPDK backend. | |
| Simple test application to verify ST 2110-20 video reception using the ST2110SourceOp operator with Linux socket backend. |
Should this documentation reflect the socket-based implementation or is there a separate DPDK variant?
| ### 2. Bind NIC to DPDK | ||
|
|
||
| ```bash | ||
| # Take interface down | ||
| sudo ifconfig mgbe0_0 down | ||
|
|
||
| # Bind to DPDK | ||
| sudo dpdk-devbind.py --bind=vfio-pci 0005:03:00.0 | ||
|
|
||
| # Verify | ||
| sudo dpdk-devbind.py --status | ||
| ``` |
There was a problem hiding this comment.
logic: DPDK binding instructions are incorrect for a socket-based operator. Users following these steps will bind their NIC away from the kernel, preventing the socket-based operator from accessing the network interface.
|
|
||
| ### Software | ||
|
|
||
| - ST2110SourceOp operator built (see `../../operators/st2110_source/BUILD.md`) |
There was a problem hiding this comment.
syntax: Referenced file BUILD.md does not exist in the file listing. Either create this file or update the reference to point to an existing build documentation.
|
|
||
| - ST2110 Operator: `../../operators/st2110_source/README.md` | ||
| - Build Instructions: `../../operators/st2110_source/BUILD.md` | ||
| - Development Plan: `../../operators/st2110_source/DEVELOPMENT_PLAN.md` |
There was a problem hiding this comment.
syntax: Referenced file DEVELOPMENT_PLAN.md does not exist in the file listing. Remove this reference or create the missing documentation.
|
|
||
| The ST 2110 Source operator receives SMPTE ST 2110-20 uncompressed video streams over IP networks. This operator is designed for professional broadcast and media production workflows requiring video ingestion from ST 2110-compliant sources. | ||
|
|
||
| ## Overview |
There was a problem hiding this comment.
@Ollie-cj, it would be great if you could add an image that represents this operator and the test application. It can be anything that you find suitable. Having an image will help with its discoverability and presentation on the Holohub website. Thanks!
|
|
||
| This application tests the ST2110 source operator by: | ||
|
|
||
| 1. Receiving ST 2110-20 video packets via DPDK |
There was a problem hiding this comment.
logic: Incorrect: operator doesn't use DPDK. Remove this line - packets are received via standard Linux recvmmsg() syscalls on UDP sockets.
| ### Software | ||
|
|
||
| - ST2110SourceOp operator built (see `../../operators/st2110_source/BUILD.md`) | ||
| - DPDK configured and NIC bound to vfio-pci driver |
There was a problem hiding this comment.
logic: Incorrect prerequisite: DPDK is not required or used. The operator uses standard Linux UDP sockets.
| The `advanced_network` section configures DPDK: | ||
|
|
||
| - **manager**: "dpdk" for DPDK backend | ||
| - **memory_regions**: HDS split (headers to CPU, payload to GPU) | ||
| - **flows**: Packet steering rules for ST 2110 stream | ||
|
|
||
| ### ST2110 Source Config | ||
|
|
||
| The `st2110_source` section configures the operator: | ||
|
|
||
| - **interface_name**: Must match interface name in advanced_network | ||
| - **width/height**: Video resolution | ||
| - **pixel_format**: Output format (NV12 recommended for NVIDIA) | ||
| - **gpu_direct**: Enable zero-copy to GPU | ||
| - **header_data_split**: Enable HDS mode |
There was a problem hiding this comment.
logic: Entire "Advanced Network Config" section describes DPDK configuration that doesn't exist in the operator. Parameters like memory_regions, flows, gpu_direct, and header_data_split are not used.
| } | ||
| }; | ||
|
|
||
| PYBIND11_MODULE(_st2110_source, m) { |
There was a problem hiding this comment.
Missing companion __init__.py file prevents Python module import. Create operators/st2110_source/python/__init__.py to re-export ST2110SourceOp from _st2110_source.
Context Used: Rule from dashboard - When generating Python __init__.py files for pybind11 bindings that handle Holoscan SDK compatibil... (source)
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@applications/st2110_demo/st2110_demo_app.py`:
- Around line 58-61: st2110_config may be None because
self.kwargs("st2110_source") can be missing; before calling
st2110_config.get(...) add a guard that verifies st2110_config is a dict (or
truthy) and if absent raise/abort with a clear error message (e.g., "Missing
required 'st2110_source' config") or supply a safe default (empty dict) so
enable_rgba and enable_nv12 are derived safely; update the block around
st2110_config, enable_rgba, and enable_nv12 to perform this validation and
logging so downstream calls don't throw on None.
- Around line 35-40: The compute method currently declares unused parameters
op_output and context which trigger ARG002; update the compute signature in
st2110_demo_app.py to rename those parameters to underscore-prefixed
placeholders (e.g., _op_output, _context) so they are intentionally ignored by
the linter while preserving the function behavior in compute(op_input,
_op_output, _context) where op_input.receive("input") remains unchanged.
In `@operators/st2110_source/BUILD.md`:
- Around line 73-79: The verification commands in BUILD.md are inconsistent with
the preceding working directory (operators/st2110_source/build): either change
the first check from build/operators/st2110_source/libst2110_source.so to just
libst2110_source.so to match the current directory, or add an explicit cd to the
repository root before running both checks so that
build/operators/st2110_source/libst2110_source.so and ldd libst2110_source.so
run from the same location; update the lines referencing
build/operators/st2110_source/libst2110_source.so and the ldd invocation to use
the same working-directory assumption.
🧹 Nitpick comments (1)
operators/st2110_source/BUILD.md (1)
108-112: CMake download example is aarch64-specific.The example tarball is
linux-aarch64; on x86_64 this will fail. Suggest adding a short note or alternate x86_64 URL to prevent confusion.📄 Suggested doc tweak
- wget https://github.com/Kitware/CMake/releases/download/v3.28.1/cmake-3.28.1-linux-aarch64.tar.gz - tar -xzf cmake-3.28.1-linux-aarch64.tar.gz -C $HOME/.local/ --strip-components=1 + # Use linux-aarch64 on ARM, linux-x86_64 on x86_64 + wget https://github.com/Kitware/CMake/releases/download/v3.28.1/cmake-3.28.1-linux-aarch64.tar.gz + tar -xzf cmake-3.28.1-linux-aarch64.tar.gz -C $HOME/.local/ --strip-components=1
xrlabs-cj
force-pushed
the
ocj/st2110_operator
branch
from
27d07a3 to
793337f
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@applications/st2110_demo/metadata.json`:
- Line 3: The metadata 'name' value is a display title but must match the
application class name; update the "name" field in metadata.json from "ST
2110-20 Video Reception Demo" to the application class identifier (e.g.,
ST2110DemoApp) so it exactly matches the class name used for the app (search for
the class symbol ST2110DemoApp in the codebase to confirm the exact casing).
- Around line 49-65: Update the run.command entries to use the built artifact
paths under the build output instead of the source placeholder: replace
occurrences of "<holohub_app_source>/st2110_demo_app.py" and
"<holohub_app_source>/st2110_demo_config*.yaml" in the "run" for the default
mode, "rgba" and "nv12" with the corresponding files located in the
holohub_app_bin build output (e.g., reference the st2110_demo_app.py and the
three st2110_demo_config*.yaml files under holohub_app_bin), so each
"run.command" invokes python3 against the built binary and config files in the
holohub_app_bin workspace rather than the source directory.
| @@ -0,0 +1,71 @@ | |||
| { | |||
| "application": { | |||
| "name": "ST 2110-20 Video Reception Demo", | |||
There was a problem hiding this comment.
Align application.name with the application class name.
The metadata name should match the application class name (e.g., ST2110DemoApp) instead of a display title to satisfy metadata schema expectations.
💡 Suggested adjustment
- "name": "ST 2110-20 Video Reception Demo",
+ "name": "ST2110DemoApp",Based on learnings: metadata.json must include 'name' field matching the class name (TitleCase with 'Op' for operators).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "name": "ST 2110-20 Video Reception Demo", | |
| "name": "ST2110DemoApp", |
🤖 Prompt for AI Agents
In `@applications/st2110_demo/metadata.json` at line 3, The metadata 'name' value
is a display title but must match the application class name; update the "name"
field in metadata.json from "ST 2110-20 Video Reception Demo" to the application
class identifier (e.g., ST2110DemoApp) so it exactly matches the class name used
for the app (search for the class symbol ST2110DemoApp in the codebase to
confirm the exact casing).
| "run": { | ||
| "command": "python3 <holohub_app_source>/st2110_demo_app.py --config <holohub_app_source>/st2110_demo_config.yaml", | ||
| "workdir": "holohub_app_bin" | ||
| } | ||
| }, | ||
| "rgba": { | ||
| "description": "Display RGBA output only", | ||
| "run": { | ||
| "command": "python3 <holohub_app_source>/st2110_demo_app.py --config <holohub_app_source>/st2110_demo_config_rgba_only.yaml", | ||
| "workdir": "holohub_app_bin" | ||
| } | ||
| }, | ||
| "nv12": { | ||
| "description": "Display NV12 output only", | ||
| "run": { | ||
| "command": "python3 <holohub_app_source>/st2110_demo_app.py --config <holohub_app_source>/st2110_demo_config_nv12_only.yaml", | ||
| "workdir": "holohub_app_bin" |
There was a problem hiding this comment.
Use build-output paths in run.command instead of <holohub_app_source>.
The metadata run commands should reference artifacts under the build output (holohub_app_bin) rather than source paths.
💡 Example adjustment (apply consistently across modes)
- "command": "python3 <holohub_app_source>/st2110_demo_app.py --config <holohub_app_source>/st2110_demo_config.yaml",
+ "command": "python3 <holohub_app_bin>/applications/st2110_demo/st2110_demo_app.py --config <holohub_app_bin>/applications/st2110_demo/st2110_demo_config.yaml",Based on learnings: run.command paths must reference the build directory structure relative to the holohub_bin workspace, not the CMake install() destinations.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "run": { | |
| "command": "python3 <holohub_app_source>/st2110_demo_app.py --config <holohub_app_source>/st2110_demo_config.yaml", | |
| "workdir": "holohub_app_bin" | |
| } | |
| }, | |
| "rgba": { | |
| "description": "Display RGBA output only", | |
| "run": { | |
| "command": "python3 <holohub_app_source>/st2110_demo_app.py --config <holohub_app_source>/st2110_demo_config_rgba_only.yaml", | |
| "workdir": "holohub_app_bin" | |
| } | |
| }, | |
| "nv12": { | |
| "description": "Display NV12 output only", | |
| "run": { | |
| "command": "python3 <holohub_app_source>/st2110_demo_app.py --config <holohub_app_source>/st2110_demo_config_nv12_only.yaml", | |
| "workdir": "holohub_app_bin" | |
| "run": { | |
| "command": "python3 <holohub_app_bin>/applications/st2110_demo/st2110_demo_app.py --config <holohub_app_bin>/applications/st2110_demo/st2110_demo_config.yaml", | |
| "workdir": "holohub_app_bin" | |
| } | |
| }, | |
| "rgba": { | |
| "description": "Display RGBA output only", | |
| "run": { | |
| "command": "python3 <holohub_app_bin>/applications/st2110_demo/st2110_demo_app.py --config <holohub_app_bin>/applications/st2110_demo/st2110_demo_config_rgba_only.yaml", | |
| "workdir": "holohub_app_bin" | |
| } | |
| }, | |
| "nv12": { | |
| "description": "Display NV12 output only", | |
| "run": { | |
| "command": "python3 <holohub_app_bin>/applications/st2110_demo/st2110_demo_app.py --config <holohub_app_bin>/applications/st2110_demo/st2110_demo_config_nv12_only.yaml", | |
| "workdir": "holohub_app_bin" |
🤖 Prompt for AI Agents
In `@applications/st2110_demo/metadata.json` around lines 49 - 65, Update the
run.command entries to use the built artifact paths under the build output
instead of the source placeholder: replace occurrences of
"<holohub_app_source>/st2110_demo_app.py" and
"<holohub_app_source>/st2110_demo_config*.yaml" in the "run" for the default
mode, "rgba" and "nv12" with the corresponding files located in the
holohub_app_bin build output (e.g., reference the st2110_demo_app.py and the
three st2110_demo_config*.yaml files under holohub_app_bin), so each
"run.command" invokes python3 against the built binary and config files in the
holohub_app_bin workspace rather than the source directory.
793337f to
18d0fd5
Compare
|
Hi @Ollie-cj, thanks for addressing the issues in the PR. It seems that something went awry during your recent updates, and this PR is no longer based on the latest commits. It shows changes in 630 files and about 47k lines of code! Could you please rebase your branch with the latest changes on the |
bhashemian
added
the
Action Required by Author
Ollie-cj
force-pushed
the
ocj/st2110_operator
branch
from
2f1be98 to
9ce82f8
Compare
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (3)
operators/st2110_source/st2110_kernels.cuh (1)
23-25: Prefer#ifdef __cplusplusover#if __cplusplus.
#if __cplusplusevaluates the macro numerically (relying on undefined identifiers resolving to 0 in C99), while#ifdef __cplusplusunambiguously tests for definition. The latter is the standard idiom in all system and CUDA headers.📝 Suggested fix
-#if __cplusplus +#ifdef __cplusplus extern "C" { `#endif`-#if __cplusplus +#ifdef __cplusplus } `#endif`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@operators/st2110_source/st2110_kernels.cuh` around lines 23 - 25, Replace the preprocessor conditional that uses a numeric test "#if __cplusplus" with the standard definition check "#ifdef __cplusplus" (and keep the matching closing "#endif") in the extern "C" wrapper; update the block around extern "C" { } in st2110_kernels.cuh so the guard uses "__cplusplus" via `#ifdef/`#endif to unambiguously detect C++ compilation.operators/st2110_source/st2110_source.cpp (2)
506-514: Multiplestaticlocal variables will be shared across operator instances.
logged_first_packet(line 510),debug_packet_count(line 556),rejected_count(line 572),rgba_frame_count(line 1003),nv12_frame_count(line 1111), andcompute_count(line 1139) are allstaticlocals. If multipleST2110SourceOpinstances are created (e.g., multi-stream pipeline), these counters would be shared, producing misleading logs and skipping first-packet diagnostics for the second instance.Consider making them instance members instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@operators/st2110_source/st2110_source.cpp` around lines 506 - 514, The listed local variables (logged_first_packet, debug_packet_count, rejected_count, rgba_frame_count, nv12_frame_count, compute_count) are declared as static inside methods (e.g., parse_and_copy_packet and other functions) which causes them to be shared across all ST2110SourceOp instances; change each of these to be non-static instance members on the ST2110SourceOp class (add bool/int members with the same names and appropriate initializers in the class), remove the static declarations from the local scopes (e.g., in parse_and_copy_packet and the other functions), and update all uses to reference the instance members (this->logged_first_packet, this->debug_packet_count, etc.) so counters and first-packet logging are per-operator instance. Ensure their initial values are set in the class constructor.
627-641: Floating-point byte offset calculation may truncate for non-pgroup-aligned offsets.
line_offset * bppwherebpp=2.5produces a non-integer result for odd pixel offsets, which gets truncated on implicitdouble → size_tconversion. ST 2110-20 should guarantee pgroup-aligned offsets for 4:2:2, but using integer pgroup arithmetic would be more robust.Suggested refactor using integer arithmetic
- double bpp = detected_format_.detected ? detected_format_.bytes_per_pixel : 2.5; - size_t line_start = line_number * width_.get() * bpp; - size_t dest_offset = line_start + (line_offset * bpp); + // Use integer pgroup arithmetic to avoid floating-point truncation + uint32_t pgroup_bytes = detected_format_.detected ? detected_format_.pgroup : 5; + uint32_t pgroup_pixels = (detected_format_.sampling == "YCbCr-4:2:2") ? 2 : + (detected_format_.sampling == "RGBA") ? 1 : 2; + size_t line_bytes = static_cast<size_t>(width_.get() / pgroup_pixels) * pgroup_bytes; + size_t line_start = line_number * line_bytes; + size_t dest_offset = line_start + static_cast<size_t>(line_offset / pgroup_pixels) * pgroup_bytes;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@operators/st2110_source/st2110_source.cpp` around lines 627 - 641, The code uses a double bpp and implicit double→size_t conversion for dest_offset which truncates non-integer byte offsets; change to integer (rational) arithmetic using a numerator/denominator or pgroup-based math: replace bpp (double) with integer values (e.g., bytes_per_pixel_num and bytes_per_pixel_den or pgroup_bytes and pixels_per_pgroup derived from detected_format_), compute line_start and dest_offset by multiplying integers first then dividing (e.g., line_start = (line_number * width_.get() * bytes_per_pixel_num) / bytes_per_pixel_den; dest_offset = line_start + (line_offset * bytes_per_pixel_num) / bytes_per_pixel_den) and keep bounds check against raw_frame_size_, updating references to detected_format_.bytes_per_pixel, bpp, line_start, dest_offset, raw_frame_size_, recv_buffer and payload_size accordingly to avoid truncation and ensure pgroup alignment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@applications/st2110_demo/st2110_demo_config.yaml`:
- Around line 16-18: The top comment "RGBA output only for testing" is stale and
contradicts the actual setting enable_nv12_output: true; update or remove that
comment to accurately reflect current configuration (e.g., change to "NV12
output enabled for testing" or a neutral description) so the header comment and
the enable_nv12_output flag are consistent; locate the file header comment and
the enable_nv12_output key to make the change.
In `@operators/st2110_source/BUILD.md`:
- Around line 109-113: The instructions currently hard-code an aarch64 CMake
tarball (the wget line) which breaks on x86_64; update BUILD.md to detect or
document both architectures by providing separate download URLs (e.g.,
cmake-3.28.1-linux-aarch64.tar.gz and cmake-3.28.1-linux-x86_64.tar.gz), or add
a note to replace the filename with the correct architecture before running the
provided wget/tar/ PATH export commands (the wget line, tar -xzf line, and
export PATH line). Ensure the example shows both commands or a short shell
snippet that picks the right URL based on uname -m, and keep the existing PATH
export unchanged.
- Around line 64-66: The CMake command sets CMAKE_PREFIX_PATH to
"/opt/nvidia/holoscan/lib", causing CMake to search a double "lib" path; update
the cmake invocation in BUILD.md so the -DCMAKE_PREFIX_PATH value is the
installation prefix root "/opt/nvidia/holoscan" (not ".../lib") so
find_package() will correctly resolve the Holoscan config under
lib/cmake/holoscan/.
In `@operators/st2110_source/python/__init__.py`:
- Line 16: Remove the unused "# noqa: F401" on the "import holoscan.core" line
and replace it with a plain comment explaining the side-effect import; e.g.,
next to the import statement referencing that holoscan.core registers C++ types
required before importing _st2110_source so future readers understand the
intent. Ensure the import remains (import holoscan.core) and the comment
mentions _st2110_source and the C++ type registration.
- Around line 1-20: Missing pytest unit tests for the ST2110SourceOp Python
operator; add a new pytest file named test_st2110_source.py in the same package
that imports ST2110SourceOp and runs basic tests: (1) test importability (from
the package import ST2110SourceOp), (2) test default instantiation (call
ST2110SourceOp() and assert instance type), and (3) test parameter validation by
passing invalid values to the operator's initializer and asserting it raises the
expected exceptions; reference the ST2110SourceOp symbol and its __init__
signature to locate where to validate parameters.
In `@operators/st2110_source/README.md`:
- Around line 22-43: The fenced code block containing the ASCII architecture
diagram in operators/st2110_source/README.md lacks a language tag (MD040);
update that triple-backtick fence to include a plain/text language specifier
such as ```text or ```plain so the block becomes a valid fenced code block with
a language identifier, leaving the diagram content unchanged and keeping the
surrounding backticks intact.
In `@operators/st2110_source/st2110_source.cpp`:
- Around line 209-215: The assignment that unconditionally overwrites
raw_frame_size_ (raw_frame_size_ = width_.get() * height_.get() * 3) breaks
formats that set bytes_per_pixel in configure_stream_format() and should be
removed; instead, keep the value computed in configure_stream_format() (which
uses detected_format_.bytes_per_pixel) so raw_frame_size_ correctly reflects
width_/height_ * detected_format_.bytes_per_pixel and avoid changing
payload_size_ logic—delete the offending overwrite in the
constructor/initializer near where raw_frame_size_ and payload_size_ are set
(refer to raw_frame_size_, configure_stream_format(),
detected_format_.bytes_per_pixel, and payload_size_).
- Around line 905-913: Detected kernels convert_ycbcr422_10bit_to_rgba and
convert_ycbcr422_10bit_to_nv12 are always invoked in
ST2110SourceOp::convert_and_emit_rgba (and related code) even when
stream_format_ or detected_format_ indicate 8-bit or already-RGBA input; update
the logic to check detected_format_.sampling / stream_format_ before calling
those 10-bit 4:2:2 kernels and either (a) call the correct 8-bit conversion
kernel when format is "YCbCr-4:2:2-8bit", (b) bypass conversion if input is
already "RGBA-8bit", or (c) emit a clear error log and skip/abort emission when
an unsupported combination is requested; reference convert_and_emit_rgba,
convert_ycbcr422_10bit_to_rgba, convert_ycbcr422_10bit_to_nv12, stream_format_,
and detected_format_ when making the conditional guards (or error) so only
matching-bit-depth kernels run.
In `@operators/st2110_source/st2110_source.hpp`:
- Around line 171-197: The class has many POD members left uninitialized
(socket_fd_, payload_size_, total_bytes_received_, total_packets_received_,
total_packets_dropped_, assembling_index_, last_emitted_rtp_timestamp_,
first_frame_emitted_, frames_dropped_, last_sequence_number_, last_timestamp_,
first_packet_received_, gpu_raw_buffer_, gpu_rgba_buffer_, gpu_nv12_buffer_,
cuda_stream_) which the destructor and cleanup helpers read; add in-class
initializers so these have safe defaults (e.g., socket_fd_ = -1, payload_size_ =
0, total_* = 0, assembling_index_ = -1, last_emitted_rtp_timestamp_ = 0,
first_frame_emitted_ = false, frames_dropped_ = 0, last_sequence_number_ = 0,
last_timestamp_ = 0, first_packet_received_ = false,
gpu_raw_buffer_/gpu_rgba_buffer_/gpu_nv12_buffer_ = nullptr, cuda_stream_ =
nullptr/default) by editing the member declarations in st2110_source.hpp so
destructor and free_buffers()/close_socket() no longer read uninitialized
values.
---
Nitpick comments:
In `@operators/st2110_source/st2110_kernels.cuh`:
- Around line 23-25: Replace the preprocessor conditional that uses a numeric
test "#if __cplusplus" with the standard definition check "#ifdef __cplusplus"
(and keep the matching closing "#endif") in the extern "C" wrapper; update the
block around extern "C" { } in st2110_kernels.cuh so the guard uses
"__cplusplus" via `#ifdef/`#endif to unambiguously detect C++ compilation.
In `@operators/st2110_source/st2110_source.cpp`:
- Around line 506-514: The listed local variables (logged_first_packet,
debug_packet_count, rejected_count, rgba_frame_count, nv12_frame_count,
compute_count) are declared as static inside methods (e.g.,
parse_and_copy_packet and other functions) which causes them to be shared across
all ST2110SourceOp instances; change each of these to be non-static instance
members on the ST2110SourceOp class (add bool/int members with the same names
and appropriate initializers in the class), remove the static declarations from
the local scopes (e.g., in parse_and_copy_packet and the other functions), and
update all uses to reference the instance members (this->logged_first_packet,
this->debug_packet_count, etc.) so counters and first-packet logging are
per-operator instance. Ensure their initial values are set in the class
constructor.
- Around line 627-641: The code uses a double bpp and implicit double→size_t
conversion for dest_offset which truncates non-integer byte offsets; change to
integer (rational) arithmetic using a numerator/denominator or pgroup-based
math: replace bpp (double) with integer values (e.g., bytes_per_pixel_num and
bytes_per_pixel_den or pgroup_bytes and pixels_per_pgroup derived from
detected_format_), compute line_start and dest_offset by multiplying integers
first then dividing (e.g., line_start = (line_number * width_.get() *
bytes_per_pixel_num) / bytes_per_pixel_den; dest_offset = line_start +
(line_offset * bytes_per_pixel_num) / bytes_per_pixel_den) and keep bounds check
against raw_frame_size_, updating references to
detected_format_.bytes_per_pixel, bpp, line_start, dest_offset, raw_frame_size_,
recv_buffer and payload_size accordingly to avoid truncation and ensure pgroup
alignment.
| # ST2110 Demo Configuration | ||
| # ========================== | ||
| # RGBA output only for testing |
There was a problem hiding this comment.
Stale comment — "RGBA output only" contradicts enable_nv12_output: true.
📝 Suggested fix
-# RGBA output only for testing
+# Both RGBA and NV12 outputs enabled (default mode)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # ST2110 Demo Configuration | |
| # ========================== | |
| # RGBA output only for testing | |
| # ST2110 Demo Configuration | |
| # ========================== | |
| # Both RGBA and NV12 outputs enabled (default mode) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@applications/st2110_demo/st2110_demo_config.yaml` around lines 16 - 18, The
top comment "RGBA output only for testing" is stale and contradicts the actual
setting enable_nv12_output: true; update or remove that comment to accurately
reflect current configuration (e.g., change to "NV12 output enabled for testing"
or a neutral description) so the header comment and the enable_nv12_output flag
are consistent; locate the file header comment and the enable_nv12_output key to
make the change.
| cmake .. \ | ||
| -DCMAKE_PREFIX_PATH="/opt/nvidia/holoscan/lib" \ | ||
| -DCMAKE_BUILD_TYPE=Release |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, locate and read the BUILD.md file
find . -path "*/operators/st2110_source/BUILD.md" -type fRepository: nvidia-holoscan/holohub
Length of output: 1745
🏁 Script executed:
#!/bin/bash
# If found, read the relevant section
if [ -f "operators/st2110_source/BUILD.md" ]; then
wc -l "operators/st2110_source/BUILD.md"
sed -n '55,75p' "operators/st2110_source/BUILD.md"
fiRepository: nvidia-holoscan/holohub
Length of output: 1745
🏁 Script executed:
#!/bin/bash
# Search for CMAKE_PREFIX_PATH usage in other BUILD.md or CMakeLists.txt files
rg "CMAKE_PREFIX_PATH" --type md --type cmake -B 2 -A 2 | head -50Repository: nvidia-holoscan/holohub
Length of output: 1745
🏁 Script executed:
#!/bin/bash
# Search for holoscan find_package usage patterns
rg "find_package.*holoscan" --type cmake -B 2 -A 2 | head -40Repository: nvidia-holoscan/holohub
Length of output: 1745
🌐 Web query:
Holoscan SDK installation CMake config file path location
💡 Result:
For the standard Holoscan SDK install prefix (/opt/nvidia/holoscan), the CMake package config file for find_package(holoscan CONFIG ...) is located at:
/opt/nvidia/holoscan/lib/cmake/holoscan/holoscan-config.cmake(and the directory to point CMake at is/opt/nvidia/holoscan/lib/cmake/holoscan) [1]
This matches the docs’ guidance that the SDK is found via find_package(holoscan ... PATHS "/opt/nvidia/holoscan") [2].
Use it explicitly (if needed):
set(holoscan_DIR "/opt/nvidia/holoscan/lib/cmake/holoscan")
find_package(holoscan REQUIRED CONFIG)Sources: [1] [2]
🌐 Web query:
CMAKE_PREFIX_PATH behavior find_package CMake documentation
💡 Result:
CMAKE_PREFIX_PATH is a list of installation prefixes that CMake uses as additional search roots for find_package() (and also find_program(), find_library(), find_file(), find_path()). For each prefix, the individual find_* command appends the “right” subdirectories (e.g., lib/, include/, share/, lib/cmake/<Pkg>/, etc.) according to that command’s documented search rules. [1]
How it affects find_package()
In Config mode, find_package() builds a set of candidate prefixes and then searches a standard set of package config locations under each prefix (e.g., <prefix>/lib/cmake/<name>*/, <prefix>/share/<name>*/, etc.). [2]
When constructing the prefix list, CMAKE_PREFIX_PATH participates as follows:
- CMake variable
CMAKE_PREFIX_PATH(cache/-D) is searched as part of the “CMake-specific cache variables” step (skippable withNO_CMAKE_PATHorCMAKE_FIND_USE_CMAKE_PATH=FALSE). [3] - Environment variable
CMAKE_PREFIX_PATHis also searched (as “CMake-specific environment variables”), as an additional list of prefixes (skippable withNO_CMAKE_ENVIRONMENT_PATHorCMAKE_FIND_USE_CMAKE_ENVIRONMENT_PATH=FALSE). [2]
Related knobs commonly involved
CMAKE_FIND_PACKAGE_PREFER_CONFIGcan make CMake try Config mode before Module mode. [3]NO_DEFAULT_PATH(or the more granularNO_*options likeNO_CMAKE_PATH) can disable the default prefix sources that includeCMAKE_PREFIX_PATH. [2]
Sources: CMake docs for CMAKE_PREFIX_PATH [1] and find_package() search procedure / prefix construction [2][3].
🌐 Web query:
Holoscan SDK CMake holoscanConfig.cmake installation directory
💡 Result:
holoscanConfig.cmake is installed under the Holoscan SDK install prefix in the CMake package directory:
<prefix>/<libdir>/cmake/holoscan/holoscanConfig.cmake(andholoscanConfigVersion.cmake) — the SDK’s CMake install rules place Holoscan’s CMake files under${HOLOSCAN_INSTALL_LIB_DIR}/cmake/holoscan(wherelibdiris typicallylib). (raw.githubusercontent.com)
So, with the common/default SDK prefix:
/opt/nvidia/holoscan/lib/cmake/holoscan/holoscanConfig.cmake(common in SDK images/installs that use/opt/nvidia/holoscanas the prefix). (github.com)
In CMake you can point to it via either of these:
-DHoloscan_ROOT=/opt/nvidia/holoscan(thenfind_package(holoscan CONFIG REQUIRED)), as shown in the docs. (docs.nvidia.com)- or directly:
-Dholoscan_DIR=/opt/nvidia/holoscan/lib/cmake/holoscan. (raw.githubusercontent.com)
Citations:
- 1: https://raw.githubusercontent.com/nvidia-holoscan/holoscan-sdk/main/CMakeLists.txt
- 2: https://github.com/nvidia-holoscan/meta-tegra-holoscan?utm_source=openai
- 3: https://docs.nvidia.com/holoscan/archive/2.0.0/holoscan_create_app.html?utm_source=openai
- 4: https://raw.githubusercontent.com/nvidia-holoscan/holoscan-sdk/main/CMakeLists.txt
Use /opt/nvidia/holoscan as the CMAKE_PREFIX_PATH prefix root, not /opt/nvidia/holoscan/lib.
CMAKE_PREFIX_PATH should specify the installation prefix root. When CMake searches for config files via find_package(), it appends lib/cmake/holoscan/ to the prefix to construct the search path. Setting it to /opt/nvidia/holoscan/lib results in CMake searching /opt/nvidia/holoscan/lib/lib/cmake/holoscan/ (double lib), which is incorrect. The Holoscan SDK installs its CMake config at /opt/nvidia/holoscan/lib/cmake/holoscan/, so the correct prefix is /opt/nvidia/holoscan, which aligns with the SDK's own documentation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@operators/st2110_source/BUILD.md` around lines 64 - 66, The CMake command
sets CMAKE_PREFIX_PATH to "/opt/nvidia/holoscan/lib", causing CMake to search a
double "lib" path; update the cmake invocation in BUILD.md so the
-DCMAKE_PREFIX_PATH value is the installation prefix root "/opt/nvidia/holoscan"
(not ".../lib") so find_package() will correctly resolve the Holoscan config
under lib/cmake/holoscan/.
| # Download and install locally | ||
| wget https://github.com/Kitware/CMake/releases/download/v3.28.1/cmake-3.28.1-linux-aarch64.tar.gz | ||
| tar -xzf cmake-3.28.1-linux-aarch64.tar.gz -C $HOME/.local/ --strip-components=1 | ||
| export PATH=$HOME/.local/bin:$PATH | ||
| ``` |
There was a problem hiding this comment.
CMake download URL is aarch64-specific; x86_64 users need a different binary.
The "CMake version too old" fix section only covers aarch64. An x86_64 user following this guide would download and install an incompatible binary.
📝 Suggested fix
-wget https://github.com/Kitware/CMake/releases/download/v3.28.1/cmake-3.28.1-linux-aarch64.tar.gz
-tar -xzf cmake-3.28.1-linux-aarch64.tar.gz -C $HOME/.local/ --strip-components=1
+ARCH=$(uname -m)
+wget https://github.com/Kitware/CMake/releases/download/v3.28.1/cmake-3.28.1-linux-${ARCH}.tar.gz
+tar -xzf cmake-3.28.1-linux-${ARCH}.tar.gz -C $HOME/.local/ --strip-components=1📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Download and install locally | |
| wget https://github.com/Kitware/CMake/releases/download/v3.28.1/cmake-3.28.1-linux-aarch64.tar.gz | |
| tar -xzf cmake-3.28.1-linux-aarch64.tar.gz -C $HOME/.local/ --strip-components=1 | |
| export PATH=$HOME/.local/bin:$PATH | |
| ``` | |
| # Download and install locally | |
| ARCH=$(uname -m) | |
| wget https://github.com/Kitware/CMake/releases/download/v3.28.1/cmake-3.28.1-linux-${ARCH}.tar.gz | |
| tar -xzf cmake-3.28.1-linux-${ARCH}.tar.gz -C $HOME/.local/ --strip-components=1 | |
| export PATH=$HOME/.local/bin:$PATH |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@operators/st2110_source/BUILD.md` around lines 109 - 113, The instructions
currently hard-code an aarch64 CMake tarball (the wget line) which breaks on
x86_64; update BUILD.md to detect or document both architectures by providing
separate download URLs (e.g., cmake-3.28.1-linux-aarch64.tar.gz and
cmake-3.28.1-linux-x86_64.tar.gz), or add a note to replace the filename with
the correct architecture before running the provided wget/tar/ PATH export
commands (the wget line, tar -xzf line, and export PATH line). Ensure the
example shows both commands or a short shell snippet that picks the right URL
based on uname -m, and keep the existing PATH export unchanged.
| # SPDX-FileCopyrightText: Copyright (c) 2025 XRlabs. All rights reserved. | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
| # | ||
| # Licensed under the Apache License, Version 2.0 (the "License"); | ||
| # you may not use this file except in compliance with the License. | ||
| # You may obtain a copy of the License at | ||
| # | ||
| # http://www.apache.org/licenses/LICENSE-2.0 | ||
| # | ||
| # Unless required by applicable law or agreed to in writing, software | ||
| # distributed under the License is distributed on an "AS IS" BASIS, | ||
| # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| # See the License for the specific language governing permissions and | ||
| # limitations under the License. | ||
|
|
||
| import holoscan.core # noqa: F401 | ||
|
|
||
| from ._st2110_source import ST2110SourceOp | ||
|
|
||
| __all__ = ["ST2110SourceOp"] |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Missing pytest unit tests — coding guidelines violation.
The coding guidelines require Python operators to include unit tests in the same directory using the pytest framework. No test file (test_st2110_source.py or similar) is included in this PR for the ST2110SourceOp Python binding.
Would you like me to open an issue or generate a skeleton test_st2110_source.py that covers basic import, instantiation, and parameter validation of ST2110SourceOp?
Based on coding guidelines: "Python operators must include unit tests in the same directory using pytest framework."
🧰 Tools
🪛 Ruff (0.15.1)
[warning] 16-16: Unused noqa directive (non-enabled: F401)
Remove unused noqa directive
(RUF100)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@operators/st2110_source/python/__init__.py` around lines 1 - 20, Missing
pytest unit tests for the ST2110SourceOp Python operator; add a new pytest file
named test_st2110_source.py in the same package that imports ST2110SourceOp and
runs basic tests: (1) test importability (from the package import
ST2110SourceOp), (2) test default instantiation (call ST2110SourceOp() and
assert instance type), and (3) test parameter validation by passing invalid
values to the operator's initializer and asserting it raises the expected
exceptions; reference the ST2110SourceOp symbol and its __init__ signature to
locate where to validate parameters.
| # See the License for the specific language governing permissions and | ||
| # limitations under the License. | ||
|
|
||
| import holoscan.core # noqa: F401 |
There was a problem hiding this comment.
Remove the unused # noqa: F401 directive (Ruff RUF100).
Ruff reports that F401 is not enabled in the project's configuration, making the noqa directive a no-op. If the side-effect import is intentional (it is — it ensures holoscan C++ types are registered before _st2110_source loads), document that intent with a plain comment instead.
📝 Suggested fix
-import holoscan.core # noqa: F401
+import holoscan.core # required: registers C++ GXF types before _st2110_source is loaded📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import holoscan.core # noqa: F401 | |
| import holoscan.core # required: registers C++ GXF types before _st2110_source is loaded |
🧰 Tools
🪛 Ruff (0.15.1)
[warning] 16-16: Unused noqa directive (non-enabled: F401)
Remove unused noqa directive
(RUF100)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@operators/st2110_source/python/__init__.py` at line 16, Remove the unused "#
noqa: F401" on the "import holoscan.core" line and replace it with a plain
comment explaining the side-effect import; e.g., next to the import statement
referencing that holoscan.core registers C++ types required before importing
_st2110_source so future readers understand the intent. Ensure the import
remains (import holoscan.core) and the comment mentions _st2110_source and the
C++ type registration.
| ``` | ||
| UDP Socket (multicast) | ||
| │ | ||
| ▼ | ||
| recvmmsg() batch receive | ||
| │ | ||
| ▼ | ||
| RTP/ST2110 header parsing | ||
| │ | ||
| ▼ | ||
| Triple-buffer (pinned memory) | ||
| [Assembling] [Complete] [Complete] | ||
| │ | ||
| ▼ | ||
| cudaMemcpyAsync (H2D) | ||
| │ | ||
| ├──► raw_output (YCbCr-4:2:2-10bit) | ||
| │ | ||
| ├──► CUDA kernel ──► rgba_output (RGBA 8-bit) | ||
| │ | ||
| └──► CUDA kernel ──► nv12_output (NV12 8-bit) | ||
| ``` |
There was a problem hiding this comment.
Add a language specifier to the fenced code block (markdownlint MD040).
The architecture diagram's code fence has no language tag. Use ```text or ```plain to satisfy the MD040 rule.
📝 Suggested fix
-```
+```text
UDP Socket (multicast)🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 22-22: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@operators/st2110_source/README.md` around lines 22 - 43, The fenced code
block containing the ASCII architecture diagram in
operators/st2110_source/README.md lacks a language tag (MD040); update that
triple-backtick fence to include a plain/text language specifier such as ```text
or ```plain so the block becomes a valid fenced code block with a language
identifier, leaving the diagram content unchanged and keeping the surrounding
backticks intact.
|
|
||
| // Raw frame size calculated after format detection (default: 10-bit 4:2:2 = 2.5 bpp) | ||
| raw_frame_size_ = width_.get() * height_.get() * 3; // Conservative: 3 bytes/pixel | ||
|
|
||
| // UDP socket recv() gives us payload only (no Eth/IP/UDP headers) | ||
| // Video payload per packet = total - RTP header - ST2110 header | ||
| payload_size_ = max_packet_size_.get() - rtp_header_size_.get() - sizeof(ST2110Header); |
There was a problem hiding this comment.
raw_frame_size_ overwrite — breaks RGBA-8bit and wastes memory for all other formats.
configure_stream_format() (called on line 193) correctly sets raw_frame_size_ based on detected_format_.bytes_per_pixel. Line 211 unconditionally overwrites it with width * height * 3.
For RGBA-8bit (bpp=4.0): correct size is ~8.3 MB, but overwrite sets it to ~6.2 MB. The bounds check in parse_and_copy_packet (line 634) will silently reject packets for the bottom ~25% of each frame, producing corrupted output. For 10-bit 4:2:2 (bpp=2.5): buffer is oversized by ~20%, causing unnecessary memcpy/memset overhead.
Suggested fix — remove the overwrite
- // Raw frame size calculated after format detection (default: 10-bit 4:2:2 = 2.5 bpp)
- raw_frame_size_ = width_.get() * height_.get() * 3; // Conservative: 3 bytes/pixel
-
// UDP socket recv() gives us payload only (no Eth/IP/UDP headers)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@operators/st2110_source/st2110_source.cpp` around lines 209 - 215, The
assignment that unconditionally overwrites raw_frame_size_ (raw_frame_size_ =
width_.get() * height_.get() * 3) breaks formats that set bytes_per_pixel in
configure_stream_format() and should be removed; instead, keep the value
computed in configure_stream_format() (which uses
detected_format_.bytes_per_pixel) so raw_frame_size_ correctly reflects
width_/height_ * detected_format_.bytes_per_pixel and avoid changing
payload_size_ logic—delete the offending overwrite in the
constructor/initializer near where raw_frame_size_ and payload_size_ are set
(refer to raw_frame_size_, configure_stream_format(),
detected_format_.bytes_per_pixel, and payload_size_).
| void ST2110SourceOp::convert_and_emit_rgba(OutputContext& op_output) { | ||
| HOLOSCAN_LOG_DEBUG("Converting {} → RGBA", detected_format_.sampling); | ||
|
|
||
| // Step 1: Convert raw YCbCr to RGBA using CUDA kernel | ||
| convert_ycbcr422_10bit_to_rgba(gpu_rgba_buffer_, | ||
| gpu_raw_buffer_, | ||
| width_.get(), | ||
| height_.get(), | ||
| cuda_stream_); |
There was a problem hiding this comment.
Conversion kernels are hardcoded for 10-bit 4:2:2 but stream_format supports 8-bit and RGBA.
convert_ycbcr422_10bit_to_rgba and convert_ycbcr422_10bit_to_nv12 are always called when the respective output is enabled, regardless of stream_format_. For "YCbCr-4:2:2-8bit" or "RGBA-8bit" inputs, these produce incorrect output since the kernel assumes 10-bit packed pixel groups.
At minimum, guard these calls or emit an error when RGBA/NV12 conversion is enabled with an unsupported input format:
Suggested approach — guard conversion in compute()
// Step 3: Optionally convert and emit RGBA
if (enable_rgba_output_.get()) {
+ if (detected_format_.depth != 10 || detected_format_.sampling != "YCbCr-4:2:2") {
+ HOLOSCAN_LOG_ERROR("RGBA conversion only supports YCbCr-4:2:2-10bit, got {}-{}bit",
+ detected_format_.sampling, detected_format_.depth);
+ } else {
convert_and_emit_rgba(op_output);
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@operators/st2110_source/st2110_source.cpp` around lines 905 - 913, Detected
kernels convert_ycbcr422_10bit_to_rgba and convert_ycbcr422_10bit_to_nv12 are
always invoked in ST2110SourceOp::convert_and_emit_rgba (and related code) even
when stream_format_ or detected_format_ indicate 8-bit or already-RGBA input;
update the logic to check detected_format_.sampling / stream_format_ before
calling those 10-bit 4:2:2 kernels and either (a) call the correct 8-bit
conversion kernel when format is "YCbCr-4:2:2-8bit", (b) bypass conversion if
input is already "RGBA-8bit", or (c) emit a clear error log and skip/abort
emission when an unsupported combination is requested; reference
convert_and_emit_rgba, convert_ycbcr422_10bit_to_rgba,
convert_ycbcr422_10bit_to_nv12, stream_format_, and detected_format_ when making
the conditional guards (or error) so only matching-bit-depth kernels run.
| // Socket state | ||
| int socket_fd_; | ||
|
|
||
| // Internal state | ||
| uint16_t payload_size_; // max_packet_size - header_size - rtp_header_size | ||
| int64_t total_bytes_received_; | ||
| int64_t total_packets_received_; | ||
| int64_t total_packets_dropped_; | ||
|
|
||
| // Circular queue architecture (virtual queue - no memcpy) | ||
| static constexpr int num_frame_buffers_ = 3; | ||
| std::array<FrameBuffer, num_frame_buffers_> frame_buffers_; | ||
| std::atomic<int> assembling_index_; // Index of buffer currently being assembled (-1 if none) | ||
| uint32_t last_emitted_rtp_timestamp_; // RTP timestamp of last emitted frame | ||
| bool first_frame_emitted_; // Track if we've emitted our first frame | ||
| uint32_t frames_dropped_; // Count of frames dropped due to queue overflow | ||
|
|
||
| // RTP sequence tracking for packet loss detection | ||
| uint16_t last_sequence_number_; | ||
| uint32_t last_timestamp_; | ||
| bool first_packet_received_; | ||
|
|
||
| // CUDA resources | ||
| void* gpu_raw_buffer_; // GPU buffer for raw YCbCr-4:2:2-10bit data | ||
| void* gpu_rgba_buffer_; // GPU buffer for RGBA 8-bit data (if RGBA output enabled) | ||
| void* gpu_nv12_buffer_; // GPU buffer for NV12 8-bit data (if NV12 output enabled) | ||
| cudaStream_t cuda_stream_; // CUDA stream for async H2D copy and conversions |
There was a problem hiding this comment.
Uninitialized POD members are read by the destructor.
The default constructor is = default, so these members are left uninitialized. The destructor (in st2110_source.cpp) logs total_bytes_received_, total_packets_received_, and total_packets_dropped_, and free_buffers()/close_socket() check socket_fd_, gpu_raw_buffer_, cuda_stream_, etc. If the object is destroyed before initialize() completes (e.g., an exception during setup or early lifecycle failure), this is undefined behavior.
Add in-class initializers for all POD members:
Suggested fix
// Socket state
- int socket_fd_;
+ int socket_fd_ = -1;
// Internal state
uint16_t payload_size_; // max_packet_size - header_size - rtp_header_size
- int64_t total_bytes_received_;
- int64_t total_packets_received_;
- int64_t total_packets_dropped_;
+ int64_t total_bytes_received_ = 0;
+ int64_t total_packets_received_ = 0;
+ int64_t total_packets_dropped_ = 0;
// ...
- uint32_t last_emitted_rtp_timestamp_;
- bool first_frame_emitted_;
- uint32_t frames_dropped_;
+ uint32_t last_emitted_rtp_timestamp_ = 0;
+ bool first_frame_emitted_ = false;
+ uint32_t frames_dropped_ = 0;
// RTP sequence tracking
- uint16_t last_sequence_number_;
- uint32_t last_timestamp_;
- bool first_packet_received_;
+ uint16_t last_sequence_number_ = 0;
+ uint32_t last_timestamp_ = 0;
+ bool first_packet_received_ = false;
// CUDA resources
- void* gpu_raw_buffer_;
- void* gpu_rgba_buffer_;
- void* gpu_nv12_buffer_;
- cudaStream_t cuda_stream_;
+ void* gpu_raw_buffer_ = nullptr;
+ void* gpu_rgba_buffer_ = nullptr;
+ void* gpu_nv12_buffer_ = nullptr;
+ cudaStream_t cuda_stream_ = 0;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@operators/st2110_source/st2110_source.hpp` around lines 171 - 197, The class
has many POD members left uninitialized (socket_fd_, payload_size_,
total_bytes_received_, total_packets_received_, total_packets_dropped_,
assembling_index_, last_emitted_rtp_timestamp_, first_frame_emitted_,
frames_dropped_, last_sequence_number_, last_timestamp_, first_packet_received_,
gpu_raw_buffer_, gpu_rgba_buffer_, gpu_nv12_buffer_, cuda_stream_) which the
destructor and cleanup helpers read; add in-class initializers so these have
safe defaults (e.g., socket_fd_ = -1, payload_size_ = 0, total_* = 0,
assembling_index_ = -1, last_emitted_rtp_timestamp_ = 0, first_frame_emitted_ =
false, frames_dropped_ = 0, last_sequence_number_ = 0, last_timestamp_ = 0,
first_packet_received_ = false,
gpu_raw_buffer_/gpu_rgba_buffer_/gpu_nv12_buffer_ = nullptr, cuda_stream_ =
nullptr/default) by editing the member declarations in st2110_source.hpp so
destructor and free_buffers()/close_socket() no longer read uninitialized
values.
Adds a new operator and demo application for receiving SMPTE ST 2110-20 uncompressed video streams over IP networks. The st2110_source operator receives broadcast-standard uncompressed video via multicast UDP, targeting professional media and medical imaging workflows. Features include: - Multicast UDP reception via Linux sockets (recvmmsg() batch processing) - Triple-buffer architecture with CUDA pinned memory - RTP header parsing for frame boundary detection - Multiple outputs: raw YCbCr-4:2:2-10bit, RGBA 8-bit, NV12 8-bit - CUDA kernels for 10-bit color conversion (BT.709) - Python bindings via pybind11 Uses standard Linux sockets rather than DPDK for portability across the Jetson family, where unified memory architecture reduces the benefit of GPUDirect. Platform support: x86_64, aarch64 (tested on NVIDIA Thor AGX with MGBE) Signed-off-by: Oliver Cleveley-Jones <ollie.cj@gmail.com>
Ollie-cj
force-pushed
the
ocj/st2110_operator
branch
from
bae5cec to
0e10922
Compare
…write, guard conversion kernels
- Add in-class initializers to all POD members to prevent UB if object
is destroyed before initialize() completes
- Remove critical bug: raw_frame_size_ overwrite in initialize() that
clobbered the correct value already set by configure_stream_format()
- Guard RGBA/NV12 conversion kernels to only run on YCbCr-4:2:2-10bit
input, logging an error for incompatible formats
- Replace 6 static local variables with instance members for
multi-instance safety
- Use integer pgroup arithmetic for byte offset calculation instead of
floating-point to avoid truncation
- Fix #if __cplusplus -> #ifdef __cplusplus in kernels header
- Fix BUILD.md cmake prefix path and add arch-detecting cmake install
- Minor: descriptive import comment, markdown code fence, yaml comment
Ollie-cj
force-pushed
the
ocj/st2110_operator
branch
from
0e10922 to
0557879
Compare
| import holoscan.core # noqa: F401 - required: registers C++ GXF types before _st2110_source is loaded | ||
|
|
||
| from ._st2110_source import ST2110SourceOp | ||
|
|
||
| __all__ = ["ST2110SourceOp"] |
There was a problem hiding this comment.
Missing pybind11 SDK compatibility handling. The __init__.py should wrap the import in a try/except ImportError block to provide helpful diagnostics when there's a pybind11 ABI mismatch with the installed Holoscan SDK version. This is the established pattern for holohub pybind11 operator bindings.
| import holoscan.core # noqa: F401 - required: registers C++ GXF types before _st2110_source is loaded | |
| from ._st2110_source import ST2110SourceOp | |
| __all__ = ["ST2110SourceOp"] | |
| import holoscan.core # noqa: F401 - required: registers C++ GXF types before _st2110_source is loaded | |
| try: | |
| from ._st2110_source import ST2110SourceOp | |
| except ImportError as e: | |
| pybind11_hsdk_err = 'unknown base type "holoscan::' | |
| if pybind11_hsdk_err not in str(e): | |
| raise e | |
| import sys | |
| sys.stderr.write( | |
| "\nError importing ST2110SourceOp: pybind11 binding incompatible " | |
| "with installed Holoscan SDK version.\n" | |
| "Rebuild the operator against the installed Holoscan SDK.\n\n" | |
| ) | |
| raise e | |
| __all__ = ["ST2110SourceOp"] |
Context Used: Rule from dashboard - When generating Python __init__.py files for pybind11 bindings that handle Holoscan SDK compatibil... (source)
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!
| // UDP socket recv() gives us payload only (no Eth/IP/UDP headers) | ||
| // Video payload per packet = total - RTP header - ST2110 header | ||
| payload_size_ = max_packet_size_.get() - rtp_header_size_.get() - sizeof(ST2110Header); |
There was a problem hiding this comment.
payload_size_ calculation is incorrect. The comment correctly notes that recv() gives UDP payload only (no Ethernet/IP/UDP headers), but then max_packet_size_ (default 1514, the Ethernet MTU including L2-L4 headers) is used directly without subtracting header_size_ (the 42-byte Ethernet+IP+UDP overhead). This means the computed value is 1494, while the actual max video payload per packet is (1514 - 42) - 12 - 8 = 1452.
This field is only used for logging at line 241, so it won't cause runtime errors, but the logged value will be misleading.
| // UDP socket recv() gives us payload only (no Eth/IP/UDP headers) | |
| // Video payload per packet = total - RTP header - ST2110 header | |
| payload_size_ = max_packet_size_.get() - rtp_header_size_.get() - sizeof(ST2110Header); | |
| // UDP socket recv() gives us payload only (no Eth/IP/UDP headers) | |
| // Video payload per packet = UDP payload - RTP header - ST2110 header | |
| payload_size_ = max_packet_size_.get() - header_size_.get() - rtp_header_size_.get() - sizeof(ST2110Header); |
| uint16_t line_offset = offset_and_cont & 0x7FFF; | ||
| bool continuation_bit = (offset_and_cont & 0x8000) != 0; | ||
|
|
||
| // Debug: Log ST2110 header values for first few packets | ||
| if (debug_packet_count_ < 5) { | ||
| HOLOSCAN_LOG_INFO("ST2110 header debug [packet {}]: line={}, offset={}, length={}, F={}, C={}", | ||
| debug_packet_count_, line_number, line_offset, line_length, field_bit, continuation_bit); | ||
| HOLOSCAN_LOG_INFO(" Raw bytes at ST2110 start: {:02x} {:02x} {:02x} {:02x} {:02x} {:02x} {:02x} {:02x}", | ||
| st2110_start[0], st2110_start[1], st2110_start[2], st2110_start[3], | ||
| st2110_start[4], st2110_start[5], st2110_start[6], st2110_start[7]); | ||
| debug_packet_count_++; | ||
| } | ||
|
|
||
| // Skip to payload (after ST2110 header) | ||
| const uint8_t* payload = st2110_start + sizeof(ST2110Header); | ||
| size_t payload_size = remaining_size - sizeof(ST2110Header); |
There was a problem hiding this comment.
Continuation bit parsed but not handled. Per SMPTE ST 2110-20, when continuation_bit == 1, the packet contains additional 6-byte SRD (Sample Row Data) descriptors after the first one. The payload starts after the last SRD descriptor (the one with C=0). Currently, the code always skips exactly sizeof(ST2110Header) (8 bytes = 2-byte extended seq + one 6-byte SRD), regardless of whether additional SRDs follow.
If a source sends multi-line packets (C=1), the payload pointer at line 562 will point into the second SRD descriptor rather than the actual pixel data, corrupting the reassembled frame. Additionally, only the first line's data will be placed, and subsequent lines in the same packet will be silently lost.
Consider either:
- Looping over SRD descriptors while C=1 and placing each line segment separately, or
- Rejecting packets with C=1 and logging a warning that multi-SRD packets are not supported
| // Wrap GPU raw buffer in tensor | ||
| tensor->wrapMemory(shape, | ||
| nvidia::gxf::PrimitiveType::kUnsigned8, | ||
| 1, // bytes per element (uint8) | ||
| nvidia::gxf::ComputeTrivialStrides(shape, 1), | ||
| nvidia::gxf::MemoryStorageType::kDevice, | ||
| gpu_raw_buffer_, | ||
| [](void*) { return nvidia::gxf::Success; }); // No deallocation (we manage it) |
There was a problem hiding this comment.
raw_output wraps shared GPU buffer without copying — data race with next frame. The gpu_raw_buffer_ is wrapped directly into the output tensor using wrapMemory with a no-op deallocator. This means the downstream operator receives a tensor that points to the same gpu_raw_buffer_ that will be overwritten by cudaMemcpyAsync on the next emit_next_frame() call.
If the downstream operator hasn't finished processing the tensor before the next compute() cycle overwrites gpu_raw_buffer_, it will read corrupted/mixed data. This is especially likely at high frame rates or if the downstream operator is slow.
The RGBA and NV12 emission paths correctly handle this by doing a device-to-device copy into a separately allocated tensor (reshapeCustom + cudaMemcpyAsync). The raw_output path should follow the same pattern, or document that downstream consumers must complete processing within a single compute() cycle.
Signed-off-by: B Hashemian <3968947+bhashemian@users.noreply.github.com>
| uint16_t line_offset = offset_and_cont & 0x7FFF; | ||
| bool continuation_bit = (offset_and_cont & 0x8000) != 0; | ||
|
|
||
| // Debug: Log ST2110 header values for first few packets | ||
| if (debug_packet_count_ < 5) { | ||
| HOLOSCAN_LOG_INFO("ST2110 header debug [packet {}]: line={}, offset={}, length={}, F={}, C={}", | ||
| debug_packet_count_, line_number, line_offset, line_length, field_bit, continuation_bit); | ||
| HOLOSCAN_LOG_INFO(" Raw bytes at ST2110 start: {:02x} {:02x} {:02x} {:02x} {:02x} {:02x} {:02x} {:02x}", | ||
| st2110_start[0], st2110_start[1], st2110_start[2], st2110_start[3], | ||
| st2110_start[4], st2110_start[5], st2110_start[6], st2110_start[7]); | ||
| debug_packet_count_++; | ||
| } | ||
|
|
||
| // Skip to payload (after ST2110 header) | ||
| const uint8_t* payload = st2110_start + sizeof(ST2110Header); |
There was a problem hiding this comment.
Continuation bit parsed but not handled. Per SMPTE ST 2110-20, when C=1, additional 6-byte SRD descriptors follow. Currently skips only sizeof(ST2110Header) (8 bytes), which will misalign payload pointer if multi-SRD packets arrive.
Consider rejecting C=1 packets with a warning until multi-SRD support is added.
| // Wrap GPU raw buffer in tensor | ||
| tensor->wrapMemory(shape, | ||
| nvidia::gxf::PrimitiveType::kUnsigned8, | ||
| 1, // bytes per element (uint8) | ||
| nvidia::gxf::ComputeTrivialStrides(shape, 1), | ||
| nvidia::gxf::MemoryStorageType::kDevice, | ||
| gpu_raw_buffer_, | ||
| [](void*) { return nvidia::gxf::Success; }); // No deallocation (we manage it) |
There was a problem hiding this comment.
raw_output wraps shared GPU buffer without copying — downstream race risk. gpu_raw_buffer_ is wrapped directly with no-op deallocator, but will be overwritten on next emit_next_frame() call. If downstream operator hasn't finished processing before next frame, it reads corrupted data.
RGBA/NV12 paths correctly do D2D copy via reshapeCustom + cudaMemcpyAsync. Apply same pattern here or document single-cycle processing requirement.
| import holoscan.core # noqa: F401 - required: registers C++ GXF types before _st2110_source is loaded | ||
|
|
||
| from ._st2110_source import ST2110SourceOp |
There was a problem hiding this comment.
Missing pybind11 SDK compatibility handling. Should wrap import in try/except to catch 'unknown base type "holoscan::' errors and provide helpful diagnostic message about SDK ABI mismatches.
See custom rule 870081e7 for the established pattern used in other holohub operators.
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!
Signed-off-by: B Hashemian <3968947+bhashemian@users.noreply.github.com>
| import holoscan.core # noqa: F401 - required: registers C++ GXF types before _st2110_source is loaded | ||
|
|
||
| from ._st2110_source import ST2110SourceOp |
There was a problem hiding this comment.
Missing pybind11 SDK compatibility error handling. Wrap import in try/except to catch ABI mismatches and provide helpful diagnostics:
| import holoscan.core # noqa: F401 - required: registers C++ GXF types before _st2110_source is loaded | |
| from ._st2110_source import ST2110SourceOp | |
| try: | |
| from ._st2110_source import ST2110SourceOp | |
| except ImportError as e: | |
| pybind11_hsdk_err = 'unknown base type "holoscan::' | |
| if pybind11_hsdk_err not in str(e): | |
| raise e | |
| import sys | |
| sys.stderr.write( | |
| "\nError importing ST2110SourceOp: pybind11 binding incompatible with installed Holoscan SDK version.\n" | |
| "This typically indicates an ABI mismatch between the SDK used to build the operator and the installed SDK.\n" | |
| "Rebuild the operator against the installed Holoscan SDK.\n\n" | |
| ) | |
| raise e |
Context Used: Rule from dashboard - When generating Python __init__.py files for pybind11 bindings that handle Holoscan SDK compatibil... (source)
Adds st2110_source operator and st2110_demo application for receiving SMPTE ST 2110-20 uncompressed video over multicast UDP using standard Linux sockets with CUDA pinned memory and GPU-accelerated color conversion. Features: - Triple-buffer architecture with recvmmsg() batch receive - CUDA kernels for YCbCr-4:2:2-10bit to RGBA/NV12 conversion - Python bindings via pybind11 - Multiple output modes (raw, RGBA, NV12) - Holohub CLI integration with run modes Based on PR nvidia-holoscan#1346 by @Ollie-cj and @xrlabs-cj with review feedback from @bhashemian addressed: - Removed separate shell scripts in favor of holohub CLI modes - Added in-class member initializers for FrameBuffer struct - Fixed CMAKE_PREFIX_PATH in BUILD.md - Removed holoviz from operator dependencies (SDK built-in) - Consistent multicast address across config files - Clean rebase on upstream/main (no unrelated file changes)
Adds st2110_source operator and st2110_demo application for receiving SMPTE ST 2110-20 uncompressed video over multicast UDP using standard Linux sockets with CUDA pinned memory and GPU-accelerated color conversion. Features: - Triple-buffer architecture with recvmmsg() batch receive - CUDA kernels for YCbCr-4:2:2-10bit to RGBA/NV12 conversion - Python bindings via pybind11 - Multiple output modes (raw, RGBA, NV12) - Holohub CLI integration with run modes Based on PR nvidia-holoscan#1346 by @Ollie-cj and @xrlabs-cj with review feedback from @bhashemian addressed: - Removed separate shell scripts in favor of holohub CLI modes - Added in-class member initializers for FrameBuffer struct - Fixed CMAKE_PREFIX_PATH in BUILD.md - Removed holoviz from operator dependencies (SDK built-in) - Consistent multicast address across config files - Clean rebase on upstream/main (no unrelated file changes)
|
Hi @Ollie-cj, I've created a clean PR based on your work here: #1447 It rebases your st2110_source operator and st2110_demo application onto current |
Adds st2110_source operator and st2110_demo application for receiving SMPTE ST 2110-20 uncompressed video over multicast UDP using standard Linux sockets with CUDA pinned memory and GPU-accelerated color conversion. Features: - Triple-buffer architecture with recvmmsg() batch receive - CUDA kernels for YCbCr-4:2:2-10bit to RGBA/NV12 conversion - Python bindings via pybind11 - Multiple output modes (raw, RGBA, NV12) - Holohub CLI integration with run modes Based on PR nvidia-holoscan#1346 by @Ollie-cj and @xrlabs-cj with review feedback from @bhashemian addressed: - Removed separate shell scripts in favor of holohub CLI modes - Added in-class member initializers for FrameBuffer struct - Fixed CMAKE_PREFIX_PATH in BUILD.md - Removed holoviz from operator dependencies (SDK built-in) - Consistent multicast address across config files - Clean rebase on upstream/main (no unrelated file changes) Signed-off-by: Mikael Brudfors <mbrudfors@nvidia.com>
Thank you @brudfors , I'll keep an eye on the new PR. Much appreciated |
|
closing in favor of #1447 |
## Summary Adds `st2110_source` operator and `st2110_demo` application for receiving SMPTE ST 2110-20 uncompressed video over multicast UDP. - **st2110_source operator**: Receives ST 2110-20 RTP packets via Linux sockets, reassembles frames using triple-buffer with CUDA pinned memory, and provides GPU-accelerated YCbCr-4:2:2 to RGBA/NV12 conversion - **st2110_demo application**: Python demo with Holoviz visualization, multiple run modes (default/rgba/nv12) via holohub CLI - **Python bindings** via pybind11 This is a clean rebase of PR #1346 by @Ollie-cj and @xrlabs-cj onto current `main`, with review feedback from @bhashemian addressed: - Renamed `st2110_test` to `st2110_demo` throughout - Replaced separate shell scripts with holohub CLI run modes in `metadata.json` - Simplified documentation to use `./holohub run st2110_demo` - Removed DPDK references from documentation (code uses Linux sockets) - Added in-class member initializers for `FrameBuffer` struct - Fixed `CMAKE_PREFIX_PATH` in BUILD.md - Removed `holoviz` from operator dependencies (SDK built-in) - Fixed operator metadata first tag to approved category - Added GStreamer-based software testing instructions to README - Consistent multicast address across config files ## Test plan - [x] Builds in Holoscan v3.11.0 container (`nvcr.io/nvidia/clara-holoscan/holoscan:v3.11.0-cuda12-dgpu`) on aarch64 (IGX Orin) - [x] Operator initializes correctly (stream format, socket creation, CUDA pinned memory allocation) - [x] Multicast UDP reception verified (packets received from GStreamer sender over ConnectX-7) - [x] RTP/ST2110 header parsing verified (line numbers, offsets extracted correctly) - [x] Python bindings import successfully - [x] `git diff upstream/main --stat` shows only st2110 files (no 630-file noise from original PR) - [ ] End-to-end frame display with professional ST 2110 source (requires Blackmagic or similar hardware) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * ST2110 Source operator for multicast ST 2110-20 ingest with raw, RGBA, and NV12 outputs and Python bindings. * New st2110_demo application with configurable run modes (default/rgba/nv12) and visualizer options. * **Documentation** * Added comprehensive READMEs, build guide, metadata, and multiple sample YAML configs (quick start, network/video parameters, troubleshooting). <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Mikael Brudfors <mbrudfors@nvidia.com> Signed-off-by: B Hashemian <3968947+bhashemian@users.noreply.github.com> Co-authored-by: Bruce Hashemian <3968947+bhashemian@users.noreply.github.com>
…ia-holoscan#1447) ## Summary Adds `st2110_source` operator and `st2110_demo` application for receiving SMPTE ST 2110-20 uncompressed video over multicast UDP. - **st2110_source operator**: Receives ST 2110-20 RTP packets via Linux sockets, reassembles frames using triple-buffer with CUDA pinned memory, and provides GPU-accelerated YCbCr-4:2:2 to RGBA/NV12 conversion - **st2110_demo application**: Python demo with Holoviz visualization, multiple run modes (default/rgba/nv12) via holohub CLI - **Python bindings** via pybind11 This is a clean rebase of PR nvidia-holoscan#1346 by @Ollie-cj and @xrlabs-cj onto current `main`, with review feedback from @bhashemian addressed: - Renamed `st2110_test` to `st2110_demo` throughout - Replaced separate shell scripts with holohub CLI run modes in `metadata.json` - Simplified documentation to use `./holohub run st2110_demo` - Removed DPDK references from documentation (code uses Linux sockets) - Added in-class member initializers for `FrameBuffer` struct - Fixed `CMAKE_PREFIX_PATH` in BUILD.md - Removed `holoviz` from operator dependencies (SDK built-in) - Fixed operator metadata first tag to approved category - Added GStreamer-based software testing instructions to README - Consistent multicast address across config files ## Test plan - [x] Builds in Holoscan v3.11.0 container (`nvcr.io/nvidia/clara-holoscan/holoscan:v3.11.0-cuda12-dgpu`) on aarch64 (IGX Orin) - [x] Operator initializes correctly (stream format, socket creation, CUDA pinned memory allocation) - [x] Multicast UDP reception verified (packets received from GStreamer sender over ConnectX-7) - [x] RTP/ST2110 header parsing verified (line numbers, offsets extracted correctly) - [x] Python bindings import successfully - [x] `git diff upstream/main --stat` shows only st2110 files (no 630-file noise from original PR) - [ ] End-to-end frame display with professional ST 2110 source (requires Blackmagic or similar hardware) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * ST2110 Source operator for multicast ST 2110-20 ingest with raw, RGBA, and NV12 outputs and Python bindings. * New st2110_demo application with configurable run modes (default/rgba/nv12) and visualizer options. * **Documentation** * Added comprehensive READMEs, build guide, metadata, and multiple sample YAML configs (quick start, network/video parameters, troubleshooting). <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Mikael Brudfors <mbrudfors@nvidia.com> Signed-off-by: B Hashemian <3968947+bhashemian@users.noreply.github.com> Co-authored-by: Bruce Hashemian <3968947+bhashemian@users.noreply.github.com>
3 participants
Adds a new operator and test application for receiving SMPTE ST 2110-20 uncompressed video streams over IP networks.
Overview
The
st2110_sourceoperator receives broadcast-standard uncompressed video via multicast UDP, targeting professional media and medical imaging workflows.Features
recvmmsg()batch processing)Platform Support
Files Added
operators/st2110_source/- Operator implementationapplications/st2110_test/- Test application with example configsTesting
./holohub build st2110_sourceNotes
Uses standard Linux sockets rather than DPDK for portability across the Jetson family, where unified memory architecture reduces the benefit of GPUDirect.
Summary by CodeRabbit
Release Notes
New Features
Documentation