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

Merged
merged 5 commits into from
Aug 19, 2025
Merged

Support reading pixel values from color 16 bit images #699

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5d8d665
Handle color 16 bit images
iche033 Aug 15, 2025
e9b5fdc
add test files
iche033 Aug 15, 2025
0e307e7
refactor
iche033 Aug 18, 2025
b71e351
lint and build fix
iche033 Aug 18, 2025
3276a52
cleanup
iche033 Aug 18, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions graphics/include/gz/common/Image.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ namespace gz
"RGBA_INT8",
"BGRA_INT8",
"RGB_INT16",
"RGBA_INT16",
"RGB_INT32",
"BGR_INT8",
"BGR_INT16",
Expand Down Expand Up @@ -72,6 +73,7 @@ namespace gz
RGBA_INT8,
BGRA_INT8,
RGB_INT16,
RGBA_INT16,
RGB_INT32,
BGR_INT8,
BGR_INT16,
Expand Down
89 changes: 66 additions & 23 deletions graphics/src/Image.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -415,16 +415,23 @@ math::Color Image::Pixel(unsigned int _x, unsigned int _y) const
if (!this->Valid())
return clr;

if (_x >= this->Width() || _y >= this->Height())
{
gzerr << "Image: Coordinates out of range["
<< _x << " " << _y << "] \n";
return clr;
}

FREE_IMAGE_COLOR_TYPE type = FreeImage_GetColorType(this->dataPtr->bitmap);
FREE_IMAGE_TYPE imageType = FreeImage_GetImageType(this->dataPtr->bitmap);

if (type == FIC_RGB || type == FIC_RGBALPHA)
if ((type == FIC_RGB || type == FIC_RGBALPHA) && (imageType == FIT_BITMAP))
{
RGBQUAD firgb;

if (FreeImage_GetPixelColor(this->dataPtr->bitmap, _x, _y, &firgb) == FALSE)
{
gzerr << "Image: Coordinates out of range["
<< _x << " " << _y << "] \n";
gzerr << "Failed to get pixel value at ["
<< _x << " " << _y << "] \n";
return clr;
}
clr.Set(firgb.rgbRed / 255.0f, firgb.rgbGreen / 255.0f,
Expand All @@ -435,8 +442,8 @@ math::Color Image::Pixel(unsigned int _x, unsigned int _y) const
if (this->dataPtr->PixelIndex(
this->dataPtr->bitmap, _x, _y, clr) == FALSE)
{
gzerr << "Image: Coordinates out of range ["
<< _x << " " << _y << "] \n";
gzerr << "Failed to get pixel value at ["
<< _x << " " << _y << "] \n";
return clr;
}
}
Expand Down Expand Up @@ -483,8 +490,9 @@ math::Color Image::MaxColor() const
return clr;

FREE_IMAGE_COLOR_TYPE type = FreeImage_GetColorType(this->dataPtr->bitmap);
FREE_IMAGE_TYPE imageType = FreeImage_GetImageType(this->dataPtr->bitmap);

if (type == FIC_RGB || type == FIC_RGBALPHA)
if ((type == FIC_RGB || type == FIC_RGBALPHA) && (imageType == FIT_BITMAP))
{
RGBQUAD firgb;

Expand All @@ -497,8 +505,8 @@ math::Color Image::MaxColor() const
if (FALSE ==
FreeImage_GetPixelColor(this->dataPtr->bitmap, x, y, &firgb))
{
gzerr << "Image: Coordinates out of range["
<< x << " " << y << "] \n";
gzerr << "Failed to get pixel value at ["
<< x << " " << y << "] \n";
continue;
}
clr.Set(firgb.rgbRed / 255.0f, firgb.rgbGreen / 255.0f,
Expand All @@ -522,8 +530,8 @@ math::Color Image::MaxColor() const
if (this->dataPtr->PixelIndex(
this->dataPtr->bitmap, x, y, clr) == FALSE)
{
gzerr << "Image: Coordinates out of range ["
<< x << " " << y << "] \n";
gzerr << "Failed to get pixel value at ["
<< x << " " << y << "] \n";
continue;
}

Expand All @@ -545,6 +553,9 @@ BOOL Image::Implementation::PixelIndex(
if (!_dib)
return FALSE;

if (_x >= FreeImage_GetWidth(_dib) || _y >= FreeImage_GetHeight(_dib))
return FALSE;

FREE_IMAGE_TYPE imageType = FreeImage_GetImageType(_dib);
// 8 bit images
if (imageType == FIT_BITMAP)
Expand All @@ -565,19 +576,49 @@ BOOL Image::Implementation::PixelIndex(
// 16 bit images
else if (imageType == FIT_UINT16)
{
if ((_x < FreeImage_GetWidth(_dib)) && (_y < FreeImage_GetHeight(_dib)))
{
WORD *bits = reinterpret_cast<WORD *>(FreeImage_GetScanLine(_dib, _y));
uint16_t word = static_cast<uint16_t>(bits[_x]);
// convert to float value between 0-1
float value = word / static_cast<float>(math::MAX_UI16);
_color.Set(value, value, value);
}
else
{
return FALSE;
}
WORD *bits = reinterpret_cast<WORD *>(FreeImage_GetScanLine(_dib, _y));
uint16_t word = static_cast<uint16_t>(bits[_x]);
// convert to float value between 0-1
float value = word / static_cast<float>(math::MAX_UI16);
_color.Set(value, value, value);
}
else if (imageType == FIT_INT16)
{
WORD *bits = reinterpret_cast<WORD *>(FreeImage_GetScanLine(_dib, _y));
int16_t word = static_cast<int16_t>(bits[_x]);
// convert to float value between 0-1
float value = word / static_cast<float>(math::MAX_I16);
_color.Set(value, value, value);
}
else if (imageType == FIT_RGB16)
{
const unsigned int channels = 3u;
WORD *bits = reinterpret_cast<WORD *>(FreeImage_GetScanLine(_dib, _y));
uint16_t r = static_cast<uint16_t>(bits[_x * channels]);
uint16_t g = static_cast<uint16_t>(bits[_x * channels + 1]);
uint16_t b = static_cast<uint16_t>(bits[_x * channels + 2]);
// convert to float value between 0-1
float valueR = r / static_cast<float>(math::MAX_UI16);
float valueG = g / static_cast<float>(math::MAX_UI16);
float valueB = b / static_cast<float>(math::MAX_UI16);
_color.Set(valueR, valueG, valueB);
}
else if (imageType == FIT_RGBA16)
{
const unsigned int channels = 4u;
WORD *bits = reinterpret_cast<WORD *>(FreeImage_GetScanLine(_dib, _y));
uint16_t r = static_cast<uint16_t>(bits[_x * channels]);
uint16_t g = static_cast<uint16_t>(bits[_x * channels + 1]);
uint16_t b = static_cast<uint16_t>(bits[_x * channels + 2]);
uint16_t a = static_cast<uint16_t>(bits[_x * channels + 3]);
// convert to float value between 0-1
float valueR = r / static_cast<float>(math::MAX_UI16);
float valueG = g / static_cast<float>(math::MAX_UI16);
float valueB = b / static_cast<float>(math::MAX_UI16);
float valueA = a / static_cast<float>(math::MAX_UI16);
_color.Set(valueR, valueG, valueB, valueA);
}

return TRUE;
}

Expand Down Expand Up @@ -634,6 +675,8 @@ Image::PixelFormatType Image::PixelFormat() const
}
else if (type == FIT_RGB16)
fmt = RGB_INT16;
else if (type == FIT_RGBA16)
fmt = RGBA_INT16;
else if (type == FIT_RGBF)
fmt = RGB_FLOAT32;
else if (type == FIT_UINT16 || type == FIT_INT16)
Expand Down
49 changes: 49 additions & 0 deletions graphics/src/Image_TEST.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -413,6 +413,8 @@ TEST_F(ImageTest, ConvertPixelFormat)
Image::ConvertPixelFormat("RGBA_INT8"));
EXPECT_EQ(Image::PixelFormatType::RGB_INT16,
Image::ConvertPixelFormat("RGB_INT16"));
EXPECT_EQ(Image::PixelFormatType::RGBA_INT16,
Image::ConvertPixelFormat("RGBA_INT16"));
EXPECT_EQ(Image::PixelFormatType::RGB_INT32,
Image::ConvertPixelFormat("RGB_INT32"));
EXPECT_EQ(Image::PixelFormatType::BGR_INT8,
Expand Down Expand Up @@ -673,6 +675,53 @@ TEST_F(ImageTest, Grayscale)
}
}


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

@luca-della-vedova luca-della-vedova Aug 18, 2025

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

@iche033 iche033 Aug 18, 2025

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.

{
{
common::Image img;
std::string fileName = common::testing::TestFile("data",
"rgb_16bit.png");
EXPECT_EQ(0, img.Load(fileName));
unsigned int width = 4u;
unsigned int height = 4u;
unsigned int channels = 3u;
unsigned int bpp = channels * 16u;
EXPECT_TRUE(img.Valid());
EXPECT_EQ(width, img.Width());
EXPECT_EQ(height, img.Height());
EXPECT_EQ(bpp, img.BPP());
EXPECT_EQ(width * bpp / 8u, img.Pitch());
EXPECT_EQ(common::Image::PixelFormatType::RGB_INT16, img.PixelFormat());
math::Color maxColor(0.0f, 0.0f, 0.847f);
EXPECT_NEAR(maxColor.R(), img.MaxColor().R(), 1e-3);
EXPECT_NEAR(maxColor.G(), img.MaxColor().G(), 1e-3);
EXPECT_NEAR(maxColor.B(), img.MaxColor().B(), 1e-3);
}
{
common::Image img;
std::string fileName = common::testing::TestFile("data",
"rgba_16bit.png");
EXPECT_EQ(0, img.Load(fileName));
unsigned int width = 4u;
unsigned int height = 4u;
unsigned int channels = 4u;
unsigned int bpp = channels * 16u;
EXPECT_TRUE(img.Valid());
EXPECT_EQ(width, img.Width());
EXPECT_EQ(height, img.Height());
EXPECT_EQ(bpp, img.BPP());
EXPECT_EQ(width * bpp / 8u, img.Pitch());
EXPECT_EQ(common::Image::PixelFormatType::RGBA_INT16, img.PixelFormat());
math::Color maxColor(0.0f, 0.0f, 0.847f, 0.5f);
EXPECT_NEAR(maxColor.R(), img.MaxColor().R(), 1e-3);
EXPECT_NEAR(maxColor.G(), img.MaxColor().G(), 1e-3);
EXPECT_NEAR(maxColor.B(), img.MaxColor().B(), 1e-3);
EXPECT_NEAR(maxColor.A(), img.MaxColor().A(), 1e-3);
}
}

using string_int2 = std::tuple<const char *, unsigned int, unsigned int>;

class ImagePerformanceTest : public ImageTest,
Expand Down
Binary file added test/data/rgb_16bit.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added test/data/rgba_16bit.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading