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

Update compressed_image_transport to ros2 #26

Merged
merged 5 commits into from
Aug 21, 2018
Merged

Update compressed_image_transport to ros2 #26

merged 5 commits into from
Aug 21, 2018

Conversation

mjcarroll
Copy link
Contributor

@mjcarroll mjcarroll commented Aug 14, 2018

Basic port
TODO:

  • Use ros2 parameters to set encoding
  • Use parameter events to replace dynamic reconfigure.

@j-rivero
Copy link

This PR should be reviews used changes in ros-perception/image_common#84, right?

@mjcarroll
Copy link
Contributor Author

Correct, it depends on the image_transport port to ROS2.

@@ -51,29 +53,37 @@ namespace enc = sensor_msgs::image_encodings;
namespace compressed_image_transport
{

void CompressedPublisher::advertiseImpl(ros::NodeHandle &nh, const std::string &base_topic, uint32_t queue_size,
const image_transport::SubscriberStatusCallback &user_connect_cb,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are not porting the SubscriberStatusCallback to ROS2. Looking for the similiar feature I found a answer.ros.org by William explaining in March that the feature is not implemented yet. I assume that it is still unimplemented.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, it remains unimplemented.

@@ -127,35 +138,36 @@ void CompressedPublisher::publish(const sensor_msgs::Image& message, const Publi

float cRatio = (float)(cv_ptr->image.rows * cv_ptr->image.cols * cv_ptr->image.elemSize())
/ (float)compressed.data.size();
ROS_DEBUG("Compressed Image Transport - Codec: jpg, Compression Ratio: 1:%.2f (%lu bytes)", cRatio, compressed.data.size());
//ROS_DEBUG("Compressed Image Transport - Codec: jpg, Compression Ratio: 1:%.2f (%lu bytes)", cRatio, compressed.data.size());

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RCUTILS_LOG_DEBUG here and in the rest of removals?

}
else
{
ROS_ERROR("cv::imencode (png) failed on input image");
//ROS_ERROR("cv::imencode (png) failed on input image");

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RCUTILS_LOG_ERROR here and in the rest of removals?

void CompressedPublisher::advertiseImpl(ros::NodeHandle &nh, const std::string &base_topic, uint32_t queue_size,
const image_transport::SubscriberStatusCallback &user_connect_cb,
const image_transport::SubscriberStatusCallback &user_disconnect_cb,
const ros::VoidPtr &tracked_object, bool latch)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tracked_object was removed from the arguments, just checking that it was on propose.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, it was removed in the image_transport as well.

}


void CompressedSubscriber::configCb(Config& config, uint32_t level)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure if it could help the future implementation of parameters to leave the configCb code as a comment

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made a config struct that mirrors the config, I figure we can always jump back in the git log to get this.

@j-rivero
Copy link

j-rivero commented Aug 16, 2018

Latest changes look good to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants