ros-perception / image_common

Common code for working with images in ROS
http://www.ros.org/wiki/image_common
124 stars 219 forks source link

Support zero-copy intra-process publishing #306

Closed bjsowa closed 3 months ago

bjsowa commented 4 months ago

ROS2 supports intra-process communication when composing multiple nodes in a single process. It additionally can avoid doing any copies of the data when published using std::unique_ptr<MessageT> and subscribed using shared_ptr<const MessageT> or std::unique_ptr<MessageT> (at most 1 subscriber)

This PR is my attempt to add support for publishing using std::unique_ptr without breaking any of the existing APIs.

The idea is that:

  1. Each publisher plugin can decide whether they support publishing using unique ptr (for now I don't see how any other plugin than raw can take advantage of the ownership).
  2. The new Publisher::publish(sensor_msgs::msg::Image::UniquePtr message) method first publishes using the const reference to plugins that don't support std::unique_ptr, then moves the ownership of the message to (at most 1) plugin that supports it.
  3. If there is more than one plugin that supports std::unique_ptr and has subscriptions, we pass the ownership to only one such plugin (the first we find), and for the rest we still use const reference, delegating doing any extra copies to plugin implementations.

Related issues/prs:

212

215

216 (kinda)

bjsowa commented 4 months ago

are you planning to make some changes in image_transport_plugins too ? using this new API ?

I don't see how the compressed image transports can take advantage of the image ownership as they still have to copy the data, BUT they might avoid copying the resulting compressed data by publishing it using std::unique_ptr so I was planning to make them use the more generic publish(const sensor_msgs::msg::Image & message, const PublisherT & publisher) method to publish unique_ptr instead of const reference.

bjsowa commented 4 months ago

I marked the method as deprecated. Also, I think that we should not add default implementations that are empty or don't make sense. I also can't make these methods abstract, so instead I just throw exceptions. @ahcorde What do you think?

ahcorde commented 3 months ago
bjsowa commented 3 months ago

@ahcorde Could this be backported to Jazzy?

ahcorde commented 3 months ago

https://github.com/Mergifyio backport jazzy

mergify[bot] commented 3 months ago

backport jazzy

✅ Backports have been created

* [#310 Support zero-copy intra-process publishing (backport #306)](https://github.com/ros-perception/image_common/pull/310) has been created for branch `jazzy`