Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 3 commits into from
Jan 7, 2025

Conversation

lgritz
Copy link
Collaborator

@lgritz lgritz commented Jan 6, 2025

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.

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 <[email protected]>
@lgritz lgritz requested a review from brechtvl January 6, 2025 07:01
Copy link
Contributor

@brechtvl brechtvl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

I confirm it works correctly with OSL in Blender, and with oiiotool.

Only minor comments for docs, the rest looks correct.

src/doc/builtinplugins.rst Show resolved Hide resolved
src/doc/builtinplugins.rst Outdated Show resolved Hide resolved
@brechtvl
Copy link
Contributor

brechtvl commented Jan 6, 2025

For reference, I'm attaching 8 example files from different sources, which all work correctly now oiio_png_semi_transparent.zip

I didn't find counter examples, so would be inclined to keep the default to 0.

Co-authored-by: Brecht Van Lommel <[email protected]>
Signed-off-by: Larry Gritz <[email protected]>
@lgritz
Copy link
Collaborator Author

lgritz commented Jan 6, 2025

Should I add a couple of those images to the testsuite? I assume at least the gradient ones are safe/licensed?

@lgritz
Copy link
Collaborator Author

lgritz commented Jan 6, 2025

So in short, I think the examples are showing us that Apple Preview (and maybe several other apps) are interpreting the files as if the premultiplication is happening on the sRGB values, not on linear values. So that reinforces the idea of the default being 0, but people can use the new attribute to force "correct" linear premultiplication.

@brechtvl
Copy link
Contributor

brechtvl commented Jan 6, 2025

The gradient ones were created by me, so they are indeed ok to add to the test suite. The others unfortunately not.

@lgritz lgritz merged commit 3663310 into AcademySoftwareFoundation:main Jan 7, 2025
28 checks passed
@lgritz
Copy link
Collaborator Author

lgritz commented Jan 7, 2025

@brechtvl Since it's kind of early in the month and this was obviously something you ran into in practice, would you like me to put out a tagged patch with this fix? Or can it wait until the Feb 1 patch release?

@brechtvl
Copy link
Contributor

brechtvl commented Jan 7, 2025

It can wait. We just apply patches ourselves if there is no release in time, as long as we know it's temporary it's no big deal.

@lgritz lgritz deleted the lg-png branch January 7, 2025 04:06
lgritz added a commit to lgritz/OpenImageIO that referenced this pull request Jan 7, 2025
…ftwareFoundation#4585)

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 <[email protected]>
Co-authored-by: Brecht Van Lommel <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants