Skip to content

feat: Add subscriber plugins using pluginlib - #198

Open
Nosille wants to merge 30 commits into
RobotWebTools:ros2from
Nosille:subscriber_plugins
Open

feat: Add subscriber plugins using pluginlib#198
Nosille wants to merge 30 commits into
RobotWebTools:ros2from
Nosille:subscriber_plugins

Conversation

@Nosille

@Nosille Nosille commented Feb 9, 2026

Copy link
Copy Markdown

Public API Changes

Reorganized codebase to isolate subscribers from streamers.
Make subscribers an additional pluginlib class (base_class_type="web_video_server::SubscriberFactoryInterface">)
Created image_transport and pointcloud2 subscribers

Description

This is a big change that will probably take a little back and forth. If you are not interested let me know. The goal was to make it possible to subscribe to ros msg formats other than images and convert them to image streams. This was done in a plugin-able fashion. Existing image_transport based subscription capabilities were removed from the existing streamer plugins and replaced with a new subscriber plugin. A new pointcloud2_subscriber plugin was added that subscribes to pointcloud2 msgs, projects them into a 2d image (user selected viewing frame with parameters for height, width, and focal length), then sends the resulting image to any of the existing streamers. A somewhat silly subscriber plugin example/tutorial was created in the docs that converts a string msg into an image.

@bjsowa
bjsowa self-requested a review February 9, 2026 19:24
@bjsowa

bjsowa commented Feb 9, 2026

Copy link
Copy Markdown
Member

Thank you for the PR. I'll try find some time to review it till the end of the week (ping me if I forget). My only request for now is to split it into 2 separate PRs, one that changes the classes structure and one that adds the pointcloud subscriber plugin.

@Nosille

Nosille commented Feb 14, 2026

Copy link
Copy Markdown
Author

I finally found time to remove the pointcloud2 stuff. I will make a separate pull request after this one is complete.

@bjsowa bjsowa left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Sorry I couldn't find much time to review this. I generally like the idea of separating streamers and subscribers. I quickly skimmed through the code and the solution looks alright. My only concern is that the subscriber interface only works for streamers which expect a raw image. This does not fit with the ros_compressed streamer which never decodes the image.

I'll try to take go back to it later next week. For now, try to pass CI.

Comment thread include/web_video_server/subscribers/image_transport_subscriber.hpp Outdated
Comment thread include/web_video_server/subscribers/pointcloud2_subscriber.hpp Outdated
Comment thread include/web_video_server/subscribers/test_subscriber.hpp Outdated
Comment thread CMakeLists.txt Outdated
Comment thread CMakeLists.txt Outdated
Comment thread CMakeLists.txt Outdated
Comment thread CMakeLists.txt
@Nosille
Nosille requested a review from bjsowa February 15, 2026 23:12
@Nosille

Nosille commented Feb 15, 2026

Copy link
Copy Markdown
Author

When I removed the pointcloud2 subscriber I didn't remove its dependencies. Most of these comments are a result of that. They have been removed now.

Seems like the CI/CD errors are all format related. I do not have much practice with formatters and my first attempt resulted in many more changes than I think you are looking for. Any advice? I don't see format guidance in this projects documentation.

The compressed image streamer is an odd ball that doesn't quite fit. I handled it by simply letting it stay the way it was , i.e., it doesn't actually use the subscriber list sent to it. Another weakness in my current implementation is that each streamer will use the first subscriber factory that says it can handle the current msg type. It currently isn't a problem, but could become one as redundant subscribers are added. I am a little unsure how best to handle it, and I am open to suggestions.

@bjsowa

bjsowa commented Feb 16, 2026

Copy link
Copy Markdown
Member

Seems like the CI/CD errors are all format related. I do not have much practice with formatters and my first attempt resulted in many more changes than I think you are looking for. Any advice? I don't see format guidance in this projects documentation.

For formatting, just use:

ament_uncrustify --reformat .

@bjsowa

bjsowa commented Feb 16, 2026

Copy link
Copy Markdown
Member

If you want to run clang-tidy tests locally, make sure to compile with -DCMAKE_EXPORT_COMPILE_COMMANDS=ON, for example:

colcon build --cmake-args -DCMAKE_EXPORT_COMPILE_COMMANDS=ON

then run:

ament_clang_tidy --packages-select web_video_server

If you want to make it faster, add --jobs flag, for example:

ament_clang_tidy --packages-select web_video_server --jobs 12

Some of the errors can be fixed automatically with --fix-errors flag

@bjsowa

bjsowa commented Aug 12, 2026

Copy link
Copy Markdown
Member

Hi @Nosille . Sorry for half a year of no response. Are you still down to work on this if I do a review?

@Nosille

Nosille commented Aug 13, 2026 via email

Copy link
Copy Markdown
Author

@bjsowa bjsowa left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Apart from these changes, my small grit with this feature is that Subscriber abstraction assumes that all Streamers consume sensor_msgs/msg/Image messages which is not true for ros_compressed or any other streamer in the future that won't operate on raw images.

I'm ok with merging it as it is (apart from the few changes I requested) and then figure out a slightly improved abstractions later.

Comment thread src/streamers/image_streamer.cpp Outdated
Comment thread include/web_video_server/subscriber.hpp Outdated
Comment thread src/streamers/ros_compressed_streamer.cpp Outdated
Comment thread src/subscribers/image_transport_subscriber.cpp Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This tutorial was not updated for the interface changes.

Comment thread doc/custom-subscriber-plugin.md Outdated

// Convert input msg to image
cv::Mat image(500, 1000, CV_8UC3, cv::Scalar(0, 0, 0));
cv:putText(image, input_msg->data, cv::Point(30,250), cv::FONT_HERSHEY_SIMPLEX, 1.0, cv::Scalar(255, 0, 0), 2, cv::LINE_AA);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Typo

Suggested change
cv:putText(image, input_msg->data, cv::Point(30,250), cv::FONT_HERSHEY_SIMPLEX, 1.0, cv::Scalar(255, 0, 0), 2, cv::LINE_AA);
cv::putText(image, input_msg->data, cv::Point(30,250), cv::FONT_HERSHEY_SIMPLEX, 1.0, cv::Scalar(255, 0, 0), 2, cv::LINE_AA);

Comment thread include/web_video_server/subscriber.hpp Outdated
Comment thread include/web_video_server/subscriber.hpp Outdated
Comment thread include/web_video_server/streamers/image_streamer.hpp Outdated
Comment thread include/web_video_server/streamers/libav_streamer.hpp Outdated
@Nosille

Nosille commented Aug 18, 2026

Copy link
Copy Markdown
Author

I'm ok with merging it as it is (apart from the few changes I requested) and then figure out a slightly improved abstractions later.

I think it would take little effort to swap from "sensor_msgs::msg::Image" to "cv_bridge::CvImage" if that works better for you. The underlying opencv data types could also be passed directly with a little more work. Such a changes is probably better done now than later given how fundamental the passed data type is.

If we go that route, I believe I can make the ros_compressed_streamer actually use the subscriber plugin.

@bjsowa

bjsowa commented Aug 18, 2026

Copy link
Copy Markdown
Member

I'm ok with merging it as it is (apart from the few changes I requested) and then figure out a slightly improved abstractions later.

I think it would take little effort to swap from "sensor_msgs::msg::Image" to "cv_bridge::CvImage" if that works better for you. The underlying opencv data types could also be passed directly with a little more work. Such a changes is probably better done now than later given how fundamental the passed data type is.

If we go that route, I believe I can make the ros_compressed_streamer actually use the subscriber plugin.

cv_bridge::CvImage still stores the raw image data whilst ros_compressed_streamer subscribe to compressed image and send it over TCP without decompressing the image.

@Nosille

Nosille commented Aug 18, 2026

Copy link
Copy Markdown
Author

I'm ok with merging it as it is (apart from the few changes I requested) and then figure out a slightly improved abstractions later.

I think it would take little effort to swap from "sensor_msgs::msg::Image" to "cv_bridge::CvImage" if that works better for you. The underlying opencv data types could also be passed directly with a little more work. Such a changes is probably better done now than later given how fundamental the passed data type is.
If we go that route, I believe I can make the ros_compressed_streamer actually use the subscriber plugin.

cv_bridge::CvImage still stores the raw image data whilst ros_compressed_streamer subscribe to compressed image and send it over TCP without decompressing the image.

I see. Yea, that is different. It is effectively forwarding a raw data block.

@Nosille
Nosille force-pushed the subscriber_plugins branch from 9fc001d to 9119519 Compare August 18, 2026 18:31
@Nosille
Nosille force-pushed the subscriber_plugins branch from 9119519 to 1e81f31 Compare August 18, 2026 18:38
@Nosille

Nosille commented Aug 18, 2026

Copy link
Copy Markdown
Author

I still have the example files to work on, but everything else should be addressed.

@bjsowa

bjsowa commented Aug 18, 2026

Copy link
Copy Markdown
Member

@bjsowa

bjsowa commented Aug 20, 2026

Copy link
Copy Markdown
Member

There's one clang-tidy warning left: https://github.com/RobotWebTools/web_video_server/actions/runs/32191528946/job/95886703206?pr=198#step:3:1375

Other than that, update the tutorials and we're good to go.

@Nosille
Nosille force-pushed the subscriber_plugins branch from bdf38f2 to abe32bf Compare August 21, 2026 14:09
@Nosille
Nosille force-pushed the subscriber_plugins branch from abe32bf to 3dba639 Compare August 21, 2026 14:22
@Nosille
Nosille force-pushed the subscriber_plugins branch from 88d1d53 to 7ff634e Compare August 22, 2026 14:05
@Nosille

Nosille commented Aug 22, 2026

Copy link
Copy Markdown
Author

Working on the example got me thinking more about what functions are virtual and what are not. Also moved the code to find a subscriber factory into the base streamer class.

@Nosille
Nosille force-pushed the subscriber_plugins branch 2 times, most recently from 1972159 to 3967068 Compare August 23, 2026 19:33
@Nosille
Nosille force-pushed the subscriber_plugins branch from 3967068 to 072d95a Compare August 23, 2026 19:42
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.

2 participants