ros2 / demos

Apache License 2.0
502 stars 330 forks source link

``image_tools``: Use type adapter for cv::Mat from cv_bridge #583

Closed ijnek closed 2 years ago

ijnek commented 2 years ago

https://github.com/ros2/demos/blob/9c4ced3c5be392145312e0c0d3653140a2e29cc0/image_tools/include/image_tools/cv_mat_sensor_msgs_image_type_adapter.hpp#L51

This TODO was resolved by adding the class to cv_bridge in https://github.com/ros-perception/vision_opencv/pull/441, so the class can be removed from this repo and the one from cv_bridge can be used instead.

Related: https://github.com/ros-perception/vision_opencv/pull/441#issuecomment-1236313979

ZhenshengLee commented 2 years ago

Hi @ijnek ,

First I want to say that's a wonderful job to get typeAdapter, and I have a QST

I've read the design article from https://ros.org/reps/rep-2007.html , As far as I can see, this feature did make it simple to publish custom type msg directly in publisher creation, but I don't think it can improve the performance of transimision, as the type conversion is actually not avoided.

pub_ = create_publisher<image_tools::ROSCvMatContainer>("image", qos);

So, the number of copy during type conversion with type adaption will not be avoided, am I right?

As the design article said, in the future the (de)serialization function can be avoided within typeadaption, which will certain improve performance. But now, it just make it simple for programmers to publish.

ijnek commented 2 years ago

Kudos to @audrow for implementing this originally, and @wep21 for moving it across to vision_opencv. I haven't tried out type adaption myself, so I think one of you would be better qualified to answer this.

ZhenshengLee commented 2 years ago

So, the number of copy during type conversion with type adaption will not be avoided, am I right?

I think it's true except for rclcpp-intra-process context. refer https://github.com/ros2/rclcpp/pull/1849#issuecomment-998278233

clalancette commented 2 years ago

I think it's true except for rclcpp-intra-process context. refer ros2/rclcpp#1849 (comment)

Yes, exactly. If you use intra-process, it can avoid the copy (and the conversion, for that matter) completely, but only if there is a single subscriber.

ZhenshengLee commented 2 years ago

Yes, exactly. If you use intra-process, it can avoid the copy (and the conversion, for that matter) completely, but only if there is a single subscriber.

Thanks for your quick reply! @clalancette I'd like to talk about the performance of rclcpp-intra-type-adapter and let's move to this issue https://github.com/ros2/rclcpp/issues/1860

@ijnek Thanks!

clalancette commented 2 years ago

I ended up closing the related pull request, with a lot of reasons why I did so. Given that, I'm going to close this one out. If you'd still like to pursue this, please follow the steps I outlined in https://github.com/ros2/demos/pull/584#issuecomment-1263535007 , and we can continue from there.

ijnek commented 2 years ago

Thanks @clalancette