Skip to content

Support reading pixel values from color 16 bit images #699

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Aug 19, 2025
Merged

Support reading pixel values from color 16 bit images #699

merged 5 commits into from
Aug 19, 2025

Conversation

iche033
Copy link
Contributor

@iche033 iche033 commented Aug 15, 2025

🎉 New feature

Closes #698

Summary

Add support for reading RGB[A] 16 bit images.

Similar to what's done in #439, this PR redirects the Pixel(...) function calls to use the PixelIndex(...) function which has been extended to support 16 bit color image formats.

Note: this PR does not entirely address the issue with the confusing flood of console messages about Image coordinates out of range.. when a format is not supported, see #698 (comment). The message is changed to a more generic one, i.e. Failed to to get pixel value at.. so it does not lead the user down the wrong path. We should still tackle this issue but we can do that in a separate PR.

Test it

Run the UNIT_Image_TEST

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • Consider updating Python bindings (if the library has them)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers
  • Was GenAI used to generate this PR? If so, make sure to add "Generated-by" to your commits. (See this policy for more info.)

Generated-by: Remove this if GenAI was not used.

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by and Generated-by messages.

iche033 added 2 commits August 15, 2025 20:55
@iche033
Handle color 16 bit images
5d8d665 
Signed-off-by: Ian Chen <ichen@openrobotics.org>
@iche033
add test files
e9b5fdc 
Signed-off-by: Ian Chen <ichen@openrobotics.org>
@iche033 iche033 requested a review from marcoag as a code owner August 15, 2025 21:08
@github-actions github-actions bot added the 🪵 jetty Gazebo Jetty label Aug 15, 2025
@iche033 iche033 mentioned this pull request Aug 15, 2025
Closed
luca-della-vedova
luca-della-vedova reviewed Aug 18, 2025
View reviewed changes
Copy link
Member

@luca-della-vedova luca-della-vedova left a comment

Choose a reason for hiding this comment

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

Generally looks good, one nit and one more substantial comment on a duplicated implementation

graphics/src/Image_TEST.cc


/////////////////////////////////////////////////
TEST_F(ImageTest, Color16bit)
Copy link
Member

Choose a reason for hiding this comment

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

I noticed this test doesn't actually test the API that had the problematic behavior (Image.Pixel(x, y)) or rather, it only does so in a indirect way, specifically it only tests img.MaxColor(), which in this specific code path calls Image::Implementation::PixelIndex which happens to be what also Pixel would call because their logic is duplicated.

I have two alternative proposals:

  1. (My preference), I saw that MaxColor and Pixel have pretty much the same duplicated logic, which also shows in this PR's diff that has the same change implemented twice. What if we refactored so all MaxColor does is effectively just call Pixel in a loop and we only had the logic in Pixel?
  2. If the above is not possible, we should at least add a test for Pixel in these cases, so we make sure that function's implementation is correct as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep good idea, went with 1 and done in 0e307e7. I noticed that after refactoring MaxColor() to call Pixel(...), the Image test took a long time to run. Turns out it's due to repeatedly calling FreeImage_GetColorType which seems to be an expensive call. I now made it a member variable and initialize it when the image is first loaded.

ahcorde
ahcorde requested changes Aug 18, 2025
View reviewed changes
@github-project-automation github-project-automation bot moved this from Inbox to In review in Core development Aug 18, 2025
iche033 added 3 commits August 18, 2025 22:26
@iche033
refactor
0e307e7 
Signed-off-by: Ian Chen <ichen@openrobotics.org>
@iche033
lint and build fix
b71e351 
Signed-off-by: Ian Chen <ichen@openrobotics.org>
@iche033
cleanup
3276a52 
Signed-off-by: Ian Chen <ichen@openrobotics.org>
luca-della-vedova
luca-della-vedova approved these changes Aug 19, 2025
View reviewed changes
Copy link
Member

@luca-della-vedova luca-della-vedova left a comment

Choose a reason for hiding this comment

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

Looks great thanks for iterating so quickly! Appreciate that we got rid of the duplicated implementation as well!

I do wish there was const-by-default throughout but it's just my Rust self talking and would explode the diff 😅, this is good to go!

@luca-della-vedova luca-della-vedova linked an issue Aug 19, 2025 that may be closed by this pull request
Image's Pixel method fails with 16 bit images #698
Closed
@azeey azeey added this to the Jetty Release milestone Aug 19, 2025
@iche033
Copy link
Contributor Author

iche033 commented Aug 19, 2025

hmm Windows test failures do not seem to be related. I see them failing in #696 and #693 as well. It did pass in #700 so could be flaky or machine-related.

@iche033 iche033 merged commit 3887950 into main Aug 19, 2025
12 of 13 checks passed
@github-project-automation github-project-automation bot moved this from In review to Done in Core development Aug 19, 2025
@iche033 iche033 deleted the iche033/rgb16 branch August 19, 2025 20:59
@iche033
Copy link
Contributor Author

iche033 commented Aug 19, 2025

@Mergifyio backport gz-common6 gz-common5

@mergify Mergify
Copy link
Contributor

mergify bot commented Aug 19, 2025 •
edited
Loading

backport gz-common6 gz-common5

✅ Backports have been created

This was referenced Aug 19, 2025
Merged
Merged
@iche033 iche033 mentioned this pull request Aug 19, 2025
Merged
9 tasks
iche033 added a commit that referenced this pull request Aug 20, 2025
@mergify @iche033
Support reading pixel values from color 16 bit images (backport #699) (…
8c10de0 
…#702)

Signed-off-by: Ian Chen <ichen@openrobotics.org>
Co-authored-by: Ian Chen <ichen@openrobotics.org>
@iche033 iche033 mentioned this pull request Sep 8, 2025
Merged
10 tasks
@mergify mergify bot mentioned this pull request Sep 10, 2025
Merged
10 tasks
@mergify mergify bot mentioned this pull request Sep 10, 2025
Merged
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ahcorde ahcorde ahcorde approved these changes

@luca-della-vedova luca-della-vedova luca-della-vedova approved these changes

@marcoag marcoag Awaiting requested review from marcoag marcoag is a code owner

Assignees

No one assigned

Labels

🪵 jetty Gazebo Jetty

Projects

Core development
Status: Done

Milestone

Jetty Release

Development

Successfully merging this pull request may close these issues.

Image's Pixel method fails with 16 bit images

4 participants

@iche033 @ahcorde @luca-della-vedova @azeey