image_common feat: enable plugin blacklist

feat: enable plugin blacklist

Open wep21 opened this issue 1 year ago • 1 comments

I enabled plugin blacklist for image transport publisher. usage:

ros2 run usb_cam usb_cam_node_exe --ros-args -p image_raw.disable_pub_plugins:=["image_transport/compressed"]

❯ ros2 topic list
/camera_info
/image_raw
/image_raw/compressedDepth
/image_raw/theora
/parameter_events
/rosout

Nov 19 '22 06:11 wep21

@gbiggs @jacobperron @ijnek Could you review this PR? Also, I wolud like to backport this change into humble.

Nov 19 '22 06:11 wep21

As this seems like a revival of a feature that existed in ROS 1, I'd like the maintainers to decide on these changes. I do agree that a publisher getting created for every image transport plugin installed is annoying, but a whitelist sounds like a better idea than a blacklist.

Nov 23 '22 14:11 ijnek

friendly ping @gbiggs @jacobperron

Dec 04 '22 04:12 wep21

Please change this to use a whitelist rather than a blacklist, with an additional option to enable all publishers.

Dec 04 '22 22:12 gbiggs

@gbiggs I changed this to whitelist at https://github.com/ros-perception/image_common/pull/264/commits/a6356ca4e6bd1bba3d2985ba505b7c38024374ff.

Dec 07 '22 15:12 wep21

Should probably add all available transports to the whitelist is there are no plugins listed under enable_pub_plugins. Otherwise, this change would break the default behavior of making all installed transports available (which many users depend on).

Dec 09 '22 12:12 ijnek

@ijnek okay, I will set all the plugins as a default parameter.

Dec 09 '22 16:12 wep21

@wep21 Apologies for the delay. I'm just reviewing again, and I think there is no need for a separate std::set and a std::vector, if you simplify:

  for (const auto & lookup_name : loader->getDeclaredClasses()) {
    const std::string transport_name = erase_last_copy(lookup_name, "_pub");
    if (whitelist.count(transport_name)) {
      try {
        auto pub = loader->createUniqueInstance(lookup_name);
        pub->advertise(node, image_topic, custom_qos, options);
        impl_->publishers_.push_back(std::move(pub));
      } catch (const std::runtime_error & e) {
        RCLCPP_ERROR(
          impl_->logger_, "Failed to load plugin %s, error string: %s\n",
          lookup_name.c_str(), e.what());
      }
    }
  }

to:

for (transport_name : whitelist_vec) {
  try {
    auto pub = loader->createUniqueInstance(transport_name);
    pub->advertise(node, image_topic, custom_qos, options);
    impl_->publishers_.push_back(std::move(pub));
  } catch (const std::runtime_error & e) {
    RCLCPP_ERROR(
      impl_->logger_, "Failed to load plugin %s, error string: %s\n",
      transport_name.c_str(), e.what());
  }
}

Please check that these changes do work, as I haven't tested it.

Dec 15 '22 05:12 ijnek

@ijnek I addressed your review here. Could you review my changes? cc @gbiggs

Dec 29 '22 15:12 wep21