Support for passing parameters to cv::imencode through "toCompressedImageMsg" function. #521
Conversation
… for toCompressedImageMsg
| const Format dst_format = | ||
| JPG) const; | ||
| const Format dst_format = JPG, | ||
| const std::vector<int> ¶ms=std::vector<int>()) const; |
There was a problem hiding this comment.
There need to be spaces around = to follow the style in the rest of the code. I also suggest using the curly braces initialiser ({}) in place of the explicit constructor for easier readability.
Adding a new parameter to a function breaks the ABI, meaning that the new library can not be replaced without recompiling all packages that link it. For backward compatibility, you have to keep a function with the old signature that calls your new function with an empty params.
There was a problem hiding this comment.
Thanks for the detailed feedback. I'll let a function with the original signature. I thought since the parameter is default it will not alter the other packages i did not thought of compiling.
| @@ -464,15 +464,21 @@ CvImagePtr cvtColor( | |||
|
|
|||
| /////////////////////////////////////// CompressedImage /////////////////////////////////////////// | |||
|
|
|||
| sensor_msgs::msg::CompressedImage::SharedPtr CvImage::toCompressedImageMsg(const Format dst_format) | |||
| sensor_msgs::msg::CompressedImage::SharedPtr CvImage::toCompressedImageMsg(const Format dst_format,const std::vector<int> ¶ms) | |||
There was a problem hiding this comment.
Add a space between the end of a parameter and the start of a new one. See the style in other parts of the code.
| sensor_msgs::msg::CompressedImage::SharedPtr toCompressedImageMsg( | ||
| const std::vector<int> ¶ms=std::vector<int>(), const Format dst_format = JPG) const; |
There was a problem hiding this comment.
Why do you need a second function of the same name with swapped parameter order?
There was a problem hiding this comment.
I thought of this like, may be i want only provide parameters for the cv::imencode. Otherwise the dest_format need to be explicitly specified in each call we want provide params to cv::imencode.
| */ | ||
| void toCompressedImageMsg( | ||
| sensor_msgs::msg::CompressedImage & ros_image, | ||
| const Format dst_format = JPG) const; | ||
| const Format dst_format = JPG, const std::vector<int> ¶ms=std::vector<int>()) const; |
There was a problem hiding this comment.
Use the curly braces initialiser ({}).
| sensor_msgs::msg::CompressedImage::SharedPtr CvImage::toCompressedImageMsg(const std::vector<int> ¶ms, const Format dst_format) | ||
| const | ||
| { | ||
| return toCompressedImageMsg(dst_format, params); | ||
| } | ||
|
|
There was a problem hiding this comment.
What is this function actually doing? It just swaps the parameters.
There was a problem hiding this comment.
That's only the implementation for providing parameters to cv::imencode without explicitly providing the format.
| void toCompressedImageMsg( | ||
| sensor_msgs::msg::CompressedImage & ros_image, | ||
| const std::vector<int> ¶ms=std::vector<int>(), const Format dst_format = JPG) const; |
There was a problem hiding this comment.
Here too. Why do we need the same function with swapped parameters?
There was a problem hiding this comment.
The same reason like the other function. If this is not neccessary i can remove it (swapping the parameters) and only provide cv::imencode params alongside with dst_format.
|
Hello again, |
|
Hey @emrekuru97, thanks for opening this PR, and thanks for reviewing @christianrauch! Despite the method "toCompressedImageMsg" existing in this package, moving forward, I don't think we should be extending cv_bridge's capability to compress images. cv_bridge's responsibility is to convert ROS Image msg <-> OpenCV image. The thing that immediately came to mind when I read this was the compressed transport plugin, which is used to transport compressed image messages over a topic. In that plugin, the conversion takes place, and then the compression is called afterwards. Would this work for your use case? (or could you just use the compressed publisher? |
Support for passing parameters to cv::imencode through
toCompressedImageMsgfunction, i need this functionality so that the compression quality can be passed throughtoCompressedImageMsgfunction, other pepople can pass other options too. Tried to keep current functionality and generated possible other signatures for the functiontoCompressedImageMsg. Could not find a formatter in the repo so did not formatting, the default ros formatter changed a lot so i let it.