feat: Add GStreamer video recorder with comprehensive sample application #1243
Conversation
GilShpr
requested review from
jjomier,
rahul-imaging and
tbirdso
and removed request for
jjomier
GilShpr
force-pushed
the
gshapira/holoscan_gstreamer_bridge
branch
from
fdd7d8a to
7dcd59c
Compare
Greptile Summary
Confidence Score: 5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant PatternGenOperator
participant GstVideoRecorderOperator
participant GstSrcBridge
participant GStreamer
User->>PatternGenOperator: "start()"
User->>GstVideoRecorderOperator: "start()"
GstVideoRecorderOperator->>GStreamer: "create pipeline elements"
GstVideoRecorderOperator->>GStreamer: "set pipeline to PLAYING"
loop For each frame
PatternGenOperator->>PatternGenOperator: "generate pattern tensor"
PatternGenOperator->>GstVideoRecorderOperator: "send TensorMap"
GstVideoRecorderOperator->>GstSrcBridge: "create_buffer_from_tensor_map()"
GstSrcBridge->>GstSrcBridge: "wrap tensor memory (zero-copy)"
GstSrcBridge->>GStreamer: "push buffer to pipeline"
GStreamer->>GStreamer: "encode and write to file"
end
User->>GstVideoRecorderOperator: "stop()"
GstVideoRecorderOperator->>GstSrcBridge: "send_eos()"
GstSrcBridge->>GStreamer: "send EOS signal"
GStreamer->>GStreamer: "finalize file"
GstVideoRecorderOperator->>GStreamer: "wait for EOS complete"
GstVideoRecorderOperator->>GStreamer: "set pipeline to NULL"
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
28 files reviewed, 1 comment
Edit Code Review Agent Settings | Greptile
React with 👍 or 👎 to share your feedback on this new summary format
| } | ||
| return; |
There was a problem hiding this comment.
logic: debug_info is allocated by GStreamer and never freed, causing a memory leak.
| } | |
| return; | |
| gst_message_parse_error(msg.get(), &error, &debug_info); | |
| gst::Error gst_error(error); | |
| HOLOSCAN_LOG_ERROR("GStreamer error: {}", gst_error->message); | |
| if (debug_info) { | |
| HOLOSCAN_LOG_DEBUG("Debug info: {}", debug_info); | |
| g_free(debug_info); | |
| } |
There was a problem hiding this comment.
28 files reviewed, 1 comment
Edit Code Review Agent Settings | Greptile
React with 👍 or 👎 to share your feedback on this new summary format
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a GStreamer integration: new holoscan GStreamer bridge, RAII GStreamer wrappers, a GstVideoRecorder operator and application, supporting CMake entries, Docker/devcontainer updates, install script, metadata, README, and build/install rules. Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes
Pre-merge checks✅ Passed checks (2 passed)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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 |
GilShpr
force-pushed
the
gshapira/holoscan_gstreamer_bridge
branch
2 times, most recently
from
d0cc336 to
ba85437
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
applications/gstreamer/gst_video_recorder/README.md (1)
140-142: Consider adding language identifier to fenced code block.The diagram would benefit from a language identifier (e.g., ```text) for proper rendering and syntax highlighting.
Apply this diff:
-``` +```text PatternGenOperator → GstVideoRecorderOperator → GStreamer Encoding Pipeline → File</blockquote></details> <details> <summary>operators/gstreamer/gst_video_recorder_operator.cpp (1)</summary><blockquote> `350-448`: **Leverage `gst_util_set_object_arg` as a generic fallback for encoder properties** `set_encoder_property` does a good job covering the common scalar GTypes, but it currently: - Ignores GStreamer properties with more complex types (enums, flags, ranges, boxed types, etc.). - Re-implements string→value parsing logic that GStreamer already provides. You can improve robustness and future-proofing by falling back to `gst_util_set_object_arg()` for property types you don’t explicitly handle. That lets GStreamer introspection parse strings into the correct types and gives you automatic support for enums/flags. For example: ```diff switch (ptype) { case G_TYPE_STRING: g_object_set(encoder, key.c_str(), value.c_str(), nullptr); return true; @@ case G_TYPE_DOUBLE: try { double double_val = std::stod(value); g_object_set(encoder, key.c_str(), double_val, nullptr); return true; } catch (const std::exception& e) { HOLOSCAN_LOG_ERROR("Failed to convert '{}' to double for property '{}': {}", value, key, e.what()); } break; default: - HOLOSCAN_LOG_WARN("Unsupported property type for '{}' (type: {}), skipping", - key, g_type_name(ptype)); - break; + // Fallback to generic GStreamer parser for unhandled property types + HOLOSCAN_LOG_DEBUG("Falling back to gst_util_set_object_arg for '{}' (type: {})", + key, g_type_name(ptype)); + gst_util_set_object_arg(G_OBJECT(encoder), key.c_str(), value.c_str()); + return true; }This keeps your explicit conversions for simple numeric/boolean types but greatly increases compatibility with different encoders and property types.
applications/gstreamer/gst_video_recorder/gst_video_recorder.cpp (1)
473-500: Guard against division by zero inColorBarsPatternGeneratorfor very small widths
ColorBarsPatternGenerator::generate_pattern_datacomputes:int bar_width = width / SMPTE_COLOR_BARS; ... int bar_idx = x / bar_width;If
width < SMPTE_COLOR_BARS(7),bar_widthbecomes 0 andx / bar_widthwill divide by zero. The CLI path currently enforceswidth >= 64, butPatternGenOperatoris generic and could be reused with a smaller width elsewhere.A small defensive clamp makes this safe with negligible cost:
void generate_pattern_data(uint8_t* data, int width, int height) override { @@ - int bar_width = width / SMPTE_COLOR_BARS; + int bar_width = width / SMPTE_COLOR_BARS; + if (bar_width <= 0) { + bar_width = 1; // Avoid division by zero for very small widths + } @@ int idx = (y * width + x) * RGBA_CHANNELS; int bar_idx = x / bar_width; if (bar_idx >= SMPTE_COLOR_BARS) bar_idx = SMPTE_COLOR_BARS - 1;This keeps behavior identical for normal resolutions while making the pattern generator robust in edge cases.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
.devcontainer/devcontainer.json(1 hunks)applications/CMakeLists.txt(1 hunks)applications/gstreamer/CMakeLists.txt(1 hunks)applications/gstreamer/gst_video_recorder/CMakeLists.txt(1 hunks)applications/gstreamer/gst_video_recorder/Dockerfile(1 hunks)applications/gstreamer/gst_video_recorder/README.md(1 hunks)applications/gstreamer/gst_video_recorder/gst_video_recorder.cpp(1 hunks)applications/gstreamer/gst_video_recorder/install_deps.sh(1 hunks)applications/gstreamer/gst_video_recorder/metadata.json(1 hunks)operators/CMakeLists.txt(1 hunks)operators/gstreamer/CMakeLists.txt(1 hunks)operators/gstreamer/gst/buffer.cpp(1 hunks)operators/gstreamer/gst/buffer.hpp(1 hunks)operators/gstreamer/gst/caps.cpp(1 hunks)operators/gstreamer/gst/caps.hpp(1 hunks)operators/gstreamer/gst/config.hpp(1 hunks)operators/gstreamer/gst/error.hpp(1 hunks)operators/gstreamer/gst/memory.cpp(1 hunks)operators/gstreamer/gst/memory.hpp(1 hunks)operators/gstreamer/gst/message.cpp(1 hunks)operators/gstreamer/gst/message.hpp(1 hunks)operators/gstreamer/gst/object.hpp(1 hunks)operators/gstreamer/gst/video_info.cpp(1 hunks)operators/gstreamer/gst/video_info.hpp(1 hunks)operators/gstreamer/gst_src_bridge.cpp(1 hunks)operators/gstreamer/gst_src_bridge.hpp(1 hunks)operators/gstreamer/gst_video_recorder_operator.cpp(1 hunks)operators/gstreamer/gst_video_recorder_operator.hpp(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-11-10T20:29:02.443Z
Learnt from: bhashemian
Repo: nvidia-holoscan/holohub PR: 1226
File: utilities/cli/util.py:32-32
Timestamp: 2025-11-10T20:29:02.443Z
Learning: In the holohub repository, the `DEFAULT_BASE_SDK_VERSION` constant in `utilities/cli/util.py` is a fallback default value. The `tested_versions` fields in application `metadata.json` files (e.g., in `applications/multiai_ultrasound/*/metadata.json`) are maintained independently and do not need to match the `DEFAULT_BASE_SDK_VERSION`. Each application tracks its own tested versions separately.
Applied to files:
applications/gstreamer/gst_video_recorder/Dockerfile
📚 Learning: 2025-10-30T19:04:41.239Z
Learnt from: bhashemian
Repo: nvidia-holoscan/holohub PR: 1203
File: applications/video_streaming/video_streaming_server/cpp/metadata.json:42-51
Timestamp: 2025-10-30T19:04:41.239Z
Learning: In the video_streaming application structure, umbrella metadata (applications/video_streaming/metadata.json) uses language-qualified mode names like "server_cpp" and "server_python" to distinguish between implementations, while component-specific metadata files (e.g., applications/video_streaming/video_streaming_server/cpp/metadata.json) use simple mode names like "server" since they are already scoped to a specific language implementation by their directory structure.
Applied to files:
applications/gstreamer/gst_video_recorder/metadata.json
📚 Learning: 2025-10-22T16:21:10.281Z
Learnt from: cdinea
Repo: nvidia-holoscan/holohub PR: 1170
File: applications/video_streaming/video_streaming_client/cpp/streaming_client_demo.yaml:42-49
Timestamp: 2025-10-22T16:21:10.281Z
Learning: V4L2VideoCaptureOp in Holoscan SDK performs internal conversion from the camera's native pixel format (specified by the pixel_format parameter) to RGBA8888 output, regardless of the input format (e.g., YUYV, MJPEG). When using V4L2 as a source, downstream operators should expect RGBA8888 (4-channel) output format.
Applied to files:
operators/gstreamer/gst/video_info.cpp
📚 Learning: 2025-10-22T16:53:45.393Z
Learnt from: cdinea
Repo: nvidia-holoscan/holohub PR: 1170
File: operators/video_streaming/streaming_client_enhanced/python/CMakeLists.txt:16-24
Timestamp: 2025-10-22T16:53:45.393Z
Learning: The pybind11_add_holohub_module CMake macro in cmake/pybind11_add_holohub_module.cmake encapsulates all pybind11 setup internally, including finding pybind11, linking against holoscan::pybind11 conditionally, and linking the C++ operator target. Operator Python bindings in holohub should only call this macro without additional pybind11 setup.
Applied to files:
operators/CMakeLists.txt
🧬 Code graph analysis (16)
operators/gstreamer/gst_video_recorder_operator.hpp (1)
applications/gstreamer/gst_video_recorder/gst_video_recorder.cpp (4)
spec(510-519)spec(510-510)input(544-565)input(544-544)
operators/gstreamer/gst/message.cpp (2)
operators/gstreamer/gst/error.hpp (2)
Error(43-48)Error(43-43)operators/gstreamer/gst/message.hpp (1)
debug_info(59-59)
operators/gstreamer/gst/memory.hpp (1)
operators/gstreamer/gst/buffer.hpp (3)
memory(67-67)info(88-88)info(94-94)
operators/gstreamer/gst/memory.cpp (2)
operators/gstreamer/gst/memory.hpp (3)
offset(49-49)info(57-57)info(63-63)operators/gstreamer/gst/buffer.cpp (6)
map(55-58)map(55-55)flags(34-37)flags(34-34)unmap(60-62)unmap(60-60)
operators/gstreamer/gst/caps.cpp (4)
operators/gstreamer/gst/caps.hpp (5)
Caps(48-48)Caps(55-55)index(63-63)fieldname(72-72)feature_name(99-99)operators/gstreamer/gst_src_bridge.cpp (1)
caps(324-324)operators/gstreamer/gst/video_info.cpp (1)
VideoInfo(27-31)operators/gstreamer/gst/video_info.hpp (1)
VideoInfo(42-42)
operators/gstreamer/gst/caps.hpp (1)
operators/gstreamer/gst_src_bridge.cpp (1)
caps(324-324)
operators/gstreamer/gst_video_recorder_operator.cpp (3)
operators/gstreamer/gst/object.hpp (1)
static_cast(77-77)operators/gstreamer/gst/message.hpp (3)
Message(44-44)Message(44-44)debug_info(59-59)operators/gstreamer/gst_src_bridge.hpp (2)
tensor_map(113-113)buffer(98-98)
operators/gstreamer/gst/error.hpp (1)
operators/gstreamer/gst/object.hpp (5)
ptr_(71-71)ptr_(74-74)ptr_(80-83)ptr_(86-86)static_cast(77-77)
operators/gstreamer/gst/video_info.cpp (2)
operators/gstreamer/gst/video_info.hpp (5)
VideoInfo(42-42)plane_index(55-55)plane_index(62-62)plane_index(69-69)plane_index(76-76)operators/gstreamer/gst/caps.cpp (2)
get_size(69-69)get_size(69-69)
operators/gstreamer/gst/buffer.hpp (4)
operators/gstreamer/gst_src_bridge.cpp (1)
buffer(444-444)operators/gstreamer/gst_src_bridge.hpp (1)
buffer(98-98)operators/gstreamer/gst/memory.hpp (2)
info(57-57)info(63-63)operators/gstreamer/gst/buffer.cpp (2)
flags(34-37)flags(34-34)
operators/gstreamer/gst/object.hpp (1)
operators/gstreamer/gst/error.hpp (3)
ptr_(54-54)ptr_(60-60)static_cast(66-66)
operators/gstreamer/gst_src_bridge.hpp (1)
operators/gstreamer/gst_src_bridge.cpp (3)
GstSrcBridge(310-373)GstSrcBridge(375-375)buffer(444-444)
applications/gstreamer/gst_video_recorder/gst_video_recorder.cpp (1)
operators/gstreamer/gst_video_recorder_operator.hpp (2)
spec(73-73)input(96-96)
operators/gstreamer/gst/buffer.cpp (4)
operators/gstreamer/gst/object.hpp (1)
static_cast(77-77)operators/gstreamer/gst/buffer.hpp (4)
memory(67-67)idx(80-80)info(88-88)info(94-94)operators/gstreamer/gst/memory.hpp (4)
Memory(41-41)Memory(41-41)info(57-57)info(63-63)operators/gstreamer/gst/memory.cpp (4)
map(32-35)map(32-32)unmap(37-39)unmap(37-37)
operators/gstreamer/gst_src_bridge.cpp (5)
operators/gstreamer/gst/memory.hpp (2)
Memory(41-41)Memory(41-41)operators/gstreamer/gst/caps.cpp (4)
to_string(82-86)to_string(82-82)Caps(28-28)Caps(30-34)operators/gstreamer/gst_src_bridge.hpp (6)
GstSrcBridge(57-57)GstSrcBridge(62-62)GstSrcBridge(65-65)GstSrcBridge(67-67)buffer(98-98)tensor_map(113-113)operators/gstreamer/gst/caps.hpp (2)
Caps(48-48)Caps(55-55)operators/gstreamer/gst/buffer.hpp (1)
memory(67-67)
operators/gstreamer/gst/video_info.hpp (1)
operators/gstreamer/gst_src_bridge.cpp (1)
caps(324-324)
🪛 Clang (14.0.6)
operators/gstreamer/gst/config.hpp
[error] 21-21: 'gst/gstversion.h' file not found
(clang-diagnostic-error)
operators/gstreamer/gst_video_recorder_operator.hpp
[error] 21-21: 'map' file not found
(clang-diagnostic-error)
operators/gstreamer/gst/message.hpp
[error] 21-21: 'gst/gst.h' file not found
(clang-diagnostic-error)
operators/gstreamer/gst/memory.hpp
[error] 21-21: 'gst/gst.h' file not found
(clang-diagnostic-error)
operators/gstreamer/gst/caps.hpp
[error] 21-21: 'gst/gst.h' file not found
(clang-diagnostic-error)
operators/gstreamer/gst/error.hpp
[error] 21-21: 'glib.h' file not found
(clang-diagnostic-error)
operators/gstreamer/gst/buffer.hpp
[error] 21-21: 'gst/gst.h' file not found
(clang-diagnostic-error)
operators/gstreamer/gst/object.hpp
[error] 21-21: 'gst/gst.h' file not found
(clang-diagnostic-error)
operators/gstreamer/gst_src_bridge.hpp
[error] 21-21: 'gst/app/gstappsrc.h' file not found
(clang-diagnostic-error)
applications/gstreamer/gst_video_recorder/gst_video_recorder.cpp
[error] 1-1: 'iostream' file not found
(clang-diagnostic-error)
operators/gstreamer/gst/video_info.hpp
[error] 21-21: 'gst/video/video.h' file not found
(clang-diagnostic-error)
🪛 GitHub Actions: Check CLI
operators/gstreamer/CMakeLists.txt
[error] 23-23: CMake could not find a package configuration file provided by "holoscan". Add holoscan to CMAKE_PREFIX_PATH or set holoscan_DIR to a directory containing holoscanConfig.cmake or holoscan-config.cmake.
🪛 GitHub Actions: Check Compliance
applications/gstreamer/gst_video_recorder/install_deps.sh
[error] 1-1: Copyright header missing or formatted incorrectly (manual fix required)
applications/gstreamer/gst_video_recorder/gst_video_recorder.cpp
[error] 1-1: Copyright header missing or formatted incorrectly (manual fix required)
🪛 GitHub Actions: Check linting
applications/gstreamer/gst_video_recorder/gst_video_recorder.cpp
[error] 1-1: cpplint reported 109 errors during C++ linting in the 'holohub lint' step (e.g., missing copyright header, trailing whitespace, line length, braces/else formatting). Command './holohub lint' failed.
🪛 GitHub Actions: Check metadata validity
applications/gstreamer/gst_video_recorder/metadata.json
[error] 1-1: invalid: 'requirements' is a required property
🪛 LanguageTool
applications/gstreamer/gst_video_recorder/README.md
[grammar] ~83-~83: Ensure spelling is correct
Context: ...les #### Record 10 seconds of video at 30fps (300 frames) ```bash gst-video-recorde...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~89-~89: Use a hyphen to join words.
Context: ...t 300 -o video.mp4 #### Record high quality H.265 video bash gst-video-r...
(QB_NEW_EN_HYPHEN)
🪛 markdownlint-cli2 (0.18.1)
applications/gstreamer/gst_video_recorder/README.md
140-140: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
179-179: Bare URL used
(MD034, no-bare-urls)
🔇 Additional comments (50)
.devcontainer/devcontainer.json (1)
36-36: LGTM! GPU runtime flag enables CUDA GStreamer support.Adding
"--runtime=nvidia"is the correct and sufficient way to enable GPU acceleration inside the container for the CUDA-enabled GStreamer components introduced in this PR. This flag complements the existing"NVIDIA_DRIVER_CAPABILITIES": "all"setting on line 45.Verify that the GStreamer and CUDA dependencies are properly installed in the Dockerfile (
.devcontainer/Dockerfile), since the runtime flag alone only provides access to the NVIDIA runtime—the libraries and development headers must still be present in the container image.operators/gstreamer/gst/error.hpp (1)
37-70: LGTM!The RAII wrapper for GError is well-designed with proper lifetime management through std::shared_ptr and a custom deleter. The null-check in the deleter prevents issues with nullptr, and the explicit constructor prevents implicit conversions.
operators/gstreamer/gst/video_info.hpp (1)
35-110: LGTM!The VideoInfo class provides a clean wrapper around GstVideoInfo with comprehensive accessors for video metadata. The API design is well-suited for both packed and planar video formats, with proper per-plane accessors. Documentation clearly indicates error handling behavior.
operators/gstreamer/gst/caps.hpp (1)
41-106: LGTM!The Caps wrapper provides a well-designed API with proper RAII semantics through inheritance from Object<>. The use of std::optional for get_video_info() correctly handles the case where caps may not contain video information. Default parameters for index make the API more convenient for the common single-structure case.
operators/gstreamer/gst/caps.cpp (7)
28-34: LGTM!The constructors are well-implemented. The string constructor properly validates the parsed caps and provides a clear error message on failure.
36-43: LGTM!Proper bounds checking and null-safety. The method gracefully handles invalid inputs by returning nullptr.
45-52: LGTM!Consistent defensive coding with proper validation. The method follows the same pattern as get_structure_name.
54-61: LGTM!The video caps detection logic is correct, checking both for null media_type and "video/" prefix. The VideoInfo constructor will handle validation of the actual video caps structure.
63-69: LGTM!Both methods correctly delegate to GStreamer APIs. The comment about GStreamer semantics for is_empty() is helpful.
71-80: LGTM!The has_feature implementation correctly iterates through all structures to find the feature. Input validation is thorough.
82-86: LGTM!Proper use of std::unique_ptr with custom deleter for managing the gchar* returned by GStreamer. The nullptr check prevents issues.
operators/gstreamer/gst/video_info.cpp (11)
27-31: LGTM!The constructor properly validates that the caps contain valid video information and provides a clear error message on failure.
33-33: LGTM!Correctly uses the GStreamer macro to get the total buffer size.
35-40: LGTM!Proper validation of plane_index with safe fallback to 0 for invalid indices.
42-47: LGTM!Consistent validation validation pattern with get_stride.
49-54: LGTM!Consistent validation and usage of GStreamer macros.
55-61: LGTM!The per-plane size calculation (stride × height) is correct for planar video formats. Reusing the existing accessor methods is clean.
[approve__code_changes]
63-63: LSLGTM!Correct
65-65: LGTM!
67-67: LGTM!
69-69: LGTM!
71-71: LGTM!operators/gstreamer/gst_src_bridge.hpp (1)
48-150: LGTM!The GstSrcBridge class is well-designed with:
- Clear separation of concerns between configuration and runtime state
- Proper prevention of copying/moving through deleted operators
- Well-documented public API
- Appropriate abstractions for different memory types (host/CUDA)
- Thoughtful frame counting for accurate timestamp calculation
The comment on frame_count_ explaining its purpose in avoiding rounding error accumulation is particularly helpful.
operators/CMakeLists.txt (1)
29-29: LGTM!The addition of the gstreamer subdirectory follows the established pattern and maintains alphabetical ordering with the surrounding operator entries.
applications/gstreamer/CMakeLists.txt (1)
1-17: LGTM!The CMake configuration correctly registers the new GStreamer video recorder application target.
applications/CMakeLists.txt (1)
59-60: LGTM!The build ordering correctly ensures that GStreamer components are configured before the h264 subdirectory, enabling the new gst_video_recorder application and GStreamer bridge integration.
applications/gstreamer/gst_video_recorder/README.md (1)
1-181: Excellent documentation!The README provides comprehensive coverage of the application's capabilities, usage patterns, and architecture. The examples are clear and cover various use cases effectively.
applications/gstreamer/gst_video_recorder/Dockerfile (1)
1-99: LGTM!The multi-stage Dockerfile is well-structured with proper dependency installation, cleanup, and environment configuration. It includes all necessary GStreamer plugins and development libraries for the video recorder application.
operators/gstreamer/gst/config.hpp (1)
23-30: LGTM!The compile-time CUDA support detection based on GStreamer version is correctly implemented. The static analysis error about missing 'gst/gstversion.h' is expected and will be resolved when GStreamer development packages are installed.
operators/gstreamer/gst/message.cpp (1)
23-43: LGTM!Both
parse_error()implementations correctly extract error information from GStreamer messages with proper memory management. The debug string is properly freed withg_free(), and theErrorwrapper takes ownership of theGErrorfor automatic cleanup.operators/gstreamer/gst/memory.cpp (1)
27-39: LGTM!All three methods implement proper null-safety checks before delegating to GStreamer APIs. The implementation follows the established pattern from
buffer.cppand correctly handles edge cases with appropriate return values.applications/gstreamer/gst_video_recorder/CMakeLists.txt (1)
32-32: Verify the install destination path.The install destination
bin/gst_video_recorderwill create a subdirectory underbin/, resulting in the executable being installed atbin/gst_video_recorder/gst-video-recorder. Typically, executables are installed directly tobin/.Confirm whether this nested structure is intentional or if it should be:
install(TARGETS gst-video-recorder DESTINATION bin)operators/gstreamer/gst_video_recorder_operator.hpp (3)
35-73: Excellent documentation!The class and method documentation is comprehensive, clearly explaining the operator's purpose, parameters, automatic detection features, and default values. This will greatly help users understand how to use the operator.
110-117: Design choices for flexible configuration.The use of
std::stringfor framerate andstd::map<std::string, std::string>for encoder properties enables flexible configuration at runtime. This is appropriate for bridging to GStreamer's dynamic property system.
118-127: LGTM on runtime member design.The runtime members use appropriate types:
shared_ptrfor the bridge (enabling shared ownership),futurefor asynchronous bus monitoring (non-blocking), and simple size_t for frame counting. The design supports proper resource management and concurrent operation.operators/gstreamer/gst/buffer.hpp (2)
32-49: Solid RAII design with explicit constructor.The Buffer class provides a well-designed RAII wrapper with an explicit constructor to prevent implicit conversions. The inheritance from the Object template ensures proper reference counting and automatic cleanup.
51-94: Well-designed Buffer API.The method set provides comprehensive access to GstBuffer operations with appropriate const-correctness. The API is intuitive and aligns well with GStreamer conventions while providing C++ RAII benefits.
operators/gstreamer/gst/message.hpp (1)
31-60: Clean Message wrapper with convenient error parsing.The Message class provides a clean RAII wrapper with two convenient overloads for error parsing. The design allows users to choose whether they need debug information, and the documentation clearly indicates automatic memory management.
operators/gstreamer/gst/memory.hpp (1)
28-64: Well-designed Memory wrapper.The Memory class provides a clean RAII wrapper with flexible
get_sizes()(optional parameters) and propermap()/unmap()pair for scoped memory access. The API design aligns well with GStreamer patterns while providing C++ safety.operators/gstreamer/gst/buffer.cpp (5)
29-32: LGTM on get_size() implementation.The method correctly validates the buffer before accessing and throws an appropriate exception on invalid state. This follows good defensive programming practices.
34-37: Correct flags() implementation.The use of
GST_BUFFER_FLAGSmacro and explicit cast toGstBufferFlagsis the standard GStreamer pattern for accessing buffer flags.
39-43: Correct ownership handling in append_memory().The implementation correctly handles GStreamer's ownership semantics by calling
memory.ref()before passing togst_buffer_append_memory(). The comment clearly explains this, which aids maintainability.
45-53: LGTM on memory accessor methods.Both
n_memory()andget_memory()correctly validate the buffer and properly handle the GstMemory lifecycle through the Memory wrapper.
55-62: Appropriate error handling in map/unmap.The error handling strategy is sound:
map()returns false to allow the caller to handle mapping failures, whileunmap()is safe to call on an invalid buffer (defensive programming). This design prevents exceptions in cleanup paths.operators/gstreamer/CMakeLists.txt (2)
113-121: Export and diagnostics look good.The export configuration and status messages are well-structured. Note that the pipeline failure regarding the missing holoscan package is an environment configuration issue (CMAKE_PREFIX_PATH needs to include the holoscan installation), not a code defect.
100-111: No action needed — the review comment is based on incorrect assumptions.The actual CMakeLists.txt at lines 100-111 contains only the 10 header files listed in the snippet. There is no
core/initializer.hppin the install list. Line 111 contains theDESTINATIONdirective, not a file reference. A previous search confirmedinitializer.hppdoes not exist in the operators/gstreamer directory.Likely an incorrect or invalid review comment.
operators/gstreamer/gst/object.hpp (4)
47-58: Excellent RAII design with floating reference handling.The constructor cleverly uses
shared_ptrwith a custom deleter to manage reference counting, and correctly handles GStreamer's floating reference pattern via the optionalRefSinkFunc. This provides proper lifetime management for GStreamer objects.
60-68: Correct special member defaults.The defaulted special members provide appropriate semantics: copy operations share ownership (via
shared_ptrcopy semantics, incrementing GStreamer refcount), and move operations transfer ownership efficiently. This is the correct design for a copyable RAII wrapper.
70-86: LGTM on accessor and ref() methods.The accessor methods are well-designed. The
ref()method is particularly important as it enables safe ownership transfer to GStreamer APIs that take ownership of passed objects, which is correctly utilized in the codebase (e.g.,buffer.cppline 42).
92-172: Well-designed helper functions and type aliases.The typed helper functions correctly handle GStreamer's gpointer-based APIs with appropriate casts. The comment on line 117 explaining the use of
reinterpret_castfor opaque C structs is helpful. The type aliases provide convenient, type-safe access to common GStreamer types with proper reference counting.
GilShpr
force-pushed
the
gshapira/holoscan_gstreamer_bridge
branch
from
ba85437 to
c7ea631
Compare
There was a problem hiding this comment.
28 files reviewed, no comments
Edit Code Review Agent Settings | Greptile
React with 👍 or 👎 to share your feedback on this new summary format
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
operators/gstreamer/CMakeLists.txt (1)
27-31: Make CUDA dependency optional to match conditional compilation design.The CMakeLists.txt requires
gstreamer-cuda-1.0(line 30), but the codebase has infrastructure for optional CUDA support via theHOLOSCAN_GSTREAMER_CUDA_SUPPORTmacro ingst/config.hpp. This creates a portability issue: builds will fail on systems with GStreamer < 1.24.0 that lack the gstreamer-cuda package, even though the code is designed to compile without CUDA support.Remove
REQUIREDfrom line 30 and conditionally include CUDA only when found:-pkg_check_modules(GSTREAMER_CUDA REQUIRED gstreamer-cuda-1.0) +pkg_check_modules(GSTREAMER_CUDA gstreamer-cuda-1.0)Then guard CUDA-specific includes and link directives (lines 64, 74, 83) with conditional checks:
target_include_directories(holoscan_gstreamer_bridge PUBLIC $<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}> $<INSTALL_INTERFACE:include> ${GSTREAMER_INCLUDE_DIRS} ${GSTREAMER_BASE_INCLUDE_DIRS} ${GSTREAMER_VIDEO_INCLUDE_DIRS} - ${GSTREAMER_CUDA_INCLUDE_DIRS} + $<$<BOOL:${GSTREAMER_CUDA_FOUND}>:${GSTREAMER_CUDA_INCLUDE_DIRS}> ${GSTREAMER_APP_INCLUDE_DIRS} )Apply similar conditionals to lines 74 and 83.
operators/gstreamer/gst_video_recorder_operator.cpp (1)
681-707: Ensure bus monitor thread is joined before dropping the last pipeline reference.The race condition is confirmed: when
bridge_is null,bus_monitor_future_.wait()is skipped (lines 692-695), butpipeline_.reset()is always executed (line 703), destroying the pipeline while the monitor thread may still be accessing the bus. This can cause undefined behavior.Always wait for
bus_monitor_future_before resetting the pipeline:void GstVideoRecorderOperator::stop() { HOLOSCAN_LOG_INFO("GstVideoRecorderOperator::stop() - Recording stopping"); - // Send EOS to signal end of stream (only if bridge was initialized) - if (bridge_) { - HOLOSCAN_LOG_INFO("Sending EOS to bridge"); - bridge_->send_eos(); - - HOLOSCAN_LOG_INFO("GstVideoRecorderOperator::stop() - EOS sent, waiting for pipeline to finish"); - - // Wait for pipeline to finish processing (EOS message on bus) - if (bus_monitor_future_.valid()) { - bus_monitor_future_.wait(); - HOLOSCAN_LOG_INFO("GstVideoRecorderOperator::stop() - Pipeline finished processing"); - } - } else { - HOLOSCAN_LOG_INFO("Bridge was never initialized, no frames were processed"); - } - - // Stop and cleanup pipeline - if (pipeline_ && pipeline_.get() && GST_IS_ELEMENT(pipeline_.get())) { - gst_element_set_state(pipeline_.get(), GST_STATE_NULL); - pipeline_.reset(); - } + // Send EOS to signal end of stream (only if bridge was initialized) + if (bridge_) { + HOLOSCAN_LOG_INFO("Sending EOS to bridge"); + bridge_->send_eos(); + } else { + HOLOSCAN_LOG_INFO("Bridge was never initialized, no frames were processed"); + } + + // Request the pipeline to stop but keep it alive until the bus-monitor task exits. + if (pipeline_ && pipeline_.get() && GST_IS_ELEMENT(pipeline_.get())) { + gst_element_set_state(pipeline_.get(), GST_STATE_NULL); + } + + // Wait for pipeline to finish processing (EOS or state-changed to NULL on the bus) + if (bus_monitor_future_.valid()) { + HOLOSCAN_LOG_INFO("GstVideoRecorderOperator::stop() - Waiting for pipeline bus monitor"); + bus_monitor_future_.wait(); + HOLOSCAN_LOG_INFO("GstVideoRecorderOperator::stop() - Pipeline bus monitor finished"); + } + + // Now it's safe to drop the last reference to the pipeline. + if (pipeline_ && pipeline_.get() && GST_IS_ELEMENT(pipeline_.get())) { + pipeline_.reset(); + } HOLOSCAN_LOG_INFO("GstVideoRecorderOperator::stop() - Stop complete"); }operators/gstreamer/gst_src_bridge.cpp (1)
269-302: Avoid wrapping GPU tensors as host memory and handlecreate_memory_wrapperfailures safely.Two critical issues confirmed:
Unsafe fallback for GPU tensors without CUDA support (lines 288-302)
Whenis_device_memory && cuda_requestedbutHOLOSCAN_GSTREAMER_CUDA_SUPPORTis not defined, control falls through to line 300, creating aHostMemoryWrapperfor a GPU device pointer. This causes undefined behavior when CPU code attempts to access it.Potential null pointer dereference
create_memory_wrappercan returnnullptr(line 276 on caps parse failure), but the call site increate_buffer_from_tensor_map(around line 473) lacks a null check before dereferencing.Apply these fixes:
// Use CudaMemoryWrapper for GPU tensors when GStreamer requests CUDA memory if (is_device_memory && cuda_requested) { #if HOLOSCAN_GSTREAMER_CUDA_SUPPORT HOLOSCAN_LOG_INFO("Creating CUDA memory wrapper ({} memory)", storage_type_str); return std::make_shared<GstSrcBridge::CudaMemoryWrapper>(video_format); #else - HOLOSCAN_LOG_WARN( - "CUDA memory requested but not available (requires GStreamer 1.24+). Falling back to host " - "memory."); + HOLOSCAN_LOG_ERROR( + "CUDA memory requested in caps ('{}') for a GPU tensor, but the code was built without " + "HOLOSCAN_GSTREAMER_CUDA_SUPPORT. Cannot safely wrap device memory. " + "Rebuild with CUDA support or use host storage.", + caps); + return nullptr; #endif // HOLOSCAN_GSTREAMER_CUDA_SUPPORT } // Use HostMemoryWrapper for CPU memory or when CUDA not requested HOLOSCAN_LOG_INFO("Creating host memory wrapper ({} memory)", storage_type_str); return std::make_shared<GstSrcBridge::HostMemoryWrapper>(video_format);And add null check in
create_buffer_from_tensor_map(around line 473):try { memory_wrapper_ = create_memory_wrapper(tensor_ptr.get(), caps_string_); + if (!memory_wrapper_) { + HOLOSCAN_LOG_ERROR( + "Memory wrapper creation returned null for tensor '{}'; aborting buffer creation", + name); + return buffer; + } } catch (const std::exception& e) { HOLOSCAN_LOG_ERROR("Failed to create memory wrapper: {}", e.what()); return buffer; }applications/gstreamer/gst_video_recorder/gst_video_recorder.cpp (1)
1-12: Add the standard license/SPDX header to satisfy compliance and linting.The file is missing the required Apache-2.0 SPDX header, causing CI "Check Compliance" failures. Add the standard header before any includes:
+/* + * SPDX-FileCopyrightText: Copyright (c) 2025 NVIDIA CORPORATION & AFFILIATES. All rights reserved. + * SPDX-License-Identifier: Apache-2.0 + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + #include <iostream>Then run
./holohub lintto address any remaining formatting issues.
🧹 Nitpick comments (3)
operators/gstreamer/gst/memory.cpp (1)
27-30: Consider consistent error handling with Buffer::get_size().
Memory::get_sizes()returns 0 on null, whileBuffer::get_size()(in buffer.cpp, Line 30) throws on invalid buffer. Since both are size-query operations on similar RAII wrappers, consistent error handling would improve API predictability.If you prefer throwing on invalid state:
gsize Memory::get_sizes(gsize* offset, gsize* max_size) const { - if (!get()) return 0; + if (!get()) throw std::runtime_error("Invalid memory"); return gst_memory_get_sizes(get(), offset, max_size); }applications/gstreamer/gst_video_recorder/CMakeLists.txt (1)
21-32: Consider consistent naming between target and install destination.The executable target is
gst-video-recorder(hyphen) but installs tobin/gst_video_recorder(underscore). This inconsistency might confuse users looking for the installed binary.Option 1: Use hyphen for both:
-install(TARGETS gst-video-recorder DESTINATION bin/gst_video_recorder) +install(TARGETS gst-video-recorder DESTINATION bin)Option 2: Use underscore for both:
-add_executable(gst-video-recorder +add_executable(gst_video_recorder gst_video_recorder.cpp ) -target_link_libraries(gst-video-recorder +target_link_libraries(gst_video_recorder PRIVATE holoscan::core holoscan_gstreamer_bridge ) -install(TARGETS gst-video-recorder DESTINATION bin/gst_video_recorder) +install(TARGETS gst_video_recorder DESTINATION bin/gst_video_recorder)operators/gstreamer/gst/caps.cpp (1)
54-61: Consider exception safety for VideoInfo construction.The VideoInfo constructor (line 60) can throw std::runtime_error (as seen in video_info.cpp lines 27-30). If invalid video caps are passed after the prefix check, the exception will propagate to the caller. Consider whether you want to:
- Let the exception propagate (current behavior - acceptable if documented)
- Catch and return std::nullopt for a more graceful API
The current behavior is acceptable if the function is documented to throw on invalid video caps.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
.devcontainer/devcontainer.json(1 hunks)applications/CMakeLists.txt(1 hunks)applications/gstreamer/CMakeLists.txt(1 hunks)applications/gstreamer/gst_video_recorder/CMakeLists.txt(1 hunks)applications/gstreamer/gst_video_recorder/Dockerfile(1 hunks)applications/gstreamer/gst_video_recorder/README.md(1 hunks)applications/gstreamer/gst_video_recorder/gst_video_recorder.cpp(1 hunks)applications/gstreamer/gst_video_recorder/install_deps.sh(1 hunks)applications/gstreamer/gst_video_recorder/metadata.json(1 hunks)operators/CMakeLists.txt(1 hunks)operators/gstreamer/CMakeLists.txt(1 hunks)operators/gstreamer/gst/buffer.cpp(1 hunks)operators/gstreamer/gst/buffer.hpp(1 hunks)operators/gstreamer/gst/caps.cpp(1 hunks)operators/gstreamer/gst/caps.hpp(1 hunks)operators/gstreamer/gst/config.hpp(1 hunks)operators/gstreamer/gst/error.hpp(1 hunks)operators/gstreamer/gst/memory.cpp(1 hunks)operators/gstreamer/gst/memory.hpp(1 hunks)operators/gstreamer/gst/message.cpp(1 hunks)operators/gstreamer/gst/message.hpp(1 hunks)operators/gstreamer/gst/object.hpp(1 hunks)operators/gstreamer/gst/video_info.cpp(1 hunks)operators/gstreamer/gst/video_info.hpp(1 hunks)operators/gstreamer/gst_src_bridge.cpp(1 hunks)operators/gstreamer/gst_src_bridge.hpp(1 hunks)operators/gstreamer/gst_video_recorder_operator.cpp(1 hunks)operators/gstreamer/gst_video_recorder_operator.hpp(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- applications/CMakeLists.txt
- applications/gstreamer/gst_video_recorder/Dockerfile
- applications/gstreamer/gst_video_recorder/metadata.json
- .devcontainer/devcontainer.json
- operators/CMakeLists.txt
- applications/gstreamer/gst_video_recorder/install_deps.sh
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-22T23:47:42.896Z
Learnt from: cdinea
Repo: nvidia-holoscan/holohub PR: 1170
File: applications/video_streaming/video_streaming_server/cpp/CMakeLists.txt:109-111
Timestamp: 2025-10-22T23:47:42.896Z
Learning: In the video streaming server application (applications/video_streaming/video_streaming_server/cpp/CMakeLists.txt), bundling libcudart.so.12 from the NGC operator binaries is intentional to ensure consistency with NGC binaries, even though the target links to CUDA::cudart.
Applied to files:
operators/gstreamer/CMakeLists.txt
🧬 Code graph analysis (15)
operators/gstreamer/gst/object.hpp (1)
operators/gstreamer/gst/error.hpp (3)
ptr_(54-54)ptr_(60-60)static_cast(66-66)
operators/gstreamer/gst/message.cpp (2)
operators/gstreamer/gst/error.hpp (2)
Error(43-48)Error(43-43)operators/gstreamer/gst/message.hpp (1)
debug_info(59-59)
operators/gstreamer/gst/video_info.cpp (2)
operators/gstreamer/gst/video_info.hpp (5)
VideoInfo(42-42)plane_index(55-55)plane_index(62-62)plane_index(69-69)plane_index(76-76)operators/gstreamer/gst/caps.cpp (2)
get_size(69-69)get_size(69-69)
operators/gstreamer/gst/memory.cpp (3)
operators/gstreamer/gst/memory.hpp (3)
offset(49-49)info(57-57)info(63-63)operators/gstreamer/gst/buffer.cpp (6)
map(55-58)map(55-55)flags(34-37)flags(34-34)unmap(60-62)unmap(60-60)operators/gstreamer/gst/buffer.hpp (2)
info(88-88)info(94-94)
operators/gstreamer/gst/memory.hpp (1)
operators/gstreamer/gst/buffer.hpp (3)
memory(67-67)info(88-88)info(94-94)
operators/gstreamer/gst_video_recorder_operator.cpp (3)
operators/gstreamer/gst/caps.cpp (2)
to_string(82-86)to_string(82-82)operators/gstreamer/gst/message.hpp (3)
Message(44-44)Message(44-44)debug_info(59-59)operators/gstreamer/gst_src_bridge.hpp (2)
tensor_map(113-113)buffer(98-98)
operators/gstreamer/gst/buffer.cpp (4)
operators/gstreamer/gst/object.hpp (1)
static_cast(77-77)operators/gstreamer/gst/buffer.hpp (4)
memory(67-67)idx(80-80)info(88-88)info(94-94)operators/gstreamer/gst/memory.hpp (4)
Memory(41-41)Memory(41-41)info(57-57)info(63-63)operators/gstreamer/gst/memory.cpp (4)
map(32-35)map(32-32)unmap(37-39)unmap(37-37)
operators/gstreamer/gst_src_bridge.hpp (1)
operators/gstreamer/gst_src_bridge.cpp (3)
GstSrcBridge(310-373)GstSrcBridge(375-375)buffer(444-444)
operators/gstreamer/gst/caps.hpp (1)
operators/gstreamer/gst_src_bridge.cpp (1)
caps(324-324)
operators/gstreamer/gst/caps.cpp (2)
operators/gstreamer/gst/caps.hpp (5)
Caps(48-48)Caps(55-55)index(63-63)fieldname(72-72)feature_name(99-99)operators/gstreamer/gst/video_info.cpp (1)
VideoInfo(27-31)
operators/gstreamer/gst_src_bridge.cpp (7)
operators/gstreamer/gst/memory.hpp (2)
Memory(41-41)Memory(41-41)operators/gstreamer/gst/error.hpp (1)
static_cast(66-66)operators/gstreamer/gst/object.hpp (1)
static_cast(77-77)operators/gstreamer/gst/caps.cpp (4)
to_string(82-86)to_string(82-82)Caps(28-28)Caps(30-34)operators/gstreamer/gst_src_bridge.hpp (6)
GstSrcBridge(57-57)GstSrcBridge(62-62)GstSrcBridge(65-65)GstSrcBridge(67-67)buffer(98-98)tensor_map(113-113)operators/gstreamer/gst/caps.hpp (2)
Caps(48-48)Caps(55-55)operators/gstreamer/gst/buffer.hpp (1)
memory(67-67)
operators/gstreamer/gst/buffer.hpp (4)
operators/gstreamer/gst_src_bridge.cpp (1)
buffer(444-444)operators/gstreamer/gst_src_bridge.hpp (1)
buffer(98-98)operators/gstreamer/gst/memory.hpp (2)
info(57-57)info(63-63)operators/gstreamer/gst/buffer.cpp (2)
flags(34-37)flags(34-34)
applications/gstreamer/gst_video_recorder/gst_video_recorder.cpp (1)
operators/gstreamer/gst_video_recorder_operator.hpp (2)
spec(73-73)input(96-96)
operators/gstreamer/gst/error.hpp (1)
operators/gstreamer/gst/object.hpp (5)
ptr_(71-71)ptr_(74-74)ptr_(80-83)ptr_(86-86)static_cast(77-77)
operators/gstreamer/gst_video_recorder_operator.hpp (1)
applications/gstreamer/gst_video_recorder/gst_video_recorder.cpp (4)
spec(510-519)spec(510-510)input(544-565)input(544-544)
🪛 Clang (14.0.6)
operators/gstreamer/gst/object.hpp
[error] 21-21: 'gst/gst.h' file not found
(clang-diagnostic-error)
operators/gstreamer/gst/memory.hpp
[error] 21-21: 'gst/gst.h' file not found
(clang-diagnostic-error)
operators/gstreamer/gst/message.hpp
[error] 21-21: 'gst/gst.h' file not found
(clang-diagnostic-error)
operators/gstreamer/gst_src_bridge.hpp
[error] 21-21: 'gst/app/gstappsrc.h' file not found
(clang-diagnostic-error)
operators/gstreamer/gst/video_info.hpp
[error] 21-21: 'gst/video/video.h' file not found
(clang-diagnostic-error)
operators/gstreamer/gst/caps.hpp
[error] 21-21: 'gst/gst.h' file not found
(clang-diagnostic-error)
operators/gstreamer/gst/buffer.hpp
[error] 21-21: 'gst/gst.h' file not found
(clang-diagnostic-error)
applications/gstreamer/gst_video_recorder/gst_video_recorder.cpp
[error] 1-1: 'iostream' file not found
(clang-diagnostic-error)
operators/gstreamer/gst/config.hpp
[error] 21-21: 'gst/gstversion.h' file not found
(clang-diagnostic-error)
operators/gstreamer/gst/error.hpp
[error] 21-21: 'glib.h' file not found
(clang-diagnostic-error)
operators/gstreamer/gst_video_recorder_operator.hpp
[error] 21-21: 'map' file not found
(clang-diagnostic-error)
🪛 GitHub Actions: Check Compliance
applications/gstreamer/gst_video_recorder/gst_video_recorder.cpp
[error] 1-1: Copyright header missing or formatted incorrectly (manual fix required)
🪛 LanguageTool
applications/gstreamer/gst_video_recorder/README.md
[grammar] ~83-~83: Ensure spelling is correct
Context: ...les #### Record 10 seconds of video at 30fps (300 frames) ```bash gst-video-recorde...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~89-~89: Use a hyphen to join words.
Context: ...t 300 -o video.mp4 #### Record high quality H.265 video bash gst-video-r...
(QB_NEW_EN_HYPHEN)
🪛 markdownlint-cli2 (0.18.1)
applications/gstreamer/gst_video_recorder/README.md
140-140: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
179-179: Bare URL used
(MD034, no-bare-urls)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: check-urls
🔇 Additional comments (51)
operators/gstreamer/gst/error.hpp (1)
37-70: LGTM!The RAII wrapper correctly manages GError lifetime using shared_ptr with a custom deleter that calls g_error_free. The design appropriately uses shared_ptr semantics for GError (which uses copy/free, not ref-counting), and the accessor methods follow standard patterns.
operators/gstreamer/gst_video_recorder_operator.hpp (1)
45-128: LGTM!The operator header is well-structured with comprehensive documentation. The parameter specifications clearly document defaults, formats, and special cases (e.g., "0/1" for live mode). The private member organization is logical, separating parameters, GStreamer management objects, and tracking state.
operators/gstreamer/gst/config.hpp (1)
23-30: LGTM!The conditional compilation approach cleanly gates CUDA support based on GStreamer version. This allows the code to compile gracefully on systems with GStreamer < 1.24.0 when paired with optional CMake detection.
operators/gstreamer/gst/message.cpp (1)
23-43: LGTM!Both
parse_errorimplementations correctly handle GStreamer memory management. The version with debug info properly copies the C string, frees it withg_free, and handles the nullptr case appropriately.operators/gstreamer/gst/object.hpp (1)
47-172: LGTM!The template-based RAII wrapper provides a clean abstraction for GStreamer's reference-counting model. The design correctly handles:
- Floating reference sinking via optional RefSinkFunc
- Reference counting through shared_ptr with custom deleters
- Type-safe wrappers via helper functions
- Conditional CUDA support
The use of
reinterpret_castat line 120 is appropriate given GStreamer's opaque C struct design.operators/gstreamer/CMakeLists.txt (1)
100-111: The review comment is based on incorrect information and should be dismissed.The actual CMakeLists.txt file (lines 100-111) does not contain
core/initializer.hpp. The install list ends withgst_video_recorder_operator.hpp, and line 111 is theDESTINATIONdirective. Additionally,core/initializer.hppdoes not exist anywhere in the repository. The code snippet shown in the review comment is accurate; the concern about this file is unfounded.Likely an incorrect or invalid review comment.
operators/gstreamer/gst/memory.cpp (2)
32-35: LGTM!The null-check and return-false pattern for
map()is consistent withBuffer::map()in buffer.cpp.
37-39: LGTM!The defensive no-op pattern for
unmap()on null is consistent withBuffer::unmap()in buffer.cpp.operators/gstreamer/gst/message.hpp (1)
31-60: LGTM!The
Messageclass follows the established RAII pattern with proper inheritance fromObjectand provides a clean interface for error parsing. The documentation is clear and comprehensive.Note: The static analysis hint about missing 'gst/gst.h' is a false positive—the header will be available in the build environment with GStreamer installed.
operators/gstreamer/gst/buffer.hpp (1)
32-95: LGTM!The
Bufferclass provides a comprehensive RAII wrapper forGstBufferwith well-documented methods. The interface is clean and follows the established pattern for GStreamer object management.Note: The static analysis hint about missing 'gst/gst.h' is a false positive—the header will be available in the build environment.
operators/gstreamer/gst/video_info.hpp (1)
28-110: LGTM!The
VideoInfoclass provides a clean abstraction overGstVideoInfowith comprehensive per-plane accessors. The documentation correctly notes thatGstVideoInfois a POD structure containing copied data, which clarifies the lifetime semantics.Note: The static analysis hint about missing 'gst/video/video.h' is a false positive—the header will be available in the build environment.
operators/gstreamer/gst/caps.hpp (1)
34-106: LGTM!The
Capsclass provides a comprehensive RAII wrapper forGstCapswith well-thought-out semantics. The use ofstd::optional<VideoInfo>forget_video_info()is appropriate, and the documentation clearly explains edge cases (e.g., empty caps vs. nullptr).Note: The static analysis hint about missing 'gst/gst.h' is a false positive—the header will be available in the build environment.
operators/gstreamer/gst/memory.hpp (1)
28-64: LGTM!The
Memoryclass provides a clean RAII wrapper forGstMemorywith appropriate methods for size queries and mapping operations. The interface aligns well with the implementation in memory.cpp.Note: The static analysis hint about missing 'gst/gst.h' is a false positive—the header will be available in the build environment.
operators/gstreamer/gst/buffer.cpp (6)
29-32: LGTM!Throwing on invalid buffer is appropriate for
get_size(), as querying the size of a null buffer indicates a logic error.
34-37: LGTM!Consistent error handling: throwing on invalid buffer for
flags()aligns withget_size()behavior.
39-43: LGTM!Proper ownership transfer via
memory.ref()ensuresgst_buffer_append_memoryreceives a new reference, as required by GStreamer semantics (where the function takes ownership).
45-48: LGTM!Consistent error handling for
n_memory().
50-53: LGTM!The method correctly wraps the returned
GstMemory*fromgst_buffer_get_memory()(which increments refcount) in theMemoryRAII wrapper.
55-62: LGTM!The map/unmap implementations use appropriate error handling:
map()returns false on invalid buffer (allowing callers to detect failure gracefully), whileunmap()is a defensive no-op on null (idempotent cleanup).operators/gstreamer/gst/caps.cpp (5)
28-28: LGTM!The constructor correctly forwards the GstCaps pointer to the Object base class.
30-34: LGTM!The string constructor correctly validates the parsed caps and provides a clear error message on failure.
36-43: LGTM!The function correctly validates the caps pointer and index bounds before accessing the structure.
45-52: LGTM!The function correctly validates inputs before accessing the structure field value.
63-86: LGTM!These utility functions are well-implemented:
is_empty()correctly documents GStreamer's nullptr behaviorhas_feature()properly validates inputs and iterates through all structuresto_string()uses RAII withunique_ptrand custom deleter for safe memory managementoperators/gstreamer/gst/video_info.cpp (2)
27-31: LGTM!The constructor correctly validates the result from
gst_video_info_from_capsand provides a clear error message on failure.
33-71: LGTM!All accessor functions are correctly implemented:
- Per-plane accessors properly validate
plane_indexbounds and return 0 for invalid indicesget_size(int plane_index)correctly calculates plane size as stride × height- All functions use appropriate GStreamer video info macros
operators/gstreamer/gst_src_bridge.hpp (1)
38-150: LGTM!The GstSrcBridge class is well-designed:
- Comprehensive documentation with usage notes
- Proper RAII with non-copyable/non-movable semantics
- Clear separation of concerns (memory wrappers, element management, buffer creation)
- Frame counter usage prevents timestamp rounding error accumulation
- Conditional CUDA support is properly gated
operators/gstreamer/gst_video_recorder_operator.cpp (11)
51-86: LGTM!The
normalize_frameratefunction correctly handles multiple input formats (integer, fraction, decimal) with proper validation and clear error messages.
101-126: LGTM!The
get_muxer_from_extensionfunction correctly handles extension mapping with case-insensitive comparison and provides sensible defaults.
137-189: LGTM!The
get_parser_from_encoderfunction uses GStreamer introspection for future-proof parser detection. The implementation correctly handles the factory lookup, pad template iteration, and codec extraction.
199-240: LGTM!The
monitor_pipeline_busfunction correctly implements bus monitoring with proper message handling and appropriate early exit conditions for errors, EOS, and pipeline state changes.
256-313: LGTM!The
create_bridge_from_tensor_mapfunction correctly detects video parameters and memory type. The device type classification properly distinguishes between device memory (kDLCUDA, kDLCUDAManaged) and host memory (including kDLCUDAHost for pinned memory).
324-337: LGTM!The
create_converter_elementfunction correctly selects the appropriate converter based on memory type.
350-448: LGTM!The
set_encoder_propertyfunction uses GStreamer introspection for type-safe property setting with comprehensive type support and proper error handling.
459-490: LGTM!The
add_and_link_source_converterfunction correctly manages GStreamer reference counting with explicitref()calls and properly sets element states before linking.
494-518: LGTM!The
setupmethod correctly configures the operator spec with well-documented parameters.
520-633: LGTM!The
startmethod correctly initializes the pipeline with auto-detection of parser and muxer. The deferred source/converter creation (added on first frame) is a good design that allows format detection from incoming data.
635-679: LGTM!The
computemethod correctly implements lazy initialization of the bridge on the first frame and properly validates buffers before pushing.operators/gstreamer/gst_src_bridge.cpp (8)
67-91: LGTM!The
MemoryWrapperabstract base class provides a clean abstraction with proper non-copyable/non-movable semantics.
96-116: LGTM!The
HostMemoryWrappercorrectly validates tensor data and usesgst_memory_new_wrappedfor zero-copy memory wrapping.
124-159: LGTM!The
CudaMemoryWrapperconstructor correctly initializes CUDA resources with proper error handling and informative error messages.
161-233: LGTM!The
wrap_memoryimplementation correctly handles CUDA device detection, lazy context creation, and device switching prevention. The video info setup and memory wrapping are properly implemented.
250-261: LGTM!The
TensorWrapperandfree_tensor_wrappercorrectly implement the GStreamer destroy notify pattern for tensor lifetime management.
310-373: LGTM!The
GstSrcBridgeconstructor correctly creates and configures the appsrc element with proper framerate parsing and live mode detection.
375-440: LGTM!These methods are correctly implemented:
push_bufferproperly usesref()for reference counting and handles all flow return codessend_eoscorrectly handles EOS signaling with appropriate loggingget_capssafely retrieves current caps from the pad
442-536: LGTM with noted null check issue.The
create_buffer_from_tensor_mapmethod is well-structured with proper timestamp calculation usinggst_util_uint64_scaleto prevent overflow. The null check issue formemory_wrapper_after line 473 is already addressed in the previous duplicate comment.applications/gstreamer/gst_video_recorder/gst_video_recorder.cpp (5)
13-288: LGTM!The helper functions and CLI parsing are well-implemented:
parse_validatedprovides comprehensive input validation with clear error messagesparse_argumentshandles all CLI options with proper validationprint_usageprovides comprehensive documentation with examples
301-501: LGTM!The pattern generator classes are well-designed:
- Clean template method pattern with abstract base class
- Animated patterns use appropriate time steps
- Checkerboard generator correctly guards against division by zero
- Color bars use
static constexprfor efficiency
506-574: LGTM!The
PatternGenOperatorcorrectly implements the Holoscan operator lifecycle with proper pattern generator selection and entity emission.
579-630: LGTM!The
GstVideoRecorderAppcorrectly composes the Holoscan pipeline, wiring the pattern generator to the GStreamer recorder with appropriate configuration.
634-678: LGTM!The
mainfunction correctly initializes GStreamer, parses arguments, and runs the application with appropriate error handling and logging.
GilShpr
force-pushed
the
gshapira/holoscan_gstreamer_bridge
branch
from
c7ea631 to
7e09ace
Compare
There was a problem hiding this comment.
28 files reviewed, no comments
Edit Code Review Agent Settings | Greptile
React with 👍 or 👎 to share your feedback on this new summary format
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (8)
applications/gstreamer/gst_video_recorder/README.md (2)
140-143: Suggest adding language specifier to fenced code block.The pipeline flow visualization would benefit from an explicit language specifier for consistent rendering across markdown parsers.
Apply this diff:
-``` +```text PatternGenOperator → GstVideoRecorderOperator → GStreamer Encoding Pipeline → File--- `179-179`: **Consider using markdown link syntax for better formatting.** The bare URL could be formatted as a proper markdown link for better readability and consistency. Apply this diff: ```diff -- GStreamer documentation: https://gstreamer.freedesktop.org/documentation/ +- [GStreamer documentation](https://gstreamer.freedesktop.org/documentation/)operators/gstreamer/gst/buffer.cpp (1)
55-62: Minor inconsistency in error handling betweenmapandunmap.
map()returnsfalseon invalid buffer (line 56), whileunmap()silently does nothing (line 61). This is acceptable since unmap is typically called after a successful map, but consider documenting this behavior if not already covered in the header.operators/gstreamer/gst/video_info.cpp (1)
35-61: Consider logging warnings for invalid plane indices.The per-plane accessor methods (lines 35-61) silently return
0whenplane_indexis out of bounds. While this prevents crashes, it could mask bugs where callers pass incorrect indices. Consider adding debug-level logging to help diagnose incorrect usage during development.Example for one method:
gsize VideoInfo::get_stride(int plane_index) const { if (plane_index < 0 || static_cast<guint>(plane_index) >= video_info_.finfo->n_planes) { + HOLOSCAN_LOG_DEBUG("Invalid plane_index {} (n_planes={})", plane_index, video_info_.finfo->n_planes); return 0; } return GST_VIDEO_INFO_PLANE_STRIDE(&video_info_, plane_index); }operators/gstreamer/gst_video_recorder_operator.cpp (1)
179-179: Address cpplint line length violations.Multiple lines exceed the 100-character limit. While not critical, addressing these will satisfy the linting pipeline.
Consider breaking long lines, especially in string literals and parameter lists. For example, line 501 could be split:
- "Encoder base name (e.g., nvh264, nvh265, x264, x265). 'enc' suffix is appended automatically.", + "Encoder base name (e.g., nvh264, nvh265, x264, x265). " + "'enc' suffix is appended automatically.",Also applies to: 205-205, 489-489, 501-501, 535-535, 576-576, 617-617, 639-639, 648-648, 650-650
applications/gstreamer/gst_video_recorder/gst_video_recorder.cpp (3)
73-73: Remove trailing whitespace to fix linting failures.Several lines have trailing whitespace which causes cpplint failures. Remove the extra spaces at the end of these lines.
Also applies to: 122-122, 127-127, 131-131, 134-134, 279-279
228-293: Fix else-brace formatting to satisfy cpplint.The else-if chain (lines 228-293) uses a formatting style that doesn't match the project's cpplint configuration. The linter expects else statements to appear on the same line as the preceding closing brace.
Reformat from:
} else if (...) {To:
} else if (...) {This applies to the entire if-else chain in the argument parsing function.
173-175: Break long lines to satisfy 100-character limit.Lines 173-175 exceed the 100-character limit. Consider breaking the help text into multiple shorter lines.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
.devcontainer/devcontainer.json(1 hunks)applications/CMakeLists.txt(1 hunks)applications/gstreamer/CMakeLists.txt(1 hunks)applications/gstreamer/gst_video_recorder/CMakeLists.txt(1 hunks)applications/gstreamer/gst_video_recorder/Dockerfile(1 hunks)applications/gstreamer/gst_video_recorder/README.md(1 hunks)applications/gstreamer/gst_video_recorder/gst_video_recorder.cpp(1 hunks)applications/gstreamer/gst_video_recorder/install_deps.sh(1 hunks)applications/gstreamer/gst_video_recorder/metadata.json(1 hunks)operators/CMakeLists.txt(1 hunks)operators/gstreamer/CMakeLists.txt(1 hunks)operators/gstreamer/gst/buffer.cpp(1 hunks)operators/gstreamer/gst/buffer.hpp(1 hunks)operators/gstreamer/gst/caps.cpp(1 hunks)operators/gstreamer/gst/caps.hpp(1 hunks)operators/gstreamer/gst/config.hpp(1 hunks)operators/gstreamer/gst/error.hpp(1 hunks)operators/gstreamer/gst/memory.cpp(1 hunks)operators/gstreamer/gst/memory.hpp(1 hunks)operators/gstreamer/gst/message.cpp(1 hunks)operators/gstreamer/gst/message.hpp(1 hunks)operators/gstreamer/gst/object.hpp(1 hunks)operators/gstreamer/gst/video_info.cpp(1 hunks)operators/gstreamer/gst/video_info.hpp(1 hunks)operators/gstreamer/gst_src_bridge.cpp(1 hunks)operators/gstreamer/gst_src_bridge.hpp(1 hunks)operators/gstreamer/gst_video_recorder_operator.cpp(1 hunks)operators/gstreamer/gst_video_recorder_operator.hpp(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- operators/gstreamer/gst/memory.cpp
- operators/gstreamer/gst/caps.cpp
- .devcontainer/devcontainer.json
- operators/gstreamer/gst/message.cpp
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-11-10T20:29:02.443Z
Learnt from: bhashemian
Repo: nvidia-holoscan/holohub PR: 1226
File: utilities/cli/util.py:32-32
Timestamp: 2025-11-10T20:29:02.443Z
Learning: In the holohub repository, the `DEFAULT_BASE_SDK_VERSION` constant in `utilities/cli/util.py` is a fallback default value. The `tested_versions` fields in application `metadata.json` files (e.g., in `applications/multiai_ultrasound/*/metadata.json`) are maintained independently and do not need to match the `DEFAULT_BASE_SDK_VERSION`. Each application tracks its own tested versions separately.
Applied to files:
applications/gstreamer/gst_video_recorder/Dockerfile
📚 Learning: 2025-10-30T19:04:41.239Z
Learnt from: bhashemian
Repo: nvidia-holoscan/holohub PR: 1203
File: applications/video_streaming/video_streaming_server/cpp/metadata.json:42-51
Timestamp: 2025-10-30T19:04:41.239Z
Learning: In the video_streaming application structure, umbrella metadata (applications/video_streaming/metadata.json) uses language-qualified mode names like "server_cpp" and "server_python" to distinguish between implementations, while component-specific metadata files (e.g., applications/video_streaming/video_streaming_server/cpp/metadata.json) use simple mode names like "server" since they are already scoped to a specific language implementation by their directory structure.
Applied to files:
applications/gstreamer/gst_video_recorder/metadata.json
📚 Learning: 2025-10-22T16:53:45.393Z
Learnt from: cdinea
Repo: nvidia-holoscan/holohub PR: 1170
File: operators/video_streaming/streaming_client_enhanced/python/CMakeLists.txt:16-24
Timestamp: 2025-10-22T16:53:45.393Z
Learning: The pybind11_add_holohub_module CMake macro in cmake/pybind11_add_holohub_module.cmake encapsulates all pybind11 setup internally, including finding pybind11, linking against holoscan::pybind11 conditionally, and linking the C++ operator target. Operator Python bindings in holohub should only call this macro without additional pybind11 setup.
Applied to files:
operators/CMakeLists.txt
📚 Learning: 2025-10-20T22:19:33.520Z
Learnt from: chengronglai
Repo: nvidia-holoscan/holohub PR: 1136
File: benchmarks/realtime_threads_benchmarking/realtime_thread_benchmark.cpp:245-251
Timestamp: 2025-10-20T22:19:33.520Z
Learning: In the Holoscan framework, operators added via add_operator() can execute without explicitly attaching a scheduling condition - they will still be scheduled and their compute() method will be invoked by the runtime.
Applied to files:
operators/CMakeLists.txt
📚 Learning: 2025-10-22T23:47:42.896Z
Learnt from: cdinea
Repo: nvidia-holoscan/holohub PR: 1170
File: applications/video_streaming/video_streaming_server/cpp/CMakeLists.txt:109-111
Timestamp: 2025-10-22T23:47:42.896Z
Learning: In the video streaming server application (applications/video_streaming/video_streaming_server/cpp/CMakeLists.txt), bundling libcudart.so.12 from the NGC operator binaries is intentional to ensure consistency with NGC binaries, even though the target links to CUDA::cudart.
Applied to files:
operators/gstreamer/CMakeLists.txt
🧬 Code graph analysis (13)
operators/gstreamer/gst/object.hpp (1)
operators/gstreamer/gst/error.hpp (3)
ptr_(54-54)ptr_(60-60)static_cast(66-66)
operators/gstreamer/gst/video_info.hpp (1)
operators/gstreamer/gst_src_bridge.cpp (1)
caps(327-327)
operators/gstreamer/gst/buffer.hpp (4)
operators/gstreamer/gst_src_bridge.cpp (1)
buffer(447-447)operators/gstreamer/gst_src_bridge.hpp (1)
buffer(98-98)operators/gstreamer/gst/memory.hpp (2)
info(57-57)info(63-63)operators/gstreamer/gst/buffer.cpp (2)
flags(34-37)flags(34-34)
operators/gstreamer/gst_video_recorder_operator.hpp (1)
applications/gstreamer/gst_video_recorder/gst_video_recorder.cpp (4)
spec(527-536)spec(527-527)input(561-582)input(561-561)
operators/gstreamer/gst/error.hpp (1)
operators/gstreamer/gst/object.hpp (5)
ptr_(71-71)ptr_(74-74)ptr_(80-83)ptr_(86-86)static_cast(77-77)
operators/gstreamer/gst_video_recorder_operator.cpp (2)
operators/gstreamer/gst/message.hpp (3)
Message(44-44)Message(44-44)debug_info(59-59)operators/gstreamer/gst_src_bridge.hpp (2)
tensor_map(113-113)buffer(98-98)
operators/gstreamer/gst/video_info.cpp (1)
operators/gstreamer/gst/video_info.hpp (5)
VideoInfo(42-42)plane_index(55-55)plane_index(62-62)plane_index(69-69)plane_index(76-76)
operators/gstreamer/gst_src_bridge.hpp (1)
operators/gstreamer/gst_src_bridge.cpp (3)
GstSrcBridge(313-376)GstSrcBridge(378-378)buffer(447-447)
operators/gstreamer/gst/buffer.cpp (3)
operators/gstreamer/gst/buffer.hpp (4)
memory(67-67)idx(80-80)info(88-88)info(94-94)operators/gstreamer/gst/memory.hpp (4)
Memory(41-41)Memory(41-41)info(57-57)info(63-63)operators/gstreamer/gst/memory.cpp (4)
map(32-35)map(32-32)unmap(37-39)unmap(37-37)
operators/gstreamer/gst_src_bridge.cpp (5)
operators/gstreamer/gst/memory.hpp (2)
Memory(41-41)Memory(41-41)operators/gstreamer/gst/caps.cpp (4)
to_string(82-86)to_string(82-82)Caps(28-28)Caps(30-34)operators/gstreamer/gst_src_bridge.hpp (6)
GstSrcBridge(57-57)GstSrcBridge(62-62)GstSrcBridge(65-65)GstSrcBridge(67-67)buffer(98-98)tensor_map(113-113)operators/gstreamer/gst/caps.hpp (2)
Caps(48-48)Caps(55-55)operators/gstreamer/gst/buffer.hpp (1)
memory(67-67)
applications/gstreamer/gst_video_recorder/gst_video_recorder.cpp (1)
operators/gstreamer/gst_video_recorder_operator.hpp (2)
spec(73-73)input(96-96)
operators/gstreamer/gst/caps.hpp (1)
operators/gstreamer/gst_src_bridge.cpp (1)
caps(327-327)
operators/gstreamer/gst/memory.hpp (1)
operators/gstreamer/gst/buffer.hpp (3)
memory(67-67)info(88-88)info(94-94)
🪛 Clang (14.0.6)
operators/gstreamer/gst/object.hpp
[error] 21-21: 'gst/gst.h' file not found
(clang-diagnostic-error)
operators/gstreamer/gst/video_info.hpp
[error] 21-21: 'gst/video/video.h' file not found
(clang-diagnostic-error)
operators/gstreamer/gst/buffer.hpp
[error] 21-21: 'gst/gst.h' file not found
(clang-diagnostic-error)
operators/gstreamer/gst_video_recorder_operator.hpp
[error] 21-21: 'map' file not found
(clang-diagnostic-error)
operators/gstreamer/gst/error.hpp
[error] 21-21: 'glib.h' file not found
(clang-diagnostic-error)
operators/gstreamer/gst/config.hpp
[error] 21-21: 'gst/gstversion.h' file not found
(clang-diagnostic-error)
operators/gstreamer/gst_src_bridge.hpp
[error] 21-21: 'gst/app/gstappsrc.h' file not found
(clang-diagnostic-error)
applications/gstreamer/gst_video_recorder/gst_video_recorder.cpp
[error] 18-18: 'iostream' file not found
(clang-diagnostic-error)
operators/gstreamer/gst/caps.hpp
[error] 21-21: 'gst/gst.h' file not found
(clang-diagnostic-error)
operators/gstreamer/gst/memory.hpp
[error] 21-21: 'gst/gst.h' file not found
(clang-diagnostic-error)
operators/gstreamer/gst/message.hpp
[error] 21-21: 'gst/gst.h' file not found
(clang-diagnostic-error)
🪛 GitHub Actions: Check linting
operators/gstreamer/gst_video_recorder_operator.hpp
[error] 109-109: cpplint: Do not leave a blank line after "private:". [whitespace/blank_line]
[error] 113-113: cpplint: Lines should be <= 100 characters long. [whitespace/line_length]
[error] 116-116: cpplint: Lines should be <= 100 characters long. [whitespace/line_length]
[error] 117-117: cpplint: Line ends in whitespace. Consider deleting these extra spaces. [whitespace/end_of_line]
[error] 122-122: cpplint: Line ends in whitespace. Consider deleting these extra spaces. [whitespace/end_of_line]
[error] 125-125: cpplint: Line ends in whitespace. Consider deleting these extra spaces. [whitespace/end_of_line]
operators/gstreamer/gst_video_recorder_operator.cpp
[error] 179-179: cpplint: Lines should be <= 100 characters long. [whitespace/line_length]
[error] 205-205: cpplint: Lines should be <= 100 characters long. [whitespace/line_length]
[error] 312-312: cpplint: Closing ) should be moved to the previous line. [whitespace/parens]
[error] 489-489: cpplint: Lines should be <= 100 characters long. [whitespace/line_length]
[error] 501-501: cpplint: Lines should be <= 100 characters long. [whitespace/line_length]
[error] 535-535: cpplint: Lines should be <= 100 characters long. [whitespace/line_length]
[error] 576-576: cpplint: Lines should be <= 100 characters long. [whitespace/line_length]
[error] 617-617: cpplint: Lines should be <= 100 characters long. [whitespace/line_length]
[error] 639-639: cpplint: Lines should be <= 100 characters long. [whitespace/line_length]
[error] 648-648: cpplint: Lines should be <= 100 characters long. [whitespace/line_length]
[error] 650-650: cpplint: Lines should be <= 100 characters long. [whitespace/line_length]
[error] 650-650: cpplint: Lines should be <= 100 characters long. [whitespace/line_length]
applications/gstreamer/gst_video_recorder/gst_video_recorder.cpp
[error] 73-73: cpplint: Line ends in whitespace. Consider deleting these extra spaces. [whitespace/end_of_line]
[error] 122-122: cpplint: Line ends in whitespace. Consider deleting these extra spaces. [whitespace/end_of_line]
[error] 127-127: cpplint: Line ends in whitespace. Consider deleting these extra spaces. [whitespace/end_of_line]
[error] 131-131: cpplint: Line ends in whitespace. Consider deleting these extra spaces. [whitespace/end_of_line]
[error] 134-134: cpplint: Line ends in whitespace. Consider deleting these extra spaces. [whitespace/end_of_line]
[error] 173-173: cpplint: Lines should be <= 100 characters long. [whitespace/line_length]
[error] 174-174: cpplint: Lines should be <= 100 characters long. [whitespace/line_length]
[error] 175-175: cpplint: Lines should be <= 100 characters long. [whitespace/line_length]
[error] 228-228: cpplint: An else should appear on the same line as the preceding } [whitespace/newline]
[error] 228-228: cpplint: If an else has a brace on one side, it should have it on both [readability/braces]
[error] 232-232: cpplint: An else should appear on the same line as the preceding } [whitespace/newline]
[error] 232-232: cpplint: If an else has a brace on one side, it should have it on both [readability/braces]
[error] 240-240: cpplint: An else should appear on the same line as the preceding } [whitespace/newline]
[error] 240-240: cpplint: If an else has a brace on one side, it should have it on both [readability/braces]
[error] 247-247: cpplint: An else should appear on the same line as the preceding } [whitespace/newline]
[error] 247-247: cpplint: If an else has a brace on one side, it should have it on both [readability/braces]
[error] 251-251: cpplint: An else should appear on the same line as the preceding } [whitespace/newline]
[error] 251-251: cpplint: If an else has a brace on one side, it should have it on both [readability/braces]
[error] 255-255: cpplint: An else should appear on the same line as the preceding } [whitespace/newline]
[error] 255-255: cpplint: If an else has a brace on one side, it should have it on both [readability/braces]
[error] 261-261: cpplint: An else should appear on the same line as the preceding } [whitespace/newline]
[error] 261-261: cpplint: If an else has a brace on one side, it should have it on both [readability/braces]
[error] 265-265: cpplint: An else should appear on the same line as the preceding } [whitespace/newline]
[error] 265-265: cpplint: If an else has a brace on one side, it should have it on both [readability/braces]
[error] 279-279: cpplint: Line ends in whitespace. Consider deleting these extra spaces. [whitespace/end_of_line]
🪛 LanguageTool
applications/gstreamer/gst_video_recorder/README.md
[grammar] ~83-~83: Ensure spelling is correct
Context: ...les #### Record 10 seconds of video at 30fps (300 frames) ```bash gst-video-recorde...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~89-~89: Use a hyphen to join words.
Context: ...t 300 -o video.mp4 #### Record high quality H.265 video bash gst-video-r...
(QB_NEW_EN_HYPHEN)
🪛 markdownlint-cli2 (0.18.1)
applications/gstreamer/gst_video_recorder/README.md
140-140: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
179-179: Bare URL used
(MD034, no-bare-urls)
🪛 Shellcheck (0.11.0)
applications/gstreamer/gst_video_recorder/install_deps.sh
[error] 16-16: The shebang must be on the first line. Delete blanks and move comments.
(SC1128)
🔇 Additional comments (31)
operators/CMakeLists.txt (1)
29-29: LGTM! Operator integration follows established patterns.The GStreamer operator is correctly added to the build system in alphabetical order, following the same pattern as other operators.
applications/gstreamer/gst_video_recorder/install_deps.sh (1)
1-74: LGTM! Dependency installation script is well-structured.The script correctly:
- Includes the required copyright header before the shebang (per project standards)
- Uses
set -efor proper error handling- Provides graceful fallback for optional CUDA support
- Offers clear user guidance with emojis for better readability
Note: The shellcheck warning about shebang placement is a false positive—project standards require the copyright header to appear first.
applications/gstreamer/gst_video_recorder/Dockerfile (1)
1-99: LGTM! Multi-stage Dockerfile follows best practices.The Dockerfile correctly:
- Uses multi-stage builds to minimize final image size
- Handles conditional Python installation with fallback to deadsnakes PPA
- Installs all required GStreamer development packages consistently with
install_deps.sh- Includes proper cleanup steps to reduce image size
- Sets appropriate environment variables (PIP_BREAK_SYSTEM_PACKAGES, HOLOSCAN_INPUT_PATH)
applications/CMakeLists.txt (1)
59-60: LGTM! Application subdirectory correctly integrated.The GStreamer subdirectory is properly added to the build system in alphabetical order, consistent with the project's organization pattern.
applications/gstreamer/CMakeLists.txt (1)
16-17: LGTM! Application declaration is correct.The application is properly declared with its operator dependency, following the standard holohub application pattern.
applications/gstreamer/gst_video_recorder/metadata.json (1)
1-48: LGTM! Metadata is complete and well-structured.The metadata file correctly includes:
- All required fields (past issue with missing
requirementsfield has been resolved)- Comprehensive system package dependencies
- Clear optional package documentation for CUDA support
- Appropriate SDK version requirements
- Relevant tags and description
operators/gstreamer/gst/config.hpp (1)
1-32: LGTM! Configuration header is well-designed.The header correctly:
- Uses proper include guards with consistent naming
- Defines compile-time CUDA support detection based on GStreamer version
- Includes clear documentation about version requirements and affected functions
- Follows a clean, single-purpose design
Note: The clang error about
gst/gstversion.hnot found is expected in the static analysis environment where GStreamer headers are not installed.applications/gstreamer/gst_video_recorder/README.md (1)
1-181: Excellent documentation! Comprehensive and well-structured.The README provides:
- Clear description of the application's purpose and capabilities
- Detailed build instructions for both containerized and local workflows
- Extensive usage examples covering various scenarios
- Architecture explanation with pipeline flow visualization
- Performance considerations and operational notes
The minor markdown formatting suggestions above are optional refinements.
operators/gstreamer/gst/error.hpp (2)
43-48: LGTM! Clean RAII wrapper with proper ownership semantics.The constructor correctly takes ownership of the GError pointer and uses a shared_ptr with a custom deleter to ensure g_error_free is called. The lambda deleter with null-check is appropriate.
21-21: Static analysis warning is a false positive.The Clang warning about 'glib.h' not found can be safely ignored. This header is provided by the GLib dependency, which the build system will supply at compile time.
operators/gstreamer/gst/object.hpp (3)
47-90: LGTM! Well-designed RAII template with clear non-polymorphic intent.The Object template provides clean ownership semantics for GStreamer objects using shared_ptr with custom deleters. The design correctly:
- Handles floating references via RefSinkFunc
- Provides defaulted copy/move semantics (appropriate with shared_ptr)
- Exposes ref() for explicit reference increment when needed
- Documents the non-polymorphic design intent
98-126: Helper functions provide clean type-safe wrappers.The typed helper functions correctly adapt GStreamer's C API (which uses gpointer) to strongly-typed pointers. The use of
reinterpret_castfor GstMiniObject types is appropriate given that these are opaque C structs.
21-21: Static analysis warning is a false positive.The Clang warning about 'gst/gst.h' not found can be safely ignored. This header is provided by the GStreamer dependency managed by the build system.
applications/gstreamer/gst_video_recorder/CMakeLists.txt (1)
16-32: LGTM! Standard CMake configuration.The CMakeLists.txt correctly:
- Defines the project and finds required holoscan package
- Creates the executable with appropriate source files
- Links necessary libraries (holoscan::core and holoscan::ops::gstreamer_bridge)
- Installs to the conventional binary location
operators/gstreamer/gst/message.hpp (2)
37-60: LGTM! Clean integration with Error wrapper.The Message class follows the established RAII pattern and provides two convenient overloads for parse_error():
- Simple version returns just the Error object
- Extended version also captures debug_info as a string
This design aligns well with the Error wrapper and provides robust error extraction from GstMessage.
21-21: Static analysis warning is a false positive.The Clang warning about 'gst/gst.h' not found can be safely ignored. This header is provided by the GStreamer build dependency.
operators/gstreamer/gst_video_recorder_operator.hpp (2)
45-107: Well-designed operator with comprehensive documentation.The GstVideoRecorderOperator is well-structured with:
- Clear lifecycle methods (setup/start/compute/stop)
- Comprehensive parameter documentation explaining defaults and behaviors
- Appropriate use of GstSrcBridge for data flow
- Async bus monitoring with std::future
The documentation clearly explains auto-detection features, supported formats, and special cases like live mode.
21-21: Static analysis warning is a false positive.The Clang warning about missing headers can be safely ignored. The build system provides these dependencies.
operators/gstreamer/gst/buffer.hpp (2)
42-95: LGTM! Comprehensive buffer wrapper with clear API.The Buffer class provides a clean RAII wrapper for GstBuffer with convenient methods for:
- Size and flag queries
- Memory block management (append, get count, get by index)
- Map/unmap operations for buffer access
The documentation clearly states error handling expectations (throwing std::runtime_error on invalid buffers).
21-21: Static analysis warning is a false positive.The Clang warning about 'gst/gst.h' not found can be safely ignored. This header is provided by the GStreamer dependency.
operators/gstreamer/gst/caps.hpp (2)
41-106: LGTM! Well-documented caps wrapper with comprehensive API.The Caps class provides:
- Two convenient constructors (from pointer or caps string)
- Methods for structure inspection (name, value retrieval)
- VideoInfo extraction via get_video_info()
- Utility methods (is_empty, get_size, has_feature, to_string)
The documentation is excellent, clearly explaining GStreamer semantics (e.g., empty caps vs nullptr, structure indexing, feature checking).
21-21: Static analysis warning is a false positive.The Clang warning about 'gst/gst.h' not found can be safely ignored. This header is provided by the GStreamer dependency.
operators/gstreamer/gst/video_info.hpp (2)
35-110: LGTM! Clean wrapper for video information.The VideoInfo class appropriately wraps GstVideoInfo as a POD-style object with:
- Constructor from Caps for convenient extraction
- Comprehensive getters for all video properties (size, stride, dimensions, format)
- Clear documentation about the POD nature (no reference holding)
The design correctly treats GstVideoInfo as a value type containing copied data rather than a reference-counted object.
21-21: Static analysis warning is a false positive.The Clang warning about 'gst/video/video.h' not found can be safely ignored. This header is provided by the GStreamer video library.
operators/gstreamer/gst/memory.hpp (1)
18-64: LGTM! Clean RAII wrapper for GStreamer memory management.The Memory class provides a well-designed RAII wrapper with automatic reference counting. The API is minimal and appropriate for zero-copy memory operations.
operators/gstreamer/gst/buffer.cpp (1)
39-43: Correct ownership transfer withmemory.ref().The call to
memory.ref()beforegst_buffer_append_memorycorrectly handles ownership transfer, as noted in the comment. GStreamer takes ownership of the passed memory, so a new reference is required.operators/gstreamer/gst_src_bridge.hpp (1)
38-150: LGTM! Well-designed bridge API with clear ownership semantics.The
GstSrcBridgeclass provides a clean abstraction for feeding data into GStreamer pipelines. The non-copyable/non-movable design (lines 65-68) and RAII approach ensure proper resource management. Documentation clearly explains EOS handling and buffer ownership.operators/gstreamer/gst_video_recorder_operator.cpp (1)
681-710: Race condition fix confirmed - pipeline shutdown is now safe.The previous race condition has been properly addressed. The code now waits for
bus_monitor_future_(lines 698-702) before resetting the pipeline (line 706), ensuring the bus monitor thread completes before the pipeline is destroyed. This prevents undefined behavior when the monitor thread accesses the bus after pipeline destruction.operators/gstreamer/gst_src_bridge.cpp (2)
288-305: GPU tensor wrapping issue properly fixed.The previous unsafe fallback has been correctly addressed. When GPU memory is requested without CUDA support (lines 294-299), the code now returns
nullptrwith a clear error message instead of incorrectly wrapping device memory as host memory. This prevents undefined behavior and crashes.
474-487: Null pointer check prevents potential crash.The null check for
memory_wrapper_(lines 477-482) after creation prevents dereferencing a null shared_ptr. This properly handles the case wherecreate_memory_wrapperreturnsnullptrdue to configuration errors.operators/gstreamer/CMakeLists.txt (1)
121-132: The review comment is based on an incorrect premise and should be disregarded.The actual CMakeLists.txt (lines 121-132) does not include
core/initializer.hppin the install list. The provided code snippet is accurate and shows only the 10 header files that are actually being installed. The review's claim that line 132 listscore/initializer.hppis factually incorrect.Likely an incorrect or invalid review comment.
GilShpr
force-pushed
the
gshapira/holoscan_gstreamer_bridge
branch
from
7e09ace to
cf36941
Compare
There was a problem hiding this comment.
28 files reviewed, no comments
Edit Code Review Agent Settings | Greptile
React with 👍 or 👎 to share your feedback on this new summary format
- Implements GstVideoRecorderOperator for encoding video streams to file - Adds RAII GStreamer wrapper classes (Buffer, Caps, Memory, etc.) - Supports zero-copy tensor-to-buffer conversion for host/CUDA memory - Auto-detects video parameters, encoders, parsers, and muxers - Includes comprehensive sample app with animated test patterns - Provides flexible CLI with encoder properties and format support - Handles H.264/H.265 encoding with MP4/MKV container formats Signed-off-by: Gil Shapira <[email protected]>
GilShpr
force-pushed
the
gshapira/holoscan_gstreamer_bridge
branch
from
cf36941 to
a48f3c6
Compare
There was a problem hiding this comment.
29 files reviewed, no comments
Edit Code Review Agent Settings | Greptile
React with 👍 or 👎 to share your feedback on this new summary format
applications/gstreamer/gst_video_recorder/gst_video_recorder.cpp
Outdated
Show resolved
Hide resolved
| // Connect the operators: pattern generator -> GStreamer recorder | ||
| add_flow(pattern_gen_op, recorder_op); |
There was a problem hiding this comment.
Several other HoloHub sample apps provide an option to select among real, live data from a V4L2 webcam and recorded playback data. Could we similarly add an option for live V4L2 input in this app, either in this PR or a followup? Live data can sometimes be more compelling for users than sample data.
| * - Fraction: "30000/1001" -> "30000/1001" (no change) | ||
| * - Decimal (converted to fraction): "29.97" -> "2997/100" | ||
| * | ||
| * @param framerate Framerate string to normalize |
There was a problem hiding this comment.
Always FPS? Or any other units to consider?
| * @param framerate Framerate string to normalize | |
| * @param framerate Framerate string to normalize (frames per second) |
Split 692-line gst_video_recorder.cpp into pattern_generator.{hpp,cpp} for better maintainability.
Replace duplicate Dockerfile package installation with unified install_deps.sh script that handles
both local and Docker environments.
Fix EOS handling race condition and add proper pipeline shutdown with configurable blocking behavior for improved reliability.
There was a problem hiding this comment.
31 files reviewed, no comments
Edit Code Review Agent Settings | Greptile
React with 👍 or 👎 to share your feedback on this new summary format
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (1)
operators/gstreamer/CMakeLists.txt (1)
57-65: Avoid exporting host GStreamer include paths from the installed targetRight now
${GSTREAMER_*_INCLUDE_DIRS}are added as unconditional PUBLIC includes. When this target is exported/installed, those absolute include paths from the build host get baked into the interface and may not make sense on consumer systems.You can keep them for the build while avoiding leaking them into the install interface by wrapping in
BUILD_INTERFACE, e.g.:target_include_directories(holoscan_gstreamer_bridge PUBLIC $<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}> $<INSTALL_INTERFACE:include> $<BUILD_INTERFACE:${GSTREAMER_INCLUDE_DIRS}> $<BUILD_INTERFACE:${GSTREAMER_BASE_INCLUDE_DIRS}> $<BUILD_INTERFACE:${GSTREAMER_VIDEO_INCLUDE_DIRS}> $<BUILD_INTERFACE:${GSTREAMER_APP_INCLUDE_DIRS}> ) if(GSTREAMER_CUDA_FOUND) target_include_directories(holoscan_gstreamer_bridge PUBLIC $<BUILD_INTERFACE:${GSTREAMER_CUDA_INCLUDE_DIRS}> ) endif()This keeps consumers from depending on build-host-specific header locations while preserving correct includes during the build.
Also applies to: 88-91
🧹 Nitpick comments (5)
applications/gstreamer/gst_video_recorder/install_deps.sh (1)
1-16: Tighten sudo/root detection and consider silencing ShellCheck SC1128Currently, when
$EUID != 0andsudois not available, the script falls into theelsebranch, prints "Running as root", and thenapt-getwill just fail with permission errors. It would be clearer to fail fast in that case instead of mis-reporting the user as root.You could do something like:
-# Detect if we need sudo (not root and sudo is available) -USE_SUDO="" -if [ "$EUID" -ne 0 ] && command -v sudo >/dev/null 2>&1; then - USE_SUDO="sudo" - echo "ℹ️ Running with sudo (not root user)" -else - echo "ℹ️ Running as root (Docker container or root user)" -fi +# Detect if we need sudo (not root and sudo is available) +USE_SUDO="" +if [ "$EUID" -ne 0 ]; then + if command -v sudo >/dev/null 2>&1; then + USE_SUDO="sudo" + echo "ℹ️ Running with sudo (not root user)" + else + echo "❌ This script must be run as root or with sudo available." + exit 1 + fi +else + echo "ℹ️ Running as root (Docker container or root user)" +fiAlso, ShellCheck’s SC1128 about the shebang not being on the first line conflicts with the project’s SPDX header requirement. If ShellCheck warnings matter for CI, you can explicitly disable that rule near the top instead of moving the shebang:
# shellcheck disable=SC1128Also applies to: 29-36
operators/gstreamer/gst/config.hpp (1)
18-32: Minor: align header-guard closing commentThe header guard macro is
HOLOSCAN__GSTREAMER__GST__CONFIG_HPP, but the closing comment says/* GST_CONFIG_HPP */. For consistency/readability, consider updating the closing comment:-#endif /* GST_CONFIG_HPP */ +#endif /* HOLOSCAN__GSTREAMER__GST__CONFIG_HPP */applications/gstreamer/gst_video_recorder/README.md (1)
138-142: Add a language to the pipeline-flow fenced code block (MD040)The pipeline flow section uses a fenced code block without a language:
### Pipeline FlowPatternGenOperator → GstVideoRecorderOperator → GStreamer Encoding Pipeline → File
To satisfy markdownlint (MD040) and be explicit, you can tag it as plain text:
-``` +```text PatternGenOperator → GstVideoRecorderOperator → GStreamer Encoding Pipeline → File</blockquote></details> <details> <summary>operators/gstreamer/gst_src_bridge.hpp (1)</summary><blockquote> `38-44`: **Clarify class‑level comment now that it depends on Holoscan TensorMap** The class‑level comment says it “contains no Holoscan-specific dependencies (except for GXF tensors)” but this header now directly depends on `holoscan::TensorMap` and Holoscan headers. To avoid confusion for users expecting a pure GStreamer bridge, consider updating the wording to something like: ```cpp // This class provides a pure GStreamer implementation for managing an appsrc element // and zero‑copy buffer creation, with Holoscan TensorMap used only as a data carrier.Or explicitly call out
TensorMapinstead of just “GXF tensors”.Also applies to: 133-138
applications/gstreamer/gst_video_recorder/pattern_generator.cpp (1)
37-121: Defensively validate allocator and frame dimensions in PatternEntityGenerator::generate
PatternEntityGenerator::generateassumesallocatoris non‑null and thatwidth/heightare positive. Misconfiguration (e.g., missing allocator or negative dimensions from config) could lead to null dereference or huge/invalid allocations when computingbuffer_size.To make this more robust, consider:
- Early‑return if
allocatoris null.- Validate
width > 0 && height > 0and log/return an empty entity otherwise.- Compute
buffer_sizeusingsize_tmath to avoid overflow on large resolutions.Example change:
holoscan::gxf::Entity PatternEntityGenerator::generate(int width, int height, nvidia::gxf::MemoryStorageType storage_type, holoscan::Allocator* allocator) { - HOLOSCAN_LOG_DEBUG("Generating {}x{} pattern entity (storage: {})", - width, height, - storage_type == nvidia::gxf::MemoryStorageType::kDevice ? "device" : "host"); + if (!allocator) { + HOLOSCAN_LOG_ERROR("Allocator is null in PatternEntityGenerator::generate"); + return holoscan::gxf::Entity{}; + } + + if (width <= 0 || height <= 0) { + HOLOSCAN_LOG_ERROR("Invalid frame dimensions {}x{} (must be positive)", width, height); + return holoscan::gxf::Entity{}; + } + + HOLOSCAN_LOG_DEBUG("Generating {}x{} pattern entity (storage: {})", + width, height, + storage_type == nvidia::gxf::MemoryStorageType::kDevice ? "device" : "host"); @@ - size_t buffer_size = width * height * RGBA_CHANNELS; + const auto buffer_size = + static_cast<size_t>(width) * static_cast<size_t>(height) * + static_cast<size_t>(RGBA_CHANNELS);This keeps behavior unchanged for valid inputs but fails fast and predictably on bad configuration.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (31)
.devcontainer/devcontainer.json(1 hunks)applications/CMakeLists.txt(1 hunks)applications/gstreamer/CMakeLists.txt(1 hunks)applications/gstreamer/gst_video_recorder/CMakeLists.txt(1 hunks)applications/gstreamer/gst_video_recorder/Dockerfile(1 hunks)applications/gstreamer/gst_video_recorder/README.md(1 hunks)applications/gstreamer/gst_video_recorder/gst_video_recorder.cpp(1 hunks)applications/gstreamer/gst_video_recorder/install_deps.sh(1 hunks)applications/gstreamer/gst_video_recorder/metadata.json(1 hunks)applications/gstreamer/gst_video_recorder/pattern_generator.cpp(1 hunks)applications/gstreamer/gst_video_recorder/pattern_generator.hpp(1 hunks)operators/CMakeLists.txt(1 hunks)operators/gstreamer/CMakeLists.txt(1 hunks)operators/gstreamer/gst/buffer.cpp(1 hunks)operators/gstreamer/gst/buffer.hpp(1 hunks)operators/gstreamer/gst/caps.cpp(1 hunks)operators/gstreamer/gst/caps.hpp(1 hunks)operators/gstreamer/gst/config.hpp(1 hunks)operators/gstreamer/gst/error.hpp(1 hunks)operators/gstreamer/gst/memory.cpp(1 hunks)operators/gstreamer/gst/memory.hpp(1 hunks)operators/gstreamer/gst/message.cpp(1 hunks)operators/gstreamer/gst/message.hpp(1 hunks)operators/gstreamer/gst/object.hpp(1 hunks)operators/gstreamer/gst/video_info.cpp(1 hunks)operators/gstreamer/gst/video_info.hpp(1 hunks)operators/gstreamer/gst_src_bridge.cpp(1 hunks)operators/gstreamer/gst_src_bridge.hpp(1 hunks)operators/gstreamer/gst_video_recorder_operator.cpp(1 hunks)operators/gstreamer/gst_video_recorder_operator.hpp(1 hunks)operators/gstreamer/metadata.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- applications/gstreamer/CMakeLists.txt
🚧 Files skipped from review as they are similar to previous changes (8)
- operators/gstreamer/gst/buffer.cpp
- applications/gstreamer/gst_video_recorder/CMakeLists.txt
- .devcontainer/devcontainer.json
- operators/CMakeLists.txt
- applications/gstreamer/gst_video_recorder/Dockerfile
- applications/CMakeLists.txt
- operators/gstreamer/gst/caps.cpp
- operators/gstreamer/gst/message.cpp
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-10-30T19:04:41.239Z
Learnt from: bhashemian
Repo: nvidia-holoscan/holohub PR: 1203
File: applications/video_streaming/video_streaming_server/cpp/metadata.json:42-51
Timestamp: 2025-10-30T19:04:41.239Z
Learning: In the video_streaming application structure, umbrella metadata (applications/video_streaming/metadata.json) uses language-qualified mode names like "server_cpp" and "server_python" to distinguish between implementations, while component-specific metadata files (e.g., applications/video_streaming/video_streaming_server/cpp/metadata.json) use simple mode names like "server" since they are already scoped to a specific language implementation by their directory structure.
Applied to files:
operators/gstreamer/metadata.jsonapplications/gstreamer/gst_video_recorder/metadata.json
📚 Learning: 2025-11-19T04:58:30.202Z
Learnt from: finalelement
Repo: nvidia-holoscan/holohub PR: 1237
File: applications/surgical_scene_recon/training/utils/sh_utils.py:1-23
Timestamp: 2025-11-19T04:58:30.202Z
Learning: When reviewing files that contain third-party code with original copyright notices (e.g., from PlenOctree, other open-source projects), do not suggest modifying the original copyright years or attribution. The original copyright notice must be preserved exactly as required by redistribution licenses (BSD, MIT, Apache, etc.). If substantial modifications are made by the current project, a separate copyright line can be added, but the original must remain unchanged.
Applied to files:
applications/gstreamer/gst_video_recorder/install_deps.sh
📚 Learning: 2025-10-22T23:47:42.896Z
Learnt from: cdinea
Repo: nvidia-holoscan/holohub PR: 1170
File: applications/video_streaming/video_streaming_server/cpp/CMakeLists.txt:109-111
Timestamp: 2025-10-22T23:47:42.896Z
Learning: In the video streaming server application (applications/video_streaming/video_streaming_server/cpp/CMakeLists.txt), bundling libcudart.so.12 from the NGC operator binaries is intentional to ensure consistency with NGC binaries, even though the target links to CUDA::cudart.
Applied to files:
operators/gstreamer/CMakeLists.txt
📚 Learning: 2025-10-22T16:21:10.281Z
Learnt from: cdinea
Repo: nvidia-holoscan/holohub PR: 1170
File: applications/video_streaming/video_streaming_client/cpp/streaming_client_demo.yaml:42-49
Timestamp: 2025-10-22T16:21:10.281Z
Learning: V4L2VideoCaptureOp in Holoscan SDK performs internal conversion from the camera's native pixel format (specified by the pixel_format parameter) to RGBA8888 output, regardless of the input format (e.g., YUYV, MJPEG). When using V4L2 as a source, downstream operators should expect RGBA8888 (4-channel) output format.
Applied to files:
operators/gstreamer/gst/video_info.cpp
📚 Learning: 2025-11-18T10:22:08.733Z
Learnt from: AndreasHeumann
Repo: nvidia-holoscan/holohub PR: 1220
File: applications/pipeline_visualization/visualizer/tensor_to_numpy.py:72-91
Timestamp: 2025-11-18T10:22:08.733Z
Learning: In applications/pipeline_visualization/visualizer/tensor_to_numpy.py, tensors always use HWC (Height, Width, Channel) layout, so removing the trailing singleton dimension (when it equals 1) is safe and expected behavior for this specific application.
Applied to files:
operators/gstreamer/gst_src_bridge.cpp
🧬 Code graph analysis (13)
operators/gstreamer/gst/memory.hpp (1)
operators/gstreamer/gst/buffer.hpp (3)
memory(67-67)info(88-88)info(94-94)
operators/gstreamer/gst/memory.cpp (3)
operators/gstreamer/gst/memory.hpp (3)
offset(49-49)info(57-57)info(63-63)operators/gstreamer/gst/buffer.cpp (6)
map(55-58)map(55-55)flags(34-37)flags(34-34)unmap(60-62)unmap(60-60)operators/gstreamer/gst/buffer.hpp (2)
info(88-88)info(94-94)
operators/gstreamer/gst_video_recorder_operator.cpp (2)
operators/gstreamer/gst/message.hpp (1)
debug_info(59-59)operators/gstreamer/gst_src_bridge.hpp (2)
tensor_map(117-117)buffer(102-102)
operators/gstreamer/gst/buffer.hpp (4)
operators/gstreamer/gst_src_bridge.cpp (1)
buffer(458-458)operators/gstreamer/gst_src_bridge.hpp (1)
buffer(102-102)operators/gstreamer/gst/memory.hpp (2)
info(57-57)info(63-63)operators/gstreamer/gst/buffer.cpp (2)
flags(34-37)flags(34-34)
operators/gstreamer/gst/object.hpp (1)
operators/gstreamer/gst/error.hpp (3)
ptr_(54-54)ptr_(60-60)static_cast(66-66)
operators/gstreamer/gst/video_info.cpp (1)
operators/gstreamer/gst/video_info.hpp (5)
VideoInfo(42-42)plane_index(55-55)plane_index(62-62)plane_index(69-69)plane_index(76-76)
operators/gstreamer/gst/caps.hpp (1)
operators/gstreamer/gst_src_bridge.cpp (1)
caps(338-338)
operators/gstreamer/gst_src_bridge.hpp (1)
operators/gstreamer/gst_src_bridge.cpp (3)
GstSrcBridge(323-387)GstSrcBridge(389-389)buffer(458-458)
applications/gstreamer/gst_video_recorder/pattern_generator.hpp (3)
operators/gstreamer/gst/buffer.hpp (1)
idx(80-80)applications/gstreamer/gst_video_recorder/gst_video_recorder.cpp (1)
allocator(272-298)operators/gstreamer/gst_video_recorder_operator.hpp (2)
spec(76-76)input(99-99)
applications/gstreamer/gst_video_recorder/pattern_generator.cpp (1)
applications/gstreamer/gst_video_recorder/pattern_generator.hpp (11)
get_pattern_name(54-54)get_pattern_name(61-61)width(100-102)data(114-114)data(125-125)data(139-139)data(150-150)set_rgba_pixel(73-79)set_rgba_pixel(73-74)spec(160-160)input(162-162)
operators/gstreamer/gst_src_bridge.cpp (5)
operators/gstreamer/gst/memory.hpp (2)
Memory(41-41)Memory(41-41)operators/gstreamer/gst/caps.cpp (4)
to_string(82-86)to_string(82-82)Caps(28-28)Caps(30-34)operators/gstreamer/gst_src_bridge.hpp (6)
GstSrcBridge(58-59)GstSrcBridge(64-64)GstSrcBridge(67-67)GstSrcBridge(69-69)buffer(102-102)tensor_map(117-117)operators/gstreamer/gst/caps.hpp (2)
Caps(48-48)Caps(55-55)operators/gstreamer/gst/buffer.hpp (1)
memory(67-67)
applications/gstreamer/gst_video_recorder/gst_video_recorder.cpp (2)
applications/gstreamer/gst_video_recorder/pattern_generator.hpp (3)
width(100-102)get_pattern_name(54-54)get_pattern_name(61-61)applications/gstreamer/gst_video_recorder/pattern_generator.cpp (4)
get_pattern_name(28-31)get_pattern_name(28-28)get_pattern_name(33-35)get_pattern_name(33-33)
operators/gstreamer/gst/error.hpp (1)
operators/gstreamer/gst/object.hpp (5)
ptr_(71-71)ptr_(74-74)ptr_(80-83)ptr_(86-86)static_cast(77-77)
🪛 Clang (14.0.6)
operators/gstreamer/gst/message.hpp
[error] 21-21: 'gst/gst.h' file not found
(clang-diagnostic-error)
operators/gstreamer/gst/memory.hpp
[error] 21-21: 'gst/gst.h' file not found
(clang-diagnostic-error)
operators/gstreamer/gst_video_recorder_operator.hpp
[error] 21-21: 'atomic' file not found
(clang-diagnostic-error)
operators/gstreamer/gst/buffer.hpp
[error] 21-21: 'gst/gst.h' file not found
(clang-diagnostic-error)
operators/gstreamer/gst/object.hpp
[error] 21-21: 'gst/gst.h' file not found
(clang-diagnostic-error)
operators/gstreamer/gst/caps.hpp
[error] 21-21: 'gst/gst.h' file not found
(clang-diagnostic-error)
operators/gstreamer/gst_src_bridge.hpp
[error] 21-21: 'gst/app/gstappsrc.h' file not found
(clang-diagnostic-error)
applications/gstreamer/gst_video_recorder/pattern_generator.hpp
[error] 21-21: 'cstdint' file not found
(clang-diagnostic-error)
applications/gstreamer/gst_video_recorder/gst_video_recorder.cpp
[error] 18-18: 'iostream' file not found
(clang-diagnostic-error)
operators/gstreamer/gst/config.hpp
[error] 21-21: 'gst/gstversion.h' file not found
(clang-diagnostic-error)
operators/gstreamer/gst/error.hpp
[error] 21-21: 'glib.h' file not found
(clang-diagnostic-error)
operators/gstreamer/gst/video_info.hpp
[error] 21-21: 'gst/video/video.h' file not found
(clang-diagnostic-error)
🪛 GitHub Actions: Check linting
operators/gstreamer/gst_video_recorder_operator.cpp
[error] 285-285: Lines should be <= 100 characters long [whitespace/line_length]
[error] 291-291: Line ends in whitespace. Consider deleting these extra spaces. [whitespace/end_of_line]
[error] 629-629: Extra space before ( in function call [whitespace/parens]
[error] 690-690: Lines should be <= 100 characters long [whitespace/line_length]
[error] 733-733: Line ends in whitespace. Consider deleting these extra spaces. [whitespace/end_of_line]
operators/gstreamer/gst_src_bridge.hpp
[error] 58-58: Line ends in whitespace. Consider deleting these extra spaces. [whitespace/end_of_line]
operators/gstreamer/gst_src_bridge.cpp
[error] 203-203: Lines should be <= 100 characters long [whitespace/line_length]
[error] 210-210: Line ends in whitespace. Consider deleting these extra spaces. [whitespace/end_of_line]
🪛 LanguageTool
applications/gstreamer/gst_video_recorder/README.md
[grammar] ~83-~83: Ensure spelling is correct
Context: ...les #### Record 10 seconds of video at 30fps (300 frames) ```bash gst-video-recorde...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~89-~89: Use a hyphen to join words.
Context: ...t 300 -o video.mp4 #### Record high quality H.265 video bash gst-video-r...
(QB_NEW_EN_HYPHEN)
🪛 markdownlint-cli2 (0.18.1)
applications/gstreamer/gst_video_recorder/README.md
140-140: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 Shellcheck (0.11.0)
applications/gstreamer/gst_video_recorder/install_deps.sh
[error] 16-16: The shebang must be on the first line. Delete blanks and move comments.
(SC1128)
🔇 Additional comments (11)
operators/gstreamer/metadata.json (1)
1-44: Operator metadata looks consistent and completeFields (SDK versions, platforms, tags, and package requirements) align with the rest of the GStreamer recorder wiring in this PR; nothing stands out as inconsistent.
applications/gstreamer/gst_video_recorder/metadata.json (1)
1-47: Application metadata is well-structured and consistentName, requirements, SDK versions, platforms, tags, and run configuration all look coherent with the rest of the gst_video_recorder app wiring.
operators/gstreamer/gst/error.hpp (1)
28-70: GError RAII wrapper looks correctThe
Errorwrapper cleanly manages::GError*with a custom-deletingstd::shared_ptr, exposes safe accessors, and avoids ref-counting assumptions; this looks solid.operators/gstreamer/gst/memory.cpp (1)
27-39: RAII wrapper behavior and null‑safety look consistent
get_sizes,map, andunmapall short‑circuit safely on null memory and delegate directly to the GStreamer APIs otherwise, matching the Buffer wrapper pattern. No changes needed.operators/gstreamer/gst/message.hpp (1)
37-60: Message wrapper and error parsing API look cleanThe RAII wrapper and the two
parse_erroroverloads provide a small, focused interface for extracting GStreamer errors and optional debug info, with clear ownership semantics. No changes needed.operators/gstreamer/gst/caps.hpp (1)
35-105: Caps wrapper API is well‑structured and matches expected usageThe Caps RAII wrapper cleanly encapsulates common
GstCapsoperations (string ctor, structure access, video info extraction, feature checks, and stringification) with clear semantics. The forward declaration ofVideoInfoplus use instd::optional<VideoInfo>is a reasonable separation of concerns given the likely implementation in caps.cpp. No changes needed.operators/gstreamer/gst/video_info.hpp (1)
28-110: VideoInfo wrapper API looks solid; static-analysis header error is likely tooling-relatedThe
VideoInfointerface cleanly mirrorsGstVideoInfo(size, planes, format, width/height) and the comments match the implementation invideo_info.cpp(e.g., returning 0 for invalid plane indices). StoringGstVideoInfoby value is appropriate here and copy semantics stay trivial.The Clang error about
'gst/video/video.h' file not foundlooks like a static-analysis/CI environment configuration issue (missing GStreamer dev headers or include paths), not a problem with this header itself.Please ensure the static-analysis job is picking up the same GStreamer include paths as your actual build (e.g., via
pkg-config --cflags gstreamer-video-1.0).operators/gstreamer/gst/memory.hpp (1)
34-63: RAII GstMemory wrapper is well‑structuredThe
Memorywrapper cleanly encapsulatesGstMemorywith the existingObjectpattern and exposes just the right primitives (get_sizes,map,unmap) forBufferto build on. No correctness or API issues stand out here.operators/gstreamer/gst/video_info.cpp (1)
27-71: VideoInfo implementation matches GStreamer expectationsThe constructor correctly defers to
gst_video_info_from_capsand throws on failure, and each accessor is a thin wrapper around the correspondingGST_VIDEO_INFO_*macro. The per‑plane methods’ index checks againstfinfo->n_planesavoid out‑of‑range access, and returning 0 on invalid indices is a sensible, documented fallback.Implementation and header are consistent; no changes needed.
applications/gstreamer/gst_video_recorder/pattern_generator.hpp (1)
28-171: Pattern generation API and operator interface look solidThe pattern enums, helper (
set_rgba_pixel),PatternEntityGeneratorhierarchy, andPatternGenOperatorparameters cleanly define the contract used by the app and bridge. No issues from a correctness or maintainability standpoint.applications/gstreamer/gst_video_recorder/gst_video_recorder.cpp (1)
94-249: CLI parsing and app composition are robust and consistentArgument parsing, range validation, and configuration logging are well-structured, and the Holoscan app cleanly connects
PatternGenOperatortoGstVideoRecorderOperator. I don’t see functional issues here.Also applies to: 261-310, 314-357
| void ColorBarsPatternGenerator::generate_pattern_data(uint8_t* data, int width, int height) { | ||
| // SMPTE color bars (7 bars) - static to avoid recreating on each call | ||
| static constexpr uint8_t colors[SMPTE_COLOR_BARS][RGB_CHANNELS] = { | ||
| {255, 255, 255}, // White | ||
| {255, 255, 0}, // Yellow | ||
| {0, 255, 255}, // Cyan | ||
| {0, 255, 0}, // Green | ||
| {255, 0, 255}, // Magenta | ||
| {255, 0, 0}, // Red | ||
| {0, 0, 255} // Blue | ||
| }; | ||
|
|
||
| int bar_width = width / SMPTE_COLOR_BARS; | ||
| for (int y = 0; y < height; y++) { | ||
| for (int x = 0; x < width; x++) { | ||
| int idx = (y * width + x) * RGBA_CHANNELS; | ||
| int bar_idx = x / bar_width; | ||
| if (bar_idx >= SMPTE_COLOR_BARS) bar_idx = SMPTE_COLOR_BARS - 1; | ||
|
|
||
| set_rgba_pixel(data, idx, | ||
| colors[bar_idx][0], // R | ||
| colors[bar_idx][1], // G | ||
| colors[bar_idx][2]); // B | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix potential divide‑by‑zero in ColorBarsPatternGenerator when width < SMPTE_COLOR_BARS
ColorBarsPatternGenerator::generate_pattern_data computes:
int bar_width = width / SMPTE_COLOR_BARS;
...
int bar_idx = x / bar_width;If width is less than SMPTE_COLOR_BARS (7), bar_width becomes 0 and x / bar_width will divide by zero. This can happen even with valid (small) widths.
You can guard against this similarly to the checkerboard square size:
- int bar_width = width / SMPTE_COLOR_BARS;
+ int bar_width = std::max(1, width / SMPTE_COLOR_BARS);
@@
- int bar_idx = x / bar_width;
+ int bar_idx = x / bar_width;
if (bar_idx >= SMPTE_COLOR_BARS) bar_idx = SMPTE_COLOR_BARS - 1;<algorithm> is already included and used elsewhere, so this is a minimal change that prevents the crash while preserving bar distribution for normal widths.
🤖 Prompt for AI Agents
In applications/gstreamer/gst_video_recorder/pattern_generator.cpp around lines
156 to 181, the computation int bar_width = width / SMPTE_COLOR_BARS can produce
zero when width < SMPTE_COLOR_BARS which leads to a divide-by-zero on int
bar_idx = x / bar_width; fix by ensuring bar_width is at least 1 (e.g., replace
with std::max(1, width / SMPTE_COLOR_BARS)) and keep the existing clamp of
bar_idx to SMPTE_COLOR_BARS - 1 so small widths don't crash and normal
distribution is preserved.
| // Get tensor shape for width and height | ||
| auto shape = tensor->shape(); | ||
| if (shape.size() < 2 || shape.size() > 3) { | ||
| HOLOSCAN_LOG_ERROR("Tensor has invalid rank {} for CUDA wrapping (expected 2 or 3)", shape.size()); | ||
| return gst::Memory(); | ||
| } | ||
|
|
||
| // Tensor is in format: [height, width, channels] or [height, width]. | ||
| gint height = shape[0]; | ||
| gint width = shape[1]; | ||
|
|
There was a problem hiding this comment.
Fix lint: long log line and trailing whitespace
The only lint issues reported here are cosmetic:
- Line ~203 exceeds the 100-character limit.
- Line ~210 has trailing whitespace.
You can address both with a small reflow:
- if (shape.size() < 2 || shape.size() > 3) {
- HOLOSCAN_LOG_ERROR("Tensor has invalid rank {} for CUDA wrapping (expected 2 or 3)", shape.size());
- return gst::Memory();
- }
+ if (shape.size() < 2 || shape.size() > 3) {
+ HOLOSCAN_LOG_ERROR(
+ "Tensor has invalid rank {} for CUDA wrapping (expected 2 or 3)",
+ shape.size());
+ return gst::Memory();
+ }
@@
- gint width = shape[1];
-
+ gint width = shape[1];📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Get tensor shape for width and height | |
| auto shape = tensor->shape(); | |
| if (shape.size() < 2 || shape.size() > 3) { | |
| HOLOSCAN_LOG_ERROR("Tensor has invalid rank {} for CUDA wrapping (expected 2 or 3)", shape.size()); | |
| return gst::Memory(); | |
| } | |
| // Tensor is in format: [height, width, channels] or [height, width]. | |
| gint height = shape[0]; | |
| gint width = shape[1]; | |
| // Get tensor shape for width and height | |
| auto shape = tensor->shape(); | |
| if (shape.size() < 2 || shape.size() > 3) { | |
| HOLOSCAN_LOG_ERROR( | |
| "Tensor has invalid rank {} for CUDA wrapping (expected 2 or 3)", | |
| shape.size()); | |
| return gst::Memory(); | |
| } | |
| // Tensor is in format: [height, width, channels] or [height, width]. | |
| gint height = shape[0]; | |
| gint width = shape[1]; |
🧰 Tools
🪛 GitHub Actions: Check linting
[error] 203-203: Lines should be <= 100 characters long [whitespace/line_length]
[error] 210-210: Line ends in whitespace. Consider deleting these extra spaces. [whitespace/end_of_line]
🤖 Prompt for AI Agents
operators/gstreamer/gst_src_bridge.cpp around lines 200 to 210: the lint issues
are a too-long log line (~line 203) and trailing whitespace (~line 210); shorten
the long HOLOSCAN_LOG_ERROR message so it stays under 100 chars (for example
split the message or reduce wording) and remove the trailing space at the end of
the width assignment line; ensure semantics unchanged and re-run linter.
| * @param block If true, push_buffer() blocks when queue is full; if false, returns immediately | ||
| * @throws std::runtime_error if initialization fails | ||
| */ | ||
| GstSrcBridge(const std::string& name, const std::string& caps_string, size_t max_buffers, |
There was a problem hiding this comment.
Fix trailing whitespace to satisfy linting
The constructor declaration line ends with trailing whitespace, which is flagged by the lint job.
- GstSrcBridge(const std::string& name, const std::string& caps_string, size_t max_buffers,
- bool block = true);
+ GstSrcBridge(const std::string& name, const std::string& caps_string, size_t max_buffers,
+ bool block = true);📝 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.
| GstSrcBridge(const std::string& name, const std::string& caps_string, size_t max_buffers, | |
| GstSrcBridge(const std::string& name, const std::string& caps_string, size_t max_buffers, | |
| bool block = true); |
🧰 Tools
🪛 GitHub Actions: Check linting
[error] 58-58: Line ends in whitespace. Consider deleting these extra spaces. [whitespace/end_of_line]
🤖 Prompt for AI Agents
In operators/gstreamer/gst_src_bridge.hpp around line 58, the constructor
declaration line has trailing whitespace causing lint failures; remove the
trailing space characters at the end of that line (and any other trailing
whitespace on the file) so the line ends immediately after the closing
parenthesis/semicolon, then save and run the linter to confirm the warning is
resolved.
| void monitor_pipeline_bus(GstElement* pipeline, std::atomic<bool>* stop_flag) { | ||
| holoscan::gst::Bus bus(gst_element_get_bus(pipeline)); | ||
|
|
||
| while (!stop_flag->load()) { | ||
| holoscan::gst::Message msg( | ||
| gst_bus_timed_pop_filtered(bus.get(), 100 * GST_MSECOND, | ||
| static_cast<GstMessageType>(GST_MESSAGE_ERROR | GST_MESSAGE_EOS | | ||
| GST_MESSAGE_STATE_CHANGED))); | ||
|
|
||
| if (msg) { | ||
| switch (GST_MESSAGE_TYPE(msg.get())) { | ||
| case GST_MESSAGE_ERROR: { | ||
| std::string debug_info; | ||
| auto error = msg.parse_error(debug_info); | ||
| HOLOSCAN_LOG_ERROR("GStreamer error: {}", error->message); | ||
| if (!debug_info.empty()) { | ||
| HOLOSCAN_LOG_DEBUG("Debug info: {}", debug_info); | ||
| } | ||
| return; | ||
| } | ||
| case GST_MESSAGE_EOS: | ||
| HOLOSCAN_LOG_INFO("End of stream reached"); | ||
| return; | ||
| case GST_MESSAGE_STATE_CHANGED: { | ||
| // Only check state changes from the pipeline (not individual elements) | ||
| if (GST_MESSAGE_SRC(msg.get()) == GST_OBJECT(pipeline)) { | ||
| GstState old_state, new_state, pending_state; | ||
| gst_message_parse_state_changed(msg.get(), &old_state, &new_state, &pending_state); | ||
|
|
||
| // If pipeline transitions to NULL, stop monitoring | ||
| if (new_state == GST_STATE_NULL && old_state != GST_STATE_NULL) { | ||
| HOLOSCAN_LOG_INFO("Pipeline transitioned to NULL state"); | ||
| return; | ||
| } | ||
| } | ||
| break; | ||
| } | ||
| default: | ||
| break; | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Reset stop_bus_monitor_ before starting a new bus-monitor thread
stop() can set stop_bus_monitor_ to true in the timeout path (lines 733–737), but start() never resets it. If this operator instance is started again after such a shutdown, monitor_pipeline_bus() will see the flag already set and exit immediately, effectively disabling bus monitoring (no error/EOS logging, weaker shutdown guarantees).
To make reuse safe and behavior predictable, reset the flag in start() before launching the async task:
void GstVideoRecorderOperator::start() {
Operator::start();
// Initialize frame counter
frame_count_ = 0;
+ // Ensure bus monitor stop flag is cleared for a fresh run.
+ stop_bus_monitor_.store(false);
+
HOLOSCAN_LOG_INFO("GstVideoRecorderOperator - Starting");Also applies to: 655-657, 725-739
🤖 Prompt for AI Agents
In operators/gstreamer/gst_video_recorder_operator.cpp around lines 204-246 (and
also affecting the start/stop logic at ~lines 655-657 and 725-739), the atomic
stop_bus_monitor_ flag is set to true in stop() on timeout but never reset in
start(), so a restarted operator will immediately exit monitor_pipeline_bus();
fix by resetting stop_bus_monitor_ to false at the beginning of start() (before
launching the async bus-monitor task) so the monitor loop can run on restarts,
and ensure any existing monitor thread is joined or completed before reusing the
flag.
| auto shape = first_tensor_ptr->shape(); | ||
| if (shape.size() < 2 || shape.size() > 3) { | ||
| HOLOSCAN_LOG_ERROR("Tensor rank is {}, expected 2 (height, width) or 3 (height, width, channels)", shape.size()); | ||
| throw std::runtime_error("Invalid tensor shape for video data"); | ||
| } | ||
|
|
||
| int height = shape[0]; | ||
| int width = shape[1]; | ||
|
|
There was a problem hiding this comment.
Fix lint: line length, extra space before (, trailing whitespace
CI lint failures for this file are all whitespace/formatting and can be fixed locally without behavior changes:
- Line ~285: log message longer than 100 chars.
- Line ~291 and ~733: trailing whitespace.
- Line ~629: extra space before
(inparser.get (). - Line ~690: log line longer than 100 chars.
Suggested minimal diff:
- if (shape.size() < 2 || shape.size() > 3) {
- HOLOSCAN_LOG_ERROR("Tensor rank is {}, expected 2 (height, width) or 3 (height, width, channels)", shape.size());
- throw std::runtime_error("Invalid tensor shape for video data");
- }
+ if (shape.size() < 2 || shape.size() > 3) {
+ HOLOSCAN_LOG_ERROR(
+ "Tensor rank is {}, expected 2 (height, width) or 3 (height, width, channels)",
+ shape.size());
+ throw std::runtime_error("Invalid tensor shape for video data");
+ }
@@
- int width = shape[1];
-
+ int width = shape[1];
@@
- gst_bin_add_many(GST_BIN(pipeline_.get()),
- encoder_.get(), parser.get (),
- muxer.get(), filesink.get(), nullptr);
+ gst_bin_add_many(GST_BIN(pipeline_.get()),
+ encoder_.get(), parser.get(),
+ muxer.get(), filesink.get(), nullptr);
@@
- if (!src_bridge_->push_buffer(std::move(buffer))) {
- HOLOSCAN_LOG_ERROR("Frame #{} - Failed to push buffer to encoding pipeline (timeout or error)",
- frame_count_);
+ if (!src_bridge_->push_buffer(std::move(buffer))) {
+ HOLOSCAN_LOG_ERROR(
+ "Frame #{} - Failed to push buffer to encoding pipeline (timeout or error)",
+ frame_count_);
@@
- HOLOSCAN_LOG_WARN("EOS not processed within {} seconds, forcing shutdown",
- kEosTimeoutSeconds.count());
+ HOLOSCAN_LOG_WARN("EOS not processed within {} seconds, forcing shutdown",
+ kEosTimeoutSeconds.count());Also applies to: 628-636, 688-691, 733-735
🧰 Tools
🪛 GitHub Actions: Check linting
[error] 285-285: Lines should be <= 100 characters long [whitespace/line_length]
[error] 291-291: Line ends in whitespace. Consider deleting these extra spaces. [whitespace/end_of_line]
🤖 Prompt for AI Agents
In operators/gstreamer/gst_video_recorder_operator.cpp around lines 283 to 291
(also apply fixes at ~628-636, ~688-691, and ~733-735): fix lint/formatting only
— shorten the HOLOSCAN_LOG_ERROR message to under 100 characters, remove any
trailing whitespace on the noted lines, and remove the extra space before the
parentheses in occurrences like `parser.get ()`; make no behavioral changes,
only adjust strings/spacing to satisfy line-length and whitespace rules.
| /** | ||
| * @brief Setup the operator specification | ||
| * | ||
| * Defines the operator's inputs and parameters: | ||
| * - input: TensorMap containing video frame tensor(s) | ||
| * - encoder: Encoder base name (e.g., "nvh264", "nvh265", "x264", "x265") - default: "nvh264" | ||
| * Note: "enc" suffix is automatically appended to form the element name | ||
| * - framerate: Video framerate as fraction or decimal - default: "30/1" | ||
| * Formats: "30/1", "30000/1001", "29.97", "60" | ||
| * Special: "0/1" for live mode (no framerate control, process frames as fast as they come) | ||
| * Note: In live mode, timestamps reflect actual frame arrival times (real-time) | ||
| * - max_buffers: Maximum number of buffers to queue (0 = unlimited) - default: 10 | ||
| * - block: Whether push_buffer() should block when the queue is full (true = block, | ||
| * false = non-blocking, may drop/timeout) - default: true | ||
| * - filename: Output video filename - default: "output.mp4" | ||
| * Note: If no extension is provided, ".mp4" is automatically appended | ||
| * - properties: Map of encoder-specific properties - default: empty map | ||
| * Examples: {"bitrate": "8000", "preset": "1", "gop-size": "30"} | ||
| * Property types are automatically detected and converted (int, uint, bool, float, etc.) | ||
| * | ||
| * Note: Width, height, format, and storage type are automatically detected from the first frame | ||
| * Note: Parser element is automatically determined from the encoder name | ||
| * Note: Muxer element is automatically determined from the file extension: | ||
| * .mp4 -> mp4mux, .mkv -> matroskamux | ||
| * Unsupported extensions default to mp4mux | ||
| */ |
There was a problem hiding this comment.
Document the format_ parameter or remove it if unused
The setup doc block lists all runtime parameters except format_, but the class declares Parameter<std::string> format_;. It’s unclear to users what this parameter controls (pixel format, caps string fragment, container format, etc.) and how it interacts with the auto‑detection described below.
Please either:
- Document
format_alongside the other parameters in the setup comment (expected values, defaults, and precedence vs. auto‑detection), or - Drop the member if it’s not actually used by the implementation.
Also applies to: 114-114
| * This class ensures proper reference counting for GstBuffer objects and provides | ||
| * automatic cleanup when destroyed. It also provides convenient member functions | ||
| * for common GstBuffer operations. | ||
| * | ||
| * The constructor wraps an existing GstBuffer pointer (or nullptr). Member functions | ||
| * will throw std::runtime_error if called on an invalid (null) buffer. | ||
| */ |
There was a problem hiding this comment.
Align Buffer null‑buffer behavior with documentation
The class‑level comment says that member functions “will throw std::runtime_error if called on an invalid (null) buffer,” but the implementation doesn’t consistently do that (e.g., map() returns false and unmap() is a no‑op on null, and get_size() likely returns 0 on null similar to Memory::get_sizes).
To avoid surprising callers, please either:
- Update the documentation to describe the actual semantics per method (e.g., which ones throw vs. which ones return sentinel values /
falseon null), or - Make the implementation consistent with the documented contract (e.g., throw on null across all member functions).
Given the existing Memory and Buffer map/unmap patterns, adjusting the comments is probably the least disruptive.
Also applies to: 55-89
🤖 Prompt for AI Agents
operators/gstreamer/gst/buffer.hpp around lines 35-41 (also applies to 55-89):
the class comment currently claims all member functions throw std::runtime_error
on a null GstBuffer, but the implementation instead returns sentinel values
(e.g., map() -> false, unmap() -> no-op, get_size() -> 0) for some methods;
update the documentation (not the code) to accurately list per-method
null-buffer semantics to match existing Memory/Buffer patterns — explicitly
state that map() returns false on null, unmap() is a no-op, get_size() returns 0
(or similar sentinel), and which methods actually throw std::runtime_error when
called on a null buffer; keep wording concise, add a short note referencing the
Memory class behavior for consistency, and ensure the comment block covers the
method groups mentioned in lines 55-89.
| template <typename T, T* (*RefFunc)(T*), void (*UnrefFunc)(T*), T* (*RefSinkFunc)(T*) = nullptr> | ||
| class Object { | ||
| public: | ||
| // Constructor from raw pointer (takes ownership) | ||
| explicit Object(T* object = nullptr) | ||
| : ptr_(object, [](T* obj) { | ||
| if (obj) UnrefFunc(obj); | ||
| }) { | ||
| if (object && RefSinkFunc) { | ||
| RefSinkFunc(object); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Unconditional RefSinkFunc can over-ref non-floating GStreamer objects
The constructor always calls RefSinkFunc(object) when non-null:
explicit Object(T* object = nullptr)
: ptr_(object, [](T* obj) {
if (obj) UnrefFunc(obj);
}) {
if (object && RefSinkFunc) {
RefSinkFunc(object);
}
}For newly-created, floating objects this is ideal (gst_object_ref_sink converts the floating ref to a normal one). But for APIs that already return a full (non-floating) reference (e.g. typical *_get_* accessors such as gst_element_get_bus or gst_element_get_static_pad), gst_object_ref_sink increments the refcount. The shared_ptr deleter only calls UnrefFunc once, so those uses will leak one reference.
Given that aliases like Bus, Pad, etc. also use gst_object_ref_sink_typed as RefSinkFunc, this is easy to hit unintentionally.
Consider making sinking explicit, for example:
- explicit Object(T* object = nullptr)
+ explicit Object(T* object = nullptr, bool sink_floating = true)
: ptr_(object, [](T* obj) {
if (obj) UnrefFunc(obj);
}) {
- if (object && RefSinkFunc) {
+ if (object && sink_floating && RefSinkFunc) {
RefSinkFunc(object);
}
}and then using sink_floating = false at call sites that wrap non-floating returns (e.g., Bus{gst_element_get_bus(pipeline), /*sink_floating=*/false}), or alternatively providing separate helper factories like Object::from_floating vs Object::adopt_ref.
🌐 Web query:
In GStreamer, does `gst_element_get_bus()` return a normal (non-floating) reference that must be unreffed by the caller, and does `gst_object_ref_sink()` increment the refcount when the object is not floating?
💡 Result:
Short answer:
-
Yes — gst_element_get_bus() / gst_pipeline_get_bus() returns a normal (owned) reference that you must gst_object_unref() (or unref via your language binding) when done. [1][3]
-
gst_object_ref_sink() will consume a floating reference without changing the refcount; if the object is not floating it increments the refcount by one. [2]
Sources:
- Gst.Element.get_bus — “The element’s Gst.Bus. unref after usage.” (GStreamer docs). [1]
- gst_object_ref_sink() documentation — describes “if the object is floating … leaves the reference count unchanged; if the object is not floating … adds a new normal reference.” (GstObject docs). [2]
- GStreamer test showing gst_pipeline_get_bus increases bus refcount (caller-owned reference). [3]
Unconditional RefSinkFunc causes reference leaks for non-floating GStreamer objects
The constructor always calls RefSinkFunc(object) when non-null:
explicit Object(T* object = nullptr)
: ptr_(object, [](T* obj) {
if (obj) UnrefFunc(obj);
}) {
if (object && RefSinkFunc) {
RefSinkFunc(object);
}
}For newly-created floating objects this is correct (gst_object_ref_sink converts floating to normal). However, APIs like gst_element_get_bus() return a normal (owned) reference that the caller must unref. When gst_object_ref_sink() is called on a non-floating object, it increments the refcount by one, but the shared_ptr deleter calls UnrefFunc only once—leaving the reference count elevated and leaking one reference.
This affects all aliases using gst_object_ref_sink_typed (Bus, Pad, Allocator, etc.) and is easy to trigger unintentionally.
Consider making sinking explicit, for example:
- explicit Object(T* object = nullptr)
+ explicit Object(T* object = nullptr, bool sink_floating = true)
: ptr_(object, [](T* obj) {
if (obj) UnrefFunc(obj);
}) {
- if (object && RefSinkFunc) {
+ if (object && sink_floating && RefSinkFunc) {
RefSinkFunc(object);
}
}Then use sink_floating = false at call sites that wrap non-floating returns (e.g., Bus{gst_element_get_bus(pipeline), /*sink_floating=*/false}), or provide separate factories like Object::from_floating vs Object::adopt_ref.
Description:
Summary by CodeRabbit
New Features
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.