feat: enable plugin allowlist #264

wep21 · 2022-11-19T06:19:55Z

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

Signed-off-by: Daisuke Nishimatsu <border_goldenmarket@yahoo.co.jp>

wep21 · 2022-11-19T06:22:16Z

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

Signed-off-by: Daisuke Nishimatsu <border_goldenmarket@yahoo.co.jp> Signed-off-by: wep21 <border_goldenmarket@yahoo.co.jp>

ijnek · 2022-11-23T14:29:50Z

image_transport/src/publisher.cpp

@@ -110,9 +110,23 @@ Publisher::Publisher(
  impl_->base_topic_ = image_topic;
  impl_->loader_ = loader;

+  uint ns_len = node->get_effective_namespace().length();


Can param_base_name just be equal to base_topic, rather than stripping off the namespace here?

@gbiggs @ijnek This comes from image_transport_plugins.
Which is better?

use base topic name
This looks like below.

/usb_cam: ros__parameters: image_raw: enable_pub_plugins: - image_transport/compressed format: jpeg jpeg_quality: 95 png_level: 3 tiff: res_unit: inch xdpi: -1 ydpi: -1

use whole topic name
This looks like below.

/usb_cam: ros__parameters: image_raw: format: jpeg jpeg_quality: 95 png_level: 3 tiff: res_unit: inch xdpi: -1 ydpi: -1 /aaa/bbb/image_raw: enable_pub_plugins: - image_transport/compressed

Just the base topic name seems better.

ijnek · 2022-11-23T14:35:36Z

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 · 2022-12-04T04:53:50Z

friendly ping @gbiggs @jacobperron

gbiggs · 2022-12-04T22:53:15Z

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

Signed-off-by: Daisuke Nishimatsu <border_goldenmarket@yahoo.co.jp>

wep21 · 2022-12-07T15:42:03Z

@gbiggs I changed this to whitelist at a6356ca.

Signed-off-by: Daisuke Nishimatsu <border_goldenmarket@yahoo.co.jp>

ijnek · 2022-12-09T12:09:56Z

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 · 2022-12-09T16:07:43Z

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

Signed-off-by: Daisuke Nishimatsu <border_goldenmarket@yahoo.co.jp>

ijnek · 2022-12-15T05:59:38Z

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

Signed-off-by: Daisuke Nishimatsu <border_goldenmarket@yahoo.co.jp>

wep21 · 2022-12-29T15:45:24Z

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

gbiggs · 2023-01-12T05:28:04Z

image_transport/src/publisher.cpp

+      param_base_name +
+      ".enable_pub_plugins").get_value<std::vector<std::string>>();
+  }
+  for (size_t i = 0; i < whitelist_vec.size(); ++i) {


What is the purpose of copying the contents of the vector into a set if you're then just going to iterate over it?

@gbiggs This is from original code, but I guess this removes duplicated plugins declared in the parameters.

gbiggs · 2023-01-12T05:28:23Z

image_transport/src/publisher.cpp

  }
-
+  std::vector<std::string> whitelist_vec;


Please use the term "allowlist" instead of "whitelist".

@gbiggs I addressed here

Signed-off-by: wep21 <daisuke.nishimatsu1021@gmail.com>

ahcorde

@gbiggs can we merge this ?

ahcorde · 2023-07-17T07:55:58Z

Linux
Linux-aarch64
Windows

ahcorde

can you take a look to this windows failures ? https://ci.ros2.org/job/ci_windows/19910/console

image_transport/src/publisher.cpp

Co-authored-by: Alejandro Hernández Cordero <ahcorde@gmail.com>

wep21 · 2023-07-17T13:49:38Z

@ahcorde Thank you for suggesting the fix. Could you run ci again?

ahcorde · 2023-07-17T13:53:02Z

Linux
Linux-aarch64
Windows

Sushant-Chavan · 2023-12-27T17:30:04Z

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

@wep21 Thank you for the PR. Do you also plan to merge these changes into humble?

chrbrcknr · 2024-01-09T08:35:48Z

Yes, it would be great to have this in humble too!

juanluishortelano · 2024-03-06T07:40:56Z

Yes, would also love to have this on humble

feat: enable plugin blacklist

116c970

Signed-off-by: Daisuke Nishimatsu <border_goldenmarket@yahoo.co.jp>

use param base name instead of base topic

c5c1d99

Signed-off-by: Daisuke Nishimatsu <border_goldenmarket@yahoo.co.jp> Signed-off-by: wep21 <border_goldenmarket@yahoo.co.jp>

wep21 force-pushed the enable-plugin-blacklist branch from 52d6751 to c5c1d99 Compare November 22, 2022 01:54

ijnek reviewed Nov 23, 2022

View reviewed changes

Daisuke Nishimatsu added 2 commits December 8, 2022 00:28

create whitelist instead of blacklist

a6356ca

Signed-off-by: Daisuke Nishimatsu <border_goldenmarket@yahoo.co.jp>

remove initial dot

d75b2a1

Signed-off-by: Daisuke Nishimatsu <border_goldenmarket@yahoo.co.jp>

wep21 changed the title ~~feat: enable plugin blacklist~~ feat: enable plugin whitelist Dec 7, 2022

apply format

8b8c6e7

Signed-off-by: Daisuke Nishimatsu <border_goldenmarket@yahoo.co.jp>

set all transport names as default

462f46f

Signed-off-by: Daisuke Nishimatsu <border_goldenmarket@yahoo.co.jp>

address review

5a92494

Signed-off-by: Daisuke Nishimatsu <border_goldenmarket@yahoo.co.jp>

ijnek approved these changes Jan 3, 2023

View reviewed changes

gbiggs reviewed Jan 12, 2023

View reviewed changes

Ronoman mentioned this pull request Feb 27, 2023

disable_pub_plugins parameter commented out in ROS2 #269

Open

rename whitelist to allowlist

f24fa62

Signed-off-by: wep21 <daisuke.nishimatsu1021@gmail.com>

wep21 requested a review from gbiggs April 26, 2023 16:22

wep21 changed the title ~~feat: enable plugin whitelist~~ feat: enable plugin allowlist Apr 26, 2023

ahcorde approved these changes Jul 17, 2023

View reviewed changes

ahcorde requested changes Jul 17, 2023

View reviewed changes

ahcorde reviewed Jul 17, 2023

View reviewed changes

image_transport/src/publisher.cpp Outdated Show resolved Hide resolved

change type of namespace length

8945cdd

Co-authored-by: Alejandro Hernández Cordero <ahcorde@gmail.com>

wep21 requested a review from ahcorde July 17, 2023 13:51

ahcorde approved these changes Jul 17, 2023

View reviewed changes

ahcorde merged commit aab4348 into ros-perception:rolling Jul 17, 2023

wep21 deleted the enable-plugin-blacklist branch July 17, 2023 20:00

bmegli mentioned this pull request Nov 27, 2023

[ROS2] How to deactivate image_transport plugins in ros2 ros-perception/image_transport_plugins#160

Closed

Serafadam mentioned this pull request Jan 15, 2024

[Question] Is there a way to stop publishing the compressed image topics? luxonis/depthai-ros#476

Closed

This was referenced Mar 15, 2024

Backport whitelist feature to humble #302

Merged

backport whitelist feature to iron #303

Merged

MartyG-RealSense mentioned this pull request Aug 1, 2024

When I try "rosbag record -a",the realsense-ros camera code "rs_camera.launch" error. IntelRealSense/realsense-ros#2947

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: enable plugin allowlist #264

feat: enable plugin allowlist #264

wep21 commented Nov 19, 2022 •

edited

Loading

wep21 commented Nov 19, 2022 •

edited

Loading

ijnek Nov 23, 2022 •

edited

Loading

wep21 Dec 7, 2022 •

edited

Loading

ijnek Dec 9, 2022

ijnek commented Nov 23, 2022

wep21 commented Dec 4, 2022

gbiggs commented Dec 4, 2022

wep21 commented Dec 7, 2022

ijnek commented Dec 9, 2022

wep21 commented Dec 9, 2022

ijnek commented Dec 15, 2022 •

edited

Loading

wep21 commented Dec 29, 2022

gbiggs Jan 12, 2023

wep21 Apr 26, 2023

gbiggs Jan 12, 2023

wep21 Apr 26, 2023

ahcorde left a comment

ahcorde commented Jul 17, 2023

ahcorde left a comment

wep21 commented Jul 17, 2023

ahcorde commented Jul 17, 2023 •

edited

Loading

Sushant-Chavan commented Dec 27, 2023

chrbrcknr commented Jan 9, 2024

juanluishortelano commented Mar 6, 2024

feat: enable plugin allowlist #264

feat: enable plugin allowlist #264

Conversation

wep21 commented Nov 19, 2022 • edited Loading

wep21 commented Nov 19, 2022 • edited Loading

ijnek Nov 23, 2022 • edited Loading

Choose a reason for hiding this comment

wep21 Dec 7, 2022 • edited Loading

Choose a reason for hiding this comment

ijnek Dec 9, 2022

Choose a reason for hiding this comment

ijnek commented Nov 23, 2022

wep21 commented Dec 4, 2022

gbiggs commented Dec 4, 2022

wep21 commented Dec 7, 2022

ijnek commented Dec 9, 2022

wep21 commented Dec 9, 2022

ijnek commented Dec 15, 2022 • edited Loading

wep21 commented Dec 29, 2022

gbiggs Jan 12, 2023

Choose a reason for hiding this comment

wep21 Apr 26, 2023

Choose a reason for hiding this comment

gbiggs Jan 12, 2023

Choose a reason for hiding this comment

wep21 Apr 26, 2023

Choose a reason for hiding this comment

ahcorde left a comment

Choose a reason for hiding this comment

ahcorde commented Jul 17, 2023

ahcorde left a comment

Choose a reason for hiding this comment

wep21 commented Jul 17, 2023

ahcorde commented Jul 17, 2023 • edited Loading

Sushant-Chavan commented Dec 27, 2023

chrbrcknr commented Jan 9, 2024

juanluishortelano commented Mar 6, 2024

wep21 commented Nov 19, 2022 •

edited

Loading

wep21 commented Nov 19, 2022 •

edited

Loading

ijnek Nov 23, 2022 •

edited

Loading

wep21 Dec 7, 2022 •

edited

Loading

ijnek commented Dec 15, 2022 •

edited

Loading

ahcorde commented Jul 17, 2023 •

edited

Loading