Skip to content
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

sensor_msgs/CompressedImage: updated description of format field #231

Merged
merged 1 commit into from
Nov 17, 2023

Conversation

ijnek
Copy link
Contributor

@ijnek ijnek commented Nov 6, 2023

Port of ros/common_msgs#184 to ROS 2.

From original PR:

The description of the format field has long been outdated. This PR fixes it to be up-to-date with the core image_transport plugins.

@ijnek ijnek requested a review from tfoote as a code owner November 6, 2023 04:01
Signed-off-by: ijnek <kenjibrameld@gmail.com>
@ijnek ijnek force-pushed the update-compressed-image-description branch from 79993d1 to 3c0c92d Compare November 6, 2023 04:02
Copy link
Collaborator

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

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

lgtm

@fujitatomoya
Copy link
Collaborator

@mjcarroll this one comes from ros/common_msgs#184, can you take a look?

Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

In short, this looks good to me and does seem to correspond to what the code currently supports. I'm going to go ahead and run CI on it, thanks!

@clalancette
Copy link
Contributor

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@clalancette clalancette merged commit a1235b5 into ros2:rolling Nov 17, 2023
3 checks passed
@ijnek
Copy link
Contributor Author

ijnek commented Nov 18, 2023

Thanks @fujitatomoya and @clalancette !

@ijnek ijnek deleted the update-compressed-image-description branch November 18, 2023 03:01
@amacneil
Copy link

The description of the format field has long been outdated. This PR fixes it to be up-to-date with the core image_transport plugins.

Is there any info on when this became outdated? Fwiw I'm pretty sure we support only the "simple" values and not the "verbose" ones in Foxglove, and I don't recall any complaints or feature requests about this, so I'm surprised to hear that the simple ones are unused.

If the field is empty or does not correspond to the above pattern, the image is treated as png image.

If I'm reading this change correctly, the old "simple" format values of jpeg and png are now unsupported, and jpeg should be interpreted as png?

@ijnek
Copy link
Contributor Author

ijnek commented Apr 5, 2024

I don't have details on when simple jpeg, png, etc. were dropped (it may still work with compressed subscribers in image_transport_plugins, i'll have to give it a try), but comprssed publisher and compressed depth publisher from image_transport_plugins has been using the new format for at least 12 years (ros-perception/image_transport_plugins@e9c2768).

As a side note, since compressed images and compressed depth images both use CompressedImage, formats like "png" could be ambiguous since both compressed png image and compressed depth png image exist.

tfoote pushed a commit that referenced this pull request Apr 10, 2024
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.

None yet

4 participants