refactor: add pipeline package #20
Conversation
ktro2828
force-pushed
the
refactor/pipeline-package
branch
from
5e86a8c to
8be29af
Compare
Signed-off-by: ktro2828 <kotaro.uetake@tier4.jp>
Signed-off-by: ktro2828 <kotaro.uetake@tier4.jp>
Signed-off-by: ktro2828 <kotaro.uetake@tier4.jp>
Signed-off-by: ktro2828 <kotaro.uetake@tier4.jp>
Signed-off-by: ktro2828 <kotaro.uetake@tier4.jp>
Signed-off-by: ktro2828 <kotaro.uetake@tier4.jp>
Signed-off-by: ktro2828 <kotaro.uetake@tier4.jp>
Signed-off-by: ktro2828 <kotaro.uetake@tier4.jp>
Signed-off-by: ktro2828 <kotaro.uetake@tier4.jp>
Signed-off-by: ktro2828 <kotaro.uetake@tier4.jp>
ktro2828
force-pushed
the
refactor/pipeline-package
branch
from
238f099 to
63a54c5
Compare
There was a problem hiding this comment.
Pull request overview
This PR introduces a new accelerated_image_processor_pipeline package that provides a modular image rectification pipeline with support for multiple backends (NPP, OpenCV CUDA, and CPU). The package includes a builder pattern for automatic backend selection, a unified rectifier interface, and comprehensive test coverage.
Key Changes:
- Implements a flexible rectifier abstraction with three backend implementations (NPP, OpenCV CUDA, CPU)
- Adds builder utilities for automatic backend selection and postprocess callback registration
- Extends common datatypes with
CameraInfostruct for camera calibration support
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| CMakeLists.txt | Build configuration with conditional compilation for NPP and OpenCV CUDA backends |
| package.xml | ROS 2 package manifest declaring dependencies |
| include/accelerated_image_processor_pipeline/builder.hpp | Builder interface for creating rectifiers with optional postprocess callbacks |
| include/accelerated_image_processor_pipeline/rectifier.hpp | Abstract rectifier base class and backend enumeration |
| src/builder.cpp | Implementation of backend selection logic |
| src/rectifier/cpu.cpp | CPU-based rectification using OpenCV |
| src/rectifier/npp.cpp | GPU-based rectification using NVIDIA NPP |
| src/rectifier/opencv_cuda.cpp | GPU-based rectification using OpenCV CUDA |
| src/rectifier/utility.hpp | Utility header for rectification map computation |
| src/rectifier/utility.cpp | Shared logic for computing rectification maps |
| test/*.cpp | Unit tests for builder and each rectifier backend |
| test/test_utility.hpp | Test fixture and utilities |
| README.md | Package documentation and usage examples |
| accelerated_image_processor_common/datatype.hpp | Added CameraInfo struct |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/accelerated_image_processor_pipeline/src/rectifier/opencv_cuda.cpp
Outdated
Show resolved
Hide resolved
Signed-off-by: ktro2828 <kotaro.uetake@tier4.jp>
Signed-off-by: ktro2828 <kotaro.uetake@tier4.jp>
Signed-off-by: ktro2828 <kotaro.uetake@tier4.jp>
ktro2828
force-pushed
the
refactor/pipeline-package
branch
from
7fce1f7 to
17501e3
Compare
manato
left a comment
manato
left a comment
There was a problem hiding this comment.
@ktro2828
I appreciate your ongoing contribution to the refactoring task! The PR-ed code is sophisticated and well organized, including newly introduced tests.
I found a point that we need to pay extra attention toward CUDA stream synchronization, so I left a comment on it. Besides, I left some "nits" comments. I'd appreciate it if you could consider them!
| camera_info_ = prepare_maps(camera_info); | ||
| } | ||
|
|
||
| const common::CameraInfo & get_camera_info() const { return camera_info_.value(); } |
There was a problem hiding this comment.
nit: Only this getter function has get_ prefix. It might be good to get rid of this prefix or add a similar prefix to other getters like alpha() and backend() for consistency.
|
|
||
| private: | ||
| common::Image process_impl(const common::Image & image) override | ||
| { |
There was a problem hiding this comment.
Is it possible to consider adding
if (is_ready()) {
/* return invalid image */
}for better encapsulation (despite the current accelerated_image_processor does not doing this)?
To this end, we may have to update accelerated_image_processor::common::BaseProcessor::process method (or override in accelerated_image_processor::pipeline::Rectifier ) so that it will check the result of process_impl is valid one.
In future expansion, such as video encoding, the process_impl (the function that takes an input image) may return nothing the because encoded result should be acquired in another thread. Considering this as well, checking the validity of the process_impl result would be great.
| dst_step_, | ||
| image.width * 3 * sizeof(Npp8u), // in byte | ||
| image.height, cudaMemcpyDeviceToHost, stream_)); | ||
|
|
There was a problem hiding this comment.
Because the current accelerated_image_processor_compression::JetsonJPEGCompressor and accelerated_image_processor_compression::NvJPEGCompressor have own CUDA stream, and no way to provide another CUDA stream is provided, we need to call cudaStreamSynchronize(stream_) here.
If we imagine a case that users want to do distortion collection then compress it, sharing CUDA stream between Rectifier and JPEGCompressor could reduce latency because we can skip synchronization here.
This stream sharing is realized here in the current accelerated_image_processor.
(Apologize, I completely missed this point during the review of accelerated_image_processor_compression)
There was a problem hiding this comment.
@manato Sorry, I missed this too...
I think we have some possible approaches. Anyway, let me discuss with you in person later when I address ROS dependent package!
Signed-off-by: ktro2828 <kotaro.uetake@tier4.jp>
…urn optional Signed-off-by: ktro2828 <kotaro.uetake@tier4.jp>
…only if is_ready() returns true Signed-off-by: ktro2828 <kotaro.uetake@tier4.jp>
PR Type
Related Links
TIER IV INTERNAL LINK
Description
This pull request introduces a new package,
accelerated_image_processor_pipeline, which provides an extensible and modular image processing pipeline with support for multiple rectification backends (CPU, OpenCV CUDA, and NVIDIA NPP). The changes include initial implementation of the pipeline structure, backend selection logic, and documentation, as well as support for camera calibration data. The most important changes are summarized below:Pipeline Core and Backend Selection:
accelerated_image_processor_pipelinepackage, including a CMake build system, ROS 2 package manifest, and aREADME.mdwith usage examples. This package provides a unified interface for image rectification using multiple backends (CPU, OpenCV CUDA, and NPP), and includes logic to select the best available backend at runtime. [1] [2] [3]Rectifierabstract base class and backend-specific rectifier classes (CpuRectifier,OpenCvCudaRectifier, andNppRectifier), each encapsulating their respective processing logic and resource management. [1] [2] [3]API and Builder Utilities:
builder.hppandbuilder.cppto create rectifier instances, supporting both free and member function callbacks for post-processing. The builder automatically selects the optimal backend based on compilation flags and hardware availability. [1] [2]Camera Calibration Support:
CameraInfostruct to the common datatypes, supporting camera frame identification, timestamping, calibration matrices, and distortion coefficients, which are used by the rectifiers for accurate image processing. [1] [2]These changes lay the foundation for a flexible, hardware-accelerated image processing pipeline with ROS 2 integration and extensible backend support.
Remarks
The following demonstrates how to use the rectifier on ROS 2 codebase:
Pre-Review Checklist for the PR Author
PR Author should check the checkboxes below when creating the PR.
Checklist for the PR Reviewer
Reviewers should check the checkboxes below before approval.
Post-Review Checklist for the PR Author
PR Author should check the checkboxes below before merging.
CI Checks
Unit testings summary