made it work on orin#1
Conversation
gardeg
added
high priority
- Fix header guard in gstreamer_to_ROS.hpp to use ROS2 style - Add missing #include <string> to both headers - Remove explicit keyword from zero-parameter constructors - Remove stray global variable re-declarations in image_to_gstreamer_node.cpp - Add config_arg to LaunchDescription in image_to_gstreamer.launch.py - Apply clang-format and trailing whitespace/EOF fixes Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
vortexuser
left a comment
vortexuser
left a comment
There was a problem hiding this comment.
First round of reviews, only looked at gstreamer_to_ROS so far
| pub_ = this->create_publisher<sensor_msgs::msg::Image>( | ||
| output_topic_, rclcpp::SensorDataQoS()); |
There was a problem hiding this comment.
Use the vortex_utils definition for Sensor data, then we can configure the depth: https://github.com/vortexntnu/vortex-utils/blob/main/vortex_utils_ros/vortex_utils_ros/qos_profiles.py
There was a problem hiding this comment.
Since this only works with H265 (without requiring major refactor), maybe rename files to clarify that it is for H265 encoding and decoding?
|
|
||
| GstCaps* caps = gst_caps_new_simple( | ||
| "application/x-rtp", "media", G_TYPE_STRING, "video", "encoding-name", | ||
| G_TYPE_STRING, "H265", "payload", G_TYPE_INT, 96, NULL); |
There was a problem hiding this comment.
Please add a comment explaining why 96, and what the rest of the parameters mean (and why they were used)
There was a problem hiding this comment.
I would like some comments explaining the different operations, it is not inherently obvious (I dont know Gst syntax), and there is a lot of functions with parameters, difficult to understand what their purpose is, and why those values were used specifically
There was a problem hiding this comment.
Decouple ROS logic from gstreamer logic, main reason is that you then can make tests for the gstreamer logic separate from ROS
There was a problem hiding this comment.
You need to add vortex utils here since you will use it for qos
| <version>0.1.0</version> | ||
| <description>ROS 2 node that fits a line over a white segmented mask using IRLS (Huber/Tukey) and OpenCV.</description> | ||
|
|
||
| <maintainer email="you@example.com">You</maintainer> |
vortexuser
left a comment
vortexuser
left a comment
There was a problem hiding this comment.
Mainly comments explaining how the different parts works, and separating the ROS Layer from the gstreamer code
| framerate: 15 | ||
| bitrate: 500000 | ||
| preset_level: 1 | ||
| iframe_interval: 15 | ||
| control_rate: 1 | ||
| pt: 96 | ||
| config_interval: 1 |
There was a problem hiding this comment.
Add comment explaining what preset_level, iframe_interval, control_rate, pt and config_interval is (not obvious)
There was a problem hiding this comment.
Should also be able to configure the input topic from here on runtime (we want to launch multiple instances of this with different input topic)
There was a problem hiding this comment.
Not a fan of having default value for IP, Port and input topic here, can lead to unexpeceted behavior
| jobs: | ||
| code-coverage: | ||
| uses: vortexntnu/vortex-ci/.github/workflows/reusable-code-coverage.yml@main | ||
| continue-on-error: true |
| image_to_gstreamer_node: | ||
| ros__parameters: | ||
| input_topic: "/zed_node/left/image_rect_color" | ||
| host: "127.0.0.1" |
There was a problem hiding this comment.
Add comment to clarify that this is the IP you want to send the gstreamer data to (or update the variable name to something more obvious than host)
gardeg
force-pushed
the
chore/make-it-work-on-orin
branch
from
7ad4639 to
7e9305e
Compare
|
We should also make it configurable so that you can change if you want to use nvidia encoding or just cpu from config, and remove default params from code if it is defined in a config file |
4 participants
it works on orin, with config launch and all the good shit