Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
ijnek opened this issue Sep 7, 2022 · 7 comments
Closed

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

ijnek opened this issue Sep 7, 2022 · 7 comments
Labels
help wanted Extra attention is needed

Comments

@ijnek
Copy link

ijnek commented Sep 7, 2022

// TODO(wjwwood): make this as a contribution to vision_opencv's cv_bridge package.

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

Related: ros-perception/vision_opencv#441 (comment)

@ZhenshengLee
Copy link

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
Copy link
Author

ijnek commented Sep 29, 2022

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
Copy link

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 ros2/rclcpp#1849 (comment)

@clalancette
Copy link
Contributor

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
Copy link

ZhenshengLee commented Sep 30, 2022

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 ros2/rclcpp#1860

@ijnek Thanks!

@clalancette
Copy link
Contributor

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 #584 (comment) , and we can continue from there.

@ijnek
Copy link
Author

ijnek commented Sep 30, 2022

Thanks @clalancette

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants