Skip to content

Commit

Permalink
fix(png): alpha premultiplication adjustment and attribute (#4585)
Browse files Browse the repository at this point in the history 
See discussion
#4054
and PR
#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 <[email protected]>
Co-authored-by: Brecht Van Lommel <[email protected]>
  • Loading branch information
@lgritz @brechtvl
lgritz and brechtvl authored Jan 7, 2025
1 parent e964a6a commit 3663310
Show file tree
Hide file tree
Showing 10 changed files with 171 additions and 29 deletions.
26 changes: 25 additions & 1 deletion src/doc/builtinplugins.rst
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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**

Expand Down Expand Up @@ -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**

Expand Down
8 changes: 8 additions & 0 deletions src/include/OpenImageIO/imageio.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
///
Expand Down
1 change: 1 addition & 0 deletions src/include/imageio_pvt.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
15 changes: 12 additions & 3 deletions src/libOpenImageIO/imageio.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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 {
Expand Down Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down
32 changes: 21 additions & 11 deletions src/png.imageio/pnginput.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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();
}
Expand All @@ -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);
};


Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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];
Expand All @@ -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;
}
}
}
Expand Down
37 changes: 24 additions & 13 deletions src/png.imageio/pngoutput.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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();
}
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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];
Expand All @@ -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;
}
}
}
Expand Down
38 changes: 38 additions & 0 deletions testsuite/png/ref/out-libpng15.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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"
Expand Down
Loading

0 comments on commit 3663310

Please sign in to comment.