From 0b255fd9dedd5c402e5fd956d36e166ebe8529b1 Mon Sep 17 00:00:00 2001 From: Larry Gritz Date: Thu, 27 Jun 2024 08:41:33 -0700 Subject: [PATCH] fix(png): Correctly read PNGs with partial alpha (#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 #4314 Closes #4054 --------- Signed-off-by: Larry Gritz --- src/ico.imageio/icooutput.cpp | 3 +- src/png.imageio/png_pvt.h | 13 ++++- src/png.imageio/pnginput.cpp | 89 ++++++++++++++++++----------- src/png.imageio/pngoutput.cpp | 39 +++++++++---- testsuite/png/ref/out-libpng15.txt | 21 ++++++- testsuite/png/ref/out.txt | 21 ++++++- testsuite/png/run.py | 3 + testsuite/png/src/alphagamma.png | Bin 0 -> 374 bytes 8 files changed, 139 insertions(+), 50 deletions(-) create mode 100644 testsuite/png/src/alphagamma.png diff --git a/src/ico.imageio/icooutput.cpp b/src/ico.imageio/icooutput.cpp index 27505395fb..18e5fdb5a3 100644 --- a/src/ico.imageio/icooutput.cpp +++ b/src/ico.imageio/icooutput.cpp @@ -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; diff --git a/src/png.imageio/png_pvt.h b/src/png.imageio/png_pvt.h index ffdb24f8eb..b0be93c0c9 100644 --- a/src/png.imageio/png_pvt.h +++ b/src/png.imageio/png_pvt.h @@ -8,6 +8,7 @@ #include #include +#include #include #include #include @@ -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& text, bool& convert_alpha, float& gamma) + std::vector& text, bool& convert_alpha, bool& srgb, + float& gamma) { // Force either 16 or 8 bit integers if (spec.format == TypeDesc::UINT8 || spec.format == TypeDesc::INT8) @@ -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(colorspace); @@ -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 diff --git a/src/png.imageio/pnginput.cpp b/src/png.imageio/pnginput.cpp index 2c58551826..5dfce4f140 100644 --- a/src/png.imageio/pnginput.cpp +++ b/src/png.imageio/pnginput.cpp @@ -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 m_config; // Saved copy of configuration spec - bool m_err = false; /// Reset everything to initial state /// @@ -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(); } @@ -82,6 +86,10 @@ class PNGInput final : public ImageInput { png_chunk_error(png_ptr, pnginput->geterror(false).c_str()); } } + + template + static void associateAlpha(T* data, int size, int channels, + int alpha_channel, bool srgb, float gamma); }; @@ -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; @@ -208,34 +222,45 @@ PNGInput::close() template -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::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 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(data[c] * alpha_associate); + DataArrayProxy 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 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); + } } } } @@ -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; diff --git a/src/png.imageio/pngoutput.cpp b/src/png.imageio/pngoutput.cpp index 4af31ee999..26e98362f9 100644 --- a/src/png.imageio/pngoutput.cpp +++ b/src/png.imageio/pngoutput.cpp @@ -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 m_scratch; std::vector m_pngtext; std::vector m_tilebuffer; @@ -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 @@ -90,7 +92,7 @@ class PNGOutput final : public ImageOutput { template void deassociateAlpha(T* data, size_t npixels, int channels, - int alpha_channel, float gamma); + int alpha_channel, bool srgb, float gamma); }; @@ -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 @@ -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(); @@ -273,9 +275,22 @@ PNGOutput::close() template 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 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 val(data); float alpha = val[alpha_channel]; @@ -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); @@ -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); diff --git a/testsuite/png/ref/out-libpng15.txt b/testsuite/png/ref/out-libpng15.txt index ccf5dc428c..e8855cd272 100644 --- a/testsuite/png/ref/out-libpng15.txt +++ b/testsuite/png/ref/out-libpng15.txt @@ -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" @@ -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" diff --git a/testsuite/png/ref/out.txt b/testsuite/png/ref/out.txt index 824b55e0df..3a8ed2f5ab 100644 --- a/testsuite/png/ref/out.txt +++ b/testsuite/png/ref/out.txt @@ -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" @@ -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" diff --git a/testsuite/png/run.py b/testsuite/png/run.py index 356629ddcd..33505f9bed 100755 --- a/testsuite/png/run.py +++ b/testsuite/png/run.py @@ -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") diff --git a/testsuite/png/src/alphagamma.png b/testsuite/png/src/alphagamma.png new file mode 100644 index 0000000000000000000000000000000000000000..a9f79e058ea7f711c8e1091cdd5e359a33b4488b GIT binary patch literal 374 zcmeAS@N?(olHy`uVBq!ia0vp^j3CUx1|;Q0k8}blmUKs7M+SzC{oH>NS%G}cd^fjZ zAPoVxw6>iCvN#JoB7uT>${@^GvDCf{C@4|l8c~v;nVE0poS&0l6kL#)oXX&yn46nu zq-UhacikDNCKRN`wIVak$}=}HJ=G(#xFo-*k^!WvBtJh#K{qd7H?g!Nzn~~Jw=}0j zA=t;&H6Y&CSFa#1T|qawv_!$M!cg1D$Xwgd$k0$hH(wz;F*^|?dqS}a&^$b%|%M`2>s9<44SG_3ELS9c7#}JO0$v^)8ueW93 XtY)q~cgs*4D8bgTe~DWM4f8n