fix(png): Correctly read PNGs with partial alpha (AcademySoftwareFoundation#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]>
Signed-off-by: Scott Wilson <[email protected]>

Author: lgritz

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;

src/png.imageio/png_pvt.h

@@ -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>
@@ -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)
@@ -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);
@@ -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

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<ImageSpec> 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<class T>
+    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<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);
+            }
         }
     }
 }
@@ -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;

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<unsigned char> m_scratch;
     std::vector<png_text> m_pngtext;
     std::vector<unsigned char> 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<class T>
     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<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];
@@ -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);

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"

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"

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")