ros-perception / image_common

Common code for working with images in ROS
http://www.ros.org/wiki/image_common
129 stars 223 forks source link

ROS2 ImageTransport subscribes with default QOS settings #156

Open mkhansenbot opened 4 years ago

mkhansenbot commented 4 years ago

https://github.com/ros-perception/image_common/blob/e3f482846435eb65e299e9e5955788dc5bcf73c6/image_transport/src/image_transport.cpp#L159

It's fine this is the default but there should be an optional parameter to override it to something else, specifically the Sensor Default QOS (best effort, etc). Sensor Default is typically used with camera image data.

mkhansenbot commented 4 years ago

I can submit a small PR to add the ability to override this default if needed, please reply either way.

(cc:@emersonknapp was the one who first found this issue)

emersonknapp commented 4 years ago

My 2c - the internal default should be sensordata as a start. Then optional override is also useful

mkhansenbot commented 4 years ago

Hoping to get a reply on this before Foxy release

ruffsl commented 4 years ago

I think this could be solved by updating the underlying message_filters package to leverage the newer options API pattern directly: https://github.com/ros2/message_filters/issues/45

I didn't have the the chance to to try this, but you may want to take a look, @mkhansenbot .

mkhansenbot commented 4 years ago

@mjcarroll @wjwwood - can either of you comment on this? I realize it's probably too late to fix for Foxy, but this seems like an important issue to fix

mkhansenbot commented 4 years ago

@ruffsl - sorry I didn't reply earlier, but I don't quite understand how message_filters helps here

ruffsl commented 4 years ago

It's used in the image_transport package, and would need to be updated so that a filter's subscribers also use the user selected QoS and message memory strategy when creating the image_transport:

https://github.com/ros-perception/image_common/blob/bd2a7f9c570f7ac7f24f1c284e53e87338482c58/image_transport/src/camera_subscriber.cpp#L51-L53

https://github.com/ros-perception/image_common/blob/bd2a7f9c570f7ac7f24f1c284e53e87338482c58/image_transport/src/camera_subscriber.cpp#L125-L128

wjwwood commented 4 years ago

If it can be added in a non-API breaking way then it could still be put in foxy. If it can be done in a non-ABI breaking way it could be added to foxy after the release too.

I don't have time to pick it up, but if someone makes pull requests we'll try to get to them. @ me or @mjcarroll and we'll try to prioritize them if a non-API breaking, but ABI breaking change is required, as that's the most time sensitive version.

mkhansenbot commented 4 years ago

Is there any plan to resolve this?

agutenkunst commented 3 years ago

This issue caused me some pain and time until I figured out why no images are received. Would love to see a fix in foxy!

uxsapien commented 2 years ago

@agutenkunst @tnajjar @jehontan

I worked around this by doing similar to the following:

using std::placeholders::_1; rmw_qos_profile_t image_qos = rmw_qos_profile_sensor_data; auto sub_ = image_transport::create_subscription(this, "/camera/image_raw", std::bind(&MySuperCoolImageNode::imageCallback, this, _1), "raw", image_qos);

The create_subscription method exposes the image QoS profile and you can manually change individual constituents of that profile if needed.

ihasdapie commented 2 years ago

This should be resolved as of https://github.com/ros-perception/image_common/pull/246 for rolling. @mkhansenbot if this addresses the problem feel free to close this issue.