Skip to content

Commit

Permalink
fix(png): Correctly read PNGs with partial alpha (AcademySoftwareFoun…
Browse files Browse the repository at this point in the history 
…dation#4315)

TL;DR: when turning unassociated alpha into associated for
gamma-corrected PNG images, we got the exponent wrong for the
linearization step, and when doing the same for sRGB images, we didn't
linearize at all.

WARNING: will change appearance of PNG files with partial alpha.

Details:

Ugh, two separate problems related to how we associate alpha (i.e.
premultiply the colors) for PNG pixels with partial alpha. The correct
thing to do is linearize the unassociated pixel value first, then
associate, then go back to the nonlinear space.

First problem: PNGs have three possible transfer functions: gamma
correction (with a particular gamma), no gamma correction / linear, and
explicitly sRGB. Guess what? We were neglecting the case of pngs tagged
as sRGB and not doing the linearize/delinearize round trip for those
images.

But if that's not enough, also for the gamma case, we were, ugh,
swapping the gamma and 1/gamma, resulting in those partial alpha pixels
ending up a whole lot darker than they should have been.

None of this affected most ordinary PNGs with no alpha channel, or where
alpha was 1.0. It only affected "edge" or "partially transparent" pixels
with 0 < alpha < 1. But it was definitely wrong before, for which I
apologize and hope you'll understand why those pixels are going to
change now (hopefully, always always for the better).

While I was at it, I also made the color space handling a little more
robust -- instead of just a straight string compare for color space
names, use the ColorConfig to check `equivalent`, which should make us a
lot more robust against aliases and whatnot.

Fixes AcademySoftwareFoundation#4314
Closes AcademySoftwareFoundation#4054

---------

Signed-off-by: Larry Gritz <[email protected]>
  • Loading branch information
@lgritz
lgritz committed Jun 27, 2024
1 parent 976f9f8 commit 0b255fd
Show file tree
Hide file tree
Showing 8 changed files with 139 additions and 50 deletions.
3 changes: 2 additions & 1 deletion src/ico.imageio/icooutput.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -319,12 +319,13 @@ ICOOutput::open(const std::string& name, const ImageSpec& userspec,
// unused still, should do conversion to unassociated
bool convert_alpha;
float gamma;
bool srgb;

png_init_io(m_png, m_file);
png_set_compression_level(m_png, Z_BEST_COMPRESSION);

PNG_pvt::write_info(m_png, m_info, m_color_type, m_spec, m_pngtext,
convert_alpha, gamma);
convert_alpha, srgb, gamma);
} else {
// write DIB header
ico_bitmapinfo bmi;
Expand Down
13 changes: 10 additions & 3 deletions src/png.imageio/png_pvt.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
#include <zlib.h>

#include <OpenImageIO/Imath.h>
#include <OpenImageIO/color.h>
#include <OpenImageIO/dassert.h>
#include <OpenImageIO/filesystem.h>
#include <OpenImageIO/fmath.h>
Expand Down Expand Up @@ -574,7 +575,8 @@ put_parameter(png_structp& sp, png_infop& ip, const std::string& _name,
///
inline const std::string
write_info(png_structp& sp, png_infop& ip, int& color_type, ImageSpec& spec,
std::vector<png_text>& text, bool& convert_alpha, float& gamma)
std::vector<png_text>& text, bool& convert_alpha, bool& srgb,
float& gamma)
{
// Force either 16 or 8 bit integers
if (spec.format == TypeDesc::UINT8 || spec.format == TypeDesc::INT8)
Expand All @@ -598,11 +600,14 @@ write_info(png_structp& sp, png_infop& ip, int& color_type, ImageSpec& spec,

gamma = spec.get_float_attribute("oiio:Gamma", 1.0);

const ColorConfig& colorconfig = ColorConfig::default_colorconfig();
string_view colorspace = spec.get_string_attribute("oiio:ColorSpace");
if (Strutil::iequals(colorspace, "Linear")) {
if (colorconfig.equivalent(colorspace, "scene_linear")
|| colorconfig.equivalent(colorspace, "linear")) {
if (setjmp(png_jmpbuf(sp))) // NOLINT(cert-err52-cpp)
return "Could not set PNG gAMA chunk";
png_set_gAMA(sp, ip, 1.0);
srgb = false;
} else if (Strutil::istarts_with(colorspace, "Gamma")) {
Strutil::parse_word(colorspace);
float g = Strutil::from_string<float>(colorspace);
Expand All @@ -611,10 +616,12 @@ write_info(png_structp& sp, png_infop& ip, int& color_type, ImageSpec& spec,
if (setjmp(png_jmpbuf(sp))) // NOLINT(cert-err52-cpp)
return "Could not set PNG gAMA chunk";
png_set_gAMA(sp, ip, 1.0f / gamma);
} else if (Strutil::iequals(colorspace, "sRGB")) {
srgb = false;
} else if (colorconfig.equivalent(colorspace, "sRGB")) {
if (setjmp(png_jmpbuf(sp))) // NOLINT(cert-err52-cpp)
return "Could not set PNG gAMA and cHRM chunk";
png_set_sRGB_gAMA_and_cHRM(sp, ip, PNG_sRGB_INTENT_ABSOLUTE);
srgb = true;
}

// Write ICC profile, if we have anything
Expand Down
89 changes: 57 additions & 32 deletions src/png.imageio/pnginput.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,9 +44,11 @@ class PNGInput final : public ImageInput {
int m_subimage; ///< What subimage are we looking at?
Imath::Color3f m_bg; ///< Background color
int m_next_scanline;
bool m_keep_unassociated_alpha; ///< Do not convert unassociated alpha
bool m_keep_unassociated_alpha; ///< Do not convert unassociated alpha
bool m_srgb = false; ///< It's an sRGB image (not gamma)
bool m_err = false;
float m_gamma = 1.0f;
std::unique_ptr<ImageSpec> m_config; // Saved copy of configuration spec
bool m_err = false;

/// Reset everything to initial state
///
Expand All @@ -58,7 +60,9 @@ class PNGInput final : public ImageInput {
m_buf.clear();
m_next_scanline = 0;
m_keep_unassociated_alpha = false;
m_srgb = false;
m_err = false;
m_gamma = 1.0;
m_config.reset();
ioproxy_clear();
}
Expand All @@ -82,6 +86,10 @@ class PNGInput final : public ImageInput {
png_chunk_error(png_ptr, pnginput->geterror(false).c_str());
}
}

template<class T>
static void associateAlpha(T* data, int size, int channels,
int alpha_channel, bool srgb, float gamma);
};


Expand Down Expand Up @@ -159,6 +167,12 @@ PNGInput::open(const std::string& name, ImageSpec& newspec)
return false;
}

m_gamma = m_spec.get_float_attribute("oiio:Gamma", 1.0f);
string_view colorspace = m_spec.get_string_attribute("oiio:ColorSpace",
"sRGB");
const ColorConfig& colorconfig(ColorConfig::default_colorconfig());
m_srgb = colorconfig.equivalent(colorspace, "sRGB");

newspec = spec();
m_next_scanline = 0;

Expand Down Expand Up @@ -208,34 +222,45 @@ PNGInput::close()


template<class T>
static void
png_associateAlpha(T* data, int size, int channels, int alpha_channel,
float gamma)
void
PNGInput::associateAlpha(T* data, int size, int channels, int alpha_channel,
bool srgb, float gamma)
{
T max = std::numeric_limits<T>::max();
if (gamma == 1) {
for (int x = 0; x < size; ++x, data += channels)
for (int c = 0; c < channels; c++)
if (c != alpha_channel) {
unsigned int f = data[c];
data[c] = (f * data[alpha_channel]) / max;
// We need to transform to linear space, associate the alpha, and then
// transform back.
if (srgb) {
for (int x = 0; x < size; ++x, data += channels) {
DataArrayProxy<T, float> val(data);
float alpha = val[alpha_channel];
if (alpha != 0.0f && alpha != 1.0f) {
for (int c = 0; c < channels; c++) {
if (c != alpha_channel) {
float f = sRGB_to_linear(val[c]);
val[c] = linear_to_sRGB(f * alpha);
}
}
} else { //With gamma correction
float inv_max = 1.0 / max;
}
}
} else if (gamma == 1.0f) {
for (int x = 0; x < size; ++x, data += channels) {
float alpha_associate = pow(data[alpha_channel] * inv_max, gamma);
// We need to transform to linear space, associate the alpha, and
// then transform back. That is, if D = data[c], we want
//
// D' = max * ( (D/max)^(1/gamma) * (alpha/max) ) ^ gamma
//
// This happens to simplify to something which looks like
// multiplying by a nonlinear alpha:
//
// D' = D * (alpha/max)^gamma
for (int c = 0; c < channels; c++)
if (c != alpha_channel)
data[c] = static_cast<T>(data[c] * alpha_associate);
DataArrayProxy<T, float> val(data);
float alpha = val[alpha_channel];
if (alpha != 0.0f && alpha != 1.0f) {
for (int c = 0; c < channels; c++)
if (c != alpha_channel)
data[c] = data[c] * alpha;
}
}
} else { // With gamma correction
float inv_gamma = 1.0f / gamma;
for (int x = 0; x < size; ++x, data += channels) {
DataArrayProxy<T, float> val(data);
float alpha = val[alpha_channel];
if (alpha != 0.0f && alpha != 1.0f) {
for (int c = 0; c < channels; c++)
if (c != alpha_channel)
val[c] = powf((powf(val[c], gamma)) * alpha, inv_gamma);
}
}
}
}
Expand Down Expand Up @@ -295,13 +320,13 @@ PNGInput::read_native_scanline(int subimage, int miplevel, int y, int /*z*/,
// PNG specifically dictates unassociated (un-"premultiplied") alpha.
// Convert to associated unless we were requested not to do so.
if (m_spec.alpha_channel != -1 && !m_keep_unassociated_alpha) {
float gamma = m_spec.get_float_attribute("oiio:Gamma", 1.0f);
if (m_spec.format == TypeDesc::UINT16)
png_associateAlpha((unsigned short*)data, m_spec.width,
m_spec.nchannels, m_spec.alpha_channel, gamma);
associateAlpha((unsigned short*)data, m_spec.width,
m_spec.nchannels, m_spec.alpha_channel, m_srgb,
m_gamma);
else
png_associateAlpha((unsigned char*)data, m_spec.width,
m_spec.nchannels, m_spec.alpha_channel, gamma);
associateAlpha((unsigned char*)data, m_spec.width, m_spec.nchannels,
m_spec.alpha_channel, m_srgb, m_gamma);
}

return true;
Expand Down
39 changes: 27 additions & 12 deletions src/png.imageio/pngoutput.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,10 +44,11 @@ class PNGOutput final : public ImageOutput {
png_structp m_png; ///< PNG read structure pointer
png_infop m_info; ///< PNG image info structure pointer
unsigned int m_dither;
int m_color_type; ///< PNG color model type
bool m_convert_alpha; ///< Do we deassociate alpha?
bool m_need_swap; ///< Do we need to swap bytes?
float m_gamma; ///< Gamma to use for alpha conversion
int m_color_type; ///< PNG color model type
bool m_convert_alpha; ///< Do we deassociate alpha?
bool m_need_swap; ///< Do we need to swap bytes?
bool m_srgb = false; ///< It's an sRGB image (not gamma)
float m_gamma = 1.0f; ///< Gamma to use for alpha conversion
std::vector<unsigned char> m_scratch;
std::vector<png_text> m_pngtext;
std::vector<unsigned char> m_tilebuffer;
Expand All @@ -60,10 +61,11 @@ class PNGOutput final : public ImageOutput {
m_info = NULL;
m_convert_alpha = true;
m_need_swap = false;
m_srgb = false;
m_err = false;
m_gamma = 1.0;
m_pngtext.clear();
ioproxy_clear();
m_err = false;
}

// Add a parameter to the output
Expand All @@ -90,7 +92,7 @@ class PNGOutput final : public ImageOutput {

template<class T>
void deassociateAlpha(T* data, size_t npixels, int channels,
int alpha_channel, float gamma);
int alpha_channel, bool srgb, float gamma);
};


Expand Down Expand Up @@ -204,7 +206,7 @@ PNGOutput::open(const std::string& name, const ImageSpec& userspec,

#if defined(PNG_SKIP_sRGB_CHECK_PROFILE) && defined(PNG_SET_OPTION_SUPPORTED)
// libpng by default checks ICC profiles and are very strict, treating
// it as a serious error if it doesn't match th profile it thinks is
// it as a serious error if it doesn't match the profile it thinks is
// right for sRGB. This call disables that behavior, which tends to have
// many false positives. Some references to discussion about this:
// https://github.com/kornelski/pngquant/issues/190
Expand All @@ -214,7 +216,7 @@ PNGOutput::open(const std::string& name, const ImageSpec& userspec,
#endif

s = PNG_pvt::write_info(m_png, m_info, m_color_type, m_spec, m_pngtext,
m_convert_alpha, m_gamma);
m_convert_alpha, m_srgb, m_gamma);

if (s.length()) {
close();
Expand Down Expand Up @@ -273,9 +275,22 @@ PNGOutput::close()
template<class T>
void
PNGOutput::deassociateAlpha(T* data, size_t npixels, int channels,
int alpha_channel, float gamma)
int alpha_channel, bool srgb, float gamma)
{
if (gamma == 1) {
if (srgb) {
for (size_t x = 0; x < npixels; ++x, data += channels) {
DataArrayProxy<T, float> val(data);
float alpha = val[alpha_channel];
if (alpha != 0.0f && alpha != 1.0f) {
for (int c = 0; c < channels; c++) {
if (c != alpha_channel) {
float f = sRGB_to_linear(val[c]);
val[c] = linear_to_sRGB(f / alpha);
}
}
}
}
} else if (gamma == 1) {
for (size_t x = 0; x < npixels; ++x, data += channels) {
DataArrayProxy<T, float> val(data);
float alpha = val[alpha_channel];
Expand Down Expand Up @@ -331,7 +346,7 @@ PNGOutput::write_scanline(int y, int z, TypeDesc format, const void* data,
TypeFloat, AutoStride, AutoStride, AutoStride);
// Deassociate alpha
deassociateAlpha(floatvals, size_t(m_spec.width), m_spec.nchannels,
m_spec.alpha_channel, m_gamma);
m_spec.alpha_channel, m_srgb, m_gamma);
data = floatvals;
format = TypeFloat;
xstride = size_t(m_spec.nchannels) * sizeof(float);
Expand Down Expand Up @@ -394,7 +409,7 @@ PNGOutput::write_scanlines(int ybegin, int yend, int z, TypeDesc format,
AutoStride);
// Deassociate alpha
deassociateAlpha(floatvals, npixels, m_spec.nchannels,
m_spec.alpha_channel, m_gamma);
m_spec.alpha_channel, m_srgb, m_gamma);
data = floatvals;
format = TypeFloat;
xstride = size_t(m_spec.nchannels) * sizeof(float);
Expand Down
21 changes: 20 additions & 1 deletion testsuite/png/ref/out-libpng15.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ Comparing "../oiio-images/oiio-logo-no-alpha.png" and "oiio-logo-no-alpha.png"
PASS
Reading ../oiio-images/oiio-logo-with-alpha.png
../oiio-images/oiio-logo-with-alpha.png : 135 x 135, 4 channel, uint8 png
SHA-1: 8AED04DCCE8F83B068C537DC0982A42EFBE431B6
SHA-1: 9F3C517AC714A93C0FE93F8B4B40C68338504DC8
channel list: R, G, B, A
Comment: "Created with GIMP"
DateTime: "2009:03:26 18:44:26"
Expand All @@ -27,6 +27,25 @@ exif.png : 64 x 64, 3 channel, uint8 png
SHA-1: 7CB41FEA50720B48BE0C145E1473982B23E9AB77
channel list: R, G, B
oiio:ColorSpace: "sRGB"
1 x 1, 4 channel, float png
channel list: R, G, B, A
ResolutionUnit: "inch"
Software: "OpenImageIO 2.4.1.1dev : oiiotool -no-autopremult SLEEP_MM.png -cut 1x1+227+1211 -o kaka.png"
XResolution: 299.999
YResolution: 299.999
Exif:ImageHistory: "oiiotool -no-autopremult SLEEP_MM.png -cut 1x1+227+1211 -o kaka.png"
oiio:ColorSpace: "Gamma2.2"
oiio:Gamma: 2.2
Stats Min: 186 186 186 127 (of 255)
Stats Max: 186 186 186 127 (of 255)
Stats Avg: 186.00 186.00 186.00 127.00 (of 255)
Stats StdDev: 0.00 0.00 0.00 0.00 (of 255)
Stats NanCount: 0 0 0 0
Stats InfCount: 0 0 0 0
Stats FiniteCount: 1 1 1 1
Constant: Yes
Constant Color: 186.00 186.00 186.00 127.00 (of 255)
Monochrome: No
smallalpha.png : 1 x 1, 4 channel, uint8 png
Pixel (0, 0): 240 108 119 1 (0.94117653 0.42352945 0.4666667 0.003921569)
Comparing "test16.png" and "ref/test16.png"
Expand Down
21 changes: 20 additions & 1 deletion testsuite/png/ref/out.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ Comparing "../oiio-images/oiio-logo-no-alpha.png" and "oiio-logo-no-alpha.png"
PASS
Reading ../oiio-images/oiio-logo-with-alpha.png
../oiio-images/oiio-logo-with-alpha.png : 135 x 135, 4 channel, uint8 png
SHA-1: 8AED04DCCE8F83B068C537DC0982A42EFBE431B6
SHA-1: 9F3C517AC714A93C0FE93F8B4B40C68338504DC8
channel list: R, G, B, A
Comment: "Created with GIMP"
DateTime: "2009:03:26 18:44:26"
Expand All @@ -31,6 +31,25 @@ exif.png : 64 x 64, 3 channel, uint8 png
Exif:FocalLength: 45.7 (45.7 mm)
Exif:WhiteBalance: 0 (auto)
oiio:ColorSpace: "sRGB"
1 x 1, 4 channel, float png
channel list: R, G, B, A
ResolutionUnit: "inch"
Software: "OpenImageIO 2.4.1.1dev : oiiotool -no-autopremult SLEEP_MM.png -cut 1x1+227+1211 -o kaka.png"
XResolution: 299.999
YResolution: 299.999
Exif:ImageHistory: "oiiotool -no-autopremult SLEEP_MM.png -cut 1x1+227+1211 -o kaka.png"
oiio:ColorSpace: "Gamma2.2"
oiio:Gamma: 2.2
Stats Min: 186 186 186 127 (of 255)
Stats Max: 186 186 186 127 (of 255)
Stats Avg: 186.00 186.00 186.00 127.00 (of 255)
Stats StdDev: 0.00 0.00 0.00 0.00 (of 255)
Stats NanCount: 0 0 0 0
Stats InfCount: 0 0 0 0
Stats FiniteCount: 1 1 1 1
Constant: Yes
Constant Color: 186.00 186.00 186.00 127.00 (of 255)
Monochrome: No
smallalpha.png : 1 x 1, 4 channel, uint8 png
Pixel (0, 0): 240 108 119 1 (0.94117653 0.42352945 0.4666667 0.003921569)
Comparing "test16.png" and "ref/test16.png"
Expand Down
3 changes: 3 additions & 0 deletions testsuite/png/run.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,9 @@
# regression test for 16 bit output bug
command += oiiotool ("--pattern fill:topleft=1,0,0,1:topright=0,1,0,1:bottomleft=0,0,1,1:bottomright=1,1,1,1 16x16 4 -d uint16 -o test16.png")

# regression test for wrong gamma correction for partial alpha
command += oiiotool ("src/alphagamma.png --printinfo:stats=1")

# Test high quality alpha deassociation using alpha value close to zero.
# This example is inspired by Yafes on the Slack.
command += oiiotool ("--pattern fill:color=0.00235,0.00106,0.00117,0.0025 1x1 4 -d uint8 -o smallalpha.png")
Expand Down
Binary file added testsuite/png/src/alphagamma.png
View file
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.

0 comments on commit 0b255fd

Please sign in to comment.