-
Notifications
You must be signed in to change notification settings - Fork 207
feat: Use pluginlib to load streamer plugins at runtime, general refactor #192
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
base: ros2
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really am not a fan of pluginlib, but I can definitely see the value or not loading every possible streamer type right away.
Code looks great, including all the cleanup on the wah. However, I think this needs some documentation on how to configure which plugins you want to load and how to launch a node properly. Can you add this in a README?
Well.. it still loads every streamer factory it can find. The pluginlib just allows declaring more streamers outside the package. Now I'm curious why you dislike pluginlib and what alternative would you recommend instead.
I could add a section on how to create a package which adds another streamer type. |
I think pluginlib has a lot of weird behavior, and often exhibits memory leaks / failure to clean up on shutdown. Plus, I'm generally of the belief that runtime configurability without compilation isn't worth forcing one to use a single abstract class and dealing with pluginlib idiosyncrasies. I prefer for people to have a flexible API and just recompile if needed. No alternatives recommended --- I think this is just personal preference, but there is no real issue.
Understood. I did think there was a path similar to image_transport as you described, where the node had a parameter listing out the plugins to load. If that's not the case, all good! That would be great, though if you prefer to do that in a follow-on I can approve this now. |
I'll add the documentation first to confirm whether this actually works |
This is PR includes quite a large refactor and multiple improvements.
Improvements
The largest improvement is using pluginlib to dynamically load streamers at runtime in a similar fashion as
image_transport
is loading it's subscriber and publisher plugins. This allows the user to extend the functionality without modifying the package, by writing plugins implementingStreamerFactoryInterface
orSnapshotStreamerFactoryInterface
.Significantly improved the stream list page. The streamer factories provide a method for getting a list of supported topics. The node is utilizing this method to show supported streamers for a given topic. Here's how it looked before:
And here's after:
Notice that
ros_compressed
is shown even for topics that don't have raw topics. (Fixes Compressed image topic not accessible #94)/snapshot
request now supporttype
parameter. This makes the snapshots not limited tojpeg
. The user can specifypng
,ros_compressed
or any format provided by a plugin implementingSnapshotStreamerFactoryInterface
. (Fixes Snapshots only in JPEG ? #109)Refactor
web_video_server
andweb_video_server_streamers
. The idea behind it is to not have to link against OpenCV and ffmpeg when developing custom streamer plugins.ImageStreamer
->StreamerInterface
- although this is the primary function ofweb_video_server
, we don't have to limit ourselves to streaming images.ImageStreamerType
->StreamerFactoryInterface
- I felt likeType
was a bit misleading as this class is actually a factory for creating streamers.ImageTransportImageStreamer
->ImageTransportStreamerBase
,LibavStreamer
->LibavStreamerBase
- to better reflect that these are common base classes used by plugins.