feat: enable plugin allowlist

wep21 commented 1 year ago

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

wep21 commented 1 year ago

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

ijnek commented 1 year ago

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.

wep21 commented 1 year ago

friendly ping @gbiggs @jacobperron

gbiggs commented 1 year ago

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

wep21 commented 1 year ago

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

ijnek commented 1 year ago

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).

wep21 commented 1 year ago

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

ijnek commented 1 year ago

@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.

wep21 commented 1 year ago

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