Image Transport rework port in progress for review#84
Conversation
VoidPtr -> std::shared_ptr<void> sensor_msgs::Image -> sensor_msgs::msg::Image sensor_msgs::ImageConstPtr -> sensor_msgs::msg::Image::ConstSharedPtr
|
Small patch to export headers and libraries to downstream package and the ament_package() to enable the full support inside ament (install setup.* files and some other friends): diff --git a/image_transport/CMakeLists.txt b/image_transport/CMakeLists.txt
index e19da36..4c7fbe3 100644
--- a/image_transport/CMakeLists.txt
+++ b/image_transport/CMakeLists.txt
@@ -59,6 +59,9 @@ install(
DESTINATION include
)
+ament_export_include_directories(include)
+ament_export_libraries(${PROJECT_NAME})
+
if(BUILD_TESTING)
find_package(ament_lint_auto REQUIRED)
ament_lint_auto_find_test_dependencies()
@@ -90,3 +93,5 @@ if(BUILD_TESTING)
target_link_libraries(${PROJECT_NAME}-single_subscriber_publisher ${PROJECT_NAME})
endif()
endif()
+
+ament_package()After these changes (and source install/setup.bash) we have one test failing: 4: Test timeout computed to be: 60
4: -- run_test.py: invoking following command in '/home/jrivero/code/ros2/image_transport_ws/src/image_common/image_transport':
4: - /home/jrivero/code/ros2/image_transport_ws/build/image_transport/image_transport-message_passing --gtest_output=xml:/home/jrivero/code/ros2/image_transport_ws/build/image_transport/test_results/image_transport/image_transport-message_passing.gtest.xml
4: [==========] Running 2 tests from 1 test case.
4: [----------] Global test environment set-up.
4: [----------] 2 tests from MessagePassingTesting
4: [ RUN ] MessagePassingTesting.one_message_passing
4: [WARN] [pluginlib.ClassLoader]: given plugin name 'lib/libimage_transport_plugins' should be '/libimage_transport_plugins' for better portability
4: unknown file: Failure
4: C++ exception with description "No plugins found! Does `rospack plugins --attrib=plugin image_transport` find any packages?" thrown in the test body.
4: [ FAILED ] MessagePassingTesting.one_message_passing (18 ms)
4: [ RUN ] MessagePassingTesting.stress_message_passing
4: [WARN] [pluginlib.ClassLoader]: given plugin name 'lib/libimage_transport_plugins' should be '/libimage_transport_plugins' for better portability
4: unknown file: Failure
4: C++ exception with description "No plugins found! Does `rospack plugins --attrib=plugin image_transport` find any packages?" thrown in the test body.
4: [ FAILED ] MessagePassingTesting.stress_message_passing (13 ms)
4: [----------] 2 tests from MessagePassingTesting (31 ms total)
4:
4: [----------] Global test environment tear-down
4: [==========] 2 tests from 1 test case ran. (31 ms total)
4: [ PASSED ] 0 tests.
4: [ FAILED ] 2 tests, listed below:
4: [ FAILED ] MessagePassingTesting.one_message_passing
4: [ FAILED ] MessagePassingTesting.stress_message_passingAnd a bunch of QA linters are also complaining: I can help fixing some of these linters. Let me know which ones do you want me to take care of. |
The latest changes make the tests to pass for me. To fix the lint_cmake, a minimal patch is required: diff --git a/image_transport/tutorial/CMakeLists.txt b/image_transport/tutorial/CMakeLists.txt
index ed5f28f..6b8455a 100644
--- a/image_transport/tutorial/CMakeLists.txt
+++ b/image_transport/tutorial/CMakeLists.txt
@@ -5,7 +5,7 @@ find_package(catkin REQUIRED COMPONENTS cv_bridge image_transport message_genera
# add the resized image message
add_message_files(DIRECTORY msg
- FILES ResizedImage.msg
+ FILES ResizedImage.msg
)
generate_messages(DEPENDENCIES sensor_msgs)To fix the other linters a bit more of work is needed. We can disable them by now or spend some hours fixing them. |
tfoote
left a comment
There was a problem hiding this comment.
Overall this looks good. There are a few places to improve the debug outputs from fprintf and also a few places to add TODOs where things were skipped for the MVP
| if (simple_impl_) return simple_impl_->sub_.getNumPublishers(); | ||
| return 0; | ||
| //if (simple_impl_) return simple_impl_->node_->count_publishers(getTopic()); | ||
| return 1; |
There was a problem hiding this comment.
Add a TODO to implement this
| virtual void shutdown() | ||
| { | ||
| if (simple_impl_) simple_impl_->sub_.shutdown(); | ||
| //if (simple_impl_) simple_impl_->sub_.shutdown(); |
| * \param base_topic The topic to subscribe to. | ||
| * \param queue_size The subscription queue size | ||
| * \param transport_hints The transport hints to pass along | ||
| * \param transportThe transport hints to pass along |
| * \brief Subscribe to an image topic, version for class member function with shared_ptr. | ||
| */ | ||
| template<class T> | ||
| void subscribe(ros::NodeHandle& nh, const std::string& base_topic, uint32_t queue_size, |
There was a problem hiding this comment.
Is there a reason to cut the shared_ptr implementation/option here?
|
|
||
| CameraPublisher::CameraPublisher(ImageTransport& image_it, ros::NodeHandle& info_nh, | ||
| const std::string& base_topic, uint32_t queue_size, | ||
| const SubscriberStatusCallback& image_connect_cb, |
There was a problem hiding this comment.
A todo to restore support for these callbacks would be good. Maybe they shouldn't be in the constructor, but as an additional method etc.
| unsubscribed_ = true; | ||
| image_sub_.unsubscribe(); | ||
| info_sub_.unsubscribe(); | ||
| //image_sub_.unsubscribe(); |
There was a problem hiding this comment.
Which pointers? I think that the only one in this scope is the timer pointer?
There was a problem hiding this comment.
The info_sub_ and image_sub_ are shared pointers to the subscribers. If we're not explicitly calling unsubscribe on them they'll keep subscribing to data. So to get them to stop subscribing you can reset the shared pointers info_sub_.reset() to stop them from subscribing, assuming that no one else has stored a reference to them.
| { | ||
| int threshold = 3 * both_received_; | ||
| if (image_received_ > threshold || info_received_ > threshold) { | ||
| /* |
| /// @todo Fix this when message_filters::Subscriber has getNumPublishers() | ||
| //if (impl_) return std::max(impl_->image_sub_.getNumPublishers(), impl_->info_sub_.getNumPublishers()); | ||
| if (impl_) return impl_->image_sub_.getNumPublishers(); | ||
| //if (impl_) return impl_->image_sub_.getNumPublishers(); |
image_transport/src/publisher.cpp
Outdated
| if (blacklist.count(transport_name)) | ||
| { | ||
| for (const auto & lookup_name: loader->getDeclaredClasses()) { | ||
| //TODO(ros2) remove boost::erase_last_copy |
There was a problem hiding this comment.
TODO resolved with local implementation
image_transport/src/publisher.cpp
Outdated
| pub->advertise(node, image_topic, custom_qos); | ||
| impl_->publishers_.push_back(std::move(pub)); | ||
| } catch (const std::runtime_error & e) { | ||
| fprintf(stderr, "Failed to load plugin %s, error string: %s\n", |
There was a problem hiding this comment.
More places here for debug macros instead of fprintf
|
| #include <sensor_msgs/Image.h> | ||
| #include <boost/noncopyable.hpp> | ||
| #include "image_transport/transport_hints.h" | ||
| #include <rclcpp/macros.hpp> |
There was a problem hiding this comment.
while implementing the theora plugin I made the following changes to be able to compile the package:
diff --git a/image_transport/include/image_transport/subscriber_plugin.h b/image_transport/include/image_transport/subscriber_plugin.h
index 61d5011..0f0afb0 100644
--- a/image_transport/include/image_transport/subscriber_plugin.h
+++ b/image_transport/include/image_transport/subscriber_plugin.h
@@ -36,6 +36,8 @@
#define IMAGE_TRANSPORT_SUBSCRIBER_PLUGIN_H
#include <rclcpp/macros.hpp>
+#include <rclcpp/node.hpp>
+#include <sensor_msgs/msg/image.hpp>
namespace image_transport
{There was a problem hiding this comment.
Included, thanks.
mikaelarguedas
left a comment
There was a problem hiding this comment.
a quick review just for the CMake and package.xml part of the PR
image_transport/CMakeLists.txt
Outdated
| find_package(rclcpp REQUIRED) | ||
| find_package(sensor_msgs REQUIRED) | ||
| find_package(tinyxml_vendor REQUIRED) | ||
| find_package(TinyXML REQUIRED) |
There was a problem hiding this comment.
is tinyxml_vendor/tinyxml used anywhere in this package?
image_transport/CMakeLists.txt
Outdated
| # Build libimage_transport | ||
| # Build image_transport library | ||
| add_library(${PROJECT_NAME} | ||
| SHARED |
There was a problem hiding this comment.
If there is a use case for this to be built statically (maybe there is not), we recommend using ament_cmake_ros to allow people to chose to build statically or dynamically via the BUILD_SHARED_LIBS CMake option
| # Install libraries | ||
| install( | ||
| TARGETS ${PROJECT_NAME} ${PROJECT_NAME}_plugins | ||
| LIBRARY DESTINATION lib |
There was a problem hiding this comment.
ARCHIVE DESTINATION lib
LIBRARY DESTINATION lib
RUNTIME DESTINATION bin
For multi OS support in the future
image_transport/CMakeLists.txt
Outdated
| # Install executables | ||
| install( | ||
| TARGETS list_transports republish | ||
| RUNTIME DESTINATION share/${PROJECT_NAME} |
There was a problem hiding this comment.
Executables should be installed to DESTINATION lib/${PROJECT_NAME} to be found by ros2 run
image_transport/CMakeLists.txt
Outdated
| DESTINATION ${CATKIN_PACKAGE_SHARE_DESTINATION} | ||
| # Install include directories | ||
| install( | ||
| DIRECTORY "include/" |
There was a problem hiding this comment.
Nit: quotes unnecessary here
image_transport/package.xml
Outdated
|
|
||
| <buildtool_depend>ament_cmake</buildtool_depend> | ||
|
|
||
| <depend>class_loader</depend> |
There was a problem hiding this comment.
This dependency seems to be unused
image_transport/package.xml
Outdated
| <depend>message_filters</depend> | ||
| <depend>sensor_msgs</depend> | ||
| <depend>pluginlib</depend> | ||
| <depend>tinyxml_vendor</depend> |
There was a problem hiding this comment.
This dependency seems to be unused
image_transport/package.xml
Outdated
|
|
||
| <test_depend>ament_lint_auto</test_depend> | ||
| <test_depend>ament_lint_common</test_depend> | ||
| <test_depend>ament_cmake_gtest</test_depend> |
There was a problem hiding this comment.
Nit: alphabetical order
| @@ -1,4 +1,4 @@ | |||
| <package> | |||
| <package format="2"> | |||
There was a problem hiding this comment.
We should update maintainer names to avoid original maintainers receiving email when we start testing / releasing this on the ROS 2 farm
|
Changes looks good to me, it works well on my Bionic system. Great work Michael, thanks. |
| const std::string & base_topic, | ||
| const Subscriber::Callback& callback, | ||
| const std::string& transport, | ||
| rmw_qos_profile_t custom_qos = rmw_qos_profile_default); |
There was a problem hiding this comment.
It would be helpful for porting existing code if *at least additionally) the previous function signature is maintained. So in this case keep a queue_size parameter and set it internally on the profile.
Same applies to other signatures.
There was a problem hiding this comment.
I guess there are several other API changes which will make it a not straight forward conversion at this point.
It looks like the hints have been removed from several signature but instead the transport string has been introduced. What is the plan around the TransportHints?
There was a problem hiding this comment.
Can do for queue_size.
I had originally cut TransportHints because it would require a parameters_client internally in order to retrieve the string parameter. The only other thing it was doing was interacting with the ROS1 transport hints, so it seemed easier to pass a string in from the outside.
I can make another similar structure if keeping the API the same is important.
There was a problem hiding this comment.
I have ported the web_video_server package already. I just noticed the effort during that work. I am not sure if anything should be done but every change in the API (as small as it might be) will be perceived as an (unnecessary?) burden by every user / developer when porting their code.
There was a problem hiding this comment.
Sounds reasonable, can I do it in a second PR so I can get this one merged?
There was a problem hiding this comment.
Absolutely. Not a blocker.
|
|
Windows and OSX failures expected, due to |
Can you clarify what deps are missing ?
While I don't think there is much we can do about the CMake warning. It would be valuable to modify the CI branch you use so that the |
|
Looks like Boost on Windows, and boost_python3 on OS X, I'll see what I can do to get them fixed. |
Builds upon: ros2/message_filters#5
Port of image_transport to work with ROS2.
To use, the branch from the ros2/message_filters repo above needs to be used.
There's now more commits in the history than I plan to have at the end. Several of the later ones are reverting non-essential changes earlier. I plan to find where they were changed and squash the revert of the changes down.