fix(png): alpha premultiplication adjustment and attribute (AcademySoftwareFoundation#4585)

lgritz · brechtvl · web-flow · commit 366331075149 · 2025-01-06T17:06:16.000-08:00
See discussion AcademySoftwareFoundation#4054 and PR AcademySoftwareFoundation#4315 In PR 4315, we fixed PNG read/write to do the required premultiplication in linear space (that is, linearize, multiply by alpha, then go back to the sRGB or gamma space). Which we really believe is "correct." Except that maybe there are a ton of incorrectly made PNG files out there (maybe most of them?) where partial alpha pixels had their premultiplication occur on the sRGB or gamma values directly. In this patch, we partly revert, allowing both potential behaviors, controlled by an attribute "png:linear_premult", which instructs the PNG driver to do any premultiplication in linear space if it's set to nonzero. It can be set globally (via `OIIO::attribute()`), as well as set/overridden on any individual file by setting the attribute in the configuration hints for an ImageInput or in the spec for an ImageOutput. As presented in this patch, we're setting the default to 0, meaning that by default we are reverting back to the old behavior of doing the premultiply of partial alpha pixels on the direct values intead of in a linear space. Applications or sites that are confident that any PNG files they encounter are "correct" can set the attribute to do the multiplication linearly. I'm not 100% confident about the default, and so am very happy to entertain arguments for keeping the default set to do the multiplication in linear space. I had to change an internal spin mutex to a recursive mutex in order to address a latent misdesign that was made symptomatic by this change. Basically, asking for an attribute inside an ImageInput::init could make a deadlock because certain attribute queries (that catalogue the list of plugins) might instantiate those plugins, thus causing their init functions to run, leading to recursive dependence on the mutex that guards attribute queries. --------- Signed-off-by: Larry Gritz <lg@larrygritz.com> Co-authored-by: Brecht Van Lommel <brecht@blender.org>
diff --git a/src/doc/builtinplugins.rst b/src/doc/builtinplugins.rst
@@ -1754,6 +1754,12 @@ attributes are supported:
      - ptr
      - Pointer to a ``Filesystem::IOProxy`` that will handle the I/O, for
        example by reading from memory rather than the file system.
+   * - ``png:linear_premult``
+     - int
+     - If nonzero, will convert  or gamma-encoded values to linear color
+       space for any premultiplication-by-alpha step done by the PNG reader.
+       If zero (the default), any needed premultiplication will happen directly
+       to the encoded values.
 
 **Configuration settings for PNG output**
 
@@ -1797,13 +1803,31 @@ control aspects of the writing itself:
        to have larger PNG files on disk, you may want to use that value for
        this attribute.
 
+   * - ``png:linear_premult``
+     - int
+     - If nonzero, will convert sRGB or gamma-encoded values to linear color
+       space for any unpremultiplication-by-alpha step done by the PNG writer.
+       If zero (the default), any needed unpremultiplication will happen
+       directly to the encoded sRGB or gamma-corrected values.
+
 **Custom I/O Overrides**
 
 PNG input and output both support the "custom I/O" feature via the special
 ``"oiio:ioproxy"`` attributes (see Sections :ref:`sec-imageoutput-ioproxy`
 and :ref:`sec-imageinput-ioproxy`) as well as the `set_ioproxy()` methods.
 
-
+**Note on premultiplication**
+
+PNG files encoded as sRGB or gamma-corrected values that also have alpha
+should (in theory) have any premultiplication performed in a linear space
+(that is, the color should first be linearized, then premultiplied by alpha,
+then converted back to the nonlinear form). However, many existing PNG files
+are apparently encoded with the assumption that any premultiplication will be
+performed directly on the encoded values, so that is the default behavior for
+OpenImageIO's PNG reader and writer will. If you want to force the reader or
+writer to linearize the values for premultiplication, you can set either the
+reader/writer configuration hint or the global OIIO attribute
+``png:linear_premult`` to 1.
 
 **Limitations**
 
diff --git a/src/include/OpenImageIO/imageio.h b/src/include/OpenImageIO/imageio.h
@@ -2908,6 +2908,14 @@ OIIO_API std::string geterror(bool clear = true);
 ///    and only set ImageDescription if the parsing fails. Otherwise, always
 ///    set ImageDescription to the first comment block. Default is 1.
 ///
+/// - `int png:linear_premult` (0)
+///
+///    If nonzero, will convert perform any necessary premultiplication by
+///    alpha steps needed of the PNG reader/writer in a linear color space.
+///    If zero (the default), any needed premultiplication will happen
+///    directly on the values, even if they are sRGB or gamma-corrected.
+///    For more information, please see OpenImageIO's documentation on the
+///    built-in PNG format support.
 ///
 /// - `int limits:channels` (1024)
 ///
diff --git a/src/include/imageio_pvt.h b/src/include/imageio_pvt.h
@@ -43,6 +43,7 @@ extern OIIO_UTIL_API int oiio_print_uncaught_errors;
 extern int oiio_log_times;
 extern int openexr_core;
 extern int jpeg_com_attributes;
+extern int png_linear_premult;
 extern int limit_channels;
 extern int limit_imagesize_MB;
 extern int imagebuf_print_uncaught_errors;
diff --git a/src/libOpenImageIO/imageio.cpp b/src/libOpenImageIO/imageio.cpp
@@ -49,6 +49,7 @@ atomic_int oiio_try_all_readers(1);
 // Should we use "Exr core C library"?
 int openexr_core(OIIO_OPENEXR_CORE_DEFAULT);
 int jpeg_com_attributes(1);
+int png_linear_premult(0);
 int tiff_half(0);
 int tiff_multithread(1);
 int dds_bc5normal(0);
@@ -72,7 +73,7 @@ using namespace pvt;
 
 namespace {
 // Hidden global OIIO data.
-static spin_mutex attrib_mutex;
+static std::recursive_mutex attrib_mutex;
 static const int maxthreads = 512;  // reasonable maximum for sanity check
 
 class TimingLog {
@@ -347,7 +348,7 @@ attribute(string_view name, TypeDesc type, const void* val)
     }
 
     // Things below here need to buarded by the attrib_mutex
-    spin_lock lock(attrib_mutex);
+    std::lock_guard lock(attrib_mutex);
     if (name == "read_chunk" && type == TypeInt) {
         oiio_read_chunk = *(const int*)val;
         return true;
@@ -372,6 +373,10 @@ attribute(string_view name, TypeDesc type, const void* val)
         jpeg_com_attributes = *(const int*)val;
         return true;
     }
+    if (name == "png:linear_premult" && type == TypeInt) {
+        png_linear_premult = *(const int*)val;
+        return true;
+    }
     if (name == "tiff:half" && type == TypeInt) {
         tiff_half = *(const int*)val;
         return true;
@@ -460,7 +465,7 @@ getattribute(string_view name, TypeDesc type, void* val)
     }
 
     // Things below here need to buarded by the attrib_mutex
-    spin_lock lock(attrib_mutex);
+    std::lock_guard lock(attrib_mutex);
     if (name == "read_chunk" && type == TypeInt) {
         *(int*)val = oiio_read_chunk;
         return true;
@@ -551,6 +556,10 @@ getattribute(string_view name, TypeDesc type, void* val)
         *(int*)val = jpeg_com_attributes;
         return true;
     }
+    if (name == "png:linear_premult" && type == TypeInt) {
+        *(int*)val = png_linear_premult;
+        return true;
+    }
     if (name == "tiff:half" && type == TypeInt) {
         *(int*)val = tiff_half;
         return true;
diff --git a/src/png.imageio/pnginput.cpp b/src/png.imageio/pnginput.cpp
@@ -45,6 +45,7 @@ class PNGInput final : public ImageInput {
     Imath::Color3f m_bg;               ///< Background color
     int m_next_scanline;
     bool m_keep_unassociated_alpha;  ///< Do not convert unassociated alpha
+    bool m_linear_premult;           ///< Do premult for sRGB images in linear
     bool m_srgb   = false;           ///< It's an sRGB image (not gamma)
     bool m_err    = false;
     float m_gamma = 1.0f;
@@ -60,9 +61,10 @@ 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_linear_premult = OIIO::get_int_attribute("png:linear_premult");
+        m_srgb           = false;
+        m_err            = false;
+        m_gamma          = 1.0;
         m_config.reset();
         ioproxy_clear();
     }
@@ -88,8 +90,8 @@ class PNGInput final : public ImageInput {
     }
 
     template<class T>
-    static void associateAlpha(T* data, int size, int channels,
-                               int alpha_channel, bool srgb, float gamma);
+    void associateAlpha(T* data, int size, int channels, int alpha_channel,
+                        bool srgb, float gamma);
 };
 
 
@@ -189,6 +191,9 @@ PNGInput::open(const std::string& name, ImageSpec& newspec,
     // Check 'config' for any special requests
     if (config.get_int_attribute("oiio:UnassociatedAlpha", 0) == 1)
         m_keep_unassociated_alpha = true;
+    m_linear_premult = config.get_int_attribute("png:linear_premult",
+                                                OIIO::get_int_attribute(
+                                                    "png:linear_premult"));
     ioproxy_retrieve_from_config(config);
     m_config.reset(new ImageSpec(config));  // save config spec
     return open(name, newspec);
@@ -229,7 +234,8 @@ PNGInput::associateAlpha(T* data, int size, int channels, int alpha_channel,
 {
     // We need to transform to linear space, associate the alpha, and then
     // transform back.
-    if (srgb) {
+    if (srgb && m_linear_premult) {
+        // sRGB with request to do premult in linear space
         for (int x = 0; x < size; ++x, data += channels) {
             DataArrayProxy<T, float> val(data);
             float alpha = val[alpha_channel];
@@ -242,25 +248,29 @@ PNGInput::associateAlpha(T* data, int size, int channels, int alpha_channel,
                 }
             }
         }
-    } else if (gamma == 1.0f) {
+    } else if (gamma != 1.0f && m_linear_premult) {
+        // Gamma correction with request to do premult in linear space
+        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 != 1.0f) {
                 for (int c = 0; c < channels; c++)
                     if (c != alpha_channel)
-                        data[c] = data[c] * alpha;
+                        val[c] = powf((powf(val[c], gamma)) * alpha, inv_gamma);
             }
         }
-    } else {  // With gamma correction
-        float inv_gamma = 1.0f / gamma;
+    } else {
+        // Do the premult directly on the values. This is correct for the
+        // "gamma=1" case, and is also commonly what is needed for many sRGB
+        // images (even though it's technically wrong in that case).
         for (int x = 0; x < size; ++x, data += channels) {
             DataArrayProxy<T, float> val(data);
             float alpha = val[alpha_channel];
             if (alpha != 1.0f) {
                 for (int c = 0; c < channels; c++)
                     if (c != alpha_channel)
-                        val[c] = powf((powf(val[c], gamma)) * alpha, inv_gamma);
+                        val[c] = val[c] * alpha;
             }
         }
     }
diff --git a/src/png.imageio/pngoutput.cpp b/src/png.imageio/pngoutput.cpp
@@ -47,6 +47,7 @@ class PNGOutput final : public ImageOutput {
     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_linear_premult;  ///< Do premult for sRGB images in linear
     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;
@@ -57,13 +58,14 @@ class PNGOutput final : public ImageOutput {
     // Initialize private members to pre-opened state
     void init(void)
     {
-        m_png           = NULL;
-        m_info          = NULL;
-        m_convert_alpha = true;
-        m_need_swap     = false;
-        m_srgb          = false;
-        m_err           = false;
-        m_gamma         = 1.0;
+        m_png            = NULL;
+        m_info           = NULL;
+        m_convert_alpha  = true;
+        m_need_swap      = false;
+        m_linear_premult = false;
+        m_srgb           = false;
+        m_err            = false;
+        m_gamma          = 1.0;
         m_pngtext.clear();
         ioproxy_clear();
     }
@@ -187,6 +189,10 @@ PNGOutput::open(const std::string& name, const ImageSpec& userspec,
 
     m_need_swap = (m_spec.format == TypeDesc::UINT16 && littleendian());
 
+    m_linear_premult = m_spec.get_int_attribute("png:linear_premult",
+                                                OIIO::get_int_attribute(
+                                                    "png:linear_premult"));
+
     png_set_filter(m_png, 0,
                    spec().get_int_attribute("png:filter", PNG_NO_FILTERS));
     // https://www.w3.org/TR/PNG-Encoders.html#E.Filter-selection
@@ -277,7 +283,8 @@ void
 PNGOutput::deassociateAlpha(T* data, size_t npixels, int channels,
                             int alpha_channel, bool srgb, float gamma)
 {
-    if (srgb) {
+    if (srgb && m_linear_premult) {
+        // sRGB with request to do unpremult in linear space
         for (size_t x = 0; x < npixels; ++x, data += channels) {
             DataArrayProxy<T, float> val(data);
             float alpha = val[alpha_channel];
@@ -290,27 +297,31 @@ PNGOutput::deassociateAlpha(T* data, size_t npixels, int channels,
                 }
             }
         }
-    } else if (gamma == 1) {
+    } else if (gamma != 1.0f && m_linear_premult) {
+        // Gamma correction with request to do unpremult in linear space
         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) {
+                // See associateAlpha() for an explanation.
+                float alpha_deassociate = pow(1.0f / val[alpha_channel], gamma);
                 for (int c = 0; c < channels; c++) {
                     if (c != alpha_channel)
-                        val[c] = data[c] / alpha;
+                        val[c] = val[c] * alpha_deassociate;
                 }
             }
         }
     } else {
+        // Do the unpremult directly on the values. This is correct for the
+        // "gamma=1" case, and is also commonly what is needed for many sRGB
+        // images (even though it's technically wrong in that case).
         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) {
-                // See associateAlpha() for an explanation.
-                float alpha_deassociate = pow(1.0f / val[alpha_channel], gamma);
                 for (int c = 0; c < channels; c++) {
                     if (c != alpha_channel)
-                        val[c] = val[c] * alpha_deassociate;
+                        val[c] = data[c] / alpha;
                 }
             }
         }
diff --git a/testsuite/png/ref/out-libpng15.txt b/testsuite/png/ref/out-libpng15.txt
@@ -27,6 +27,7 @@ exif.png             :   64 x   64, 3 channel, uint8 png
     SHA-1: 7CB41FEA50720B48BE0C145E1473982B23E9AB77
     channel list: R, G, B
     oiio:ColorSpace: "sRGB"
+alphagamma:
    1 x    1, 4 channel, float png
     channel list: R, G, B, A
     ResolutionUnit: "inch"
@@ -46,6 +47,43 @@ exif.png             :   64 x   64, 3 channel, uint8 png
     Constant: Yes
     Constant Color: 186.00 186.00 186.00 127.00 (of 255)
     Monochrome: No
+gimp_gradient:
+ 256 x  256, 4 channel, float png
+    channel list: R, G, B, A
+    Comment: "Created with GIMP"
+    DateTime: "2025:01:05 04:44:59"
+    ICCProfile: 0, 0, 2, 160, 108, 99, 109, 115, 4, 64, 0, 0, 109, 110, 116, 114, ... [672 x uint8]
+    ResolutionUnit: "inch"
+    XResolution: 300
+    YResolution: 300
+    ICCProfile:attributes: "Reflective, Glossy, Positive, Color"
+    ICCProfile:cmm_type: 1818455411
+    ICCProfile:color_space: "RGB"
+    ICCProfile:copyright: "Public Domain"
+    ICCProfile:creation_date: "2025:01:05 04:34:16"
+    ICCProfile:creator_signature: "6c636d73"
+    ICCProfile:device_class: "Display device profile"
+    ICCProfile:device_manufacturer_description: "GIMP"
+    ICCProfile:device_model_description: "sRGB"
+    ICCProfile:flags: "Not Embedded, Independent"
+    ICCProfile:manufacturer: "0"
+    ICCProfile:model: "0"
+    ICCProfile:platform_signature: "Apple Computer, Inc."
+    ICCProfile:profile_connection_space: "XYZ"
+    ICCProfile:profile_description: "GIMP built-in sRGB"
+    ICCProfile:profile_size: 672
+    ICCProfile:profile_version: "4.4.0"
+    ICCProfile:rendering_intent: "Perceptual"
+    oiio:ColorSpace: "sRGB"
+    Stats Min: 0 0 0 0 (of 255)
+    Stats Max: 255 255 0 255 (of 255)
+    Stats Avg: 142.37 105.72 0.00 154.72 (of 255)
+    Stats StdDev: 79.19 98.91 0.00 87.39 (of 255)
+    Stats NanCount: 0 0 0 0 
+    Stats InfCount: 0 0 0 0 
+    Stats FiniteCount: 65536 65536 65536 65536 
+    Constant: No
+    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
diff --git a/testsuite/png/run.py b/testsuite/png/run.py
diff --git a/testsuite/png/src/gimp_gradient.png b/testsuite/png/src/gimp_gradient.png
