H.264 Streaming Applications #923
Draft
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…rator Signed-off-by: Victor Chang <[email protected]>
Signed-off-by: Victor Chang <[email protected]>
mocsharp
force-pushed
the
vchang/mm-h264-streaming
branch
from
57b41f1 to
f1f9d06
Compare
Signed-off-by: Victor Chang <[email protected]>
Signed-off-by: Victor Chang <[email protected]>
- Added a section in the H.264 README to demonstrate chaining the H.264 video encode and decode operators. - Renamed the `tensor_to_video_buffer` operator to `append_timestamp` and updated its metadata, including the minimum required SDK version and tags. - Revised the README for the append_timestamp operator to reflect its new functionality of adding timestamps to incoming GXF entities. Signed-off-by: Victor Chang <[email protected]>
Signed-off-by: Victor Chang <[email protected]>
Member
|
Hi @mocsharp, could you please let me know if this PR is still being worked on or if it has been abandoned? Thanks |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This PR introduces H.264 video streaming capabilities to Holohub through two demonstration applications and operator enhancements. The h264_video_encode_decode application shows how to chain encoder and decoder operators in-memory without disk I/O, while dds_h264 showcases compressed video streaming over RTI Connext DDS. A new append_timestamp operator was created to inject GXF Timestamp metadata between encoder and decoder stages—required for proper frame-level decoding in H.264 workflows. The existing dds_video_publisher and dds_video_subscriber operators were extended to support both raw VideoBuffer (RGBA) and H.264-encoded Tensor inputs, with codec discrimination via the updated VideoFrame.idl schema. These changes leverage Holoscan SDK 2.1.0+'s GXFCodeletOp wrapper mechanism to integrate existing GXF video codec components without custom bindings.
Important Files Changed
| Filename | Score | Overview |
|---|---|---|
| applications/h264/h264_video_encode_decode/README.md | 0/5 | Critical naming inconsistencies throughout - references 'h264_video_decode' instead of 'h264_video_encode_decode' in commands and paths, mentions wrong application name in section title |
| operators/dds/video/dds_video_subscriber/dds_video_subscriber.cpp | 0/5 | File listed in PR but no changes provided in context - cannot review |
| operators/dds/video/dds_video_publisher/dds_video_publisher.cpp | 2/5 | Adds VideoBuffer/Tensor dual-input support and H.264 codec handling; missing CUDA error checks on device-to-host copies, hardcoded H264 assumption for all tensors without validation, potential invalid dimensions when width/height parameters not set |
| applications/h264/CMakeLists.txt | 2/5 | Registers h264_video_encode_decode application but missing video_encoder dependency despite PR description stating encoder is chained with decoder |
| operators/append_timestamp/append_timestamp.cpp | 2/5 | Implements new timestamp-injection operator; pubtime/acqtime set from separate now() calls causing drift, missing error handling on receive operation, silent failure if timestamp cannot be added |
| operators/append_timestamp/python/append_timestamp_pydoc.hpp | 2/5 | Python documentation file contains critical copy-paste error describing operator as "Tensor to VideoBuffer" conversion instead of timestamp appending |
| operators/append_timestamp/python/CMakeLists.txt | 5/5 | Standard Python binding setup using pybind11_add_holohub_module - follows repository conventions with correct class name and source specification |
| applications/h264/h264_video_encode_decode/python/metadata.json | 2/5 | Metadata declares dependencies but only lists decoder operators, missing encoder operators and append_timestamp despite PR description requiring all three |
| applications/h264/h264_video_encode_decode/python/h264_video_encode_decode.py | 3/5 | Implements encode-decode pipeline; parameter name mismatch on line 181 (scheduling_term vs async_scheduling_term) likely causes runtime error, memory pool sizing assumes RGBA (4 bytes/pixel) but YUV uses ~1.5 causing 2x over-allocation, unused operator classes add unnecessary code |
| applications/dds/dds_h264/dds_h264.cpp | 3/5 | Implements DDS H.264 streaming with publisher/subscriber modes; unused video_replayer instantiation at line 166, unused domain_id_/stream_id_ members, getopt_long option string includes unhandled 'i' and 'd' flags |
| applications/dds/dds_h264/dds_h264.yaml | 3/5 | Configuration for DDS H.264 application; missing newline at EOF, inconsistent indentation in v4l2 section, video_subscriber may be missing codec parameter for H.264 bitstream handling |
| applications/dds/dds_h264/Dockerfile | 3/5 | Sets up DDS H.264 environment; duplicate ARG declarations (lines 25-26), mkdir without -p flag may fail on rebuild, cross-directory COPY from applications/h264/ may break depending on context |
| operators/append_timestamp/python/append_timestamp.cpp | 4/5 | Python bindings for append_timestamp operator; copy-paste error at line 76 referencing wrong module name in docstring (_tensor_to_video_buffer), unused includes should be removed for cleanliness |
| operators/CMakeLists.txt | 5/5 | Registers append_timestamp operator; breaks alphabetical ordering convention documented on line 16 (minor organizational issue) |
| applications/h264/README.md | 4/5 | Adds documentation section for new application; link text on line 32 incorrectly says "H.264Video Decode Application" instead of "Encode Decode" |
| operators/dds/video/VideoFrame.idl | 4/5 | Adds Codec enum (None/H264) and transfer_start_time field; backward-incompatible change requiring all DDS endpoints to regenerate type support and redeploy simultaneously |
| applications/dds/dds_h264/README.md | 4/5 | Comprehensive documentation with setup and benchmarks; references potentially missing set_socket_buffer_sizes.sh script, capitalization inconsistency in benchmark table header (line 130) |
| applications/dds/dds_h264/CMakeLists.txt | 4/5 | Builds dds_h264 application; QoS XML copy uses copy instead of copy_if_different triggering unnecessary rebuilds, extensive operator linking may be over-linking if not all used |
| applications/h264/h264_video_encode_decode/python/h264_video_encode_decode.yaml | 4/5 | Pipeline configuration with encoder/decoder settings; missing trailing newline, hardcoded dimensions (854x480) must match dataset or pipeline fails |
| operators/append_timestamp/README.md | 4/5 | Brief operator documentation; typo 'objectS' on line 3, minimal content lacks usage examples and parameter details |
| operators/append_timestamp/CMakeLists.txt | 4.5/5 | Integrates operator into build system; follows standard pattern, hardcoded PATHS in find_package could be improved |
| applications/dds/dds_h264/qos_profiles.xml | 4/5 | DDS QoS configuration for H.264 streaming; typo 'the the' on line 29, well-structured with appropriate buffer sizes and streaming patterns |
| applications/h264/h264_video_encode_decode/python/CMakeLists.txt | 2/5 | Test configuration references wrong filenames (h264_video_decode.py/.yaml) that don't match parent directory name (h264_video_encode_decode), will fail if files don't exist |
| operators/dds/video/dds_video_subscriber/dds_video_subscriber.hpp | 4/5 | Adds FPS monitoring and latency tracking; frame_sizes member lacks trailing underscore convention used by other members |
| operators/append_timestamp/metadata.json | 5/5 | Operator metadata following schema; minor style issue of missing newline at EOF |
| applications/dds/dds_h264/metadata.json | 5/5 | Application metadata with RTI Connext dependency; missing trailing newline (minor) |
| operators/dds/video/dds_video_publisher/dds_video_publisher.hpp | 5/5 | Adds width_ and height_ parameters for codec support; minimal safe changes following existing patterns |
| applications/h264/h264_video_encode_decode/CMakeLists.txt | 5/5 | Top-level build file delegating to python/ subdirectory; follows standard Holohub pattern for Python-only applications |
| applications/dds/CMakeLists.txt | 5/5 | Registers dds_h264 application with operator dependencies; straightforward registration following repository conventions |
| operators/append_timestamp/python/append_timestamp_pydoc.hpp | 5/5 | Python documentation header following standard pattern; see separate entry for critical docstring content error |
| operators/append_timestamp/append_timestamp.hpp | 5/5 | Header declaration for new operator; follows standard Holoscan operator patterns with proper includes and license |
Confidence score: 2/5
- This PR requires careful review and will likely cause runtime failures if merged as-is due to multiple critical issues across several files
- Score reflects critical naming inconsistencies in h264_video_encode_decode README that will break user workflows, missing critical dependencies in CMakeLists.txt and metadata.json files, parameter naming bugs that will cause runtime errors, missing error handling in core operator logic, and incorrect documentation that will mislead users
- Pay close attention to applications/h264/h264_video_encode_decode/README.md (needs complete rewrite of file references), applications/h264/CMakeLists.txt (add video_encoder dependency), applications/h264/h264_video_encode_decode/python/metadata.json (add encoder dependencies), applications/h264/h264_video_encode_decode/python/h264_video_encode_decode.py (fix parameter name on line 181), applications/h264/h264_video_encode_decode/python/CMakeLists.txt (fix test file references), operators/append_timestamp/python/append_timestamp_pydoc.hpp (rewrite docstring), operators/append_timestamp/append_timestamp.cpp (add error handling and fix timestamp drift), and operators/dds/video/dds_video_publisher/dds_video_publisher.cpp (add CUDA error checks and validate codec assumptions)
Sequence Diagram
sequenceDiagram
participant User
participant Source as Video Source<br/>(Replayer/V4L2)
participant Converter as Format Converter
participant T2VB as Tensor to VideoBuffer
participant EncReq as Video Encoder Request
participant EncResp as Video Encoder Response
participant Timestamp as Append Timestamp
participant DecReq as Video Decoder Request
participant DecResp as Video Decoder Response
participant Holoviz as Holoviz
User->>Source: Start application
Source->>Converter: "output" → "source_video"<br/>(RGB888/RGBA8888)
Converter->>T2VB: "tensor" → "in_tensor"<br/>(YUV420)
T2VB->>EncReq: "out_video_buffer" → "input_frame"<br/>(VideoBuffer)
EncReq->>EncResp: Encode frame to H.264
EncResp->>Timestamp: "output_transmitter" → "in_tensor"<br/>(H.264 bitstream)
Timestamp->>DecReq: "out_tensor" → "input_frame"<br/>(H.264 + timestamp)
DecReq->>DecResp: Decode H.264 frame
DecResp->>Holoviz: "output_transmitter" → "receivers"<br/>(NV12/YUV420)
Holoviz->>User: Display video
30 files reviewed, 51 comments
| add_holohub_operator(ehr_query_llm) | ||
| add_holohub_operator(xr) | ||
|
|
||
| add_holohub_operator(append_timestamp) |
Contributor
There was a problem hiding this comment.
style: Breaks alphabetical ordering convention stated in line 16 comment. Should be placed after line 19 (after apriltag_detector).
| CPP_CMAKE_TARGET append_timestamp | ||
| CLASS_NAME "AppendTimestampOp" | ||
| SOURCES append_timestamp.cpp | ||
| ) No newline at end of file |
Contributor
There was a problem hiding this comment.
style: Missing newline at end of file
| "ranking": 1, | ||
| "dependencies": {} | ||
| } | ||
| } No newline at end of file |
Contributor
There was a problem hiding this comment.
style: Missing newline at end of file—other metadata.json files in the repository include a final newline after the closing brace
| # limitations under the License. | ||
|
|
||
| cmake_minimum_required(VERSION 3.20) | ||
| project(h264_video_encode_decode_apps LANGUAGES NONE) |
Contributor
There was a problem hiding this comment.
style: Project name uses _apps suffix, but single application directory. Consider h264_video_encode_decode to match the directory name.
| In a streaming setup, one app might use the encoder, while another app receives the encoded frames | ||
| and decodes each one. | ||
|
|
||
| [Building and Running the H.264 Video Decode Application](./h264_video_encode_decode/README.md) |
Contributor
There was a problem hiding this comment.
syntax: the link text says "Decode" but should say "Encode Decode" to match the section heading
Suggested change
| [Building and Running the H.264 Video Decode Application](./h264_video_encode_decode/README.md) | |
| [Building and Running the H.264 Video Encode Decode Application](./h264_video_encode_decode/README.md) |
| if (buffer->storage_type() == nvidia::gxf::MemoryStorageType::kHost) { | ||
| memcpy(data.data(), buffer->pointer(), data.size()); | ||
| } else { | ||
| cudaMemcpy(data.data(), buffer->pointer(), data.size(), cudaMemcpyDeviceToHost); |
Contributor
There was a problem hiding this comment.
logic: Missing error check. cudaMemcpy can fail but the return status is not checked, which could result in sending corrupted data over DDS.
| if (tensor->storage_type() == nvidia::gxf::MemoryStorageType::kHost) { | ||
| memcpy(data.data(), tensor->pointer(), data.size()); | ||
| } else { | ||
| cudaMemcpy(data.data(), tensor->pointer(), data.size(), cudaMemcpyDeviceToHost); |
Contributor
There was a problem hiding this comment.
logic: Missing error check. cudaMemcpy can fail but the return status is not checked, which could result in sending corrupted data over DDS.
Comment on lines
+106
to
+107
| frame.width(width_.get()); | ||
| frame.height(height_.get()); |
Contributor
There was a problem hiding this comment.
logic: Width and height parameters are required when input is a tensor but may be 0 if not set (default value from setup). This will result in invalid frame dimensions.
| def compose(self): | ||
| width = 854 | ||
| height = 480 | ||
| source_block_size = width * height * 3 * 4 |
Contributor
There was a problem hiding this comment.
logic: Memory calculation assumes RGBA (4 channels), but encoder expects YUV input (fewer bytes per pixel). If format_converter outputs YUV420, actual buffer size needed is ~1.5x width×height, not 3×4×width×height. Does the format_converter output RGBA or YUV, and should source_block_size match the actual encoded format?
| self, name="tensor_to_video_buffer", **self.kwargs("tensor_to_video_buffer") | ||
| ) | ||
| encoder_async_condition = AsynchronousCondition(self, "encoder_async_condition") | ||
| video_encoder_context = VideoEncoderContext(self, scheduling_term=encoder_async_condition) |
Contributor
There was a problem hiding this comment.
syntax: Parameter name mismatch: VideoEncoderContext expects async_scheduling_term (line 119), but scheduling_term is passed here.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR adds two sample applications:
applications/h264/h264_video_encode_decode: demo how to chain H.264 encoder and decoder without writing to disk. Theappend_timestampis needed between the encoder and decoder operators so the decoder can correctly decode each frame.Changes to operators:
append_timestampis a new operator that adds a GXF Timestamp object to the incoming Tensor.dds_video_publisheris updated to support VideoBuffer and GXF Entity, along with any additional parameters and codec information for the published video.dds_video_subscriberis updated to handle the H.264 codec if specified and to record latency measurements.