Enhance Rivermax Manager RX and TX to support multiple streams per thread#1131
Open
danal97 wants to merge 17 commits intonvidia-holoscan:mainfrom
Open
Enhance Rivermax Manager RX and TX to support multiple streams per thread#1131danal97 wants to merge 17 commits intonvidia-holoscan:mainfrom
danal97 wants to merge 17 commits intonvidia-holoscan:mainfrom
Conversation
danal97
force-pushed
the
october
branch
5 times, most recently
from
f580e1c to
df42744
Compare
danal97
force-pushed
the
october
branch
4 times, most recently
from
5db2626 to
3c8f9f3
Compare
Member
|
@danal97 could you please resolve the conflicts and update this PR with the latest changes in the main branch? Thanks |
danal97
force-pushed
the
october
branch
2 times, most recently
from
f89947a to
698cb39
Compare
Contributor
|
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:
WalkthroughAdds two Holohub applications (advanced networking media player and sender), new advanced_network_media RX/TX operators, extensive Rivermax manager refactors for per-thread/per-stream RX-TX handling and adaptive burst-dropping, many CMake/build updates, new ANO Rivermax apps, YAML config schema changes, and comprehensive docs/examples. Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~150 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
operators/advanced_network/README.md (1)
325-362: Consolidate duplicate "Transmit Configuration (tx)" sections.Lines 325 and 343 both have the heading "Transmit Configuration (tx)", violating the MD024 markdown rule (no duplicate headings). Additionally, lines 327-342 and 345-362 contain substantial overlap describing the same queue configuration options.
The structure indicates that the older TX section (lines 325-342) was not removed when the new, more detailed extended Rivermax TX section (starting at line 343) was added. Remove the first, incomplete section entirely and keep only the comprehensive section starting at line 343.
Apply this diff to consolidate:
-##### Transmit Configuration (tx) - -- **`queues`**: List of queues on NIC - type: `list` - full path: `cfg\interfaces\rx\queues` - - **`name`**: Name of queue - - type: `string` - - **`id`**: Integer ID used for flow connection or lookup in operator compute method - - type: `integer` - - **`cpu_core`**: CPU core ID. Should be isolated when CPU polls the NIC for best performance.. <mark>Not in use for Doca GPUNetIO</mark> - Rivermax manager can accept coma separated list of CPU IDs - - type: `string` - - **`batch_size`**: Number of packets in a batch passed from the NIC to the downstream operator. A - larger number increases throughput but reduces end-to-end latency, as it takes longer to populate a single - buffer. A smaller number reduces end-to-end latency but can also reduce throughput. - - type: `integer` - - **`memory_regions`**: List of memory regions where buffers are stored. memory regions names are configured in the [Memory Regions](#memory-regions) section - type: `list` - -##### Transmit Configuration (tx) - **`queues`**: List of queues on NIC **Type**: `list`
🟠 Major comments (17)
operators/advanced_network/advanced_network/managers/rivermax/README.md-1-517 (1)
1-517: Add usage instructions, explicit requirements section, and configuration examples.This comprehensive documentation covers architecture and operational flows well, but is missing several essential sections per coding guidelines for operator README files. The document references YAML configuration and describes initialization processes but doesn't demonstrate practical usage.
Missing Sections:
Usage Instructions (critical gap): How does a developer integrate and use the Rivermax Manager? Add a "Usage" or "Getting Started" section with:
- Basic initialization steps
- How to select between RX/TX services
- Integration with applications
Requirements (critical gap): While hardware dependencies are scattered in lines 220-230, create an explicit "Requirements" section covering:
- Hardware requirements (ConnectX-6+, MOFED driver versions)
- Software dependencies (Rivermax SDK version, C++ standard requirements)
- Memory requirements (huge pages configuration, GPU memory if applicable)
Configuration Examples (important gap): Lines 241 and 289 reference "YAML configuration" but never show an example. Add:
- Sample RX configuration (IPO/RTP receiver setup)
- Sample TX configuration (MediaSender vs MediaSenderZeroCopy)
- Memory region configuration
- Thread/core affinity settings (referenced in RDK AppSetting extension)
Recommended additions:
## Quick Start ### Basic RX Configuration Example \`\`\`yaml [example RX YAML configuration] \`\`\` ### Basic TX Configuration Example \`\`\`yaml [example TX YAML configuration] \`\`\` ## Requirements ### Hardware - ConnectX-6 or later NIC - MOFED driver version X.X+ - CPU with isolated cores for pinning ### Software - Rivermax SDK version [version] - C++17 or later - NVIDIA CUDA [version if GPU support needed] ### Memory Configuration - Huge pages enabled: [how many/setup instructions] - GPU memory [if applicable]applications/adv_networking_bench/cpp/testing/nvidia_nic_utils.py-36-40 (1)
36-40: Makeip_addresstype optional to match runtime values
ip_addressis annotated asstrbut is later set toNonewhen IP discovery fails, which can confuse type checkers and callers expecting a string. Consider updating the type to be explicitly optional and documenting the semantics:-from typing import List +from typing import List, Optional @@ - ip_address: str # IP address + ip_address: Optional[str] # IP address (None if no IPv4 address found)The rest of the
__str__update looks good and will handleNoneby printingip=None.Committable suggestion skipped: line range outside the PR's diff.
applications/adv_networking_media_player/cpp/metadata.json-16-16 (1)
16-16: Correct the category tag.The first tag in the
tagsarray serves as the category and must match one of the approved categories. "Network" is not an approved category. Based on the application's purpose, use "Networking and Distributed Computing" as the first tag.As per coding guidelines, the approved categories are: 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.
- "tags": ["Network", "Networking", "UDP", "Ethernet", "IP", "GPUDirect", "Rivermax"], + "tags": ["Networking and Distributed Computing", "UDP", "Ethernet", "IP", "GPUDirect", "Rivermax"],applications/adv_networking_media_sender/cpp/metadata.json-16-16 (1)
16-16: Correct the category tag.The first tag in the
tagsarray serves as the category and must match one of the approved categories. "Network" is not an approved category. Based on the application's purpose, use "Networking and Distributed Computing" as the first tag.As per coding guidelines, the approved categories are: 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.
- "tags": ["Network", "Networking", "UDP", "Ethernet", "IP", "GPUDirect", "Rivermax", "Media", "Media Sender"], + "tags": ["Networking and Distributed Computing", "UDP", "Ethernet", "IP", "GPUDirect", "Rivermax", "Media", "Media Sender"],operators/advanced_network/advanced_network/managers/rivermax/applications/ano_rtp_receiver.h-30-33 (1)
30-33: Remove using-namespace directives from header file.Placing
using namespacedirectives in a header file pollutes the namespace for all translation units that include this header, potentially causing naming conflicts and violating C++ best practices.Use fully qualified names or namespace aliases instead:
-using namespace rivermax::dev_kit::apps; -using namespace rivermax::dev_kit::io_node; -using namespace rivermax::dev_kit::services; -using namespace rivermax::dev_kit::core; +namespace rdk_apps = rivermax::dev_kit::apps; +namespace rdk_io = rivermax::dev_kit::io_node; +namespace rdk_services = rivermax::dev_kit::services; +namespace rdk_core = rivermax::dev_kit::core;Then use these aliases or fully qualified names throughout the file.
operators/advanced_network/advanced_network/managers/rivermax/rivermax_service/CMakeLists.txt-73-82 (1)
73-82: Bug: Second patch uses potentially stale variableNEW_CONTENTS.The second
ifblock on line 79 checksCONTENTSbut operates onNEW_CONTENTS, which is only defined if the firstifblock executed. If only the second patch is needed (first condition false, second true),NEW_CONTENTSwill be undefined, causing incorrect behavior.Apply this diff to fix the sequential patching logic:
# Will be removed once RDK makes the change file(READ "${rivermax-dev-kit_SOURCE_DIR}/source/apps/include/rdk/apps/rmax_base_app.h" CONTENTS) +set(PATCHED_CONTENTS "${CONTENTS}") if(NOT CONTENTS MATCHES "virtual ReturnStatus find_internal_stream_index\\(") - string(REPLACE "ReturnStatus find_internal_stream_index(" "virtual ReturnStatus find_internal_stream_index(" NEW_CONTENTS "${CONTENTS}") - file(WRITE "${rivermax-dev-kit_SOURCE_DIR}/source/apps/include/rdk/apps/rmax_base_app.h" "${NEW_CONTENTS}") + string(REPLACE "ReturnStatus find_internal_stream_index(" "virtual ReturnStatus find_internal_stream_index(" PATCHED_CONTENTS "${PATCHED_CONTENTS}") endif() -if (NOT CONTENTS MATCHES "virtual ReturnStatus get_app_settings\\(") - string(REPLACE "ReturnStatus get_app_settings(" "virtual ReturnStatus get_app_settings(" NEW_CONTENTS2 "${NEW_CONTENTS}") - file(WRITE "${rivermax-dev-kit_SOURCE_DIR}/source/apps/include/rdk/apps/rmax_base_app.h" "${NEW_CONTENTS2}") +if(NOT CONTENTS MATCHES "virtual ReturnStatus get_app_settings\\(") + string(REPLACE "ReturnStatus get_app_settings(" "virtual ReturnStatus get_app_settings(" PATCHED_CONTENTS "${PATCHED_CONTENTS}") endif() +if(NOT "${PATCHED_CONTENTS}" STREQUAL "${CONTENTS}") + file(WRITE "${rivermax-dev-kit_SOURCE_DIR}/source/apps/include/rdk/apps/rmax_base_app.h" "${PATCHED_CONTENTS}") +endif()operators/advanced_network/python/adv_network_common_pybind.cpp-225-235 (1)
225-235: Verifyformat_eth_addrvalidates and handles invalid MAC address formats.The wrapper calls
format_eth_addrwithout error checking. Ensure this utility validates the input string format (e.g., rejects"invalid", truncated strings, invalid hex characters) and does not cause undefined behavior. Standard validation should include format checking (XX:XX:XX:XX:XX:XX pattern), hex digit validation, and optionally semantic checks (all-zero, broadcast, multicast rejection). The wrapper should also handle and propagate any errors fromformat_eth_addrrather than silently passing invalid results toset_eth_header.applications/adv_networking_media_sender/cpp/adv_networking_media_sender.cpp-17-17 (1)
17-17: Remove unused include.The
assert.hheader is included but never used in this file. Noassert()calls are present.Apply this diff:
-#include <assert.h> #include <sys/time.h>operators/advanced_network/advanced_network/manager.cpp-319-321 (1)
319-321: Add documentation for the new stream-specific overload.The new
get_rx_burstoverload accepting a stream parameter is a key addition for multi-stream support, but it lacks documentation. Please add a comment block explaining when this overload should be used and when subclasses should override it.Apply this diff:
+/** + * @brief Get RX burst for a specific port, queue, and stream + * This method enables stream-specific burst retrieval for managers that support + * multiple streams per queue (e.g., Rivermax). Base implementation returns NOT_SUPPORTED. + * + * @param burst Pointer to receive burst parameters + * @param port Port ID + * @param q Queue ID + * @param stream Stream ID within the queue + * @return Status::NOT_SUPPORTED in base implementation, subclasses may override + */ Status Manager::get_rx_burst(BurstParams** burst, int port, int q, int stream) { return Status::NOT_SUPPORTED; }applications/adv_networking_media_player/cpp/CMakeLists.txt-26-37 (1)
26-37: Useadd_holohub_application()instead ofadd_executable()for proper Holohub build system integration.Applications in this repository must use
add_holohub_application()for build system integration rather than raw CMake commands. Replaceadd_executable(${PROJECT_NAME} ...)with the appropriateadd_holohub_application()call to ensure consistent handling of dependencies, installation, and testing across the Holohub framework.operators/advanced_network_media/advanced_network_media_rx/burst_processor.cpp-34-57 (1)
34-57: Static variables prevent correct multi-instance behavior.The
staticvariables (burst_info_initialized,last_header_stride, etc.) are shared across allBurstProcessorinstances. This conflicts with the PR's multi-stream objectives—if multiple streams use separateBurstProcessorinstances, they will incorrectly share configuration state.Consider moving these to instance members:
+// In burst_processor.h class BurstProcessor: +private: + bool burst_info_initialized_ = false; + size_t last_header_stride_ = 0; + size_t last_payload_stride_ = 0; + bool last_hds_on_ = false; + bool last_payload_on_cpu_ = false; + bool strategy_logged_ = false; // In burst_processor.cpp: void BurstProcessor::ensure_converter_configuration(BurstParams* burst) { - static bool burst_info_initialized = false; - static size_t last_header_stride = 0; - static size_t last_payload_stride = 0; - static bool last_hds_on = false; - static bool last_payload_on_cpu = false; + // Use instance members instead of static variablesCommittable suggestion skipped: line range outside the PR's diff.
applications/adv_networking_media_sender/cpp/CMakeLists.txt-52-60 (1)
52-60: Security concern:WORLD_WRITEpermission on installed files.Setting
WORLD_WRITEpermission on configuration files is overly permissive and could allow any user to modify application configuration. UseWORLD_READinstead.install( FILES ../adv_networking_media_sender.yaml DESTINATION examples/${PROJECT_NAME} COMPONENT ${PROJECT_NAME}-configs PERMISSIONS OWNER_READ OWNER_WRITE GROUP_READ GROUP_WRITE - WORLD_READ WORLD_WRITE + WORLD_READ )Apply the same fix to the other
install()blocks at lines 62-70 and 72-80.applications/adv_networking_media_player/cpp/adv_networking_media_player.cpp-406-414 (1)
406-414: Mock receiver is added but not connected to any data flow.When neither
visualizenorwrite_to_fileis enabled, themock_receiveroperator is added usingadd_operator()but there's no data flow from the RX operators to it. The RX operators created inadv_net_media_rx_mapremain unconnected, potentially causing data to be dropped or causing pipeline issues.Consider connecting the mock receiver to the RX operators or using a different approach to consume the data:
} else { HOLOSCAN_LOG_WARN("No output type (write_to_file/visualize) defined. Data will be received but not processed."); - auto mock_receiver = make_operator<ops::MockReceiverOp>( - "mock_receiver", - Arg("interface_name", interface_name), - make_condition<BooleanCondition>("is_alive", true) - ); - add_operator(mock_receiver); + for (const auto& [stream_name, rx_op] : adv_net_media_rx_map) { + auto mock_receiver = make_operator<ops::MockReceiverOp>( + "mock_receiver_" + stream_name, + Arg("interface_name", interface_name), + make_condition<BooleanCondition>("is_alive", true) + ); + add_flow(rx_op, mock_receiver); + } }operators/advanced_network/advanced_network/managers/rivermax/rivermax_mgr_impl/adv_network_rivermax_mgr.cpp-224-231 (1)
224-231: Memory leak: burst_tx_pool arrays are never freed.The destructor is empty, but
initialize()allocates arrays for each element inburst_tx_pool(lines 228-230). These allocations are never released.Add cleanup in the destructor:
RivermaxMgr::RivermaxMgrImpl::~RivermaxMgrImpl() { + for (int i = 0; i < MAX_TX_BURST; i++) { + delete[] burst_tx_pool[i].pkts[0]; + delete[] burst_tx_pool[i].pkt_lens[0]; + delete[] burst_tx_pool[i].pkt_extra_info; + } }Also applies to: 404-404
operators/advanced_network/advanced_network/managers/rivermax/rivermax_mgr_impl/burst_manager.h-33-33 (1)
33-33: Avoidusing namespaceat namespace scope in headers.Same issue as in
ano_media_sender.h- this pollutes the namespace for all includers.-using namespace rivermax::dev_kit::services; +// Prefer explicit namespace qualification or local using declarationsCommittable suggestion skipped: line range outside the PR's diff.
operators/advanced_network/advanced_network/managers/rivermax/applications/ano_media_sender.h-33-36 (1)
33-36: Avoidusing namespacedirectives at namespace scope in headers.These directives pollute the namespace for all translation units that include this header, potentially causing name collisions and ambiguity for downstream code.
Consider using explicit namespace prefixes or importing only specific symbols within function/method scope:
-using namespace rivermax::dev_kit::apps; -using namespace rivermax::dev_kit::io_node; -using namespace rivermax::dev_kit::services; -using namespace rivermax::dev_kit::core; +// Use explicit prefixes like rivermax::dev_kit::apps::AppSettings +// or import within function scope onlyCommittable suggestion skipped: line range outside the PR's diff.
operators/advanced_network/advanced_network/managers/rivermax/rivermax_mgr_impl/rivermax_mgr_service.cpp-422-436 (1)
422-436: Status check occurs after the settings pointer is already used.The
dynamic_castat line 425-426 usessettingswhich could be invalid ifget_app_settingsfailed. The status check at line 433 happens too late.bool MediaSenderService::post_init_setup() { const AppSettings* settings = nullptr; auto status = tx_service_->get_app_settings(settings); + + if (status != ReturnStatus::success) { + HOLOSCAN_LOG_ERROR("Failed to get settings from TX service"); + return false; + } + const ANOMediaSenderSettings* media_settings = dynamic_cast<const ANOMediaSenderSettings*>(settings); if (!media_settings) { HOLOSCAN_LOG_ERROR("Failed to cast AppSettings to ANOMediaSenderSettings"); return false; } - - if (status != ReturnStatus::success) { - HOLOSCAN_LOG_ERROR("Failed to get settings from TX service"); - return false; - } size_t pool_buffer_size = settings->media.bytes_per_frame * MEDIA_FRAME_POOL_SIZE;
🟡 Minor comments (28)
operators/advanced_network/CHANGELOG.md-18-27 (1)
18-27: Fix unordered list indentation.The nested list items use 4-space indentation before the dash, but Markdown best practices and markdownlint (MD007) require 2-space indentation for nested lists.
Apply this diff to correct the indentation:
Rivermax Manager: - - **Framework Migration** - - Migrated from legacy rmax-ral-lib to modern rivermax-dev-kit framework (1.70.32). - - **SMPTE 2110-20 Support** - - Implemented hardware-accelerated video streaming with support for multiple memory modes (GPU, host pinned, huge pages) and video formats (YCbCr 4:2:2/4:4:4/4:2:0, RGB 8/10/12-bit). - - **Zero-Copy Performance** - - Added MediaSenderZeroCopyService that forwards received frames without owning memory pools for improved performance. - - **Enhanced Configuration** - - Added configurable packet chunking (`num_of_packets_in_chunk`) and burst flags for better flow control. - - **Code Quality** - - Standardized naming convention from 'rmax' to 'rivermax' project-wide and removed redundant documentation. + - **Framework Migration** + - Migrated from legacy rmax-ral-lib to modern rivermax-dev-kit framework (1.70.32). + - **SMPTE 2110-20 Support** + - Implemented hardware-accelerated video streaming with support for multiple memory modes (GPU, host pinned, huge pages) and video formats (YCbCr 4:2:2/4:4:4/4:2:0, RGB 8/10/12-bit). + - **Zero-Copy Performance** + - Added MediaSenderZeroCopyService that forwards received frames without owning memory pools for improved performance. + - **Enhanced Configuration** + - Added configurable packet chunking (`num_of_packets_in_chunk`) and burst flags for better flow control. + - **Code Quality** + - Standardized naming convention from 'rmax' to 'rivermax' project-wide and removed redundant documentation.operators/advanced_network/README.md-235-235 (1)
235-235: Fix hard tabs and list indentation per linter violations.Lines 235 and 261 contain hard tabs (MD010) that should be replaced with spaces. Additionally, Line 261 has incorrect list indentation (MD007: expected 2 spaces, actual 1 space).
Replace hard tabs with spaces throughout these lines and adjust list indentation to 2 spaces.
- - **`memory_registration`**: Flag, when enabled, reduces the number of memory keys in use by registering all the memory in a single pass on the application side. + - **`memory_registration`**: Flag, when enabled, reduces the number of memory keys in use by registering all the memory in a single pass on the application side.Apply similar formatting to line 261 and any other hard tabs in the document.
Also applies to: 261-261
operators/advanced_network/README.md-355-355 (1)
355-355: Fix sentence structure: provide subject for dangling phrase.Line 355 begins with a sentence fragment. The phrase "Should be isolated when CPU polls the NIC..." lacks a clear grammatical subject. Revise to form a complete sentence.
- - **`cpu_core`**: CPU core ID. Should be isolated when CPU polls the NIC for best performance. <mark>Not in use for DOCA GPUNetIO</mark>. Rivermax Manager can accept comma-separated list of CPU IDs. + - **`cpu_core`**: CPU core ID on which the NIC should poll for best performance. <mark>Not in use for DOCA GPUNetIO</mark>. Rivermax Manager can accept comma-separated list of CPU IDs.operators/advanced_network/advanced_network/managers/rivermax/rivermax_mgr_impl/rivermax_chunk_consumer_ano.h-94-98 (1)
94-98: Silent packet discarding may cause data loss.When
chunk_size > max_burst_size_, the code logs a warning but silently discards the excess packets. This could lead to data loss that's difficult to debug. Consider either:
- Returning an error status to signal the overflow condition
- Processing packets in multiple batches
- Adding metrics/counters to track discarded packets
operators/advanced_network/advanced_network/managers/rivermax/rivermax_mgr_impl/rivermax_queue_configs.cpp-321-326 (1)
321-326: Inconsistent stream ID validation between IPO and RTP receivers.IPO receiver (lines 322-325) has stream ID sequential check commented out, while RTP receiver (lines 386-390) enforces it. This inconsistency may cause confusion. Consider aligning the behavior or documenting the difference.
applications/adv_networking_bench/cpp/testing/benchmark_utils.py-598-603 (1)
598-603: Handle edge case wherefile_pathhas no directory component.If
file_pathis just a filename (e.g.,"output.bin"),os.path.dirname()returns an empty string, andos.makedirs("")will raiseFileNotFoundError.Apply this diff:
# Create the directory if it doesn't exist try: - os.makedirs(os.path.dirname(file_path), exist_ok=True) + dir_name = os.path.dirname(file_path) + if dir_name: + os.makedirs(dir_name, exist_ok=True) except OSError as e: - logger.error(f"Failed to create directory: {e}") + logger.exception(f"Failed to create directory: {e}") raiseapplications/adv_networking_media_player/python/metadata.json-16-16 (1)
16-16: Use approved category tag.The first tag "Network" is not in the approved categories list. Based on coding guidelines, the category must be one of the approved values. For networking-related applications, use "Networking and Distributed Computing" instead.
Apply this diff:
- "tags": ["Network", "Networking", "UDP", "Ethernet", "IP", "GPUDirect", "Rivermax"], + "tags": ["Networking and Distributed Computing", "Networking", "UDP", "Ethernet", "IP", "GPUDirect", "Rivermax"],As per coding guidelines, the category (first tag in the tags array) must match one of the approved categories.
operators/advanced_network_media/advanced_network_media_rx/metadata.json-15-15 (1)
15-15: Use approved category tag.The first tag "Network" is not in the approved categories list. For networking-related operators, use "Networking and Distributed Computing" as the first tag.
Apply this diff:
- "tags": ["Network", "Networking", "DPDK", "UDP", "Ethernet", "IP", "GPUDirect", "Rivermax"], + "tags": ["Networking and Distributed Computing", "Networking", "DPDK", "UDP", "Ethernet", "IP", "GPUDirect", "Rivermax"],As per coding guidelines, the category (first tag in the tags array) must match one of the approved categories.
applications/adv_networking_media_sender/cpp/adv_networking_media_sender.cpp-43-46 (1)
43-46: Remove duplicate log message.Lines 43 and 46 both log the manager type with nearly identical messages. Keep one and remove the other to avoid redundancy.
Apply this diff:
HOLOSCAN_LOG_INFO("Using Advanced Network manager {}", advanced_network::manager_type_to_string(mgr_type)); - HOLOSCAN_LOG_INFO("Using ANO manager {}", advanced_network::manager_type_to_string(mgr_type)); if (!tx_en) {applications/adv_networking_bench/adv_networking_bench_rivermax_tx_rx.yaml-65-86 (1)
65-86: Align RX configuration format with other Rivermax YAMLs.This file uses a legacy flat format for Rivermax RX settings (lines 73-84) with direct IP address arrays, while
adv_networking_bench_rivermax_rx.yamluses the newer hierarchicalrx_threads→network_settingsstructure. This inconsistency may confuse users and doesn't showcase the multi-stream per thread capability.Consider updating to match the hierarchical format:
rivermax_rx_settings: settings_type: "ipo_receiver" memory_registration: true #allocator_type: "huge_page_2mb" verbose: true max_path_diff_us: 10000 ext_seq_num: true sleep_between_operations_us: 0 - local_ip_addresses: - - 2.1.0.12 - - 2.1.0.12 - source_ip_addresses: - - 2.1.0.2 - - 2.1.0.2 - destination_ip_addresses: - - 224.1.1.1 - - 224.1.1.2 - destination_ports: - - 50001 - - 50001 + rx_threads: + - thread_id: 0 + network_settings: + - stream_id: 0 + local_ip_addresses: + - 2.1.0.12 + source_ip_addresses: + - 2.1.0.2 + destination_ip_addresses: + - 224.1.1.1 + destination_ports: + - 50001 + - stream_id: 1 + local_ip_addresses: + - 2.1.0.12 + source_ip_addresses: + - 2.1.0.2 + destination_ip_addresses: + - 224.1.1.2 + destination_ports: + - 50001 stats_report_interval_ms: 1000 send_packet_ext_info: trueoperators/advanced_network_media/advanced_network_media_rx/burst_processor.cpp-105-115 (1)
105-115: Staticstrategy_loggedalso affects multi-instance behavior.Similar to the configuration statics, this
static bool strategy_loggedmeans the strategy detection message will only be logged for the first stream that detects a strategy, not for each stream independently. Consider making this an instance member as well.applications/adv_networking_media_sender/adv_networking_media_sender.yaml-1-3 (1)
1-3: Update copyright year for new file.This is a new file added in the PR, but the copyright header shows 2022-2023. Consider updating to 2023-2025 to match other new files in this PR.
%YAML 1.2 -# SPDX-FileCopyrightText: Copyright (c) 2022-2023 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-FileCopyrightText: Copyright (c) 2023-2025 NVIDIA CORPORATION & AFFILIATES. All rights reserved.applications/adv_networking_media_player/cpp/CMakeLists.txt-1-2 (1)
1-2: Update copyright year for new file.The copyright shows 2022-2023 but this is a new file added in this PR. Consider updating to 2023-2025 for consistency.
-# SPDX-FileCopyrightText: Copyright (c) 2022-2023 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-FileCopyrightText: Copyright (c) 2023-2025 NVIDIA CORPORATION & AFFILIATES. All rights reserved.applications/adv_networking_media_player/python/adv_networking_media_player.py-3-4 (1)
3-4: Update copyright year for new file.Similar to the YAML file, the copyright header shows 2022-2023 but this is a new file. Consider updating to 2023-2025.
-# SPDX-FileCopyrightText: Copyright (c) 2022-2023 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-FileCopyrightText: Copyright (c) 2023-2025 NVIDIA CORPORATION & AFFILIATES. All rights reserved.operators/advanced_network_media/advanced_network_media_rx/burst_processor.cpp-1-3 (1)
1-3: Update copyright year for consistency.The copyright shows 2023 only. For consistency with other new files in this PR, consider updating to 2023-2025.
/* - * SPDX-FileCopyrightText: Copyright (c) 2023 NVIDIA CORPORATION & AFFILIATES. All rights reserved. + * SPDX-FileCopyrightText: Copyright (c) 2023-2025 NVIDIA CORPORATION & AFFILIATES. All rights reserved.operators/advanced_network_media/advanced_network_media_rx/adv_network_media_rx.cpp-395-399 (1)
395-399: Typo in parameter description: "devices" should be "device".The description says
'host' or 'devices'but should say'host' or 'device'to match the actual comparison logic at line 95.spec.param(memory_location_, "memory_location", "Memory Location for Output Frames", - "Memory location for output frames ('host' or 'devices')", + "Memory location for output frames ('host' or 'device')", std::string("device"));applications/adv_networking_media_sender/python/adv_networking_media_sender.py-3-3 (1)
3-3: Update copyright year for new file.The copyright year
2022-2023appears outdated for a new file. Consider updating to include 2025.-# SPDX-FileCopyrightText: Copyright (c) 2022-2023 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-FileCopyrightText: Copyright (c) 2022-2025 NVIDIA CORPORATION & AFFILIATES. All rights reserved.applications/adv_networking_bench/cpp/rivermax_bench_op_tx.h-568-574 (1)
568-574: Typo in video format string: "YUV442" should likely be "YUV444".Line 572 maps "YUV442" to
YCbCr_4_4_4, but the standard naming for 4:4:4 subsampling would be "YUV444". This appears to be a typo.if (format == "RGB888") return VideoSampling::RGB; if (format == "YUV422") return VideoSampling::YCbCr_4_2_2; if (format == "YUV420") return VideoSampling::YCbCr_4_2_0; - if (format == "YUV442") return VideoSampling::YCbCr_4_4_4; + if (format == "YUV444") return VideoSampling::YCbCr_4_4_4; return VideoSampling::Unknown;applications/adv_networking_media_player/cpp/adv_networking_media_player.cpp-129-129 (1)
129-129: Off-by-one error in frame recording check.The condition
frames_recorded_ > num_of_frames_to_record_.get()allows recordingnum_of_frames_to_record + 1frames before stopping. Should use>=for exact count.- if (frames_recorded_ > num_of_frames_to_record_.get()) { return; } + if (frames_recorded_ >= num_of_frames_to_record_.get()) { return; }operators/advanced_network/advanced_network/managers/rivermax/rivermax_mgr_impl/burst_manager.cpp-400-402 (1)
400-402: Missing braces around multi-statement conditional.Line 400-401 appears to be a conditional check, but line 401 is executed unconditionally due to missing braces. If
burst_out_size_validation is intended to be conditional, add braces.- if (burst_out_size_ > RivermaxBurst::MAX_PKT_IN_BURST || burst_out_size_ == 0) - burst_out_size_ = RivermaxBurst::MAX_PKT_IN_BURST; + if (burst_out_size_ > RivermaxBurst::MAX_PKT_IN_BURST || burst_out_size_ == 0) { + burst_out_size_ = RivermaxBurst::MAX_PKT_IN_BURST; + }applications/adv_networking_bench/cpp/rivermax_bench_op_tx.h-387-392 (1)
387-392: Infinite retry loop whenadd_item_blockingfails.If
add_item_blockingreturns a non-SUCCESS status other than queue being full (e.g., if stopped), the inner while loop will spin indefinitely with only a sleep, never breaking out. Theadd_item_blockingalready handles waiting for space, so the retry loop may be unnecessary.Consider simplifying to handle the stop case:
- while (!stop_) { - auto status = frame_buffer_queue_.add_item_blocking(frame_buffer); - - if (status == Status::SUCCESS) { break; } - std::this_thread::sleep_for(std::chrono::microseconds(sleep_duration_microseconds_)); - } + auto status = frame_buffer_queue_.add_item_blocking(frame_buffer); + if (status != Status::SUCCESS) { + HOLOSCAN_LOG_DEBUG("Failed to add frame to queue: {}", static_cast<int>(status)); + }applications/adv_networking_bench/cpp/rivermax_bench_op_tx.h-520-525 (1)
520-525: Missing CUDA error checking oncudaMemcpy.The
cudaMemcpycall on line 522 doesn't check for errors. If the copy fails, the corrupt or incomplete data will be sent.Consider using the CUDA_TRY pattern or checking the return value:
// Copy frame data into the burst if (payload_memory_.get() == "device") { - cudaMemcpy(cur_msg_->pkts[0][0], frame->get(), frame->get_size(), cudaMemcpyDefault); + cudaError_t err = cudaMemcpy(cur_msg_->pkts[0][0], frame->get(), frame->get_size(), cudaMemcpyDefault); + if (err != cudaSuccess) { + HOLOSCAN_LOG_ERROR("cudaMemcpy failed: {}", cudaGetErrorString(err)); + free_tx_burst(cur_msg_); + cur_msg_ = nullptr; + return; + } } else { std::memcpy(cur_msg_->pkts[0][0], frame->get(), frame->get_size()); }operators/advanced_network/advanced_network/managers/rivermax/applications/ano_media_sender.h-74-75 (1)
74-75: Incorrect comment: "recv flows" should be "send flows" or similar.The comment says "Network recv flows" but this is the sender application, and the member stores thread/stream configuration for sending.
- /* Network recv flows */ + /* Per-thread stream configurations */ std::vector<std::vector<TwoTupleFlow>> m_threads_streams;operators/advanced_network/advanced_network/managers/rivermax/rivermax_mgr_impl/rivermax_mgr_service.cpp-352-358 (1)
352-358:newoperator throws on failure; null check is ineffective.The
new byte_t[pool_buffer_size]expression will throwstd::bad_allocon failure rather than returningnullptr. The null check will never trigger.Consider using
new (std::nothrow)if you want to handle allocation failure gracefully:} else { - pool_buffer = new byte_t[pool_buffer_size]; - if (pool_buffer == nullptr) { - HOLOSCAN_LOG_ERROR("Failed to allocate host memory"); - return nullptr; - } + pool_buffer = new (std::nothrow) byte_t[pool_buffer_size]; + if (pool_buffer == nullptr) { + HOLOSCAN_LOG_ERROR("Failed to allocate host memory"); + return nullptr; + } HOLOSCAN_LOG_INFO("Allocated host memory for frames memory pool at address: {}", pool_buffer); }operators/advanced_network/advanced_network/managers/rivermax/rivermax_mgr_impl/rivermax_config_manager.cpp-359-360 (1)
359-360: Copy-paste error: Log message says "RX" but this is a TX function.The log message incorrectly refers to "Rivermax RX ANO settings" in the TX configuration path.
Apply this fix:
HOLOSCAN_LOG_INFO( - "Rivermax RX ANO settings for {} ({}) on port {}", q.common_.name_, queue_id, port_id); + "Rivermax TX ANO settings for {} ({}) on port {}", q.common_.name_, queue_id, port_id);operators/advanced_network/advanced_network/managers/rivermax/rivermax_mgr_impl/rivermax_config_manager.cpp-857-883 (1)
857-883: Unreachable code after try block.The
return true;at line 882 is unreachable because all paths in the try block either return or fall through to the catch block which also returns.Remove the unreachable statement:
} catch (const std::out_of_range& e) { if (num_of_mrs == 1) HOLOSCAN_LOG_ERROR("Invalid memory region for Rivermax RX queue: {}", queue_mr_names[0]); else HOLOSCAN_LOG_ERROR("Invalid memory region for Rivermax RX queue: {} or {}", queue_mr_names[0], queue_mr_names[1]); return false; } - - return true; }operators/advanced_network/advanced_network/managers/rivermax/rivermax_mgr_impl/rivermax_mgr_service.h-85-90 (1)
85-90: Uninitialized protected membersport_id_andqueue_id_.These members are declared but not initialized in the base class constructor. If derived classes access them before explicitly setting values, this could lead to undefined behavior.
Consider adding default initialization:
protected: uint32_t service_id_; ///< Unique identifier for the service - uint16_t port_id_; ///< Port ID derived from the service ID - uint16_t queue_id_; ///< Queue ID derived from the service ID + uint16_t port_id_ = 0; ///< Port ID derived from the service ID + uint16_t queue_id_ = 0; ///< Queue ID derived from the service ID bool initialized_ = false; ///< Flag indicating whether the service is initialized };operators/advanced_network/advanced_network/managers/rivermax/rivermax_mgr_impl/rivermax_config_manager.cpp-779-801 (1)
779-801: Dead variableto_reset_cores_vectorinvalidate_cores.This variable is declared but never used, likely copied from
parse_and_set_coreswhere it serves a purpose.Remove the unused variable:
int ConfigManagerUtilities::validate_cores(const std::string& cores) { std::istringstream iss(cores); std::string coreStr; - bool to_reset_cores_vector = true; int num_cores = 0;
Comment on lines
+106
to
+110
| add_custom_target(adv_networking_bench_rivermax_tx_rx_yaml | ||
| COMMAND ${CMAKE_COMMAND} -E copy "${CMAKE_CURRENT_SOURCE_DIR}/../adv_networking_bench_rivermax_tx_rx.yaml" ${CMAKE_CURRENT_BINARY_DIR} | ||
| DEPENDS "${CMAKE_CURRENT_SOURCE_DIR}/../adv_networking_bench_rivermax_rx.yaml" | ||
| ) | ||
| add_dependencies(adv_networking_bench adv_networking_bench_rivermax_tx_rx_yaml) |
Contributor
There was a problem hiding this comment.
Fix incorrect dependency for rivermax_tx_rx target.
Line 108 has the wrong dependency file. The target copies adv_networking_bench_rivermax_tx_rx.yaml but depends on adv_networking_bench_rivermax_rx.yaml, which will cause incorrect rebuild behavior.
Apply this diff:
add_custom_target(adv_networking_bench_rivermax_tx_rx_yaml
COMMAND ${CMAKE_COMMAND} -E copy "${CMAKE_CURRENT_SOURCE_DIR}/../adv_networking_bench_rivermax_tx_rx.yaml" ${CMAKE_CURRENT_BINARY_DIR}
- DEPENDS "${CMAKE_CURRENT_SOURCE_DIR}/../adv_networking_bench_rivermax_rx.yaml"
+ DEPENDS "${CMAKE_CURRENT_SOURCE_DIR}/../adv_networking_bench_rivermax_tx_rx.yaml"
)🤖 Prompt for AI Agents
In applications/adv_networking_bench/cpp/CMakeLists.txt around lines 106 to 110,
the custom target adv_networking_bench_rivermax_tx_rx_yaml copies
adv_networking_bench_rivermax_tx_rx.yaml but incorrectly lists
adv_networking_bench_rivermax_rx.yaml as a DEPENDS; update the DEPENDS to
reference the correct source file (adv_networking_bench_rivermax_tx_rx.yaml with
the same ../ path) so the target depends on the actual file being copied and
rebuilds correctly.
Comment on lines
+208
to
+219
| void ANOMediaSenderApp::distribute_work_for_threads() | ||
| { | ||
| m_streams_per_thread.reserve(m_media_sender_settings->thread_settings.size()); | ||
| m_media_sender_settings->num_of_total_streams = 0; | ||
| m_media_sender_settings->num_of_threads = m_media_sender_settings->thread_settings.size(); | ||
| int thread_idx = 0; | ||
| for (const auto& thread : m_media_sender_settings->thread_settings) { | ||
| m_streams_per_thread[thread_idx] = thread.stream_network_settings.size(); | ||
| m_media_sender_settings->num_of_total_streams += thread.stream_network_settings.size(); | ||
| thread_idx++; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Potential out-of-bounds access due to reserve vs resize.
m_streams_per_thread.reserve() only allocates capacity but does not create elements. Accessing m_streams_per_thread[thread_idx] on line 215 causes undefined behavior since the vector is empty.
Apply this diff to fix:
void ANOMediaSenderApp::distribute_work_for_threads()
{
- m_streams_per_thread.reserve(m_media_sender_settings->thread_settings.size());
+ m_streams_per_thread.resize(m_media_sender_settings->thread_settings.size());
m_media_sender_settings->num_of_total_streams = 0;
m_media_sender_settings->num_of_threads = m_media_sender_settings->thread_settings.size();
int thread_idx = 0;
for (const auto& thread : m_media_sender_settings->thread_settings) {
m_streams_per_thread[thread_idx] = thread.stream_network_settings.size();
m_media_sender_settings->num_of_total_streams += thread.stream_network_settings.size();
thread_idx++;
}
}📝 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.
Suggested change
| void ANOMediaSenderApp::distribute_work_for_threads() | |
| { | |
| m_streams_per_thread.reserve(m_media_sender_settings->thread_settings.size()); | |
| m_media_sender_settings->num_of_total_streams = 0; | |
| m_media_sender_settings->num_of_threads = m_media_sender_settings->thread_settings.size(); | |
| int thread_idx = 0; | |
| for (const auto& thread : m_media_sender_settings->thread_settings) { | |
| m_streams_per_thread[thread_idx] = thread.stream_network_settings.size(); | |
| m_media_sender_settings->num_of_total_streams += thread.stream_network_settings.size(); | |
| thread_idx++; | |
| } | |
| } | |
| void ANOMediaSenderApp::distribute_work_for_threads() | |
| { | |
| m_streams_per_thread.resize(m_media_sender_settings->thread_settings.size()); | |
| m_media_sender_settings->num_of_total_streams = 0; | |
| m_media_sender_settings->num_of_threads = m_media_sender_settings->thread_settings.size(); | |
| int thread_idx = 0; | |
| for (const auto& thread : m_media_sender_settings->thread_settings) { | |
| m_streams_per_thread[thread_idx] = thread.stream_network_settings.size(); | |
| m_media_sender_settings->num_of_total_streams += thread.stream_network_settings.size(); | |
| thread_idx++; | |
| } | |
| } |
🤖 Prompt for AI Agents
In
operators/advanced_network/advanced_network/managers/rivermax/applications/ano_media_sender.cpp
around lines 208-219, m_streams_per_thread.reserve(...) is used but later
indexed with m_streams_per_thread[thread_idx], which is undefined because
reserve does not create elements; change the allocation to create elements
(e.g., call
m_streams_per_thread.resize(m_media_sender_settings->thread_settings.size())
before the loop) or alternatively replace the indexed assignment with
m_streams_per_thread.push_back(...) inside the loop so elements are valid when
written, and keep the rest of the counting logic unchanged.
Comment on lines
+325
to
+332
| } else if (media_sender_builder_->memory_pool_location_ == MemoryKind::HOST_PINNED) { | ||
| cudaError_t cuda_status = cudaHostAlloc(&pool_buffer, pool_buffer_size, cudaHostAllocDefault); | ||
| if (cuda_status != cudaSuccess) { | ||
| HOLOSCAN_LOG_ERROR("Failed to allocate pinned host memory: {}", | ||
| cudaGetErrorString(cuda_status)); | ||
| cudaFree(pool_buffer); | ||
| return nullptr; | ||
| } |
Contributor
There was a problem hiding this comment.
Critical: Wrong API used to free host-pinned memory.
When cudaHostAlloc fails, cudaFree is called on pool_buffer. However, pool_buffer was never successfully allocated (the allocation just failed), so calling cudaFree on an uninitialized/null pointer is undefined behavior. Additionally, if the intent was cleanup, cudaFreeHost should be used for memory allocated with cudaHostAlloc, not cudaFree.
} else if (media_sender_builder_->memory_pool_location_ == MemoryKind::HOST_PINNED) {
cudaError_t cuda_status = cudaHostAlloc(&pool_buffer, pool_buffer_size, cudaHostAllocDefault);
if (cuda_status != cudaSuccess) {
HOLOSCAN_LOG_ERROR("Failed to allocate pinned host memory: {}",
cudaGetErrorString(cuda_status));
- cudaFree(pool_buffer);
return nullptr;
}🤖 Prompt for AI Agents
In
operators/advanced_network/advanced_network/managers/rivermax/rivermax_mgr_impl/rivermax_mgr_service.cpp
around lines 325-332, the code calls cudaFree after cudaHostAlloc failure on
pool_buffer which is undefined and the wrong API for host-pinned memory;
initialize pool_buffer to nullptr before allocation, remove the
cudaFree/cudaFreeHost call in the immediate failure branch (just log the error
and return nullptr), and if you need to free previously allocated pinned memory
in other error paths use cudaFreeHost (not cudaFree) to release host-pinned
allocations.
bhashemian
added
the
Action Required by Author
Member
|
@danal97 could you please let me know what is the status of this PR? Thanks |
Contributor
Author
|
Hi @bhashemian, |
- Add AdvNetworkMediaRxOp for receiving media streams over Rivermax - Supports packet-to-frame conversion with ANO Burst processing - Configurable video formats, bit depths, and frame dimensions - Output as VideoBuffer or Tensor entities - Add AdvNetworkMediaTxOp for transmitting media streams over Rivermax - Processes VideoBuffer/Tensor inputs for network transmission - Configurable network interface and queue management - Support for various video formats Features: - SMPTE 2110 compliance for professional media over IP - GPU acceleration and GPUDirect support - Low latency optimizations - Python bindings for both operators These operators build upon the Advanced Network library to provide specialized functionality for broadcast and media streaming use cases requiring strict timing and high throughput performance. Signed-off-by: Rony Rado <rrado@nvidia.com>
- Add high-performance media streaming application built on Advanced Network Media Tx operator - Utilizes AdvNetworkMediaTxOp for media transmission over Rivermax - Integrates with Advanced Network Manager for optimized network resource management - SMPTE 2110 compliant for professional broadcast applications - Real-time transmission of media files with precise timing control - Support multiple video formats and resolutions - RGB888, YUV420, NV12 and other common video formats - Configurable bit depths (8, 10, 12, 16-bit) - Multiple resolution support up to 4K and beyond - GPU acceleration with GPUDirect for zero-copy operations * Implement dual language support - C++ implementation for maximum performance - Python implementation for ease of use and development * Support professional broadcast features - Frame-accurate timing for live streaming applications - Looping playback for continuous streaming - VideoStreamReplayer integration for file-based sources Pipeline: VideoStreamReplayer → AdvNetworkMediaTxOp → Advanced Network Manager → Network Interface This application demonstrates how to use the Advanced Network Media operators for professional-grade media transmission in broadcast and media production environments requiring ultra-low latency and high throughput performance. Signed-off-by: Rony Rado <rrado@nvidia.com>
- Add high-performance media receiving application built on Advanced Network Media Rx operator - Utilizes AdvNetworkMediaRxOp for professional media reception over Rivermax - Integrates with Advanced Network Manager for optimized network resource management - SMPTE 2110 compliant for professional broadcast applications - Real-time reception and processing of media streams with ultra-low latency - Support flexible output modes and format handling - Real-time visualization using HolovizOp for live monitoring - File output capability for recording and analysis - Format conversion support for optimal display - Support multiple video formats and resolutions - RGB888, YUV420, NV12, RGBA and other common video formats - Configurable bit depths (8, 10, 12, 16-bit) - Multiple resolution support up to 4K and beyond - GPU acceleration with GPUDirect for zero-copy operations - Implement dual language support - C++ implementation for maximum performance - Python implementation for ease of use and development Pipeline: Network Interface → Advanced Network Manager → AdvNetworkMediaRxOp → [FormatConverter] → HolovizOp/FramesWriter This application demonstrates how to use the Advanced Network Media operators for professional-grade media reception in broadcast and media production environments requiring ultra-low latency and high throughput performance. Signed-off-by: Rony Rado <rrado@nvidia.com>
…configurable thresholds
- Add adaptive burst dropping system for Rivermax burst pool management
- Implements capacity-aware burst dropping to prevent memory pool exhaustion
- Configurable thresholds for warning (25%), critical (10%), and recovery (50%) levels
- Critical threshold mode drops new bursts when pool capacity falls below threshold
- Automatic recovery when pool capacity returns to healthy levels
- Add comprehensive pool capacity monitoring and statistics
- Extend YAML configuration support for burst pool adaptive dropping
- New "burst_pool_adaptive_dropping" configuration section
- Configurable enabled/disabled flag and threshold percentages
This enhancement prevents memory pool exhaustion in high-throughput ANO Rivermax
operations by intelligently dropping incoming bursts when system capacity
is critically low, ensuring system stability while maintaining optimal
performance under normal operating conditions.
Signed-off-by: Rony Rado <rrado@nvidia.com>
Lint fixes Signed-off-by: Rony Rado <rrado@nvidia.com>
- Install holoscan python SDK - Install libvulkan for holoviz operator - Install xvfb and x11vnc for headless servers Signed-off-by: Rony Rado <rrado@nvidia.com>
…kMediaTxOpImpl Signed-off-by: Dana Lessner <dlessner@nvidia.com>
Signed-off-by: Dana Lessner <dlessner@nvidia.com>
Signed-off-by: Dana Lessner <dlessner@nvidia.com>
The shared manager.cpp and manager.h depend on DPDK headers so the Rivermax build target requires DPDK to be present. - Dockerfile - change the rivermax stage to extend from dpdk instead of base-deps. - manager.cpp DPDK include guard only covered DPDK/GPUNetIO/RDMA builds. Extend it to cover ANO_MGR_RIVERMAX as well. - RivermaxMgr::get_segment_packet_length() and get_packet_length() had uint16_t return types, mismatching the uint32_t declared in the Manager base class. Signed-off-by: Dana Lessner <dlessner@nvidia.com>
Contributor
Greptile SummaryThis PR enhances the Rivermax network manager to support multiple streams per thread for both RX and TX paths, adds two new Holoscan applications ( Key changes include migrating from a single shared burst queue to a per-stream Progress from prior review rounds: Several previously flagged blocking issues have been resolved — the unresolved git merge conflict in the destructor, the malformed copy constructor in Remaining new concerns introduced by this PR:
Confidence Score: 4/5The PR has fixed the hardest compile-blocking issues from prior rounds, but several P1 runtime correctness bugs (dangling raw pointer, non-thread-safe rand(), stream_id=0 bypass) remain unresolved, and a new logic-ordering bug was introduced in post_init_setup Score of 4 reflects that blocking compile errors have been resolved and the architecture is sound, but multiple runtime correctness issues carried over from prior review rounds persist, plus a new P1 logic error in the TX post-init path rivermax_mgr_service.cpp (post_init_setup status/cast ordering), adv_network_rivermax_mgr.cpp (rand() in legacy get_rx_burst), adv_network_media_rx.cpp (stream_id=0 sentinel conflicts with YAML default), burst_manager.h (hot-path timing overhead) Important Files Changed
Sequence DiagramsequenceDiagram
participant YAML as YAML Config
participant RCM as RivermaxConfigManager
participant RMSVC as RivermaxMgrImpl
participant RXSVC as IPO/RTPReceiverService
participant BM as RxBurstsManager
participant RXQ as AnoBurstsQueue[stream_id]
YAML->>RCM: parse_configuration()
RCM->>RCM: parse thread_settings → stream_network_settings
RCM->>RMSVC: config_holder (per queue)
RMSVC->>RMSVC: create_burst_queues_for_streams()
loop per stream_id in thread_settings
RMSVC->>RXQ: make_shared<AnoBurstsQueue>()
RMSVC->>RMSVC: rx_bursts_out_queues_map_[svc_id][stream_id] = queue
end
RMSVC->>RXSVC: new ServiceType(service_id, builder, queue_map)
RXSVC->>BM: new RxBurstsManager(..., queue_map)
RXSVC->>RXSVC: initialize() — set_receive_data_consumer per stream
RMSVC->>RXSVC: apply_burst_pool_configuration()
Note over BM: Packet ingress loop
BM->>BM: submit_next_packet(stream_id, pkt_data)
BM->>BM: cur_out_burst_[stream_id].append_packet()
BM->>RXQ: enqueue_burst(cur_out_burst_[stream_id])
Note over RMSVC: Consumer path (stream-aware)
RMSVC->>RXQ: get_rx_burst(port, q, stream_id)
RXQ-->>RMSVC: shared_ptr<RivermaxBurst>
RMSVC-->>RMSVC: *burst = out_burst_shared.get()
Greploops — Automatically fix all review issues by running Reviews (8): Last reviewed commit: "Merge branch 'main' into october" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+432
to
+445
| <<<<<<< HEAD | ||
| while (rx_bursts_out_queue_->available_bursts() > 0) { | ||
| burst = rx_bursts_out_queue_->dequeue_burst(); | ||
| if (burst == nullptr) break; | ||
| if (burst == nullptr) | ||
| break; | ||
| rx_bursts_mempool_->enqueue_burst(burst); | ||
| ======= | ||
| for (auto& [stream_id, rx_bursts_out_queue] : rx_bursts_out_queue_) { | ||
| while (rx_bursts_out_queue->available_bursts() > 0) { | ||
| burst = rx_bursts_out_queue->dequeue_burst(); | ||
| if (burst == nullptr) break; | ||
| rx_bursts_mempool_->enqueue_burst(burst); | ||
| } | ||
| >>>>>>> 6bf7a063 (Enhance Rivermax RX manager to support multiple streams per thread) |
Contributor
There was a problem hiding this comment.
The destructor contains unresolved merge conflict markers (<<<<<<<, =======, >>>>>>>). This will prevent the translation unit from compiling.
The conflicting region shows two different implementations of the burst cleanup loop:
HEADversion iterates using the old single-queue API:rx_bursts_out_queue_->...- The incoming branch version iterates over the new map:
for (auto& [stream_id, rx_bursts_out_queue] : rx_bursts_out_queue_) { ... }
The correct resolution is the map-based loop (the >>>>>>> side), since rx_bursts_out_queue_ is now std::unordered_map<int, std::shared_ptr<AnoBurstsQueue>>. The conflict must be resolved before merging.
Comment on lines
+558
to
+559
| rx_bursts_mempool_->enqueue_burst(cur_out_burst_); | ||
| reset_current_burst(); |
Contributor
There was a problem hiding this comment.
stream_id argument
Inside enqueue_and_reset_current_burst(int stream_id), the burst-drop path passes the entire map (cur_out_burst_) to enqueue_burst, and calls reset_current_burst() without the required stream_id argument:
rx_bursts_mempool_->enqueue_burst(cur_out_burst_); // cur_out_burst_ is a map, not a shared_ptr
reset_current_burst(); // missing stream_id parametercur_out_burst_ is std::unordered_map<int, std::shared_ptr<RivermaxBurst>>, so passing it where a std::shared_ptr<RivermaxBurst> is expected is a type error, and reset_current_burst() no longer has a zero-argument overload. Both will fail to compile.
Suggested change
| rx_bursts_mempool_->enqueue_burst(cur_out_burst_); | |
| reset_current_burst(); | |
| rx_bursts_mempool_->enqueue_burst(cur_out_burst_[stream_id]); | |
| reset_current_burst(stream_id); |
Enhanced the Rivermax manager to support multiple streams on the same thread with stream-specific packet processing and queueing capabilities. Key changes: - Enhanced RDK applications (ipo_receiver and rtp_receiver) to get an extension to RDK AppSetting in order to set threads setting. Each thread has it's dedicated core, ID and streams settings. - Enhanced burst manager to support per-stream ID burst queues. (Each stream ID gets its own dedicated burst queue for packet segregation). - Maintained backward compatibility: bench RX processes bursts regardless of stream ID. Signed-off-by: Dana Lessner <dlessner@nvidia.com>
Enhance the media player application to receive and process multiple video streams simultaneously using the advanced networking with Rivermax manager. This application served as a reference implementation for multi-stream media reception scenarios in advanced networking deployment. Key changes: - Receive multiple streams with Rivermax manager. - Save each stream to different file. - Add MockReceiverOp to simulate data reception for testing Note: The application supports visualization using holoviz - this is currently limited and allowed only when receiving a single stream. Signed-off-by: Dana Lessner <dlessner@nvidia.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
applications/adv_networking_bench/cpp/rivermax_bench_op_tx.h (1)
1-2:⚠️ Potential issue | 🟡 MinorPipeline failure: Update copyright year.
The pipeline reports the copyright header is incomplete. The copyright year range should include the current year (2026).
Proposed fix
/* - * SPDX-FileCopyrightText: Copyright (c) 2023-2025, NVIDIA CORPORATION & AFFILIATES. All rights reserved. + * SPDX-FileCopyrightText: Copyright (c) 2023-2026, NVIDIA CORPORATION & AFFILIATES. All rights reserved.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@applications/adv_networking_bench/cpp/rivermax_bench_op_tx.h` around lines 1 - 2, Update the SPDX copyright header to include the current year (2026): locate the top comment containing the string "SPDX-FileCopyrightText" in rivermax_bench_op_tx.h and change the year range from "2023-2025" to "2023-2026" so the header reads the full inclusive range.
♻️ Duplicate comments (1)
applications/adv_networking_media_player/cpp/adv_networking_media_player.cpp (1)
384-384:⚠️ Potential issue | 🔴 CriticalType mismatch: accessing
unordered_map<string, ...>with integer key.
adv_net_media_rx_mapis declared asstd::unordered_map<std::string, std::shared_ptr<ops::AdvNetworkMediaRxOp>>but is accessed withadv_net_media_rx_map[0]using an integer key. This will implicitly convert0to a string"0", which is unlikely to match any stream name. This was flagged in a previous review.Proposed fix
- add_flow(adv_net_media_rx_map[0], visualizer, {{"out_video_buffer", "receivers"}}); + // Get first stream from the map + add_flow(adv_net_media_rx_map.begin()->second, visualizer, {{"out_video_buffer", "receivers"}});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@applications/adv_networking_media_player/cpp/adv_networking_media_player.cpp` at line 384, The call to add_flow uses adv_net_media_rx_map[0], but adv_net_media_rx_map is an std::unordered_map<std::string, std::shared_ptr<ops::AdvNetworkMediaRxOp>> so indexing with 0 converts to the string "0" incorrectly; replace adv_net_media_rx_map[0] with the correct string key (e.g., adv_net_media_rx_map.at("expected_stream_name")) or, if you meant the first element, use adv_net_media_rx_map.begin()->second to pass the actual shared_ptr; update the add_flow invocation accordingly (refer to adv_net_media_rx_map and the add_flow call around visualizer/out_video_buffer).
🧹 Nitpick comments (6)
applications/adv_networking_media_sender/cpp/adv_networking_media_sender.cpp (1)
17-19: Potentially unused includes.
assert.h,sys/time.h, andarpa/inet.hare included but don't appear to be used in this file. Consider removing unused includes to improve compile times and clarity.Proposed fix
-#include <assert.h> -#include <sys/time.h> -#include <arpa/inet.h> - `#include` "holoscan/holoscan.hpp"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@applications/adv_networking_media_sender/cpp/adv_networking_media_sender.cpp` around lines 17 - 19, The three headers included at the top (`#include` <assert.h>, `#include` <sys/time.h>, `#include` <arpa/inet.h>) appear unused in adv_networking_media_sender.cpp; remove these unused includes to reduce compile noise and improve clarity, but first search for any uses of assert, gettimeofday/struct timeval, or inet_* functions in functions like main or any net/send helpers in this file — if none are referenced, delete those three include lines; if a symbol is needed, keep only the specific header(s) that provide it.applications/adv_networking_media_sender/README.md (1)
164-164: Optional: Consider hyphenating compound adjectives.For consistency with technical writing standards, "High Performance" could be hyphenated when used as a compound adjective (e.g., "High-Performance Networking tutorial"). This is a minor stylistic suggestion.
Also applies to: 499-499, 541-541
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@applications/adv_networking_media_sender/README.md` at line 164, Change the phrase "High Performance Networking tutorial" to "High-Performance Networking tutorial" wherever it functions as a compound adjective (specifically update the occurrence "High Performance Networking tutorial" and the other instances of "High Performance" in this README), ensuring consistency across the three occurrences so the compound adjective is hyphenated.applications/adv_networking_media_sender/python/adv_networking_media_sender.py (1)
33-95: Code duplication:check_rx_tx_enabledis duplicated across player and sender.This function is nearly identical to the one in
adv_networking_media_player.py. Consider extracting it to a shared utility module (e.g., inholohub.advanced_network_common) to avoid maintenance burden and ensure consistent behavior.Would you like me to help create a shared utility module for this function?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@applications/adv_networking_media_sender/python/adv_networking_media_sender.py` around lines 33 - 95, Extract the duplicated check_rx_tx_enabled function into a shared utility module (e.g., create holohub.advanced_network_common with a public function check_rx_tx_enabled(app, require_rx=True, require_tx=False)), move the implementation as-is (preserving logging, sys.exit paths and fallback logic), then replace the local definitions in adv_networking_media_sender.py and adv_networking_media_player.py with an import from holohub.advanced_network_common and call the shared check_rx_tx_enabled; update any module imports and run tests to ensure behavior and signatures remain identical.applications/adv_networking_bench/cpp/rivermax_bench_op_tx.h (1)
610-627: Code aftershutdown()is unreachable;shutdown()callsexit(1).Lines 615-627 continue execution (e.g., accessing
depth_it->second) after callingshutdown()at lines 617 and 622. Sinceshutdown()terminates the process viaexit(1), this code is unreachable but the compiler may not optimize it away and it obscures intent.Add explicit
returnstatements (or mark the function[[noreturn]]) to clarify control flow and avoid compiler warnings:Proposed fix
auto format_it = COLOR_DEPTH_TO_PIXEL_RATIO.find(sampling_format); if (format_it == COLOR_DEPTH_TO_PIXEL_RATIO.end()) { shutdown("Unsupported sampling format"); + return 0; // Unreachable, but satisfies compiler } auto depth_it = format_it->second.find(bit_depth); if (depth_it == format_it->second.end()) { shutdown("Unsupported bit depth"); + return 0; // Unreachable, but satisfies compiler }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@applications/adv_networking_bench/cpp/rivermax_bench_op_tx.h` around lines 610 - 627, In calculate_frame_size, the calls to shutdown("...") end the process but code after them (using format_it and depth_it) appears reachable; add an explicit return to clarify control flow — after the first shutdown("Unsupported sampling format") add `return 0;` and after the second shutdown("Unsupported bit depth") add `return 0;` so calculate_frame_size returns a size_t in those branches and the compiler/reader won't treat subsequent code as reachable; reference symbols: calculate_frame_size, shutdown, format_it, depth_it.applications/adv_networking_media_player/python/adv_networking_media_player.py (1)
75-95: Consider narrowing exception types for more precise error handling.While catching broad
Exceptionis acceptable for application-level error handling (as noted by Ruff BLE001), the nested try-except at lines 78-95 could benefit from catching more specific exceptions likeKeyErrorfor config parsing failures. This would help distinguish between "config not found" vs "unexpected errors."However, given this is application-level code with proper logging, this is a minor suggestion.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@applications/adv_networking_media_player/python/adv_networking_media_player.py` around lines 75 - 95, The nested broad except should be narrowed to handle configuration-missing errors only: replace the inner "except Exception as e2:" that wraps the app.from_config("advanced_network_media_rx") / app.from_config("advanced_network_media_tx") calls with a specific exception catch (e.g., "except KeyError as e2:" or the application’s config-specific exception type) so missing/lookup failures are handled and logged as before for require_rx/require_tx, and add a separate "except Exception as e2: raise" (or re-raise after logging) to avoid silently swallowing unexpected errors; reference the require_rx/require_tx variables and the app.from_config calls to locate the block to change.applications/adv_networking_media_player/cpp/adv_networking_media_player.cpp (1)
250-251: Flushing file stream on every frame write impacts performance.Calling
file_stream_.flush()after every frame write (line 251) significantly impacts I/O performance, especially at high frame rates. Consider removing the flush or making it configurable, allowing the OS to batch writes.Proposed fix - flush periodically or on close
- // Flush to ensure data is written - file_stream_.flush(); + // Note: Flushing is handled by destructor/close for better performance + // file_stream_.flush(); // Uncomment for immediate persistence at cost of performance🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@applications/adv_networking_media_player/cpp/adv_networking_media_player.cpp` around lines 250 - 251, The call to file_stream_.flush() after every frame write causes heavy I/O slowdown; remove the per-frame flush and instead make flushing configurable and/or periodic: replace the immediate file_stream_.flush() call with a configuration option (e.g., bool flush_each_frame or int flush_interval_frames) and only call file_stream_.flush() when flush_each_frame is true, when a frame counter reaches flush_interval_frames, or when closing the stream (e.g., in the class destructor or Close method); update the code path where file_stream_.flush() is invoked (the frame write code in adv_networking_media_player.cpp that currently calls file_stream_.flush()) to use this new behavior and add any necessary members and initialization to the class (e.g., flush_each_frame, flush_interval_frames, and a frame_counter).
🤖 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/adv_networking_media_player/cpp/adv_networking_media_player.cpp`:
- Around line 274-277: The HOLOSCAN_LOG_INFO message inside
MockReceiverOp::initialize() references the wrong class name; update the log
string in the initialize() override (the HOLOSCAN_LOG_INFO(...) call) to use
"MockReceiverOp::initialize()" (or the correct MockReceiverOp class identifier)
so the log reflects the actual class, leaving the rest of the initialize()
implementation (including holoscan::Operator::initialize()) unchanged.
- Around line 274-275: MockReceiverOp::initialize() declares an unused variable
cuda_error; remove the unused cudaError_t cuda_error declaration from
MockReceiverOp::initialize(), or if a CUDA API call was intended, capture and
check its return value (e.g., assign the CUDA call result to cuda_error and
handle errors via cudaGetErrorString) — locate the declaration inside
MockReceiverOp::initialize and either delete it or replace it with proper error
handling for the relevant CUDA call.
- Line 295: Remove the redundant blank line at the start of the code block
beginning around line 295 (delete the extra empty line so the block starts
immediately with code), shorten the two overlong lines around lines 344 and 404
to be <=100 characters by splitting long string literals or expressions across
multiple concatenated pieces or by introducing a short local variable, and move
the stray closing parenthesis for the multi-line expression near line 362 up
onto the previous line so the ')' closes the argument list on the same line as
the last parameter; look for the enclosing function/block in
adv_networking_media_player.cpp that contains these lines and apply the three
edits there.
- Around line 395-397: The add_flow call linking
adv_net_media_rx_map[stream_name] to frames_writer is missing explicit port
mapping; update the call that uses adv_net_media_rx_map and FramesWriterOp so
the source output port "out_video_buffer" from AdvNetworkMediaRxOp is mapped to
the destination input port "input" on FramesWriterOp (i.e., call add_flow with
the source op, destination op, source port "out_video_buffer" and destination
port "input").
In `@applications/adv_networking_media_player/README.md`:
- Around line 503-505: Replace the unsafe global wildcard safe.directory example
with a repository-specific trust entry: locate the git command shown (`git
config --global --add safe.directory '*'`) and update the example to demonstrate
adding a single repo path (not a wildcard), or better yet show using the local
repo config instead of global; ensure the documented command uses a concrete
repo path placeholder (e.g., <repo-path>) rather than '*' and mention preferring
repo-scoped config over a global wildcard.
- Around line 156-162: The fenced code block showing the stream diagram is
missing a language tag; update the triple-backtick fence around the ASCII
diagram in the README (the block containing "---- stream ID 0 ---
AdvNetworkMediaRxOp --- FramesWriterOp" and "Receiving 2 streams") to use a
language identifier such as ```text (or ```bash if you prefer), preserving the
exact diagram content and spacing so markdownlint MD040 is satisfied.
In `@applications/adv_networking_media_sender/cpp/CMakeLists.txt`:
- Around line 62-70: Create the missing CMakeLists.txt.install file referenced
by the install(...) call so the install rule in CMakeLists.txt can succeed; add
a new file named CMakeLists.txt.install in the cpp/ directory containing the
minimal CMake project header and any example-specific commands/variables needed
when placed into examples/${PROJECT_NAME} (mirror the top-level CMakeLists
structure used by this project), ensure it uses the same PROJECT_NAME and
expected variables so the install( FILES CMakeLists.txt.install RENAME
CMakeLists.txt DESTINATION examples/${PROJECT_NAME} ) step produces a valid
CMakeLists.txt in the installed examples tree.
- Around line 23-24: Remove the ineffective CUDA architecture setting: delete
the line that calls set(CMAKE_CUDA_ARCHITECTURES "70;80;90") from the CMakeLists
(this project only declares the CXX language and does not enable CUDA), or if
you intended to target CUDA consumers, wrap that set in a guard that only runs
when CUDA is enabled (e.g. check CMAKE_CUDA_COMPILER or call
enable_language(CUDA) before setting CMAKE_CUDA_ARCHITECTURES) so the variable
actually takes effect.
---
Outside diff comments:
In `@applications/adv_networking_bench/cpp/rivermax_bench_op_tx.h`:
- Around line 1-2: Update the SPDX copyright header to include the current year
(2026): locate the top comment containing the string "SPDX-FileCopyrightText" in
rivermax_bench_op_tx.h and change the year range from "2023-2025" to "2023-2026"
so the header reads the full inclusive range.
---
Duplicate comments:
In
`@applications/adv_networking_media_player/cpp/adv_networking_media_player.cpp`:
- Line 384: The call to add_flow uses adv_net_media_rx_map[0], but
adv_net_media_rx_map is an std::unordered_map<std::string,
std::shared_ptr<ops::AdvNetworkMediaRxOp>> so indexing with 0 converts to the
string "0" incorrectly; replace adv_net_media_rx_map[0] with the correct string
key (e.g., adv_net_media_rx_map.at("expected_stream_name")) or, if you meant the
first element, use adv_net_media_rx_map.begin()->second to pass the actual
shared_ptr; update the add_flow invocation accordingly (refer to
adv_net_media_rx_map and the add_flow call around visualizer/out_video_buffer).
---
Nitpick comments:
In `@applications/adv_networking_bench/cpp/rivermax_bench_op_tx.h`:
- Around line 610-627: In calculate_frame_size, the calls to shutdown("...") end
the process but code after them (using format_it and depth_it) appears
reachable; add an explicit return to clarify control flow — after the first
shutdown("Unsupported sampling format") add `return 0;` and after the second
shutdown("Unsupported bit depth") add `return 0;` so calculate_frame_size
returns a size_t in those branches and the compiler/reader won't treat
subsequent code as reachable; reference symbols: calculate_frame_size, shutdown,
format_it, depth_it.
In
`@applications/adv_networking_media_player/cpp/adv_networking_media_player.cpp`:
- Around line 250-251: The call to file_stream_.flush() after every frame write
causes heavy I/O slowdown; remove the per-frame flush and instead make flushing
configurable and/or periodic: replace the immediate file_stream_.flush() call
with a configuration option (e.g., bool flush_each_frame or int
flush_interval_frames) and only call file_stream_.flush() when flush_each_frame
is true, when a frame counter reaches flush_interval_frames, or when closing the
stream (e.g., in the class destructor or Close method); update the code path
where file_stream_.flush() is invoked (the frame write code in
adv_networking_media_player.cpp that currently calls file_stream_.flush()) to
use this new behavior and add any necessary members and initialization to the
class (e.g., flush_each_frame, flush_interval_frames, and a frame_counter).
In
`@applications/adv_networking_media_player/python/adv_networking_media_player.py`:
- Around line 75-95: The nested broad except should be narrowed to handle
configuration-missing errors only: replace the inner "except Exception as e2:"
that wraps the app.from_config("advanced_network_media_rx") /
app.from_config("advanced_network_media_tx") calls with a specific exception
catch (e.g., "except KeyError as e2:" or the application’s config-specific
exception type) so missing/lookup failures are handled and logged as before for
require_rx/require_tx, and add a separate "except Exception as e2: raise" (or
re-raise after logging) to avoid silently swallowing unexpected errors;
reference the require_rx/require_tx variables and the app.from_config calls to
locate the block to change.
In
`@applications/adv_networking_media_sender/cpp/adv_networking_media_sender.cpp`:
- Around line 17-19: The three headers included at the top (`#include` <assert.h>,
`#include` <sys/time.h>, `#include` <arpa/inet.h>) appear unused in
adv_networking_media_sender.cpp; remove these unused includes to reduce compile
noise and improve clarity, but first search for any uses of assert,
gettimeofday/struct timeval, or inet_* functions in functions like main or any
net/send helpers in this file — if none are referenced, delete those three
include lines; if a symbol is needed, keep only the specific header(s) that
provide it.
In
`@applications/adv_networking_media_sender/python/adv_networking_media_sender.py`:
- Around line 33-95: Extract the duplicated check_rx_tx_enabled function into a
shared utility module (e.g., create holohub.advanced_network_common with a
public function check_rx_tx_enabled(app, require_rx=True, require_tx=False)),
move the implementation as-is (preserving logging, sys.exit paths and fallback
logic), then replace the local definitions in adv_networking_media_sender.py and
adv_networking_media_player.py with an import from
holohub.advanced_network_common and call the shared check_rx_tx_enabled; update
any module imports and run tests to ensure behavior and signatures remain
identical.
In `@applications/adv_networking_media_sender/README.md`:
- Line 164: Change the phrase "High Performance Networking tutorial" to
"High-Performance Networking tutorial" wherever it functions as a compound
adjective (specifically update the occurrence "High Performance Networking
tutorial" and the other instances of "High Performance" in this README),
ensuring consistency across the three occurrences so the compound adjective is
hyphenated.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c814587c-7c00-49c6-bd4f-3447e3a8c52e
📒 Files selected for processing (22)
applications/CMakeLists.txtapplications/adv_networking_bench/adv_networking_bench_rivermax_rx.yamlapplications/adv_networking_bench/adv_networking_bench_rivermax_tx.yamlapplications/adv_networking_bench/cpp/rivermax_bench_op_tx.happlications/adv_networking_media_player/CMakeLists.txtapplications/adv_networking_media_player/README.mdapplications/adv_networking_media_player/adv_networking_media_player.yamlapplications/adv_networking_media_player/cpp/CMakeLists.txtapplications/adv_networking_media_player/cpp/adv_networking_media_player.cppapplications/adv_networking_media_player/cpp/metadata.jsonapplications/adv_networking_media_player/python/CMakeLists.txtapplications/adv_networking_media_player/python/adv_networking_media_player.pyapplications/adv_networking_media_player/python/metadata.jsonapplications/adv_networking_media_sender/CMakeLists.txtapplications/adv_networking_media_sender/README.mdapplications/adv_networking_media_sender/adv_networking_media_sender.yamlapplications/adv_networking_media_sender/cpp/CMakeLists.txtapplications/adv_networking_media_sender/cpp/adv_networking_media_sender.cppapplications/adv_networking_media_sender/cpp/metadata.jsonapplications/adv_networking_media_sender/python/CMakeLists.txtapplications/adv_networking_media_sender/python/adv_networking_media_sender.pyapplications/adv_networking_media_sender/python/metadata.json
✅ Files skipped from review due to trivial changes (9)
- applications/adv_networking_media_player/CMakeLists.txt
- applications/adv_networking_media_player/python/CMakeLists.txt
- applications/adv_networking_media_sender/CMakeLists.txt
- applications/adv_networking_media_sender/python/metadata.json
- applications/adv_networking_media_sender/cpp/metadata.json
- applications/adv_networking_media_player/adv_networking_media_player.yaml
- applications/adv_networking_media_player/python/metadata.json
- applications/adv_networking_media_player/cpp/metadata.json
- applications/adv_networking_media_player/cpp/CMakeLists.txt
🚧 Files skipped from review as they are similar to previous changes (4)
- applications/CMakeLists.txt
- applications/adv_networking_media_sender/python/CMakeLists.txt
- applications/adv_networking_media_sender/adv_networking_media_sender.yaml
- applications/adv_networking_bench/adv_networking_bench_rivermax_rx.yaml
Comment on lines
+23
to
+24
| # Global variables | ||
| set(CMAKE_CUDA_ARCHITECTURES "70;80;90") |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if any linked libraries require CUDA architectures to be set
rg -l "CMAKE_CUDA_ARCHITECTURES" operators/advanced_network_media/
rg "CUDA" applications/adv_networking_media_sender/cpp/adv_networking_media_sender.cppRepository: nvidia-holoscan/holohub
Length of output: 49
🏁 Script executed:
# Find and examine the advanced_network_media operator structure
find operators -name "*advanced_network_media*" -type d
# Check CMakeLists for any CUDA-related operator
fd "CMakeLists.txt" operators/ --exec grep -l "advanced_network_media_tx" {} \;
# Look at the holoscan core linking and what it requires
rg "holoscan::core" applications/adv_networking_media_sender/cpp/CMakeLists.txt -A 2 -B 2Repository: nvidia-holoscan/holohub
Length of output: 553
🏁 Script executed:
# Check the actual CMakeLists.txt of the adv_networking_media_sender application
cat -n applications/adv_networking_media_sender/cpp/CMakeLists.txt | head -50Repository: nvidia-holoscan/holohub
Length of output: 2124
🏁 Script executed:
# Check what holoscan::advanced_network provides (is it CUDA-based?)
find operators -name CMakeLists.txt -path "*advanced_network*" | grep -v media | head -5 | xargs catRepository: nvidia-holoscan/holohub
Length of output: 12685
🏁 Script executed:
# Check if there are any CUDA-related settings or enablement elsewhere
grep -r "enable_language" applications/adv_networking_media_sender/
# Check if any .cu files exist in the application directory
find applications/adv_networking_media_sender -name "*.cu"
# Verify the exact architectures used by each linked library
grep -r "CUDA_ARCHITECTURES" operators/advanced_network/Repository: nvidia-holoscan/holohub
Length of output: 271
Remove CMAKE_CUDA_ARCHITECTURES — it has no effect in this CXX-only project.
The project declares only the CXX language (line 17) and does not call enable_language(CUDA). Setting CMAKE_CUDA_ARCHITECTURES only takes effect when CUDA is enabled, so this line has no functional impact. The linked libraries (e.g., holoscan::advanced_network) already have their own CUDA architectures set at compile time and are not affected by this setting in the consumer project.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@applications/adv_networking_media_sender/cpp/CMakeLists.txt` around lines 23
- 24, Remove the ineffective CUDA architecture setting: delete the line that
calls set(CMAKE_CUDA_ARCHITECTURES "70;80;90") from the CMakeLists (this project
only declares the CXX language and does not enable CUDA), or if you intended to
target CUDA consumers, wrap that set in a guard that only runs when CUDA is
enabled (e.g. check CMAKE_CUDA_COMPILER or call enable_language(CUDA) before
setting CMAKE_CUDA_ARCHITECTURES) so the variable actually takes effect.
Comment on lines
+62
to
+70
| install( | ||
| FILES CMakeLists.txt.install | ||
| RENAME CMakeLists.txt | ||
| DESTINATION examples/${PROJECT_NAME} | ||
| COMPONENT ${PROJECT_NAME}-cppsrc | ||
| PERMISSIONS OWNER_READ OWNER_WRITE | ||
| GROUP_READ | ||
| WORLD_READ | ||
| ) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if CMakeLists.txt.install exists
fd -t f "CMakeLists.txt.install" applications/adv_networking_media_sender/Repository: nvidia-holoscan/holohub
Length of output: 49
🏁 Script executed:
#!/bin/bash
# List all files in the target directory to see what exists
echo "=== Files in applications/adv_networking_media_sender/ ==="
fd -t f . applications/adv_networking_media_sender/ --max-depth 1
echo ""
echo "=== Subdirectories ==="
fd -t d . applications/adv_networking_media_sender/ --max-depth 1Repository: nvidia-holoscan/holohub
Length of output: 425
🏁 Script executed:
#!/bin/bash
# Show the full CMakeLists.txt around the lines in question
echo "=== Full context of CMakeLists.txt (lines 50-75) ==="
sed -n '50,75p' applications/adv_networking_media_sender/CMakeLists.txtRepository: nvidia-holoscan/holohub
Length of output: 119
🏁 Script executed:
#!/bin/bash
# Read the cpp CMakeLists.txt file
echo "=== Checking cpp/CMakeLists.txt existence and size ==="
wc -l applications/adv_networking_media_sender/cpp/CMakeLists.txt
echo ""
echo "=== Lines 50-75 of cpp/CMakeLists.txt ==="
sed -n '50,75p' applications/adv_networking_media_sender/cpp/CMakeLists.txtRepository: nvidia-holoscan/holohub
Length of output: 809
🏁 Script executed:
#!/bin/bash
# List files in cpp directory
echo "=== Files in applications/adv_networking_media_sender/cpp/ ==="
fd -t f . applications/adv_networking_media_sender/cpp/ --max-depth 1Repository: nvidia-holoscan/holohub
Length of output: 325
Add missing CMakeLists.txt.install file.
The install rule at lines 62-70 references CMakeLists.txt.install, but this file does not exist in the cpp/ directory. The CMake build will fail when trying to install this non-existent file. Create the file with the appropriate content for the examples directory.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@applications/adv_networking_media_sender/cpp/CMakeLists.txt` around lines 62
- 70, Create the missing CMakeLists.txt.install file referenced by the
install(...) call so the install rule in CMakeLists.txt can succeed; add a new
file named CMakeLists.txt.install in the cpp/ directory containing the minimal
CMake project header and any example-specific commands/variables needed when
placed into examples/${PROJECT_NAME} (mirror the top-level CMakeLists structure
used by this project), ensure it uses the same PROJECT_NAME and expected
variables so the install( FILES CMakeLists.txt.install RENAME CMakeLists.txt
DESTINATION examples/${PROJECT_NAME} ) step produces a valid CMakeLists.txt in
the installed examples tree.
531edfe to
39958b1
Compare
Signed-off-by: Dana Lessner <dlessner@nvidia.com>
Signed-off-by: Dana Lessner <dlessner@nvidia.com>
Signed-off-by: Dana Lessner <dlessner@nvidia.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
♻️ Duplicate comments (7)
applications/adv_networking_media_player/cpp/adv_networking_media_player.cpp (2)
274-284:⚠️ Potential issue | 🟡 MinorFix incorrect class name in log message and remove unused variable.
Two issues in
MockReceiverOp::initialize():
- The log message references
AdvNetworkingBenchDefaultRxOpbut this isMockReceiverOpcuda_erroris declared but never usedProposed fix
void initialize() override { - cudaError_t cuda_error; - HOLOSCAN_LOG_INFO("AdvNetworkingBenchDefaultRxOp::initialize()"); + HOLOSCAN_LOG_INFO("MockReceiverOp::initialize()"); holoscan::Operator::initialize();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@applications/adv_networking_media_player/cpp/adv_networking_media_player.cpp` around lines 274 - 284, The initialize() method in MockReceiverOp incorrectly logs "AdvNetworkingBenchDefaultRxOp" and declares an unused variable cuda_error; update the HOLOSCAN_LOG_INFO call in MockReceiverOp::initialize() to use the correct class name "MockReceiverOp::initialize()" (or similar correct identifier) and remove the unused cuda_error declaration, keeping the call to holoscan::Operator::initialize() and the port_id_ validation logic unchanged.
383-383:⚠️ Potential issue | 🔴 CriticalType mismatch: accessing
unordered_map<string, ...>with integer key.
adv_net_media_rx_mapis declared asstd::unordered_map<std::string, ...>but accessed withadv_net_media_rx_map[0]using an integer key. This implicitly converts0to"0", which won't match actual stream names from the YAML config.Proposed fix - use iterator to get first element
- add_flow(adv_net_media_rx_map[0], visualizer, {{"out_video_buffer", "receivers"}}); + add_flow(adv_net_media_rx_map.begin()->second, visualizer, {{"out_video_buffer", "receivers"}});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@applications/adv_networking_media_player/cpp/adv_networking_media_player.cpp` at line 383, The call add_flow(adv_net_media_rx_map[0], visualizer, {{"out_video_buffer", "receivers"}}) is using an integer index on adv_net_media_rx_map (declared as std::unordered_map<std::string, ...>); replace that lookup with a proper map access (e.g., use adv_net_media_rx_map.begin()->second to pass the first element’s value or use adv_net_media_rx_map.find("<stream_name>")->second when you have the actual stream key) so add_flow receives the correct mapped value; update the call site that references adv_net_media_rx_map accordingly and ensure you check that begin()/find() is valid before dereferencing.applications/adv_networking_media_player/README.md (2)
156-162:⚠️ Potential issue | 🟡 MinorAdd a language tag to the fenced code block.
This code fence is missing a language identifier (markdownlint MD040). Use
textfor ASCII diagrams.Proposed fix
-``` +```text ---- stream ID 0 --- AdvNetworkMediaRxOp --- FramesWriterOp | Receiving 2 streams -| | ---- stream ID 1 --- AdvNetworkMediaRxOp --- FramesWriterOp</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@applications/adv_networking_media_player/README.mdaround lines 156 - 162,
The fenced ASCII diagram block in README.md is missing a language tag
(markdownlint MD040); update the opening backticks for the ASCII diagram (the
diagram containing "Receiving 2 streams" and the stream ID lines) fromtotext so the block becomes a ```text fenced code block and preserves the ASCII
diagram formatting.</details> --- `499-505`: _⚠️ Potential issue_ | _🟠 Major_ **Avoid recommending global wildcard `safe.directory`.** `git config --global --add safe.directory '*'` disables Git's repository ownership safety checks for all repos on the machine. This is a security risk. Recommend a repo-specific path instead. <details> <summary>Safer alternative</summary> ```diff ### Git Configuration (if needed) If you encounter Git-related issues during build: ```bash -git config --global --add safe.directory '*' +# Prefer repo-specific trust instead of a global wildcard +git config --global --add safe.directory /workspace/holohub ``` ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@applications/adv_networking_media_player/README.md` around lines 499 - 505, Replace the unsafe global wildcard command `git config --global --add safe.directory '*'` in the "Git Configuration" section of README.md with a repo-specific trust recommendation; for example instruct users to run `git config --global --add safe.directory /workspace/holohub` or (better) use a local repo-specific config (e.g., `git -C /path/to/repo config --add safe.directory $(pwd)`), updating the displayed code block and the explanatory text accordingly so the guidance no longer recommends blanket global trust. ``` </details> </blockquote></details> <details> <summary>operators/advanced_network/advanced_network/managers/rivermax/applications/ano_media_sender.cpp (1)</summary><blockquote> `203-213`: _⚠️ Potential issue_ | _🔴 Critical_ **`reserve()` does not make `m_streams_per_thread[thread_idx]` valid.** After Line 204 the vector still has size 0, so Line 209 writes out of bounds. Use `resize()` before indexed assignment or replace the indexed write with `push_back()`. <details> <summary>🐛 Minimal fix</summary> ```diff void ANOMediaSenderApp::distribute_work_for_threads() { - m_streams_per_thread.reserve(m_media_sender_settings->thread_settings.size()); + m_streams_per_thread.resize(m_media_sender_settings->thread_settings.size()); m_media_sender_settings->num_of_total_streams = 0; m_media_sender_settings->num_of_threads = m_media_sender_settings->thread_settings.size(); int thread_idx = 0; ``` </details> This read-only check confirms the `reserve()` + indexed-write pattern is still present in the checked-in file. ```shell #!/bin/bash python - <<'PY' from pathlib import Path path = Path("operators/advanced_network/advanced_network/managers/rivermax/applications/ano_media_sender.cpp") snippet = "\n".join(path.read_text().splitlines()[202:213]) print(snippet) assert "m_streams_per_thread.reserve" in snippet assert "m_streams_per_thread[" in snippet assert "resize" not in snippet print("Found reserve() with indexed assignment and no resize().") PY ``` <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@operators/advanced_network/advanced_network/managers/rivermax/applications/ano_media_sender.cpp` around lines 203 - 213, The code in ANOMediaSenderApp::distribute_work_for_threads uses m_streams_per_thread.reserve(...) but then writes by index (m_streams_per_thread[thread_idx]) which is out-of-bounds; change the logic to either call m_streams_per_thread.resize(m_media_sender_settings->thread_settings.size()) before the loop or replace the indexed assignment with m_streams_per_thread.push_back(thread.stream_network_settings.size()), and keep the rest of the loop (updating m_media_sender_settings->num_of_total_streams and thread_idx or remove thread_idx if using push_back) so m_streams_per_thread correctly contains one entry per thread. ``` </details> </blockquote></details> <details> <summary>operators/advanced_network/advanced_network/managers/rivermax/applications/ano_ipo_receiver.cpp (1)</summary><blockquote> `164-173`: _⚠️ Potential issue_ | _🔴 Critical_ **`reserve()` leaves `m_streams_per_thread` empty before the indexed write.** After Line 165 the vector still has no elements, so Line 169 writes out of bounds. Use `resize()` before `operator[]` or append with `push_back()`. <details> <summary>🐛 Minimal fix</summary> ```diff void ANOIPOReceiverApp::distribute_work_for_threads() { - m_streams_per_thread.reserve(m_ipo_receiver_settings->thread_settings.size()); + m_streams_per_thread.resize(m_ipo_receiver_settings->thread_settings.size()); m_ipo_receiver_settings->num_of_total_streams = 0; int thread_index = 0; for (const auto& thread : m_ipo_receiver_settings->thread_settings) { ``` </details> This read-only check confirms the `reserve()` + indexed-write pattern is still present in the checked-in file. ```shell #!/bin/bash python - <<'PY' from pathlib import Path path = Path("operators/advanced_network/advanced_network/managers/rivermax/applications/ano_ipo_receiver.cpp") snippet = "\n".join(path.read_text().splitlines()[163:173]) print(snippet) assert "m_streams_per_thread.reserve" in snippet assert "m_streams_per_thread[" in snippet assert "resize" not in snippet print("Found reserve() with indexed assignment and no resize().") PY ``` <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@operators/advanced_network/advanced_network/managers/rivermax/applications/ano_ipo_receiver.cpp` around lines 164 - 173, In ANOIPOReceiverApp::distribute_work_for_threads the code uses m_streams_per_thread.reserve(...) then writes via m_streams_per_thread[thread_index], which is out-of-bounds because reserve() does not create elements; change the logic to either call m_streams_per_thread.resize(m_ipo_receiver_settings->thread_settings.size()) before the loop so operator[] is safe, or keep reserve() and replace the indexed assignment with m_streams_per_thread.push_back(thread.stream_network_settings.size()); also keep updating m_ipo_receiver_settings->num_of_total_streams as shown. ``` </details> </blockquote></details> <details> <summary>operators/advanced_network/advanced_network/managers/rivermax/applications/ano_rtp_receiver.cpp (1)</summary><blockquote> `154-163`: _⚠️ Potential issue_ | _🔴 Critical_ **`reserve()` leaves `m_streams_per_thread` empty here.** After Line 155 the vector has capacity, not elements, so Line 159 writes out of bounds. Use `resize()` before indexed assignment or switch to `push_back()`. <details> <summary>🐛 Minimal fix</summary> ```diff void ANORTPReceiverApp::distribute_work_for_threads() { - m_streams_per_thread.reserve(m_rtp_receiver_settings->thread_settings.size()); + m_streams_per_thread.resize(m_rtp_receiver_settings->thread_settings.size()); m_rtp_receiver_settings->num_of_total_streams = 0; int thread_index = 0; for (const auto& thread : m_rtp_receiver_settings->thread_settings) { ``` </details> This read-only check confirms the `reserve()` + indexed-write pattern is still present in the checked-in file. ```shell #!/bin/bash python - <<'PY' from pathlib import Path path = Path("operators/advanced_network/advanced_network/managers/rivermax/applications/ano_rtp_receiver.cpp") snippet = "\n".join(path.read_text().splitlines()[153:163]) print(snippet) assert "m_streams_per_thread.reserve" in snippet assert "m_streams_per_thread[" in snippet assert "resize" not in snippet print("Found reserve() with indexed assignment and no resize().") PY ``` <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@operators/advanced_network/advanced_network/managers/rivermax/applications/ano_rtp_receiver.cpp` around lines 154 - 163, In distribute_work_for_threads(), m_streams_per_thread.reserve(...) only allocates capacity so the later indexed writes m_streams_per_thread[thread_index] are out-of-bounds; fix by either calling m_streams_per_thread.resize(m_rtp_receiver_settings->thread_settings.size()) before the loop or replace the indexed assignment with m_streams_per_thread.push_back(thread.stream_network_settings.size()) while keeping the loop and updating num_of_total_streams; ensure you update/remove thread_index accordingly. ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🟡 Minor comments (16)</summary><blockquote> <details> <summary>applications/adv_networking_bench/cpp/rivermax_bench_op_tx.h-566-573 (1)</summary><blockquote> `566-573`: _⚠️ Potential issue_ | _🟡 Minor_ **Documentation does not match implementation.** The `@note` states "exits the application if the video sampling format is not supported", but the function returns `VideoSampling::Unknown` instead. The exit actually occurs in `calculate_frame_size()` when the lookup fails. The same issue exists for `get_color_bit_depth()` at line 583. <details> <summary>Suggested documentation fix</summary> ```diff /** * `@brief`: Returns the corresponding VideoSampling enum value for the given video sampling format. * * `@param` format a string representing the video sampling format * * `@return` the corresponding VideoSampling enum value * - * `@note` exits the application if the video sampling format is not supported + * `@return` VideoSampling::Unknown if the format is not supported */ ``` And similarly for `get_color_bit_depth()`: ```diff /** * `@brief` Returns the corresponding ColorBitDepth enum value for the given bit depth. * * `@param` bit_depth the bit depth to map * * `@return` the corresponding ColorBitDepth enum value * - * `@note` exits the application if the bit depth is not supported + * `@return` ColorBitDepth::Unknown if the bit depth is not supported */ ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@applications/adv_networking_bench/cpp/rivermax_bench_op_tx.h` around lines 566 - 573, The doc comment for get_video_sampling_format incorrectly states it exits on unsupported formats; instead update the comment to state the function returns VideoSampling::Unknown for unsupported formats (and does not exit), and mention that calculate_frame_size() is responsible for handling unknown values and exiting if needed; do the same for get_color_bit_depth() so both function comments accurately reflect that they return an "Unknown" sentinel rather than terminating the application. ``` </details> </blockquote></details> <details> <summary>applications/adv_networking_media_player/cpp/adv_networking_media_player.cpp-401-408 (1)</summary><blockquote> `401-408`: _⚠️ Potential issue_ | _🟡 Minor_ **Fix multi-line string literals flagged by linter.** The cpplint check is failing due to multi-line string literals. Use string concatenation instead. <details> <summary>Proposed fix</summary> ```diff } else { - HOLOSCAN_LOG_ERROR("Number of frames_writer entries must match number of - advanced_network_media_rx entries"); + HOLOSCAN_LOG_ERROR( + "Number of frames_writer entries must match number of " + "advanced_network_media_rx entries"); exit(1); } } else { - HOLOSCAN_LOG_WARN("No output type (write_to_file/visualize) defined. Data will be received - but not processed."); + HOLOSCAN_LOG_WARN( + "No output type (write_to_file/visualize) defined. " + "Data will be received but not processed."); auto mock_receiver = make_operator<ops::MockReceiverOp>( ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@applications/adv_networking_media_player/cpp/adv_networking_media_player.cpp` around lines 401 - 408, The linter flags the broken multi-line string literals used in the HOLOSCAN_LOG_ERROR and HOLOSCAN_LOG_WARN calls; update the calls (HOLOSCAN_LOG_ERROR and HOLOSCAN_LOG_WARN) to use proper C++ string concatenation or a single continuous string (e.g., "Number of frames_writer entries must match number of advanced_network_media_rx entries" and "No output type (write_to_file/visualize) defined. Data will be received but not processed.") so the log messages are not split across lines. ``` </details> </blockquote></details> <details> <summary>operators/advanced_network/advanced_network/managers/rivermax/applications/ano_media_sender.h-42-42 (1)</summary><blockquote> `42-42`: _⚠️ Potential issue_ | _🟡 Minor_ **Fix access specifier indentation and add required blank lines.** Pipeline failures indicate multiple cpplint violations: access specifiers (`public:`, `private:`) should be indented by 1 space inside classes/structs, and should be preceded by blank lines (except the first one). <details> <summary>✏️ Proposed fix for indentation and blank lines</summary> ```diff struct ANOMediaSenderSettings : AppSettings { -public: + public: static constexpr uint32_t DEFAULT_NUM_OF_PACKETS_IN_CHUNK_FHD = 16; ... class ANOMediaSenderSettingsValidator : public ISettingsValidator<ANOMediaSenderSettings> { -public: + public: ReturnStatus validate(const std::shared_ptr<ANOMediaSenderSettings>& settings) const override; }; class ANOMediaSenderApp : public RmaxBaseApp { -private: + private: /* Settings builder pointer */ ... std::unordered_map<size_t, std::pair<size_t, size_t>> m_stream_id_map; -public: + + public: /** ... static uint64_t get_time_ns(void* context = nullptr); -private: + + private: ReturnStatus initialize_app_settings() final; ``` </details> Also applies to: 54-54, 64-64, 81-81, 129-129 <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@operators/advanced_network/advanced_network/managers/rivermax/applications/ano_media_sender.h` at line 42, Indent and blank-line the class/struct access specifiers in ano_media_sender.h: ensure each access label ('public:', 'private:', 'protected:') is indented by a single space (e.g., " public:") and is preceded by a blank line except for the very first access specifier in the class/struct; update each occurrence noted (the 'public:'/'private:' tokens in the file) to follow this style so cpplint no longer flags indentation/spacing violations. ``` </details> </blockquote></details> <details> <summary>operators/advanced_network/advanced_network/managers/rivermax/applications/ano_ipo_receiver.h-36-36 (1)</summary><blockquote> `36-36`: _⚠️ Potential issue_ | _🟡 Minor_ **Fix access specifier indentation and add required blank lines.** Pipeline failures indicate multiple cpplint violations for access specifier formatting. <details> <summary>✏️ Proposed fix for indentation and blank lines</summary> ```diff struct ANOIPOReceiverSettings : AppSettings { -public: + public: static constexpr uint32_t DEFAULT_NUM_OF_PACKETS_IN_CHUNK = 262144; ... class ANOIPOReceiverSettingsValidator : public ISettingsValidator<ANOIPOReceiverSettings> { -public: + public: ReturnStatus validate(const std::shared_ptr<ANOIPOReceiverSettings>& settings) const override; }; class ANOIPOReceiverApp : public RmaxReceiverBaseApp { -private: + private: /* Settings builder pointer */ ... std::unordered_map<size_t, std::pair<size_t, size_t>> m_stream_id_map; -public: + + public: /** ... void distribute_work_for_threads(); -private: + + private: + ReturnStatus initialize_app_settings() final; ``` </details> Also applies to: 51-51, 57-57, 68-68, 103-103 <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@operators/advanced_network/advanced_network/managers/rivermax/applications/ano_ipo_receiver.h` at line 36, Unindent all access specifiers in the header so "public:", "protected:", and "private:" start at column 1 (no leading spaces) and ensure there is a single blank line after each access specifier; update every occurrence of these tokens in ano_ipo_receiver.h (the ones currently indented) so they follow cpplint formatting rules and add the required blank line separation between the specifier and the following member declarations. ``` </details> </blockquote></details> <details> <summary>operators/advanced_network/advanced_network/managers/rivermax/rivermax_mgr_impl/rivermax_config_manager.cpp-274-274 (1)</summary><blockquote> `274-274`: _⚠️ Potential issue_ | _🟡 Minor_ **Fix line length to comply with cpplint (≤100 characters).** <details> <summary>✏️ Proposed fix</summary> ```diff - auto rivermax_rtp_receiver_settings_validator = std::make_shared<ANORTPReceiverSettingsValidator>(); + auto rivermax_rtp_receiver_settings_validator = + std::make_shared<ANORTPReceiverSettingsValidator>(); ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@operators/advanced_network/advanced_network/managers/rivermax/rivermax_mgr_impl/rivermax_config_manager.cpp` at line 274, The line creating rivermax_rtp_receiver_settings_validator with std::make_shared<ANORTPReceiverSettingsValidator>() exceeds cpplint's 100-char limit; split the statement across lines to reduce length (for example place the = and std::make_shared call on the next line or split the template argument), or introduce a shorter typedef/using for the shared_ptr type, ensuring the variable name rivermax_rtp_receiver_settings_validator and the constructor ANORTPReceiverSettingsValidator remain unchanged. ``` </details> </blockquote></details> <details> <summary>operators/advanced_network/advanced_network/managers/rivermax/rivermax_mgr_impl/rivermax_config_manager.cpp-399-404 (1)</summary><blockquote> `399-404`: _⚠️ Potential issue_ | _🟡 Minor_ **Fix line length to comply with cpplint (≤100 characters).** Lines 399 and 404 exceed the 100-character limit. <details> <summary>✏️ Proposed fix</summary> ```diff - if (!rivermax_tx_config.split_boundary && rivermax_tx_config.memory_pool_location == MemoryKind::DEVICE) { + if (!rivermax_tx_config.split_boundary && + rivermax_tx_config.memory_pool_location == MemoryKind::DEVICE) { HOLOSCAN_LOG_ERROR("GPU memory pool is supported only in header-data split mode"); return false; } - auto rivermax_media_sender_settings_validator = std::make_shared<ANOMediaSenderSettingsValidator>(); + auto rivermax_media_sender_settings_validator = + std::make_shared<ANOMediaSenderSettingsValidator>(); ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@operators/advanced_network/advanced_network/managers/rivermax/rivermax_mgr_impl/rivermax_config_manager.cpp` around lines 399 - 404, The two lines exceed the 100-character cpplint limit; break long statements into shorter lines and/or reflow the text: split the if-check/logging block that uses rivermax_tx_config and MemoryKind::DEVICE so the HOLOSCAN_LOG_ERROR call and its message are on a separate indented line, and similarly split the creation of rivermax_media_sender_settings_validator (std::make_shared<ANOMediaSenderSettingsValidator>()) across lines so no single line exceeds 100 characters while preserving the same logic and variable names. ``` </details> </blockquote></details> <details> <summary>operators/advanced_network/advanced_network/managers/rivermax/rivermax_mgr_impl/packet_processor.h-100-100 (1)</summary><blockquote> `100-100`: _⚠️ Potential issue_ | _🟡 Minor_ **Fix line length to comply with cpplint (≤100 characters).** Pipeline failure indicates this line exceeds 100 characters. <details> <summary>✏️ Proposed fix</summary> ```diff - Status process_packets(const PacketsChunkParams& params, size_t& processed_packets, int stream_id) override { + Status process_packets(const PacketsChunkParams& params, + size_t& processed_packets, + int stream_id) override { ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@operators/advanced_network/advanced_network/managers/rivermax/rivermax_mgr_impl/packet_processor.h` at line 100, The declaration of process_packets exceeds the 100-character cpplint limit; shorten the line by breaking the function signature across two lines or using type aliases so it fits ≤100 chars. Edit the method signature for Status process_packets(const PacketsChunkParams& params, size_t& processed_packets, int stream_id) in the class/implementation so it is wrapped (e.g., put parameters on a new line) or rename long parameter identifiers, ensuring the declaration still overrides the base method and keeps the exact function name process_packets and parameter types PacketsChunkParams, size_t& processed_packets, int stream_id. ``` </details> </blockquote></details> <details> <summary>operators/advanced_network/advanced_network/managers/rivermax/applications/ano_ipo_receiver.h-74-74 (1)</summary><blockquote> `74-74`: _⚠️ Potential issue_ | _🟡 Minor_ **Mark single-parameter constructor as `explicit`.** Pipeline failure: Single-parameter constructors should be marked explicit to prevent unintended implicit conversions. <details> <summary>✏️ Proposed fix</summary> ```diff - ANOIPOReceiverApp(std::shared_ptr<ISettingsBuilder<ANOIPOReceiverSettings>> settings_builder); + explicit ANOIPOReceiverApp( + std::shared_ptr<ISettingsBuilder<ANOIPOReceiverSettings>> settings_builder); ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@operators/advanced_network/advanced_network/managers/rivermax/applications/ano_ipo_receiver.h` at line 74, The single-argument constructor ANOIPOReceiverApp(std::shared_ptr<ISettingsBuilder<ANOIPOReceiverSettings>> settings_builder) should be marked explicit to avoid unintended implicit conversions; update the declaration of ANOIPOReceiverApp to add the explicit keyword before the constructor to prevent implicit construction from a shared_ptr and rebuild. ``` </details> </blockquote></details> <details> <summary>operators/advanced_network/advanced_network/managers/rivermax/applications/ano_media_sender.h-174-176 (1)</summary><blockquote> `174-176`: _⚠️ Potential issue_ | _🟡 Minor_ **Fix comment spacing.** Pipeline failures indicate comments should have at least two spaces before them. <details> <summary>✏️ Proposed fix</summary> ```diff -} // namespace holoscan::advanced_network +} // namespace holoscan::advanced_network -#endif // RIVERMAX_ANO_APPLICATIONS_MEDIA_SENDER_H_ +#endif // RIVERMAX_ANO_APPLICATIONS_MEDIA_SENDER_H_ ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@operators/advanced_network/advanced_network/managers/rivermax/applications/ano_media_sender.h` around lines 174 - 176, Adjust the trailing comment spacing so there are at least two spaces before the // comment markers: update the closing namespace comment for namespace holoscan::advanced_network and the include-guard comment for RIVERMAX_ANO_APPLICATIONS_MEDIA_SENDER_H_ to ensure two spaces precede the // markers (e.g., " // namespace holoscan::advanced_network" and " // RIVERMAX_ANO_APPLICATIONS_MEDIA_SENDER_H_"). ``` </details> </blockquote></details> <details> <summary>operators/advanced_network/advanced_network/managers/rivermax/rivermax_mgr_impl/rivermax_config_manager.cpp-241-241 (1)</summary><blockquote> `241-241`: _⚠️ Potential issue_ | _🟡 Minor_ **Fix line length to comply with cpplint (≤100 characters).** <details> <summary>✏️ Proposed fix</summary> ```diff - auto rivermax_ipo_receiver_settings_validator = std::make_shared<ANOIPOReceiverSettingsValidator>(); + auto rivermax_ipo_receiver_settings_validator = + std::make_shared<ANOIPOReceiverSettingsValidator>(); ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@operators/advanced_network/advanced_network/managers/rivermax/rivermax_mgr_impl/rivermax_config_manager.cpp` at line 241, The line creating rivermax_ipo_receiver_settings_validator exceeds cpplint's 100-char limit; break the statement into multiple lines so it fits style rules—for example, declare the variable name and assign std::make_shared<ANOIPOReceiverSettingsValidator>() on the next indented line or use an intermediate type alias; update the statement that creates rivermax_ipo_receiver_settings_validator (the std::make_shared<ANOIPOReceiverSettingsValidator>() call) to be split across lines to keep each line ≤100 chars. ``` </details> </blockquote></details> <details> <summary>operators/advanced_network/advanced_network/managers/rivermax/applications/ano_rtp_receiver.h-36-36 (1)</summary><blockquote> `36-36`: _⚠️ Potential issue_ | _🟡 Minor_ **Fix access specifier indentation and add required blank lines.** Pipeline failures indicate multiple cpplint violations for access specifier formatting. <details> <summary>✏️ Proposed fix for indentation and blank lines</summary> ```diff struct ANORTPReceiverSettings : AppSettings { -public: + public: static constexpr uint32_t DEFAULT_NUM_OF_PACKETS_IN_CHUNK = 262144; ... class ANORTPReceiverSettingsValidator : public ISettingsValidator<ANORTPReceiverSettings> { -public: + public: ReturnStatus validate(const std::shared_ptr<ANORTPReceiverSettings>& settings) const override; }; class ANORTPReceiverApp : public RmaxReceiverBaseApp { -private: + private: /* Settings builder pointer */ ... std::unordered_map<size_t, std::pair<size_t, size_t>> m_stream_id_map; -public: + + public: /** ... */ -private: + + private: ReturnStatus initialize_app_settings() final; ``` </details> Also applies to: 48-48, 54-54, 65-65, 99-99 <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@operators/advanced_network/advanced_network/managers/rivermax/applications/ano_rtp_receiver.h` at line 36, Remove leading indentation on access specifier labels and ensure they are followed by a single blank line: locate the access labels (public:, protected:, private:) in ano_rtp_receiver.h and change them to start at column 1 (no leading spaces/tabs) and add one blank line immediately after each label so member declarations are separated (this should be applied to all occurrences noted around lines 36, 48, 54, 65, 99). ``` </details> </blockquote></details> <details> <summary>operators/advanced_network/advanced_network/managers/rivermax/applications/ano_media_sender.h-87-87 (1)</summary><blockquote> `87-87`: _⚠️ Potential issue_ | _🟡 Minor_ **Mark single-parameter constructor as `explicit`.** Pipeline failure: Single-parameter constructors should be marked explicit to prevent unintended implicit conversions. <details> <summary>✏️ Proposed fix</summary> ```diff - ANOMediaSenderApp(std::shared_ptr<ISettingsBuilder<ANOMediaSenderSettings>> settings_builder); + explicit ANOMediaSenderApp( + std::shared_ptr<ISettingsBuilder<ANOMediaSenderSettings>> settings_builder); ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@operators/advanced_network/advanced_network/managers/rivermax/applications/ano_media_sender.h` at line 87, The single-argument constructor ANOMediaSenderApp(std::shared_ptr<ISettingsBuilder<ANOMediaSenderSettings>> settings_builder) should be marked explicit to avoid unintended implicit conversions; update the declaration of ANOMediaSenderApp in the class to use explicit and also update any out-of-class definition (the ANOMediaSenderApp(...) definition) to match the explicit declaration signature so the compiler remains consistent. ``` </details> </blockquote></details> <details> <summary>operators/advanced_network/advanced_network/managers/rivermax/applications/ano_rtp_receiver.h-71-71 (1)</summary><blockquote> `71-71`: _⚠️ Potential issue_ | _🟡 Minor_ **Mark single-parameter constructor as `explicit`.** Pipeline failure: Single-parameter constructors should be marked explicit to prevent unintended implicit conversions. <details> <summary>✏️ Proposed fix</summary> ```diff - ANORTPReceiverApp(std::shared_ptr<ISettingsBuilder<ANORTPReceiverSettings>> settings_builder); + explicit ANORTPReceiverApp( + std::shared_ptr<ISettingsBuilder<ANORTPReceiverSettings>> settings_builder); ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@operators/advanced_network/advanced_network/managers/rivermax/applications/ano_rtp_receiver.h` at line 71, The single-parameter constructor ANORTPReceiverApp(std::shared_ptr<ISettingsBuilder<ANORTPReceiverSettings>> settings_builder) should be marked explicit to prevent unintended implicit conversions; change the constructor declaration for ANORTPReceiverApp to use the explicit keyword (i.e., explicit ANORTPReceiverApp(...)) so the class constructor is not eligible for implicit conversions. ``` </details> </blockquote></details> <details> <summary>operators/advanced_network/advanced_network/managers/rivermax/rivermax_mgr_impl/packet_processor.h-75-75 (1)</summary><blockquote> `75-75`: _⚠️ Potential issue_ | _🟡 Minor_ **Fix line length to comply with cpplint (≤100 characters).** Pipeline failure indicates this line exceeds 100 characters. <details> <summary>✏️ Proposed fix</summary> ```diff - virtual Status process_packets(const PacketsChunkParams& params, size_t& processed_packets, int stream_id) = 0; + virtual Status process_packets(const PacketsChunkParams& params, + size_t& processed_packets, + int stream_id) = 0; ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@operators/advanced_network/advanced_network/managers/rivermax/rivermax_mgr_impl/packet_processor.h` at line 75, The declaration of process_packets in packet_processor.h is over 100 characters; split the function signature across multiple lines to reduce line length so it meets cpplint. For example, put the return type and function name on one line and move the parameter list (PacketsChunkParams, size_t& processed_packets, int stream_id) onto one or two subsequent indented lines, keeping the pure virtual "= 0;" at the end; update the declaration of virtual Status process_packets(...) accordingly so the symbol names (process_packets, PacketsChunkParams) remain unchanged. ``` </details> </blockquote></details> <details> <summary>operators/advanced_network/advanced_network/managers/rivermax/applications/ano_media_sender.cpp-194-200 (1)</summary><blockquote> `194-200`: _⚠️ Potential issue_ | _🟡 Minor_ **Fix the cpplint issues blocking this file.** Lines 196, 248, and 318 match the current lint failures, so the pipeline stays red until these whitespace fixes land. <details> <summary>🔧 Minimal fixes</summary> ```diff ReturnStatus ANOMediaSenderApp::set_rivermax_clock() { ReturnStatus rc = set_rivermax_ptp_clock(&m_device_interface); - if(rc == ReturnStatus::success) { + if (rc == ReturnStatus::success) { uint64_t ptp_time = 0; rc = get_rivermax_ptp_time_ns(ptp_time); } @@ auto network_address = FourTupleFlow( thread_idx, m_app_settings->local_ip, m_app_settings->source_port, m_app_settings->destination_ip, - m_app_settings->destination_port - ); + m_app_settings->destination_port); @@ -} // namespace holoscan::advanced_network +} // namespace holoscan::advanced_network ``` </details> Also applies to: 242-248, 318-318 <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@operators/advanced_network/advanced_network/managers/rivermax/applications/ano_media_sender.cpp` around lines 194 - 200, The file has cpplint whitespace violations around control statements and operators (e.g. in ANOMediaSenderApp::set_rivermax_clock where "if(rc == ReturnStatus::success)" and similar occurrences fail lint); update spacing to match style (add a space after "if", ensure a space before/after binary operators, keep consistent single blank lines) for the offending symbols/functions: ANOMediaSenderApp::set_rivermax_clock, the use of set_rivermax_ptp_clock, get_rivermax_ptp_time_ns and the local variable ptp_time; apply the same spacing fixes to the other reported locations (around lines referenced 242-248 and 318) so cpplint passes. ``` </details> </blockquote></details> <details> <summary>operators/advanced_network/advanced_network/managers/rivermax/applications/ano_rtp_receiver.cpp-62-68 (1)</summary><blockquote> `62-68`: _⚠️ Potential issue_ | _🟡 Minor_ **Fix the constructor brace placement to unblock cpplint.** The opening brace on Line 68 matches the current lint failure for this file. <details> <summary>🔧 Minimal fix</summary> ```diff ANORTPReceiverApp::ANORTPReceiverApp( std::shared_ptr<ISettingsBuilder<ANORTPReceiverSettings>> settings_builder) : RmaxReceiverBaseApp(), m_settings_builder(std::move(settings_builder)), m_threads_streams(), - m_devices_ips() -{ + m_devices_ips() { m_num_paths_per_stream = 1; // RTP receiver supports only single path per stream } ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@operators/advanced_network/advanced_network/managers/rivermax/applications/ano_rtp_receiver.cpp` around lines 62 - 68, The opening brace for the constructor ANORTPReceiverApp(...) is on its own line causing a cpplint style failure; move the brace to the end of the constructor signature line so the definition reads ANORTPReceiverApp(...) : RmaxReceiverBaseApp(), ... { and keep the member initializer list and body unchanged (adjust the brace placement in the ANORTPReceiverApp constructor only). ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🧹 Nitpick comments (7)</summary><blockquote> <details> <summary>applications/adv_networking_bench/cpp/rivermax_bench_op_tx.h (2)</summary><blockquote> `614-627`: **Consider marking `shutdown()` as `[[noreturn]]` for better static analysis.** Since `shutdown()` calls `exit(1)` and never returns, marking it with `[[noreturn]]` would: 1. Help the compiler optimize code paths 2. Suppress potential warnings about using iterators after error checks 3. Provide better documentation of the function's behavior <details> <summary>Suggested fix for shutdown() declaration</summary> ```diff -void shutdown(std::string message) { +[[noreturn]] void shutdown(const std::string& message) { advanced_network::shutdown(); HOLOSCAN_LOG_ERROR("{}", message); exit(1); } ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@applications/adv_networking_bench/cpp/rivermax_bench_op_tx.h` around lines 614 - 627, The shutdown() function should be annotated as non-returning to aid static analysis and optimization; update its declaration and definition (the shutdown(...) symbol used in rivermax_bench_op_tx.h) to include the [[noreturn]] attribute (or compiler-specific equivalent) so callers like the checks around COLOR_DEPTH_TO_PIXEL_RATIO and depth_it are recognized as terminating; ensure you change all forward declarations/definitions of shutdown() in the codebase to match and rebuild. ``` </details> --- `543-544`: **Minor formatting inconsistency.** Missing space before closing brace for consistency with typical enum formatting. <details> <summary>Suggested fix</summary> ```diff - enum class VideoSampling { RGB, YCbCr_4_2_2, YCbCr_4_2_0, YCbCr_4_4_4, Unknown}; - enum class ColorBitDepth { _8, _10, _12, Unknown}; + enum class VideoSampling { RGB, YCbCr_4_2_2, YCbCr_4_2_0, YCbCr_4_4_4, Unknown }; + enum class ColorBitDepth { _8, _10, _12, Unknown }; ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@applications/adv_networking_bench/cpp/rivermax_bench_op_tx.h` around lines 543 - 544, The enum declarations VideoSampling and ColorBitDepth have no space before the closing brace (e.g., "Unknown}"); update both enum class lines to add a space before the closing brace so they read "Unknown }" (or follow the project's enum formatting style) to ensure consistent formatting across the file. ``` </details> </blockquote></details> <details> <summary>applications/adv_networking_media_player/cpp/adv_networking_media_player.cpp (1)</summary><blockquote> `394-395`: **Add explicit port mapping in `add_flow` for clarity and robustness.** The `add_flow` call lacks explicit port mapping. While this may work if both operators have single default ports, explicit mapping prevents potential runtime failures if operator interfaces change. <details> <summary>Proposed fix</summary> ```diff if (adv_net_media_rx_map.find(stream_name) != adv_net_media_rx_map.end()) { - add_flow(adv_net_media_rx_map[stream_name], frames_writer); + add_flow(adv_net_media_rx_map[stream_name], frames_writer, + {{"out_video_buffer", "input"}}); } else { ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@applications/adv_networking_media_player/cpp/adv_networking_media_player.cpp` around lines 394 - 395, The add_flow call for adv_net_media_rx_map[stream_name] -> frames_writer lacks explicit port mapping which can break if operator ports change; update the call in adv_networking_media_player.cpp where add_flow(adv_net_media_rx_map[stream_name], frames_writer) is invoked to pass an explicit port mapping (source output port name/index and destination input port name/index) so the correct output port of the adv_net_media_rx operator is wired to the correct input port of frames_writer; use the actual port identifiers used by those operators (e.g., "output"/"input" or their numeric indices) when constructing the mapping. ``` </details> </blockquote></details> <details> <summary>operators/advanced_network/advanced_network/managers/rivermax/applications/utils.h (1)</summary><blockquote> `27-43`: **Consider initializing primitive member variables to avoid undefined values.** The `source_port`, `destination_port`, and `stream_id` fields lack default initializers. While they may be immediately assigned after construction, uninitialized primitives can cause subtle bugs if accessed before assignment. <details> <summary>✏️ Proposed fix</summary> ```diff struct StreamNetworkSettings { std::string local_ip; std::vector<std::string> local_ips; std::string source_ip; std::vector<std::string> source_ips; std::string destination_ip; std::vector<std::string> destination_ips; - uint16_t source_port; + uint16_t source_port = 0; std::vector<uint16_t> destination_ports; - uint16_t destination_port; - uint32_t stream_id; + uint16_t destination_port = 0; + uint32_t stream_id = 0; }; struct ThreadSettings { std::vector<StreamNetworkSettings> stream_network_settings; - int thread_id; + int thread_id = 0; }; ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@operators/advanced_network/advanced_network/managers/rivermax/applications/utils.h` around lines 27 - 43, The structs StreamNetworkSettings and ThreadSettings contain primitive members that are not default-initialized; specifically initialize source_port, destination_port, and stream_id in StreamNetworkSettings (e.g., to 0) to avoid undefined values and consider initializing thread_id in ThreadSettings as well; update the StreamNetworkSettings and ThreadSettings definitions so their primitive fields have sensible default values (e.g., = 0) to ensure safe construction and predictable behavior. ``` </details> </blockquote></details> <details> <summary>operators/advanced_network/advanced_network/managers/rivermax/applications/ano_rtp_receiver.h (1)</summary><blockquote> `21-21`: **Consider removing unused `<chrono>` include.** The `<chrono>` header is included but no chrono types appear to be used in this header file. If it's needed by the implementation file, move the include there. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@operators/advanced_network/advanced_network/managers/rivermax/applications/ano_rtp_receiver.h` at line 21, The header includes <chrono> but no chrono types are used in ano_rtp_receiver.h; remove the unused `#include` <chrono> from this header (or if chrono types are used only in implementation, add the include to the corresponding .cpp where functions like class AnoRtpReceiver or methods declared in this header are defined) to avoid unnecessary dependency and compile overhead. ``` </details> </blockquote></details> <details> <summary>operators/advanced_network/advanced_network/managers/rivermax/rivermax_mgr_impl/rivermax_config_manager.cpp (1)</summary><blockquote> `855-877`: **Remove unused variable `to_reset_cores_vector`.** The variable `to_reset_cores_vector` is declared on line 858 but never used in the `validate_cores` function. This appears to be leftover from copy-pasting from `parse_and_set_cores`. <details> <summary>✏️ Proposed fix</summary> ```diff int ConfigManagerUtilities::validate_cores(const std::string& cores) { std::istringstream iss(cores); std::string coreStr; - bool to_reset_cores_vector = true; int num_cores = 0; while (std::getline(iss, coreStr, ',')) { try { ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@operators/advanced_network/advanced_network/managers/rivermax/rivermax_mgr_impl/rivermax_config_manager.cpp` around lines 855 - 877, Remove the unused local variable to_reset_cores_vector from the validate_cores function; locate the variable declaration in validate_cores (currently declared as bool to_reset_cores_vector = true) and delete it so the function only keeps variables actually used (iss, coreStr, num_cores), ensuring the function still compiles and behavior remains unchanged; this removes the leftover from parse_and_set_cores. ``` </details> </blockquote></details> <details> <summary>operators/advanced_network/advanced_network/managers/rivermax/applications/ano_ipo_receiver.h (1)</summary><blockquote> `21-21`: **Consider removing unused `<chrono>` include.** The `<chrono>` header is included but no chrono types appear to be used in this header file. If it's needed by the implementation file, move the include there. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@operators/advanced_network/advanced_network/managers/rivermax/applications/ano_ipo_receiver.h` at line 21, Remove the unused `#include` <chrono> from the header ano_ipo_receiver.h (the include line that currently pulls in <chrono>) and, if any chrono types are actually used only in the implementation, add the <chrono> include to the corresponding .cpp implementation file (where functions or methods defined for classes declared in ano_ipo_receiver.h, e.g., the ANO_IPO_Receiver class or related methods, use chrono types); this keeps the header minimal and moves implementation-only dependencies into the translation unit. ``` </details> </blockquote></details> </blockquote></details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: Organization UI **Review profile**: CHILL **Plan**: Pro **Run ID**: `df0aa913-6040-4a69-8f04-6c5a2bcf0aa1` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 00c606b12f0d8068d332a52ada177dcbb598c7ad and 6e2f79636d45af4e4f5b6d8e0fd2913a77d11262. </details> <details> <summary>📒 Files selected for processing (17)</summary> * `applications/adv_networking_bench/adv_networking_bench_rivermax_rx.yaml` * `applications/adv_networking_bench/cpp/rivermax_bench_op_tx.h` * `applications/adv_networking_media_player/README.md` * `applications/adv_networking_media_player/cpp/adv_networking_media_player.cpp` * `operators/advanced_network/advanced_network/managers/rivermax/applications/CMakeLists.txt` * `operators/advanced_network/advanced_network/managers/rivermax/applications/ano_ipo_receiver.cpp` * `operators/advanced_network/advanced_network/managers/rivermax/applications/ano_ipo_receiver.h` * `operators/advanced_network/advanced_network/managers/rivermax/applications/ano_media_sender.cpp` * `operators/advanced_network/advanced_network/managers/rivermax/applications/ano_media_sender.h` * `operators/advanced_network/advanced_network/managers/rivermax/applications/ano_rtp_receiver.cpp` * `operators/advanced_network/advanced_network/managers/rivermax/applications/ano_rtp_receiver.h` * `operators/advanced_network/advanced_network/managers/rivermax/applications/utils.h` * `operators/advanced_network/advanced_network/managers/rivermax/rivermax_mgr_impl/packet_processor.h` * `operators/advanced_network/advanced_network/managers/rivermax/rivermax_mgr_impl/rivermax_chunk_consumer_ano.h` * `operators/advanced_network/advanced_network/managers/rivermax/rivermax_mgr_impl/rivermax_config_manager.cpp` * `operators/advanced_network/advanced_network/managers/rivermax/rivermax_mgr_impl/rivermax_config_manager.h` * `operators/advanced_network/advanced_network/managers/rivermax/rivermax_service/CMakeLists.txt` </details> <details> <summary>✅ Files skipped from review due to trivial changes (1)</summary> * operators/advanced_network/advanced_network/managers/rivermax/applications/CMakeLists.txt </details> <details> <summary>🚧 Files skipped from review as they are similar to previous changes (2)</summary> * operators/advanced_network/advanced_network/managers/rivermax/rivermax_mgr_impl/rivermax_config_manager.h * applications/adv_networking_bench/adv_networking_bench_rivermax_rx.yaml </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Comment on lines
355
to
365
| if (!rx_service->initialize()) { | ||
| HOLOSCAN_LOG_ERROR("Failed to initialize RX service"); | ||
| return false; | ||
| } | ||
|
|
||
| rx_services_[service_id] = std::move(rx_service); | ||
|
|
||
| // Apply burst pool adaptive dropping configuration | ||
| apply_burst_pool_configuration_to_service(service_id); | ||
|
|
||
| return true; |
Contributor
There was a problem hiding this comment.
rx_service undeclared, compile error
Every case in the switch block at lines 336–353 executes a return, so lines 355–365 are unreachable. Worse, the variable rx_service is no longer declared anywhere in this function (it was removed along with the old if/else chain), so the compiler will reject this translation unit with an "undeclared identifier" error before it ever reaches the unreachable-code analysis.
The practical consequence is that apply_burst_pool_configuration_to_service(service_id) is never called, meaning the adaptive burst-dropping feature (low/critical/recovery thresholds configured in YAML) is silently inactive at runtime.
The dead block should be removed, and apply_burst_pool_configuration_to_service(service_id) should be called inside common_initialize_rx_service after rx_services_[service_id] = std::move(rx_service):
// In common_initialize_rx_service, before the final return true:
rx_services_[service_id] = std::move(rx_service);
apply_burst_pool_configuration_to_service(service_id);
return true;Then delete lines 355–365 from initialize_rx_service.
Comment on lines
+51
to
+52
| burst_pool_recovery_threshold_percent(other.burst_pool_recovery_threshold_percent) {} | ||
| thread_settings(other.thread_settings) {} |
Contributor
There was a problem hiding this comment.
The copy constructor is terminated prematurely with {} on line 51 and then thread_settings(other.thread_settings) {} appears as a stray statement outside the constructor body. The compiler will reject this as a syntax error.
The fix is to move the comma after burst_pool_recovery_threshold_percent and remove the premature {}:
Suggested change
| burst_pool_recovery_threshold_percent(other.burst_pool_recovery_threshold_percent) {} | |
| thread_settings(other.thread_settings) {} | |
| burst_pool_recovery_threshold_percent(other.burst_pool_recovery_threshold_percent), | |
| thread_settings(other.thread_settings) {} |
Comment on lines
+555
to
+559
| if (should_drop_burst_due_to_capacity()) { | ||
| // Drop the completed burst by returning it to memory pool instead of enqueuing to output | ||
| // queue (counter is already incremented inside should_drop_burst_due_to_capacity) | ||
| rx_bursts_mempool_->enqueue_burst(cur_out_burst_); | ||
| reset_current_burst(); |
Contributor
There was a problem hiding this comment.
stream_id — compile errors in burst drop path
Inside enqueue_and_reset_current_burst(int stream_id), the burst-drop path contains two errors:
- Line 558:
rx_bursts_mempool_->enqueue_burst(cur_out_burst_)passes the wholestd::unordered_map<int, std::shared_ptr<RivermaxBurst>>where astd::shared_ptr<RivermaxBurst>is expected — this is a type error that will not compile. - Line 559:
reset_current_burst()is called with no argument, but the only overload now requires astream_idparameter.
Both will prevent the translation unit from compiling. The correct fix:
Suggested change
| if (should_drop_burst_due_to_capacity()) { | |
| // Drop the completed burst by returning it to memory pool instead of enqueuing to output | |
| // queue (counter is already incremented inside should_drop_burst_due_to_capacity) | |
| rx_bursts_mempool_->enqueue_burst(cur_out_burst_); | |
| reset_current_burst(); | |
| rx_bursts_mempool_->enqueue_burst(cur_out_burst_[stream_id]); | |
| reset_current_burst(stream_id); |
danal97
force-pushed
the
october
branch
2 times, most recently
from
406a820 to
8f2e32b
Compare
danal97
force-pushed
the
october
branch
2 times, most recently
from
78c7e46 to
2260601
Compare
for more information, see https://pre-commit.ci
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (9)
operators/advanced_network/advanced_network/managers/rivermax/rivermax_service/CMakeLists.txt (1)
76-85:⚠️ Potential issue | 🟠 MajorPatch both replacements against the same buffer.
This reintroduces the earlier bug: if the first
if()does not run, Line 83 reads${NEW_CONTENTS}before it is set, and Line 84 can write an emptyrmax_base_app.h.Suggested patch
# Will be removed once RDK makes the change file(READ "${rivermax-dev-kit_SOURCE_DIR}/source/apps/include/rdk/apps/rmax_base_app.h" CONTENTS) +set(PATCHED_CONTENTS "${CONTENTS}") if(NOT CONTENTS MATCHES "virtual ReturnStatus find_internal_stream_index\\(") - string(REPLACE "ReturnStatus find_internal_stream_index(" "virtual ReturnStatus find_internal_stream_index(" NEW_CONTENTS "${CONTENTS}") - file(WRITE "${rivermax-dev-kit_SOURCE_DIR}/source/apps/include/rdk/apps/rmax_base_app.h" "${NEW_CONTENTS}") + string(REPLACE "ReturnStatus find_internal_stream_index(" "virtual ReturnStatus find_internal_stream_index(" PATCHED_CONTENTS "${PATCHED_CONTENTS}") endif() -if(NOT CONTENTS MATCHES "virtual ReturnStatus get_app_settings\\(") - string(REPLACE "ReturnStatus get_app_settings(" "virtual ReturnStatus get_app_settings(" NEW_CONTENTS2 "${NEW_CONTENTS}") - file(WRITE "${rivermax-dev-kit_SOURCE_DIR}/source/apps/include/rdk/apps/rmax_base_app.h" "${NEW_CONTENTS2}") +if(NOT PATCHED_CONTENTS MATCHES "virtual ReturnStatus get_app_settings\\(") + string(REPLACE "ReturnStatus get_app_settings(" "virtual ReturnStatus get_app_settings(" PATCHED_CONTENTS "${PATCHED_CONTENTS}") endif() +file(WRITE "${rivermax-dev-kit_SOURCE_DIR}/source/apps/include/rdk/apps/rmax_base_app.h" "${PATCHED_CONTENTS}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@operators/advanced_network/advanced_network/managers/rivermax/rivermax_service/CMakeLists.txt` around lines 76 - 85, The two string replacements are applied to different variables causing NEW_CONTENTS to be used before set and potentially writing an empty file; change the logic to operate on a single buffer (e.g., initialize a buffer variable from CONTENTS, update that buffer for each replacement and write it once) so both checks/str-replace operations use the same up-to-date buffer instead of separate NEW_CONTENTS/NEW_CONTENTS2, and only call file(WRITE) with the final buffer; key symbols: file(READ), CONTENTS, NEW_CONTENTS, NEW_CONTENTS2, and the two if() blocks that match "find_internal_stream_index(" and "get_app_settings(".operators/advanced_network/advanced_network/managers/rivermax/rivermax_mgr_impl/rivermax_mgr_service.cpp (2)
371-378:⚠️ Potential issue | 🔴 CriticalCritical: Calling cudaFree on failed cudaHostAlloc allocation.
When
cudaHostAllocfails,pool_bufferwas never successfully allocated (it remains uninitialized or null). CallingcudaFree(pool_buffer)is undefined behavior. Additionally,cudaFreeHostshould be used for host-pinned memory, notcudaFree.🐛 Proposed fix
} else if (media_sender_builder_->memory_pool_location_ == MemoryKind::HOST_PINNED) { cudaError_t cuda_status = cudaHostAlloc(&pool_buffer, pool_buffer_size, cudaHostAllocDefault); if (cuda_status != cudaSuccess) { HOLOSCAN_LOG_ERROR("Failed to allocate pinned host memory: {}", cudaGetErrorString(cuda_status)); - cudaFree(pool_buffer); return nullptr; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@operators/advanced_network/advanced_network/managers/rivermax/rivermax_mgr_impl/rivermax_mgr_service.cpp` around lines 371 - 378, The error handling after media_sender_builder_->memory_pool_location_ == MemoryKind::HOST_PINNED is incorrect: do not call cudaFree(pool_buffer) when cudaHostAlloc fails because pool_buffer was never allocated and pinned host memory must be freed with cudaFreeHost when allocated; instead remove the cudaFree(pool_buffer) on failure and ensure pool_buffer is initialized (e.g., nullptr) before calling cudaHostAlloc, and where the allocated pinned buffer is later freed use cudaFreeHost(pool_buffer) (only if allocation succeeded) rather than cudaFree; keep the existing cudaGetErrorString(cuda_status) log but only attempt any free when pool_buffer is valid.
126-137:⚠️ Potential issue | 🔴 CriticalCritical: Use-after-free with shared_ptr raw pointer extraction.
Same issue as in
adv_network_rivermax_mgr.cpp:dequeue_burst().get()returns a raw pointer from a temporary shared_ptr that is destroyed immediately, leaving*burstas a dangling pointer.operators/advanced_network/advanced_network/managers/rivermax/rivermax_mgr_impl/adv_network_rivermax_mgr.cpp (1)
587-591:⚠️ Potential issue | 🔴 CriticalCritical: Use-after-free with shared_ptr raw pointer extraction.
dequeue_burst()returns ashared_ptr<RivermaxBurst>. Calling.get()and assigning to*burstresults in the shared_ptr being destroyed at statement end, potentially freeing the burst while the caller still holds a raw pointer.This was flagged in a previous review and remains unaddressed.
🐛 Suggested fix: Preserve shared_ptr lifetime
Consider one of these approaches:
- Store the shared_ptr in a container that extends its lifetime
- Change the API to return
shared_ptr<BurstParams>instead of raw pointer- Transfer ownership semantics to the queue (release from shared_ptr management)
// Example: Store in member to extend lifetime auto out_burst_shared = queue_it->second[stream]->dequeue_burst(); if (out_burst_shared == nullptr) { return Status::NULL_PTR; } // Store shared_ptr somewhere to keep it alive active_bursts_[service_id].push_back(out_burst_shared); *burst = out_burst_shared.get();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@operators/advanced_network/advanced_network/managers/rivermax/rivermax_mgr_impl/adv_network_rivermax_mgr.cpp` around lines 587 - 591, The code extracts a raw pointer from the shared_ptr returned by queue->dequeue_burst() (out_burst_shared.get()) then lets out_burst_shared go out of scope, causing a use-after-free; change this by preserving the shared_ptr lifetime instead of returning a raw pointer: capture the shared_ptr from dequeue_burst() (dequeue_burst, out_burst_shared) and store it in a member container (e.g., active_bursts_ keyed by the service/stream or similar) so the object stays alive while callers use the raw pointer (assign *burst = out_burst_shared.get()), and ensure you remove/cleanup that stored shared_ptr when the burst is processed/returned; alternatively, change the API to return a shared_ptr<RivermaxBurst> from the method that currently expects a raw burst pointer.operators/advanced_network/advanced_network/managers/rivermax/applications/ano_rtp_receiver.cpp (1)
162-171:⚠️ Potential issue | 🔴 CriticalCritical:
reserve()followed by direct indexing causes undefined behavior.
reserve()only allocates capacity without creating elements. Direct indexing at line 167 (m_streams_per_thread[thread_index] = ...) accesses uninitialized memory.The past review marked this as addressed, but the issue persists.
🐛 Proposed fix: Use resize() instead of reserve()
void ANORTPReceiverApp::distribute_work_for_threads() { - m_streams_per_thread.reserve(m_rtp_receiver_settings->thread_settings.size()); + m_streams_per_thread.resize(m_rtp_receiver_settings->thread_settings.size()); m_rtp_receiver_settings->num_of_total_streams = 0; int thread_index = 0; for (const auto& thread : m_rtp_receiver_settings->thread_settings) { m_streams_per_thread[thread_index] = thread.stream_network_settings.size();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@operators/advanced_network/advanced_network/managers/rivermax/applications/ano_rtp_receiver.cpp` around lines 162 - 171, In distribute_work_for_threads(), m_streams_per_thread is reserved but not sized, so the direct indexing m_streams_per_thread[thread_index] causes undefined behavior; replace the reserve call with resize (or otherwise ensure m_streams_per_thread has thread_settings.size() elements) before the loop, then populate elements and update m_rtp_receiver_settings->num_of_total_streams based on each thread.stream_network_settings.size(); ensure the code references the same members (m_streams_per_thread, m_rtp_receiver_settings, thread_settings, stream_network_settings) and preserves the thread_index logic.operators/advanced_network/advanced_network/managers/rivermax/applications/ano_ipo_receiver.cpp (1)
182-189:⚠️ Potential issue | 🔴 CriticalResize
m_streams_per_threadbefore indexing it.
reserve()only changes capacity. Line 187 writes throughm_streams_per_thread[thread_index]while the vector still has size 0, which is undefined behavior.Patch
- m_streams_per_thread.reserve(m_ipo_receiver_settings->thread_settings.size()); + m_streams_per_thread.resize(m_ipo_receiver_settings->thread_settings.size());#!/bin/bash set -euo pipefail file='operators/advanced_network/advanced_network/managers/rivermax/applications/ano_ipo_receiver.cpp' sed -n '182,189p' "$file"Expected: the snippet shows
reserve(...)followed by an indexed write tom_streams_per_thread[...].🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@operators/advanced_network/advanced_network/managers/rivermax/applications/ano_ipo_receiver.cpp` around lines 182 - 189, In ANOIPOReceiverApp::distribute_work_for_threads the vector m_streams_per_thread is reserved but not resized before writing via m_streams_per_thread[thread_index]; change the code to resize m_streams_per_thread to m_ipo_receiver_settings->thread_settings.size() (or otherwise ensure its size is at least thread count) before the loop, then continue assigning m_streams_per_thread[thread_index] and updating m_ipo_receiver_settings->num_of_total_streams as before.applications/adv_networking_media_player/cpp/adv_networking_media_player.cpp (1)
342-345:⚠️ Potential issue | 🔴 CriticalUse the actual string-keyed stream lookup in the visualize path.
adv_net_media_rx_mapis keyed bystd::string, soadv_net_media_rx_map[0]is the wrong access. In this single-stream branch,begin()->secondis enough after the existing empty check.Patch
- add_flow(adv_net_media_rx_map[0], visualizer, {{"out_video_buffer", "receivers"}}); + add_flow(adv_net_media_rx_map.begin()->second, + visualizer, + {{"out_video_buffer", "receivers"}});#!/bin/bash set -euo pipefail file='applications/adv_networking_media_player/cpp/adv_networking_media_player.cpp' echo "Map declaration:" sed -n '342,345p' "$file" echo echo "Visualize branch lookup:" sed -n '378,382p' "$file"Expected: the declaration shows a
std::unordered_map<std::string, ...>, while the visualize branch indexes it with[0].Also applies to: 375-382
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@applications/adv_networking_media_player/cpp/adv_networking_media_player.cpp` around lines 342 - 345, The visualize branch incorrectly indexes adv_net_media_rx_map with adv_net_media_rx_map[0] even though the map is keyed by std::string; after verifying the empty check on adv_net_media_rx_map, replace the numeric indexing with a string-keyed lookup or simply use adv_net_media_rx_map.begin()->second in the single-stream branch (referencing adv_net_media_rx_map and multi_streams_adv_net_media_rx_yaml) so the code retrieves the first mapped std::shared_ptr<ops::AdvNetworkMediaRxOp> correctly for visualization.operators/advanced_network/advanced_network/managers/rivermax/applications/ano_media_sender.cpp (2)
73-85:⚠️ Potential issue | 🟠 MajorDon’t infer
num_of_packets_in_chunk_specifiedfrom the current value.
init_default_values()seeds the FHD chunk count on Line 26. When a UHD config leavesnum_of_packets_in_chunkunset, Lines 83-85 still see that FHD default, mark it as explicitly specified, and prevent the UHD default from being applied.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@operators/advanced_network/advanced_network/managers/rivermax/applications/ano_media_sender.cpp` around lines 73 - 85, post_load_settings() must not infer m_app_settings->num_of_packets_in_chunk_specified by comparing the current value to a computed default (because init_default_values() seeds an FHD value). Remove the value-based check and instead rely on an explicit "was this field provided by the config" indicator set during parsing (or introduce one if missing); only set m_app_settings->num_of_packets_in_chunk_specified = true when that explicit parsed-flag is true, and otherwise choose the chunk default based on resolution and leave specified=false. Reference: ANOMediaSenderApp::post_load_settings, m_app_settings->num_of_packets_in_chunk, m_app_settings->num_of_packets_in_chunk_specified, and init_default_values().
207-214:⚠️ Potential issue | 🔴 CriticalResize
m_streams_per_threadbefore indexed writes.
reserve()only allocates capacity. Line 213 writesm_streams_per_thread[thread_idx]while the vector still has size 0.Patch
- m_streams_per_thread.reserve(m_media_sender_settings->thread_settings.size()); + m_streams_per_thread.resize(m_media_sender_settings->thread_settings.size());#!/bin/bash set -euo pipefail file='operators/advanced_network/advanced_network/managers/rivermax/applications/ano_media_sender.cpp' sed -n '207,214p' "$file"Expected: the snippet shows
reserve(...)followed by an indexed write tom_streams_per_thread[...].🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@operators/advanced_network/advanced_network/managers/rivermax/applications/ano_media_sender.cpp` around lines 207 - 214, In distribute_work_for_threads(), m_streams_per_thread is reserved but not resized so the indexed write m_streams_per_thread[thread_idx] is out-of-bounds; change the allocation to set the vector size (e.g., call resize with m_media_sender_settings->thread_settings.size()) before the loop or replace the indexed assignment with push_back, ensuring m_streams_per_thread has capacity and valid indices when writing; update references to m_streams_per_thread, thread_idx, and the loop over m_media_sender_settings->thread_settings accordingly.
🧹 Nitpick comments (8)
applications/adv_networking_bench/adv_networking_bench_rivermax_tx.yaml (1)
73-85: Consider collapsing both streams onto the same bench TX thread.As written, this sample still assigns one stream per
thread_id, so it doesn't actually exercise the new multi-streams-per-thread path this PR adds. If this YAML is meant to validate the feature manually, put bothnetwork_settingsentries under the same thread.Suggested patch
tx_threads: - thread_id: 0 network_settings: - stream_id: 0 local_ip_address: 1.1.164.19 destination_ip_address: 224.1.50.1 destination_port: 50505 - - thread_id: 1 - network_settings: - stream_id: 1 local_ip_address: 1.1.164.19 destination_ip_address: 224.1.50.2 destination_port: 50505🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@applications/adv_networking_bench/adv_networking_bench_rivermax_tx.yaml` around lines 73 - 85, The YAML currently assigns each stream to its own tx_threads entry (thread_id: 0 and thread_id: 1) so it doesn't exercise multi-streams-per-thread; collapse both network_settings entries under the same tx_threads item (e.g., put the stream_id: 0 and stream_id: 1 blocks both inside the same tx_threads entry for thread_id: 0) so a single thread_id contains multiple network_settings entries, keeping stream_id, local_ip_address, destination_ip_address, and destination_port intact.applications/adv_networking_bench/cpp/rivermax_bench_op_tx.h (1)
414-420: Make this header helperinlineor move it to a.cpp.A non-
inlinefree function defined in a header will turn into an ODR/multiple-definition problem as soon as a second translation unit includesrivermax_bench_op_tx.h.Suggested patch
-void shutdown(std::string message) { +[[noreturn]] inline void shutdown(const std::string& message) { HOLOSCAN_LOG_ERROR("{}", message); if (advanced_network::is_initialized()) { advanced_network::shutdown(); } exit(1); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@applications/adv_networking_bench/cpp/rivermax_bench_op_tx.h` around lines 414 - 420, The free function shutdown(std::string message) defined in the header will cause ODR/multiple-definition errors when included by multiple translation units; either mark the function inline (e.g., inline void shutdown(std::string message) { ... }) or move its definition into a .cpp and keep only a declaration in the header; update any callers to link against the compiled .cpp and ensure the function signature (shutdown) and its calls to advanced_network::is_initialized() and advanced_network::shutdown() remain unchanged.operators/advanced_network_media/advanced_network_media_rx/adv_network_media_rx.cpp (1)
460-467: Consider adding frame exhaustion recovery instead of throwing.When
frames_pool_is empty, the operator throws an exception which will crash the pipeline. For a real-time streaming scenario, consider logging an error and dropping the incoming burst gracefully, or implementing backpressure signaling.💡 Alternative: Drop burst gracefully on pool exhaustion
std::shared_ptr<FrameBufferBase> get_allocated_frame() { if (frames_pool_.empty()) { ANM_LOG_ERROR("Frame pool exhausted - dropping incoming data"); return nullptr; // Caller must handle nullptr } auto frame = frames_pool_.front(); frames_pool_.pop_front(); return frame; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@operators/advanced_network_media/advanced_network_media_rx/adv_network_media_rx.cpp` around lines 460 - 467, The get_allocated_frame() method currently throws when frames_pool_ is empty which can crash the pipeline; change it to handle pool exhaustion gracefully by logging an error (e.g., ANM_LOG_ERROR) and returning nullptr instead of throwing, or implement a blocking wait/backpressure using a condition variable to wait for a returned frame; update all callers of get_allocated_frame() to check for nullptr (or the blocking behavior) and drop the incoming burst or signal backpressure accordingly so the operator no longer crashes on pool exhaustion.operators/advanced_network/advanced_network/managers/rivermax/rivermax_mgr_impl/rivermax_config_manager.cpp (1)
124-127: Return value inconsistency: RX returns -1 on failure, TX returns 0.
parse_rx_queuesreturns-1on append failure (Line 126), butparse_tx_queuesreturns0on append failure (Line 302). This inconsistency could confuse callers. Consider aligning both to return-1on failure for consistency.♻️ Suggested fix for TX queue parsing
for (const auto& q : queues) { if (!tx_config_manager->append_candidate_for_tx_queue(port_id, q)) { HOLOSCAN_LOG_WARN("Failed to append TX queue configuration for queue - Exiting"); - return 0; + return -1; } rivermax_tx_config_found++; }Then update the caller at Line 87 to check
< 0consistently for both RX and TX.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@operators/advanced_network/advanced_network/managers/rivermax/rivermax_mgr_impl/rivermax_config_manager.cpp` around lines 124 - 127, parse_rx_queues returns -1 on append failure while parse_tx_queues returns 0, causing inconsistent error semantics; update parse_tx_queues so that when tx_config_manager->append_candidate_for_tx_queue(...) fails it returns -1 (matching parse_rx_queues) and then update the caller that invokes parse_rx_queues/parse_tx_queues (the earlier check that currently expects 0) to check for a negative return (< 0) consistently; ensure both functions (parse_rx_queues and parse_tx_queues) document/propagate the same failure code so callers can uniformly detect errors.operators/advanced_network/advanced_network/managers/rivermax/applications/ano_rtp_receiver.h (1)
30-34: Avoidusing namespacedirectives in header files.These
using namespacedirectives will pollute the namespace of any file that includes this header, potentially causing naming conflicts. Consider using explicit namespace prefixes or placing the directives inside the class/function scope.♻️ Suggested approach
Either:
- Remove the
using namespacedirectives and fully qualify types (e.g.,rivermax::dev_kit::apps::RmaxReceiverBaseApp)- Or move them inside the class definition if they're only needed there
- Or use namespace aliases:
namespace rdk_apps = rivermax::dev_kit::apps;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@operators/advanced_network/advanced_network/managers/rivermax/applications/ano_rtp_receiver.h` around lines 30 - 34, The header contains top-level using namespace directives (rivermax::dev_kit::apps, ::io_node, ::services, ::core) which pollute global scope; remove those directives from the header and update all references in this file to either fully-qualified names (e.g., rivermax::dev_kit::apps::RmaxReceiverBaseApp), a local namespace alias (e.g., namespace rdk_apps = rivermax::dev_kit::apps; used inside the header), or move any needed using-directives inside the class/inline function scope so they are not exported to including translation units; update usages of types/functions that relied on the removed using-directives accordingly.operators/advanced_network/advanced_network/managers/rivermax/rivermax_mgr_impl/adv_network_rivermax_mgr.cpp (1)
610-621: Non-deterministic stream selection may cause unfair load distribution.Using
rand() % number_of_streamsfor selecting a random stream:
- Is not thread-safe (
rand()is not reentrant)- May not provide uniform distribution
Consider using
<random>facilities or a round-robin approach for more predictable behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@operators/advanced_network/advanced_network/managers/rivermax/rivermax_mgr_impl/adv_network_rivermax_mgr.cpp` around lines 610 - 621, The code currently picks a stream using rand() % number_of_streams (in the block that accesses queue_it->second and calls dequeue_burst()), which is neither thread-safe nor uniformly distributed; replace this with a deterministic, thread-safe selection: either implement a per-service round-robin index (e.g., store an atomic<size_t> or map of indices keyed by service_id and use fetch_add(1) % number_of_streams to pick the iterator) or use C++ <random> thread-local RNG (std::mt19937 seeded per-thread) with std::uniform_int_distribution to pick the index; then advance the iterator the same way and call dequeue_burst() as before, ensuring any new state (the index map or thread-local RNG) is properly initialized and synchronized.operators/advanced_network/advanced_network/managers/rivermax/rivermax_mgr_impl/burst_manager.cpp (1)
555-592: Clarify:should_drop_burst_adaptiveis markedconstbut modifies atomic state.The method modifies
in_critical_dropping_mode_(an atomic). While this works because atomics support modification through const references, theconstqualifier is semantically misleading since the method has observable side effects on the object's state.Consider removing
constfrom the method signature to better reflect its behavior, or document that the method has side effects on mutable state.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@operators/advanced_network/advanced_network/managers/rivermax/rivermax_mgr_impl/burst_manager.cpp` around lines 555 - 592, The method RxBurstsManager::should_drop_burst_adaptive is currently declared const but mutates the atomic in_critical_dropping_mode_, which is semantically misleading; fix by removing the const qualifier from the should_drop_burst_adaptive declaration and definition (update the header and implementation to match) so the method's signature accurately reflects that it may modify object state, or alternatively mark the member in_critical_dropping_mode_ as mutable if you want to preserve the const API—choose one approach and update all declarations/usages to compile cleanly.operators/advanced_network/advanced_network/managers/rivermax/applications/ano_ipo_receiver.h (1)
30-34: Avoidusing namespacedirectives in header files.Same issue as in
ano_rtp_receiver.h- these directives pollute the namespace of includers. Consider using explicit prefixes or namespace aliases.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@operators/advanced_network/advanced_network/managers/rivermax/applications/ano_ipo_receiver.h` around lines 30 - 34, The header currently exposes `using namespace rivermax::dev_kit::apps`, `...::io_node`, `...::services`, and `...::core`, which pollutes consumers' namespaces; remove these `using namespace` directives from ano_ipo_receiver.h and replace internal uses with either fully-qualified names (rivermax::dev_kit::apps::...) or add short namespace aliases (e.g., alias names for apps/io_node/services/core) scoped in the header implementation or inside the rivermax namespace; ensure all symbols referenced in this header (classes, typedefs, function declarations) are updated to the chosen explicit qualifiers so that includers are not affected (mirror the fix you applied in ano_rtp_receiver.h).
🤖 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/adv_networking_media_player/cpp/adv_networking_media_player.cpp`:
- Around line 278-281: The code calls exit(1) when
ano::get_port_id(interface_name_.get()) returns -1 (and in other places listed)
which bypasses stack unwinding and ano::shutdown(); instead refactor to
propagate errors: replace direct exit(1) in the port_id_ check (and the other
failing checks around initialization mentioned) with returning/throwing a
descriptive exception or error code from the function that reports the failure
(include interface_name_.get() and the failing function like ano::get_port_id in
the message), and ensure top-level scope performs cleanup by calling
ano::shutdown() (or use an RAII wrapper) in a catch/finally path so Rivermax/ANO
are always cleaned up before process termination.
In `@applications/adv_networking_media_player/README.md`:
- Around line 348-360: The example under rx_threads/network_settings uses scalar
values for fields like local_ip_addresses, source_ip_addresses,
destination_ip_addresses and destination_ports but the RX schema expects lists;
update the README example so each of those fields is shown as a YAML list (e.g.,
local_ip_addresses: [2.1.0.12] or using "-" entries) for both stream_id: 0 and
stream_id: 1 to match the list-shaped schema and avoid copy/paste errors.
In `@applications/adv_networking_media_sender/adv_networking_media_sender.yaml`:
- Around line 77-80: The YAML sets memory_pool_location but the parser only
honors that key when use_internal_memory_pool is enabled, so either enable the
internal pool or remove the no-op key: add use_internal_memory_pool: true
alongside memory_pool_location (keeping memory_registration/memory_allocation)
or delete memory_pool_location to avoid confusion; target the
adv_networking_media_sender config keys memory_pool_location and
use_internal_memory_pool (and adjust comments/allocator_type as needed).
In
`@operators/advanced_network/advanced_network/managers/rivermax/rivermax_mgr_impl/burst_manager.h`:
- Around line 21-25: The header is missing the <unordered_map> include required
by declarations and parameters that use std::unordered_map; add `#include`
<unordered_map> to the top of the file so RxBurstsManager (and its constructor
that accepts std::unordered_map<int, std::shared_ptr<AnoBurstsQueue>> /
std::unordered_map<int, std::shared_ptr<RivermaxBurst>>) and the member
variables declared as std::unordered_map<int, std::shared_ptr<AnoBurstsQueue>>
and std::unordered_map<int, std::shared_ptr<RivermaxBurst>> compile as a
self-contained header.
- Around line 404-414: Validate stream_id exists in rx_bursts_out_queue_ before
using operator[] in both get_rx_burst and enqueue_and_reset_current_burst: check
rx_bursts_out_queue_.count(stream_id) or find(stream_id) and return an
appropriate error Status (or false) when missing instead of dereferencing a
possibly-null shared_ptr; then use the iterator->second (or operator[]) only
after the presence check, mirroring the validation pattern used in
RivermaxManagerRxService::get_rx_burst().
In
`@operators/advanced_network/advanced_network/managers/rivermax/rivermax_mgr_impl/rivermax_mgr_service.h`:
- Around line 21-23: The header is missing an explicit include for
<unordered_map> even though RivermaxManagerRxService exposes
std::unordered_map<int, std::shared_ptr<AnoBurstsQueue>> in its public API; add
the include directive for <unordered_map> at the top of rivermax_mgr_service.h
so the definition of std::unordered_map is available (alongside the existing
<memory>, <cstdint>, <sstream> includes) and rebuild to ensure
RivermaxManagerRxService and the references to AnoBurstsQueue compile without
relying on transitive includes.
In
`@operators/advanced_network/advanced_network/managers/rivermax/rivermax_mgr_impl/rivermax_queue_configs.cpp`:
- Around line 439-464: The validator must reject empty or under-specified TX
thread settings: in RivermaxCommonTxQueueValidator::validate ensure
settings->thread_settings is non-empty and return ReturnStatus::failure with a
clear log if empty; also verify that the resolved cpu_cores count
(settings->cpu_cores or however cpu cores are exposed) matches
settings->thread_settings.size() and return failure if they differ; keep the
existing per-thread check for empty thread.stream_network_settings but perform
the thread_settings emptiness and cpu_cores-size checks before any indexing or
loops (so convert_settings() and app_threads_cores accesses cannot go
out-of-bounds).
- Around line 345-395: Summary: Replace the sequential stream_id validation with
duplicate-detection so sparse IDs are allowed but duplicates are rejected.
Locate the sequential check that compares stream.thread_id/stream.stream_id (the
commented-out block and the earlier thread check) in rivermax_queue_configs.cpp
and remove the sequential-only requirement; instead, in the scope that iterates
thread.stream_network_settings maintain a container (e.g., std::set) of seen
stream.stream_id values (per-thread or global according to current design) and
if a stream.stream_id is already present log an error mentioning the duplicate
stream_id and thread ID and return ReturnStatus::failure; apply the same
duplicate-detection change to the other similar checks referenced (the blocks
handling RTP/IPO/TX around the other stream validation sites) so all protocols
reject duplicates but accept sparse/non-sequential IDs (use identifiers
stream.stream_id and m_stream_id_map to guide where duplication would collide).
---
Duplicate comments:
In
`@applications/adv_networking_media_player/cpp/adv_networking_media_player.cpp`:
- Around line 342-345: The visualize branch incorrectly indexes
adv_net_media_rx_map with adv_net_media_rx_map[0] even though the map is keyed
by std::string; after verifying the empty check on adv_net_media_rx_map, replace
the numeric indexing with a string-keyed lookup or simply use
adv_net_media_rx_map.begin()->second in the single-stream branch (referencing
adv_net_media_rx_map and multi_streams_adv_net_media_rx_yaml) so the code
retrieves the first mapped std::shared_ptr<ops::AdvNetworkMediaRxOp> correctly
for visualization.
In
`@operators/advanced_network/advanced_network/managers/rivermax/applications/ano_ipo_receiver.cpp`:
- Around line 182-189: In ANOIPOReceiverApp::distribute_work_for_threads the
vector m_streams_per_thread is reserved but not resized before writing via
m_streams_per_thread[thread_index]; change the code to resize
m_streams_per_thread to m_ipo_receiver_settings->thread_settings.size() (or
otherwise ensure its size is at least thread count) before the loop, then
continue assigning m_streams_per_thread[thread_index] and updating
m_ipo_receiver_settings->num_of_total_streams as before.
In
`@operators/advanced_network/advanced_network/managers/rivermax/applications/ano_media_sender.cpp`:
- Around line 73-85: post_load_settings() must not infer
m_app_settings->num_of_packets_in_chunk_specified by comparing the current value
to a computed default (because init_default_values() seeds an FHD value). Remove
the value-based check and instead rely on an explicit "was this field provided
by the config" indicator set during parsing (or introduce one if missing); only
set m_app_settings->num_of_packets_in_chunk_specified = true when that explicit
parsed-flag is true, and otherwise choose the chunk default based on resolution
and leave specified=false. Reference: ANOMediaSenderApp::post_load_settings,
m_app_settings->num_of_packets_in_chunk,
m_app_settings->num_of_packets_in_chunk_specified, and init_default_values().
- Around line 207-214: In distribute_work_for_threads(), m_streams_per_thread is
reserved but not resized so the indexed write m_streams_per_thread[thread_idx]
is out-of-bounds; change the allocation to set the vector size (e.g., call
resize with m_media_sender_settings->thread_settings.size()) before the loop or
replace the indexed assignment with push_back, ensuring m_streams_per_thread has
capacity and valid indices when writing; update references to
m_streams_per_thread, thread_idx, and the loop over
m_media_sender_settings->thread_settings accordingly.
In
`@operators/advanced_network/advanced_network/managers/rivermax/applications/ano_rtp_receiver.cpp`:
- Around line 162-171: In distribute_work_for_threads(), m_streams_per_thread is
reserved but not sized, so the direct indexing
m_streams_per_thread[thread_index] causes undefined behavior; replace the
reserve call with resize (or otherwise ensure m_streams_per_thread has
thread_settings.size() elements) before the loop, then populate elements and
update m_rtp_receiver_settings->num_of_total_streams based on each
thread.stream_network_settings.size(); ensure the code references the same
members (m_streams_per_thread, m_rtp_receiver_settings, thread_settings,
stream_network_settings) and preserves the thread_index logic.
In
`@operators/advanced_network/advanced_network/managers/rivermax/rivermax_mgr_impl/adv_network_rivermax_mgr.cpp`:
- Around line 587-591: The code extracts a raw pointer from the shared_ptr
returned by queue->dequeue_burst() (out_burst_shared.get()) then lets
out_burst_shared go out of scope, causing a use-after-free; change this by
preserving the shared_ptr lifetime instead of returning a raw pointer: capture
the shared_ptr from dequeue_burst() (dequeue_burst, out_burst_shared) and store
it in a member container (e.g., active_bursts_ keyed by the service/stream or
similar) so the object stays alive while callers use the raw pointer (assign
*burst = out_burst_shared.get()), and ensure you remove/cleanup that stored
shared_ptr when the burst is processed/returned; alternatively, change the API
to return a shared_ptr<RivermaxBurst> from the method that currently expects a
raw burst pointer.
In
`@operators/advanced_network/advanced_network/managers/rivermax/rivermax_mgr_impl/rivermax_mgr_service.cpp`:
- Around line 371-378: The error handling after
media_sender_builder_->memory_pool_location_ == MemoryKind::HOST_PINNED is
incorrect: do not call cudaFree(pool_buffer) when cudaHostAlloc fails because
pool_buffer was never allocated and pinned host memory must be freed with
cudaFreeHost when allocated; instead remove the cudaFree(pool_buffer) on failure
and ensure pool_buffer is initialized (e.g., nullptr) before calling
cudaHostAlloc, and where the allocated pinned buffer is later freed use
cudaFreeHost(pool_buffer) (only if allocation succeeded) rather than cudaFree;
keep the existing cudaGetErrorString(cuda_status) log but only attempt any free
when pool_buffer is valid.
In
`@operators/advanced_network/advanced_network/managers/rivermax/rivermax_service/CMakeLists.txt`:
- Around line 76-85: The two string replacements are applied to different
variables causing NEW_CONTENTS to be used before set and potentially writing an
empty file; change the logic to operate on a single buffer (e.g., initialize a
buffer variable from CONTENTS, update that buffer for each replacement and write
it once) so both checks/str-replace operations use the same up-to-date buffer
instead of separate NEW_CONTENTS/NEW_CONTENTS2, and only call file(WRITE) with
the final buffer; key symbols: file(READ), CONTENTS, NEW_CONTENTS,
NEW_CONTENTS2, and the two if() blocks that match "find_internal_stream_index("
and "get_app_settings(".
---
Nitpick comments:
In `@applications/adv_networking_bench/adv_networking_bench_rivermax_tx.yaml`:
- Around line 73-85: The YAML currently assigns each stream to its own
tx_threads entry (thread_id: 0 and thread_id: 1) so it doesn't exercise
multi-streams-per-thread; collapse both network_settings entries under the same
tx_threads item (e.g., put the stream_id: 0 and stream_id: 1 blocks both inside
the same tx_threads entry for thread_id: 0) so a single thread_id contains
multiple network_settings entries, keeping stream_id, local_ip_address,
destination_ip_address, and destination_port intact.
In `@applications/adv_networking_bench/cpp/rivermax_bench_op_tx.h`:
- Around line 414-420: The free function shutdown(std::string message) defined
in the header will cause ODR/multiple-definition errors when included by
multiple translation units; either mark the function inline (e.g., inline void
shutdown(std::string message) { ... }) or move its definition into a .cpp and
keep only a declaration in the header; update any callers to link against the
compiled .cpp and ensure the function signature (shutdown) and its calls to
advanced_network::is_initialized() and advanced_network::shutdown() remain
unchanged.
In
`@operators/advanced_network_media/advanced_network_media_rx/adv_network_media_rx.cpp`:
- Around line 460-467: The get_allocated_frame() method currently throws when
frames_pool_ is empty which can crash the pipeline; change it to handle pool
exhaustion gracefully by logging an error (e.g., ANM_LOG_ERROR) and returning
nullptr instead of throwing, or implement a blocking wait/backpressure using a
condition variable to wait for a returned frame; update all callers of
get_allocated_frame() to check for nullptr (or the blocking behavior) and drop
the incoming burst or signal backpressure accordingly so the operator no longer
crashes on pool exhaustion.
In
`@operators/advanced_network/advanced_network/managers/rivermax/applications/ano_ipo_receiver.h`:
- Around line 30-34: The header currently exposes `using namespace
rivermax::dev_kit::apps`, `...::io_node`, `...::services`, and `...::core`,
which pollutes consumers' namespaces; remove these `using namespace` directives
from ano_ipo_receiver.h and replace internal uses with either fully-qualified
names (rivermax::dev_kit::apps::...) or add short namespace aliases (e.g., alias
names for apps/io_node/services/core) scoped in the header implementation or
inside the rivermax namespace; ensure all symbols referenced in this header
(classes, typedefs, function declarations) are updated to the chosen explicit
qualifiers so that includers are not affected (mirror the fix you applied in
ano_rtp_receiver.h).
In
`@operators/advanced_network/advanced_network/managers/rivermax/applications/ano_rtp_receiver.h`:
- Around line 30-34: The header contains top-level using namespace directives
(rivermax::dev_kit::apps, ::io_node, ::services, ::core) which pollute global
scope; remove those directives from the header and update all references in this
file to either fully-qualified names (e.g.,
rivermax::dev_kit::apps::RmaxReceiverBaseApp), a local namespace alias (e.g.,
namespace rdk_apps = rivermax::dev_kit::apps; used inside the header), or move
any needed using-directives inside the class/inline function scope so they are
not exported to including translation units; update usages of types/functions
that relied on the removed using-directives accordingly.
In
`@operators/advanced_network/advanced_network/managers/rivermax/rivermax_mgr_impl/adv_network_rivermax_mgr.cpp`:
- Around line 610-621: The code currently picks a stream using rand() %
number_of_streams (in the block that accesses queue_it->second and calls
dequeue_burst()), which is neither thread-safe nor uniformly distributed;
replace this with a deterministic, thread-safe selection: either implement a
per-service round-robin index (e.g., store an atomic<size_t> or map of indices
keyed by service_id and use fetch_add(1) % number_of_streams to pick the
iterator) or use C++ <random> thread-local RNG (std::mt19937 seeded per-thread)
with std::uniform_int_distribution to pick the index; then advance the iterator
the same way and call dequeue_burst() as before, ensuring any new state (the
index map or thread-local RNG) is properly initialized and synchronized.
In
`@operators/advanced_network/advanced_network/managers/rivermax/rivermax_mgr_impl/burst_manager.cpp`:
- Around line 555-592: The method RxBurstsManager::should_drop_burst_adaptive is
currently declared const but mutates the atomic in_critical_dropping_mode_,
which is semantically misleading; fix by removing the const qualifier from the
should_drop_burst_adaptive declaration and definition (update the header and
implementation to match) so the method's signature accurately reflects that it
may modify object state, or alternatively mark the member
in_critical_dropping_mode_ as mutable if you want to preserve the const
API—choose one approach and update all declarations/usages to compile cleanly.
In
`@operators/advanced_network/advanced_network/managers/rivermax/rivermax_mgr_impl/rivermax_config_manager.cpp`:
- Around line 124-127: parse_rx_queues returns -1 on append failure while
parse_tx_queues returns 0, causing inconsistent error semantics; update
parse_tx_queues so that when
tx_config_manager->append_candidate_for_tx_queue(...) fails it returns -1
(matching parse_rx_queues) and then update the caller that invokes
parse_rx_queues/parse_tx_queues (the earlier check that currently expects 0) to
check for a negative return (< 0) consistently; ensure both functions
(parse_rx_queues and parse_tx_queues) document/propagate the same failure code
so callers can uniformly detect errors.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8e2a2e70-0341-43d9-8ca9-70b8e6852208
📒 Files selected for processing (34)
applications/adv_networking_bench/adv_networking_bench_rivermax_rx.yamlapplications/adv_networking_bench/adv_networking_bench_rivermax_tx.yamlapplications/adv_networking_bench/cpp/rivermax_bench_op_tx.happlications/adv_networking_media_player/README.mdapplications/adv_networking_media_player/adv_networking_media_player.yamlapplications/adv_networking_media_player/cpp/adv_networking_media_player.cppapplications/adv_networking_media_sender/adv_networking_media_sender.yamlapplications/adv_networking_media_sender/cpp/CMakeLists.txtoperators/advanced_network/Dockerfileoperators/advanced_network/advanced_network/managers/rivermax/CMakeLists.txtoperators/advanced_network/advanced_network/managers/rivermax/adv_network_rivermax_mgr.hoperators/advanced_network/advanced_network/managers/rivermax/applications/CMakeLists.txtoperators/advanced_network/advanced_network/managers/rivermax/applications/ano_ipo_receiver.cppoperators/advanced_network/advanced_network/managers/rivermax/applications/ano_ipo_receiver.hoperators/advanced_network/advanced_network/managers/rivermax/applications/ano_media_sender.cppoperators/advanced_network/advanced_network/managers/rivermax/applications/ano_media_sender.hoperators/advanced_network/advanced_network/managers/rivermax/applications/ano_rtp_receiver.cppoperators/advanced_network/advanced_network/managers/rivermax/applications/ano_rtp_receiver.hoperators/advanced_network/advanced_network/managers/rivermax/applications/utils.hoperators/advanced_network/advanced_network/managers/rivermax/rivermax_mgr_impl/adv_network_rivermax_mgr.cppoperators/advanced_network/advanced_network/managers/rivermax/rivermax_mgr_impl/burst_manager.cppoperators/advanced_network/advanced_network/managers/rivermax/rivermax_mgr_impl/burst_manager.hoperators/advanced_network/advanced_network/managers/rivermax/rivermax_mgr_impl/packet_processor.hoperators/advanced_network/advanced_network/managers/rivermax/rivermax_mgr_impl/rivermax_chunk_consumer_ano.hoperators/advanced_network/advanced_network/managers/rivermax/rivermax_mgr_impl/rivermax_config_manager.cppoperators/advanced_network/advanced_network/managers/rivermax/rivermax_mgr_impl/rivermax_config_manager.hoperators/advanced_network/advanced_network/managers/rivermax/rivermax_mgr_impl/rivermax_mgr_service.cppoperators/advanced_network/advanced_network/managers/rivermax/rivermax_mgr_impl/rivermax_mgr_service.hoperators/advanced_network/advanced_network/managers/rivermax/rivermax_mgr_impl/rivermax_queue_configs.cppoperators/advanced_network/advanced_network/managers/rivermax/rivermax_mgr_impl/rivermax_queue_configs.hoperators/advanced_network/advanced_network/managers/rivermax/rivermax_service/CMakeLists.txtoperators/advanced_network_media/CMakeLists.txtoperators/advanced_network_media/advanced_network_media_rx/adv_network_media_rx.cppoperators/advanced_network_media/advanced_network_media_rx/adv_network_media_rx.h
✅ Files skipped from review due to trivial changes (4)
- operators/advanced_network/advanced_network/managers/rivermax/applications/CMakeLists.txt
- operators/advanced_network_media/CMakeLists.txt
- applications/adv_networking_media_player/adv_networking_media_player.yaml
- operators/advanced_network/advanced_network/managers/rivermax/applications/ano_media_sender.h
🚧 Files skipped from review as they are similar to previous changes (7)
- operators/advanced_network/advanced_network/managers/rivermax/CMakeLists.txt
- operators/advanced_network/advanced_network/managers/rivermax/rivermax_mgr_impl/rivermax_config_manager.h
- operators/advanced_network/Dockerfile
- applications/adv_networking_media_sender/cpp/CMakeLists.txt
- operators/advanced_network/advanced_network/managers/rivermax/rivermax_mgr_impl/packet_processor.h
- operators/advanced_network/advanced_network/managers/rivermax/rivermax_mgr_impl/rivermax_chunk_consumer_ano.h
- operators/advanced_network/advanced_network/managers/rivermax/adv_network_rivermax_mgr.h
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
operators/advanced_network/advanced_network/managers/rivermax/rivermax_mgr_impl/rivermax_config_manager.cpp (1)
777-783:⚠️ Potential issue | 🟠 MajorReject invalid
memory_pool_locationvalues instead of forcingDEVICE.A typo here currently logs an error and then continues with GPU allocation anyway. That hides the bad config and can materially change runtime behavior; with split mode enabled it will even start successfully with the wrong memory pool. Return
falseand surface the invalid value to the user. As per coding guidelinesCode contributions must include appropriate error handling and validation for invalid inputs.Suggested fix
if (rivermax_tx_config.use_internal_memory_pool) { rivermax_tx_config.memory_pool_location = GetMemoryKindFromString( tx_settings["memory_pool_location"].template as<std::string>("device")); if (rivermax_tx_config.memory_pool_location == MemoryKind::INVALID) { - rivermax_tx_config.memory_pool_location = MemoryKind::DEVICE; - HOLOSCAN_LOG_ERROR("Invalid memory pool location, setting to DEVICE"); + HOLOSCAN_LOG_ERROR("Invalid memory pool location: {}", + tx_settings["memory_pool_location"].template as<std::string>("")); + return false; } } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@operators/advanced_network/advanced_network/managers/rivermax/rivermax_mgr_impl/rivermax_config_manager.cpp` around lines 777 - 783, The code currently overrides an invalid memory_pool_location to DEVICE and continues; instead, in the Rivermax config parsing code (the use of rivermax_tx_config.memory_pool_location set from GetMemoryKindFromString and the surrounding logic in the RivermaxConfigManager parsing function), detect when GetMemoryKindFromString returns MemoryKind::INVALID, log an explicit error including the offending string value, do not assign MemoryKind::DEVICE, and return false (or otherwise abort config load) so the bad config is surfaced to the caller; update the error path in the function that processes rivermax_tx_config to propagate failure instead of silently forcing DEVICE.
♻️ Duplicate comments (9)
operators/advanced_network/advanced_network/managers/rivermax/rivermax_service/CMakeLists.txt (1)
76-85:⚠️ Potential issue | 🔴 CriticalLogic bug:
NEW_CONTENTSmay be undefined in the second patch block.If the first condition (line 78) is false (i.e.,
virtualis already present forfind_internal_stream_index),NEW_CONTENTSis never set. Line 83 then uses an undefinedNEW_CONTENTS, which will corrupt the file or cause unexpected behavior.🐛 Proposed fix to handle independent patches correctly
# Will be removed once RDK makes the change file(READ "${rivermax-dev-kit_SOURCE_DIR}/source/apps/include/rdk/apps/rmax_base_app.h" CONTENTS) +set(PATCHED_CONTENTS "${CONTENTS}") if(NOT CONTENTS MATCHES "virtual ReturnStatus find_internal_stream_index\\(") - string(REPLACE "ReturnStatus find_internal_stream_index(" "virtual ReturnStatus find_internal_stream_index(" NEW_CONTENTS "${CONTENTS}") - file(WRITE "${rivermax-dev-kit_SOURCE_DIR}/source/apps/include/rdk/apps/rmax_base_app.h" "${NEW_CONTENTS}") + string(REPLACE "ReturnStatus find_internal_stream_index(" "virtual ReturnStatus find_internal_stream_index(" PATCHED_CONTENTS "${PATCHED_CONTENTS}") endif() -if(NOT CONTENTS MATCHES "virtual ReturnStatus get_app_settings\\(") - string(REPLACE "ReturnStatus get_app_settings(" "virtual ReturnStatus get_app_settings(" NEW_CONTENTS2 "${NEW_CONTENTS}") - file(WRITE "${rivermax-dev-kit_SOURCE_DIR}/source/apps/include/rdk/apps/rmax_base_app.h" "${NEW_CONTENTS2}") +if(NOT PATCHED_CONTENTS MATCHES "virtual ReturnStatus get_app_settings\\(") + string(REPLACE "ReturnStatus get_app_settings(" "virtual ReturnStatus get_app_settings(" PATCHED_CONTENTS "${PATCHED_CONTENTS}") endif() +file(WRITE "${rivermax-dev-kit_SOURCE_DIR}/source/apps/include/rdk/apps/rmax_base_app.h" "${PATCHED_CONTENTS}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@operators/advanced_network/advanced_network/managers/rivermax/rivermax_service/CMakeLists.txt` around lines 76 - 85, The second replacement block uses NEW_CONTENTS which may be undefined if the first if branch didn't run; update the logic in CMakeLists to base the second string(REPLACE) on the original CONTENTS when NEW_CONTENTS isn't set (or compute NEW_CONTENTS unconditionally), so that the replacement for "ReturnStatus get_app_settings(" -> "virtual ReturnStatus get_app_settings(" operates on a valid input; reference the symbols CONTENTS, NEW_CONTENTS, NEW_CONTENTS2 and the target header rmax_base_app.h and the function names find_internal_stream_index and get_app_settings to locate and fix the code.operators/advanced_network/advanced_network/managers/rivermax/applications/ano_ipo_receiver.cpp (1)
182-190:⚠️ Potential issue | 🔴 Critical
reserve()doesn't makem_streams_per_thread[thread_index]valid.The vector still has size 0 here, so the indexed writes are undefined behavior on the first multi-thread config.
Minimal fix
- m_streams_per_thread.reserve(m_ipo_receiver_settings->thread_settings.size()); + m_streams_per_thread.resize(m_ipo_receiver_settings->thread_settings.size());Verification
#!/bin/bash file='operators/advanced_network/advanced_network/managers/rivermax/applications/ano_ipo_receiver.cpp' sed -n '182,190p' "$file" rg -n 'm_streams_per_thread\.reserve|m_streams_per_thread\[thread_index\]' "$file"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@operators/advanced_network/advanced_network/managers/rivermax/applications/ano_ipo_receiver.cpp` around lines 182 - 190, In ANOIPOReceiverApp::distribute_work_for_threads the call to m_streams_per_thread.reserve(...) only allocates capacity but does not change the vector size, so indexed writes like m_streams_per_thread[thread_index] are undefined; fix by resizing the vector to thread_settings.size() (or use push_back) before the loop so m_streams_per_thread[thread_index] is valid, then compute per-thread stream counts and update m_ipo_receiver_settings->num_of_total_streams as before.applications/adv_networking_media_player/README.md (1)
348-360:⚠️ Potential issue | 🟠 MajorKeep the
*_addressesanddestination_portsexamples list-shaped.The parser reads these keys as sequences under
network_settings, so the scalar example here is copy/paste incompatible with the actual schema.Documentation fix
- stream_id: 0 # Stream 1 network config - local_ip_addresses: 2.1.0.12 - source_ip_addresses: 2.1.0.11 - destination_ip_addresses: 224.1.1.1 - destination_ports: 50000 + local_ip_addresses: + - 2.1.0.12 + source_ip_addresses: + - 2.1.0.11 + destination_ip_addresses: + - 224.1.1.1 + destination_ports: + - 50000Apply the same list form to the
stream_id: 1block below.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@applications/adv_networking_media_player/README.md` around lines 348 - 360, The examples under rx_threads → network_settings use scalar values but must be list-shaped to match the parser/schema; update both stream_id: 0 and stream_id: 1 blocks so keys local_ip_addresses, source_ip_addresses, destination_ip_addresses, and destination_ports are shown as sequences (lists) rather than scalars; locate the network_settings entries under the rx_threads/thread_id: 0 block and change those four keys for stream_id 1 to the same list form used in stream_id 0 (e.g., square-bracket or dash list) so copy/paste matches the schema.operators/advanced_network/advanced_network/managers/rivermax/applications/ano_rtp_receiver.cpp (1)
162-170:⚠️ Potential issue | 🔴 Critical
reserve()doesn't makem_streams_per_thread[thread_index]valid.The vector size is still 0 after
reserve(), so these indexed writes are undefined behavior.Minimal fix
- m_streams_per_thread.reserve(m_rtp_receiver_settings->thread_settings.size()); + m_streams_per_thread.resize(m_rtp_receiver_settings->thread_settings.size());Verification
#!/bin/bash file='operators/advanced_network/advanced_network/managers/rivermax/applications/ano_rtp_receiver.cpp' sed -n '162,170p' "$file" rg -n 'm_streams_per_thread\.reserve|m_streams_per_thread\[thread_index\]' "$file"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@operators/advanced_network/advanced_network/managers/rivermax/applications/ano_rtp_receiver.cpp` around lines 162 - 170, In distribute_work_for_threads(), reserve() was used but leaves m_streams_per_thread size 0 so indexed writes are undefined; change the code to either call m_streams_per_thread.resize(m_rtp_receiver_settings->thread_settings.size()) before the loop or replace the indexed assignment with push_back/emplace_back (e.g., m_streams_per_thread.push_back(thread.stream_network_settings.size())) while updating num_of_total_streams, referencing the distribute_work_for_threads function and the m_streams_per_thread vector.operators/advanced_network/advanced_network/managers/rivermax/rivermax_mgr_impl/adv_network_rivermax_mgr.cpp (1)
587-590:⚠️ Potential issue | 🔴 CriticalDon't hand out a raw burst pointer backed by a temporary
shared_ptr.Both overloads assign
out_burst_shared.get()to*burstand then destroy the only obvious owner at function exit. That leaves callers with a danglingBurstParams*before they even reachfree_rx_burst().Verification
#!/bin/bash file='operators/advanced_network/advanced_network/managers/rivermax/rivermax_mgr_impl/adv_network_rivermax_mgr.cpp' sed -n '569,621p' "$file" rg -n 'dequeue_burst\(|out_burst_shared|\.get\(\)' "$file"Also applies to: 618-620
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@operators/advanced_network/advanced_network/managers/rivermax/rivermax_mgr_impl/adv_network_rivermax_mgr.cpp` around lines 587 - 590, The code hands out a raw BurstParams* from out_burst_shared.get() while out_burst_shared is destroyed at function exit, leaving callers with a dangling pointer; change the API to propagate ownership of the shared_ptr instead of just the raw pointer—e.g., replace the out parameter burst (BurstParams*) with a std::shared_ptr<BurstParams>& or add an out parameter std::shared_ptr<BurstParams>* and assign out_burst_shared to it (or return the shared_ptr), so dequeue_burst's local out_burst_shared is moved/copied to the caller and remains alive until free_rx_burst releases it (adjust callers and free_rx_burst to accept/consume the shared_ptr).operators/advanced_network/advanced_network/managers/rivermax/rivermax_mgr_impl/rivermax_queue_configs.cpp (2)
343-395:⚠️ Potential issue | 🟠 MajorValidate unique
stream_ids, not dense0..N-1numbering.The runtime treats
stream_idas an opaque lookup key, so sparse IDs are fine but duplicates collide. RTP still rejects sparse IDs via the sequential check at Lines 421-425, while IPO/TX never reject duplicates at all. Track seen IDs across all configured streams and fail only on duplicates. As per coding guidelinesCode contributions must include appropriate error handling and validation for invalid inputs.Also applies to: 408-435, 445-463
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@operators/advanced_network/advanced_network/managers/rivermax/rivermax_mgr_impl/rivermax_queue_configs.cpp` around lines 343 - 395, Replace the current sequential stream_id check with duplicate detection: remove the commented/implicit sequential check that expects 0..N-1 and instead maintain a std::unordered_set<int> (e.g., seen_stream_ids) at the start of the validation function and, for each stream in thread.stream_network_settings, check if stream.stream_id is already in seen_stream_ids; if so log an error via HOLOSCAN_LOG_ERROR including the duplicate stream.stream_id and thread.thread_id and return ReturnStatus::failure, otherwise insert it and continue; apply the same change for the other similar validation blocks that iterate stream_network_settings so all duplicates across threads are detected.
439-450:⚠️ Potential issue | 🟠 MajorReject empty TX
thread_settingsbefore conversion.
RivermaxCommonTxQueueValidator::validate()still accepts an emptythread_settings, andconvert_settings()then dereferencesthread_settings[0].stream_network_settings[0]at Lines 610-611. Unlike the RX path, TX also never validates that the parsed CPU-core count matchesthread_settings.size(), so malformed affinity configs still slip through. As per coding guidelinesCode contributions must include appropriate error handling and validation for invalid inputs.Also applies to: 603-611
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@operators/advanced_network/advanced_network/managers/rivermax/rivermax_mgr_impl/rivermax_queue_configs.cpp` around lines 439 - 450, In RivermaxCommonTxQueueValidator::validate, add an early check to reject an entirely empty settings->thread_settings (log an error and return ReturnStatus::failure) before any conversion, and also validate that the parsed CPU/core count (the value derived from ValidatorUtils::validate_core or the CPU affinity parsing logic used elsewhere) matches settings->thread_settings.size(); if it does not match, log a clear error and return failure. Ensure these checks occur before convert_settings() is called and before any code that dereferences settings->thread_settings[0].stream_network_settings, so convert_settings()/the loop cannot access out-of-range elements.applications/adv_networking_media_player/cpp/adv_networking_media_player.cpp (2)
344-345:⚠️ Potential issue | 🔴 CriticalUse the actual string key when wiring the visualizer.
adv_net_media_rx_mapis keyed bystd::string, soadv_net_media_rx_map[0]does not fetch the first RX operator. It attempts a lookup via a null C-string conversion and can create a bogus entry or crash. Since Lines 371-374 already reject the multi-stream case,adv_net_media_rx_map.begin()->secondis the safe choice here.Suggested fix
- add_flow(adv_net_media_rx_map[0], visualizer, {{"out_video_buffer", "receivers"}}); + add_flow(adv_net_media_rx_map.begin()->second, + visualizer, + {{"out_video_buffer", "receivers"}});#!/bin/bash # Verify that the map is string-keyed and is still being indexed with [0]. rg -n -C2 'unordered_map<std::string, std::shared_ptr<ops::AdvNetworkMediaRxOp>>' \ applications/adv_networking_media_player/cpp/adv_networking_media_player.cpp rg -n -C2 'adv_net_media_rx_map\[0\]' \ applications/adv_networking_media_player/cpp/adv_networking_media_player.cppAlso applies to: 378-382
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@applications/adv_networking_media_player/cpp/adv_networking_media_player.cpp` around lines 344 - 345, The code incorrectly indexes the string-keyed map adv_net_media_rx_map using adv_net_media_rx_map[0]; change usages to fetch the single entry safely (e.g., use adv_net_media_rx_map.begin()->second) when multi_streams_adv_net_media_rx_yaml has been rejected, and update the other identical occurrences (the wiring-to-visualizer spots) to the same pattern to avoid invalid key lookups or accidental insertions.
278-281:⚠️ Potential issue | 🟠 MajorPropagate failures instead of calling
exit(1)from the app.Once ANO is initialized, these exits bypass stack unwinding and the normal
ano::shutdown()path at Line 438. A bad port/config now aborts the process without releasing Rivermax resources. Throw/return an error tomain()and put shutdown behind an RAII guard or a top-level catch. As per coding guidelinesAll code must adhere to Holoscan SDK coding standards for style compliance, descriptive naming, minimal abbreviations, inline documentation, and error handling.Also applies to: 323-327, 335-338, 363-365, 371-374, 397-404, 438-438
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@applications/adv_networking_media_player/cpp/adv_networking_media_player.cpp` around lines 278 - 281, Replace direct calls to exit(1) in the RX port validation (where port_id_ = ano::get_port_id(interface_name_.get()) and HOLOSCAN_LOG_ERROR is invoked) with propagation of an error to main()—either throw a descriptive exception or return an error code from the function containing this logic so the caller can handle it; ensure ano::shutdown() is invoked in all termination paths by introducing an RAII guard or adding a top-level try/catch in main() that calls ano::shutdown() (or performs proper cleanup) before rethrowing or returning the error. Apply the same change to the other occurrences noted (around lines with port checks and HOLOSCAN_LOG_ERROR) so no code path calls exit() directly after ANO initialization. Ensure log messages remain descriptive and preserve existing symbols (port_id_, interface_name_.get(), ano::get_port_id, HOLOSCAN_LOG_ERROR, ano::shutdown(), main()) to locate edits.
🧹 Nitpick comments (2)
operators/advanced_network/advanced_network/managers/rivermax/applications/utils.h (2)
28-44: Consider adding documentation for these public data structures.These structs are exported as part of the public API but lack documentation explaining:
- The purpose of singular vs. plural fields (e.g.,
local_ipvslocal_ips)- When each form should be used
- The meaning of
stream_idandthread_idBrief inline comments would improve usability for consumers of this API. As per coding guidelines: "Complex logic must include inline comments explaining purpose."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@operators/advanced_network/advanced_network/managers/rivermax/applications/utils.h` around lines 28 - 44, Add brief inline documentation comments to the public structs StreamNetworkSettings and ThreadSettings: annotate each field (local_ip, local_ips, source_ip, source_ips, destination_ip, destination_ips, source_port, destination_port, destination_ports, stream_id, thread_id) explaining the difference between singular and plural forms (single value vs. list of alternatives or bindings), when to use each, and the semantics of stream_id (unique identifier for the stream) and thread_id (identifier for the worker thread). Keep comments short, declarative, and placed above or next to each field to satisfy the coding guideline for public API documentation.
23-23: Unused include.
<unordered_map>is included but not used in this header. Consider removing it to keep includes minimal.♻️ Proposed fix
`#include` <cstdint> `#include` <string> -#include <unordered_map> `#include` <vector>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@operators/advanced_network/advanced_network/managers/rivermax/applications/utils.h` at line 23, Remove the unused include <unordered_map> from utils.h: locate the include line in advanced_network/managers/rivermax/applications/utils.h and delete the "#include <unordered_map>" entry so the header only contains the actually required includes; verify no symbols from std::unordered_map are used in any declarations within utils.h (e.g., in function prototypes or class members) before removing.
🤖 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/adv_networking_media_player/cpp/adv_networking_media_player.cpp`:
- Around line 250-251: The per-frame call to file_stream_.flush() makes every
frame write synchronous; remove the immediate flush from the hot frame-write
path (the code that calls file_stream_.flush() after writing frames) and instead
perform flushing only on stream close or at a coarse interval; update the class
that owns file_stream_ (e.g., the frame-writer/producer method and the
destructor/Close/Stop method) to call file_stream_.flush() on shutdown, or add a
periodic flush mechanism (timer or counter + FLUSH_INTERVAL) that flushes
periodically rather than per frame to avoid blocking I/O.
In
`@operators/advanced_network_media/advanced_network_media_rx/adv_network_media_rx.cpp`:
- Around line 141-157: The code currently defaults unknown
parent_.output_format_.get() and parent_.memory_location_.get() values to
VIDEO_BUFFER and Device silently; change this to validate and fail fast: in the
block that reads parent_.output_format_.get() map explicit strings ("tensor" ->
OutputFormatType::TENSOR, "video_buffer" or "video" ->
OutputFormatType::VIDEO_BUFFER) and on any other value log an error via
ANM_CONFIG_LOG (including the invalid string) and return an error status or
throw so initialization fails; do the same for parent_.memory_location_.get()
mapping ("host" -> nvidia::gxf::MemoryStorageType::kHost, "device" ->
nvidia::gxf::MemoryStorageType::kDevice) and reject unknown values instead of
silently defaulting, referencing output_format_, OutputFormatType,
memory_location_str, and storage_type_ to locate the code to change.
- Around line 288-294: The current branch skips the Rivermax stream-aware
dequeue when stream_id == 0, causing the receiver not to be pinned to stream 0;
change the conditional so that when ano::get_manager_type() ==
ano::ManagerType::RIVERMAX you always use the Rivermax-specific overload and
call RivermaxMgr::get_rx_burst(&burst, port_id_, parent_.queue_id_.get(),
parent_.stream_id_.get()) (keep the static_cast<ano::RivermaxMgr*>(mgr)), and
only fall back to mgr->get_rx_burst(&burst, port_id_, parent_.queue_id_.get())
for non-Rivermax manager types.
- Around line 440-444: release_func currently captures raw this and writes to
frames_pool_, which can lead to use-after-free when AdvNetworkMediaRxOpImpl is
destroyed via cleanup() and pimpl_ is deleted; allocate the reusable-frame pool
behind shared lifetime (e.g., std::shared_ptr to the pool or a small struct
holding frames_pool_) as a member or local shared resource and change
release_func to capture that std::shared_ptr instead of this so callbacks keep
the pool alive; update both occurrences (the lambda at release_func and the
similar lambda around lines 722-727) to push into the shared pool via the shared
pointer and remove direct references to this in those lambdas.
In
`@operators/advanced_network/advanced_network/managers/rivermax/applications/ano_ipo_receiver.cpp`:
- Around line 38-80: Validation currently iterates settings->thread_settings but
doesn't reject duplicate stream.stream_id values, which causes m_stream_id_map
to overwrite earlier entries; add a global std::unordered_set<std::string> (or
appropriate type) named e.g. seen_stream_ids before the outer loop and for each
stream check if stream.stream_id is already in seen_stream_ids—if so, print an
error via std::cerr (e.g. "Duplicate stream_id: ...") and return
ReturnStatus::failure; otherwise insert the id and continue validation (keep all
existing ValidatorUtils checks intact).
In
`@operators/advanced_network/advanced_network/managers/rivermax/applications/ano_ipo_receiver.h`:
- Around line 30-33: Remove the four "using namespace" directives in
ano_ipo_receiver.h (the lines starting with "using namespace
rivermax::dev_kit::apps;", "io_node;", "services;", and "core;") because headers
must not pollute includers' namespaces; instead either (A) introduce short
namespace aliases for those long namespaces and use those aliases in this header
(for example use an alias for rivermax::dev_kit::apps,
rivermax::dev_kit::io_node, rivermax::dev_kit::services and
rivermax::dev_kit::core) or (B) move the using directives into the corresponding
.cpp and fully qualify any types referenced in the header (replace unqualified
types in class/method signatures with rivermax::dev_kit::... qualified names) so
the header no longer relies on global using directives.
In
`@operators/advanced_network/advanced_network/managers/rivermax/applications/ano_rtp_receiver.cpp`:
- Around line 35-57: The validator currently iterates settings->thread_settings
and stream.stream_network_settings but does not check for duplicate
stream.stream_id; add a uniqueness check per thread (e.g., use a local
std::unordered_set<uint32_t> seen_ids inside the loop over each thread) and
reject duplicate stream.stream_id values by returning an error
(ReturnStatus::failure or a specific validation status) before inserting into
m_stream_id_map to prevent overwriting; update the validation block around the
loop over stream_network_settings (referenced symbols:
settings->thread_settings, stream.stream_network_settings, stream.stream_id,
m_stream_id_map) to perform this duplicate detection and return immediately on
duplicates.
In
`@operators/advanced_network/advanced_network/managers/rivermax/rivermax_mgr_impl/adv_network_rivermax_mgr.cpp`:
- Around line 610-617: The current "any stream" logic picks a single random
queue (using number_of_streams, queue_it->second and it) and can return NULL_PTR
even when sibling queues have bursts; change it to probe up to number_of_streams
queues starting from a random index and advance/wrap (e.g., start_index = rand()
% number_of_streams; loop i from 0 to number_of_streams-1, compute idx =
(start_index + i) % number_of_streams, get iterator to queue_it->second[idx],
check for non-empty burst and return it immediately), only returning NULL_PTR or
Status::INVALID_PARAMETER after all queues are found empty; keep using
service_id in error logs unchanged.
In
`@operators/advanced_network/advanced_network/managers/rivermax/rivermax_mgr_impl/rivermax_config_manager.cpp`:
- Around line 299-303: The loop in parse_tx_queues uses return 0 on append
failure which conflates "no TX queues configured" and "append failed"; change
the error return to a negative sentinel (e.g., return -1) when
append_candidate_for_tx_queue(port_id, q) fails, and update the caller
(parse_configuration) to check for < 0 (matching the RX error handling) so a
malformed TX queue causes a hard failure; reference functions: parse_tx_queues,
append_candidate_for_tx_queue, and parse_configuration.
- Around line 570-580: The RX default for memory_registration is currently set
to true causing one-MR RX queues to fail because
RivermaxCommonRxQueueValidator::validate() rejects memory_registration when
header-data split is disabled (split_boundary==0); update the assignment to
rivermax_rx_config.memory_registration so its default is false (change
rx_settings["memory_registration"].as<bool>(true) to .as<bool>(false)) --
alternatively, make it conditional using q_item["split_boundary"] (if
split_boundary==0 then force memory_registration=false) to keep backwards
compatibility.
In
`@operators/advanced_network/advanced_network/managers/rivermax/rivermax_mgr_impl/rivermax_queue_configs.cpp`:
- Around line 606-611: The code in rivermax_queue_configs.cpp copies only the
first stream's local_ip into ANOMediaSenderSettings::local_ip
(target_settings->local_ip =
source_settings->thread_settings[0].stream_network_settings[0].local_ip), which
hides per-stream local_ip_address set by parse_common_tx_settings(); update the
builder to either validate that every stream in
source_settings->thread_settings[*].stream_network_settings[*].local_ip is
identical (and return/log an error via existing validation/error paths if not)
or extend ANOMediaSenderSettings to carry per-thread/per-stream local IPs and
copy the full stream_network_settings local_ip values through (adjusting uses of
ANOMediaSenderSettings::local_ip accordingly). Ensure you reference
thread_settings, stream_network_settings, parse_common_tx_settings(), and
ANOMediaSenderSettings when making the change and add appropriate error handling
for mismatched interfaces.
---
Outside diff comments:
In
`@operators/advanced_network/advanced_network/managers/rivermax/rivermax_mgr_impl/rivermax_config_manager.cpp`:
- Around line 777-783: The code currently overrides an invalid
memory_pool_location to DEVICE and continues; instead, in the Rivermax config
parsing code (the use of rivermax_tx_config.memory_pool_location set from
GetMemoryKindFromString and the surrounding logic in the RivermaxConfigManager
parsing function), detect when GetMemoryKindFromString returns
MemoryKind::INVALID, log an explicit error including the offending string value,
do not assign MemoryKind::DEVICE, and return false (or otherwise abort config
load) so the bad config is surfaced to the caller; update the error path in the
function that processes rivermax_tx_config to propagate failure instead of
silently forcing DEVICE.
---
Duplicate comments:
In
`@applications/adv_networking_media_player/cpp/adv_networking_media_player.cpp`:
- Around line 344-345: The code incorrectly indexes the string-keyed map
adv_net_media_rx_map using adv_net_media_rx_map[0]; change usages to fetch the
single entry safely (e.g., use adv_net_media_rx_map.begin()->second) when
multi_streams_adv_net_media_rx_yaml has been rejected, and update the other
identical occurrences (the wiring-to-visualizer spots) to the same pattern to
avoid invalid key lookups or accidental insertions.
- Around line 278-281: Replace direct calls to exit(1) in the RX port validation
(where port_id_ = ano::get_port_id(interface_name_.get()) and HOLOSCAN_LOG_ERROR
is invoked) with propagation of an error to main()—either throw a descriptive
exception or return an error code from the function containing this logic so the
caller can handle it; ensure ano::shutdown() is invoked in all termination paths
by introducing an RAII guard or adding a top-level try/catch in main() that
calls ano::shutdown() (or performs proper cleanup) before rethrowing or
returning the error. Apply the same change to the other occurrences noted
(around lines with port checks and HOLOSCAN_LOG_ERROR) so no code path calls
exit() directly after ANO initialization. Ensure log messages remain descriptive
and preserve existing symbols (port_id_, interface_name_.get(),
ano::get_port_id, HOLOSCAN_LOG_ERROR, ano::shutdown(), main()) to locate edits.
In `@applications/adv_networking_media_player/README.md`:
- Around line 348-360: The examples under rx_threads → network_settings use
scalar values but must be list-shaped to match the parser/schema; update both
stream_id: 0 and stream_id: 1 blocks so keys local_ip_addresses,
source_ip_addresses, destination_ip_addresses, and destination_ports are shown
as sequences (lists) rather than scalars; locate the network_settings entries
under the rx_threads/thread_id: 0 block and change those four keys for stream_id
1 to the same list form used in stream_id 0 (e.g., square-bracket or dash list)
so copy/paste matches the schema.
In
`@operators/advanced_network/advanced_network/managers/rivermax/applications/ano_ipo_receiver.cpp`:
- Around line 182-190: In ANOIPOReceiverApp::distribute_work_for_threads the
call to m_streams_per_thread.reserve(...) only allocates capacity but does not
change the vector size, so indexed writes like
m_streams_per_thread[thread_index] are undefined; fix by resizing the vector to
thread_settings.size() (or use push_back) before the loop so
m_streams_per_thread[thread_index] is valid, then compute per-thread stream
counts and update m_ipo_receiver_settings->num_of_total_streams as before.
In
`@operators/advanced_network/advanced_network/managers/rivermax/applications/ano_rtp_receiver.cpp`:
- Around line 162-170: In distribute_work_for_threads(), reserve() was used but
leaves m_streams_per_thread size 0 so indexed writes are undefined; change the
code to either call
m_streams_per_thread.resize(m_rtp_receiver_settings->thread_settings.size())
before the loop or replace the indexed assignment with push_back/emplace_back
(e.g., m_streams_per_thread.push_back(thread.stream_network_settings.size()))
while updating num_of_total_streams, referencing the distribute_work_for_threads
function and the m_streams_per_thread vector.
In
`@operators/advanced_network/advanced_network/managers/rivermax/rivermax_mgr_impl/adv_network_rivermax_mgr.cpp`:
- Around line 587-590: The code hands out a raw BurstParams* from
out_burst_shared.get() while out_burst_shared is destroyed at function exit,
leaving callers with a dangling pointer; change the API to propagate ownership
of the shared_ptr instead of just the raw pointer—e.g., replace the out
parameter burst (BurstParams*) with a std::shared_ptr<BurstParams>& or add an
out parameter std::shared_ptr<BurstParams>* and assign out_burst_shared to it
(or return the shared_ptr), so dequeue_burst's local out_burst_shared is
moved/copied to the caller and remains alive until free_rx_burst releases it
(adjust callers and free_rx_burst to accept/consume the shared_ptr).
In
`@operators/advanced_network/advanced_network/managers/rivermax/rivermax_mgr_impl/rivermax_queue_configs.cpp`:
- Around line 343-395: Replace the current sequential stream_id check with
duplicate detection: remove the commented/implicit sequential check that expects
0..N-1 and instead maintain a std::unordered_set<int> (e.g., seen_stream_ids) at
the start of the validation function and, for each stream in
thread.stream_network_settings, check if stream.stream_id is already in
seen_stream_ids; if so log an error via HOLOSCAN_LOG_ERROR including the
duplicate stream.stream_id and thread.thread_id and return
ReturnStatus::failure, otherwise insert it and continue; apply the same change
for the other similar validation blocks that iterate stream_network_settings so
all duplicates across threads are detected.
- Around line 439-450: In RivermaxCommonTxQueueValidator::validate, add an early
check to reject an entirely empty settings->thread_settings (log an error and
return ReturnStatus::failure) before any conversion, and also validate that the
parsed CPU/core count (the value derived from ValidatorUtils::validate_core or
the CPU affinity parsing logic used elsewhere) matches
settings->thread_settings.size(); if it does not match, log a clear error and
return failure. Ensure these checks occur before convert_settings() is called
and before any code that dereferences
settings->thread_settings[0].stream_network_settings, so convert_settings()/the
loop cannot access out-of-range elements.
In
`@operators/advanced_network/advanced_network/managers/rivermax/rivermax_service/CMakeLists.txt`:
- Around line 76-85: The second replacement block uses NEW_CONTENTS which may be
undefined if the first if branch didn't run; update the logic in CMakeLists to
base the second string(REPLACE) on the original CONTENTS when NEW_CONTENTS isn't
set (or compute NEW_CONTENTS unconditionally), so that the replacement for
"ReturnStatus get_app_settings(" -> "virtual ReturnStatus get_app_settings("
operates on a valid input; reference the symbols CONTENTS, NEW_CONTENTS,
NEW_CONTENTS2 and the target header rmax_base_app.h and the function names
find_internal_stream_index and get_app_settings to locate and fix the code.
---
Nitpick comments:
In
`@operators/advanced_network/advanced_network/managers/rivermax/applications/utils.h`:
- Around line 28-44: Add brief inline documentation comments to the public
structs StreamNetworkSettings and ThreadSettings: annotate each field (local_ip,
local_ips, source_ip, source_ips, destination_ip, destination_ips, source_port,
destination_port, destination_ports, stream_id, thread_id) explaining the
difference between singular and plural forms (single value vs. list of
alternatives or bindings), when to use each, and the semantics of stream_id
(unique identifier for the stream) and thread_id (identifier for the worker
thread). Keep comments short, declarative, and placed above or next to each
field to satisfy the coding guideline for public API documentation.
- Line 23: Remove the unused include <unordered_map> from utils.h: locate the
include line in advanced_network/managers/rivermax/applications/utils.h and
delete the "#include <unordered_map>" entry so the header only contains the
actually required includes; verify no symbols from std::unordered_map are used
in any declarations within utils.h (e.g., in function prototypes or class
members) before removing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 545bee83-2abc-46dc-840f-043cbb7166bc
📒 Files selected for processing (30)
applications/adv_networking_bench/adv_networking_bench_rivermax_rx.yamlapplications/adv_networking_bench/adv_networking_bench_rivermax_tx.yamlapplications/adv_networking_bench/cpp/rivermax_bench_op_tx.happlications/adv_networking_media_player/README.mdapplications/adv_networking_media_player/cpp/adv_networking_media_player.cppapplications/adv_networking_media_sender/adv_networking_media_sender.yamlapplications/adv_networking_media_sender/cpp/CMakeLists.txtoperators/advanced_network/Dockerfileoperators/advanced_network/advanced_network/managers/rivermax/CMakeLists.txtoperators/advanced_network/advanced_network/managers/rivermax/applications/CMakeLists.txtoperators/advanced_network/advanced_network/managers/rivermax/applications/ano_ipo_receiver.cppoperators/advanced_network/advanced_network/managers/rivermax/applications/ano_ipo_receiver.hoperators/advanced_network/advanced_network/managers/rivermax/applications/ano_media_sender.cppoperators/advanced_network/advanced_network/managers/rivermax/applications/ano_media_sender.hoperators/advanced_network/advanced_network/managers/rivermax/applications/ano_rtp_receiver.cppoperators/advanced_network/advanced_network/managers/rivermax/applications/ano_rtp_receiver.hoperators/advanced_network/advanced_network/managers/rivermax/applications/utils.hoperators/advanced_network/advanced_network/managers/rivermax/rivermax_mgr_impl/adv_network_rivermax_mgr.cppoperators/advanced_network/advanced_network/managers/rivermax/rivermax_mgr_impl/burst_manager.cppoperators/advanced_network/advanced_network/managers/rivermax/rivermax_mgr_impl/burst_manager.hoperators/advanced_network/advanced_network/managers/rivermax/rivermax_mgr_impl/packet_processor.hoperators/advanced_network/advanced_network/managers/rivermax/rivermax_mgr_impl/rivermax_chunk_consumer_ano.hoperators/advanced_network/advanced_network/managers/rivermax/rivermax_mgr_impl/rivermax_config_manager.cppoperators/advanced_network/advanced_network/managers/rivermax/rivermax_mgr_impl/rivermax_config_manager.hoperators/advanced_network/advanced_network/managers/rivermax/rivermax_mgr_impl/rivermax_mgr_service.cppoperators/advanced_network/advanced_network/managers/rivermax/rivermax_mgr_impl/rivermax_mgr_service.hoperators/advanced_network/advanced_network/managers/rivermax/rivermax_mgr_impl/rivermax_queue_configs.cppoperators/advanced_network/advanced_network/managers/rivermax/rivermax_mgr_impl/rivermax_queue_configs.hoperators/advanced_network/advanced_network/managers/rivermax/rivermax_service/CMakeLists.txtoperators/advanced_network_media/advanced_network_media_rx/adv_network_media_rx.cpp
✅ Files skipped from review due to trivial changes (4)
- operators/advanced_network/advanced_network/managers/rivermax/applications/CMakeLists.txt
- operators/advanced_network/advanced_network/managers/rivermax/rivermax_mgr_impl/packet_processor.h
- applications/adv_networking_media_sender/adv_networking_media_sender.yaml
- operators/advanced_network/advanced_network/managers/rivermax/applications/ano_media_sender.cpp
🚧 Files skipped from review as they are similar to previous changes (6)
- applications/adv_networking_bench/adv_networking_bench_rivermax_tx.yaml
- applications/adv_networking_media_sender/cpp/CMakeLists.txt
- operators/advanced_network/Dockerfile
- applications/adv_networking_bench/cpp/rivermax_bench_op_tx.h
- operators/advanced_network/advanced_network/managers/rivermax/rivermax_mgr_impl/burst_manager.h
- operators/advanced_network/advanced_network/managers/rivermax/rivermax_mgr_impl/rivermax_mgr_service.cpp
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Enhanced the Rivermax RX manager to support multiple streams on the same thread with stream-specific packet processing and queueing capabilities.
Enhance TX manager to support sending multiple streams on the same thread.
Key changes:
AppSettingin order to set threads setting. Each thread has it's dedicated core, ID and streams settings.This PR is rebased to Rony's PR/970.
The last 4 commits are new.
Summary by CodeRabbit
New Features
Documentation
Enhancements
Chores