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 images being loaded without premultiplying its alpha channels #682

Merged
merged 19 commits into from
Jul 13, 2023

Conversation

Yttruire
Copy link
Contributor

PNG images are guaranteed by the file standards to not contain premultiplied alpha channels, and use straight alpha channels. As a result, PNGs will be rendered wrongly as the un-premultiplied alpha channels are used to render the image.

This PR fixes LoadImage. It will now check if the loaded image is a PNG and if so, premultiplies the alpha channels, then flags the image as having a premultiplied alpha channel. If images that are not PNG, it will assume that they are already premultiplied and simply flag them as premultiplied. This PR also adds two shards PremultiplyAlpha and DemultiplyAlpha for manually premultiplying the alpha and demultiplying alpha of an image.

Some refactoring is also done for the premultiplying and demultiplying of the alpha channels. LoadImage's code for loading byte inputs and files have also been streamlined in the process.

Note: Checking image file type for PNG can be done according to the specifications @ http://www.libpng.org/pub/png/spec/1.2/PNG-Structure.html

Additional list of file signatures: https://en.wikipedia.org/wiki/List_of_file_signatures

Fixes #671

@Yttruire Yttruire requested review from sinkingsugar and guusw May 25, 2023 09:53
@Yttruire Yttruire self-assigned this May 25, 2023
Copy link
Member

@guusw guusw left a comment

Choose a reason for hiding this comment

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

Minor comments, but looks good.

Bit annoying that only UI expects pre-multiplied alpha currently, but no easy way around it since it has to go through egui.

src/core/shards/imaging.cpp Outdated Show resolved Hide resolved
src/core/shards/imaging.cpp Outdated Show resolved Hide resolved
src/core/shards/serialization.cpp Outdated Show resolved Hide resolved
@guusw
Copy link
Member

guusw commented May 29, 2023

Also, you need to move the premultiple/unpremultiply template functions to a header (can add imaging.hpp) if you want to use them in serialization.cpp

@Yttruire Yttruire requested a review from guusw May 31, 2023 07:16
Copy link
Member

@guusw guusw left a comment

Choose a reason for hiding this comment

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

Looks good but can you check CI runs and fix the errors @Yttruire

@codecov
Copy link

codecov bot commented Jun 12, 2023

Codecov Report

Merging #682 (2400064) into devel (b15611f) will increase coverage by 0.06%.
The diff coverage is 78.52%.

@@            Coverage Diff             @@
##            devel     #682      +/-   ##
==========================================
+ Coverage   80.56%   80.62%   +0.06%     
==========================================
  Files         257      258       +1     
  Lines       33046    33135      +89     
==========================================
+ Hits        26623    26716      +93     
+ Misses       6423     6419       -4     
Impacted Files Coverage Δ
shards/modules/core/serialization.cpp 83.27% <66.66%> (+6.32%) ⬆️
shards/modules/imaging/imaging.cpp 79.09% <76.66%> (-1.23%) ⬇️
shards/modules/imaging/imaging.hpp 100.00% <100.00%> (ø)

... and 2 files with indirect coverage changes

@Yttruire
Copy link
Contributor Author

CI should be fixed now, seems to have been an issue with visibility of the implemented functions within the header file

@Yttruire Yttruire requested a review from guusw June 12, 2023 08:18
@@ -313,10 +397,10 @@ struct Resize {
void registerShards() {
REGISTER_SHARD("Convolve", Convolve);
Copy link
Member

Choose a reason for hiding this comment

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

Wonder if it's time to introduce a Image prefix and change those into like Image.Resize?

Copy link
Member

Choose a reason for hiding this comment

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

@sinkingsugar
Copy link
Member

@Yttruire why this is still not merged? cc @guusw

@guusw
Copy link
Member

guusw commented Jul 6, 2023

@Yttruire Can you add some add a test case for PremultiplyAlpha & DemultiplyAlpha in the image script (or a new script)

@Yttruire Yttruire force-pushed the zh/fix-premultiplied-alpha-channels branch from 2cabb2d to 66ffac6 Compare July 7, 2023 07:48
@Yttruire Yttruire requested review from guusw and removed request for guusw July 11, 2023 01:37
@sinkingsugar sinkingsugar merged commit ac5a20e into devel Jul 13, 2023
@sinkingsugar sinkingsugar deleted the zh/fix-premultiplied-alpha-channels branch July 13, 2023 04:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Problem with alpha channels when dealing with LoadImage and GFX.Texture
3 participants