fix(png): Correctly read PNGs with partial alpha #4315
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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 <lg@larrygritz.com>
Signed-off-by: Larry Gritz <lg@larrygritz.com>
|
Any concerns? I'm going to merge this because I need to do builds with this fix at work. If anybody finds fault with it we can amend with another PR to fix anything remaining. I won't backport it to a release branch until after the patch release on Jul 1, since it's relatively untested. It should see enough use in master to feel safe backporting it for the Aug 1 release. |
lgritz
added a commit
to lgritz/OpenImageIO
that referenced
this pull request
Jun 27, 2024
…dation#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 <lg@larrygritz.com>
lgritz
added a commit
to lgritz/OpenImageIO
that referenced
this pull request
Jun 27, 2024
…dation#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 <lg@larrygritz.com>
lgritz
added a commit
to lgritz/OpenImageIO
that referenced
this pull request
Jun 27, 2024
…dation#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 <lg@larrygritz.com>
lgritz
added a commit
to lgritz/OpenImageIO
that referenced
this pull request
Jul 16, 2024
…dation#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 <lg@larrygritz.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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