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

Image Format Extensions & libpng #1548

Merged
merged 23 commits into from Feb 16, 2019

Conversation

Projects
None yet
4 participants
@RobertBColton
Copy link
Member

commented Feb 14, 2019

Now that we have finished most of the new backend and stuff, I decided it's a great time to finish the switch from LodePNG to libpng. This is the cleanest, fastest, and most complete of my attempts compared to #1442 which had a lot of duplicate code. The primary motivation for using libpng is because it is generally available in package managers, unlike LodePNG, and it also happens to be faster according to my benchmarks performed in #1391 where libpng was requested.

It's important to mention I wasted a lot of time on the gcc32 and clang32 jobs trying to get this to build until I realized the compiler was still being built 64 bit while it's the engine that was 32 bit. I finally got to the real solution in b274489 thanks to Josh where we just install the 32 bit one after emake is built.

Instead of updating the MinGW deps zip for fundies's jobs, I decided to add support for image format extensions instead. I was unable to get a libpng that would cooperate with the outdated MinGW on Ubuntu Trusty, which motivated me to do this. Anyway, that's why the libpng stuff here is being submitted as an extension that is enabled by default, but which the user can also disable at whim. This opens the door to users adding additional image format extensions if they so choose.

libpng Extension

@codecov

This comment has been minimized.

Copy link

commented Feb 14, 2019

Codecov Report

Merging #1548 into master will increase coverage by 0.04%.
The diff coverage is 90.9%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #1548      +/-   ##
=========================================
+ Coverage   17.56%   17.6%   +0.04%     
=========================================
  Files         165     166       +1     
  Lines       17129   17134       +5     
=========================================
+ Hits         3008    3016       +8     
+ Misses      14121   14118       -3
Impacted Files Coverage Δ
...GMAsystem/SHELL/Universal_System/image_formats.cpp 22.62% <82.35%> (-15.5%) ⬇️
.../Universal_System/Extensions/libpng/libpng-ext.cpp 93.87% <93.87%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ff78397...46814b8. Read the comment docs.

@RobertBColton RobertBColton referenced this pull request Feb 14, 2019

Closed

Switch to LibPNG #1391

@RobertBColton RobertBColton force-pushed the libpng-util branch 10 times, most recently from ed45d88 to 1d43b75 Feb 14, 2019

@RobertBColton RobertBColton force-pushed the libpng-util branch 6 times, most recently from 176acda to a85b27a Feb 15, 2019

@RobertBColton RobertBColton changed the title Switch to libpng Image Format Extensions & libpng Feb 15, 2019

@RobertBColton RobertBColton force-pushed the libpng-util branch from a85b27a to be18ebb Feb 15, 2019

@RobertBColton RobertBColton force-pushed the libpng-util branch from be18ebb to 119388a Feb 15, 2019

@RobertBColton RobertBColton force-pushed the libpng-util branch from 119388a to 85eab7c Feb 15, 2019

@RobertBColton RobertBColton force-pushed the libpng-util branch from 3efa7e5 to f43a807 Feb 15, 2019

@faissaloo

This comment has been minimized.

Copy link
Contributor

commented Feb 15, 2019

Yo this is rad

@RobertBColton

This comment has been minimized.

Copy link
Member Author

commented Feb 15, 2019

Ye son, I'm trying to get it green so when Josh gets here tonight we can get the review finally done. I had it green just before I went to run errands but I backtracked a little bit to double check some things while I have time.

Edit: Ok, I think this is the last of my force pushes, definitely don't need the PKG_CONFIG_PATH hack now but I guess the 32 bit libpng does need to be installed after building the frontend for now. Maybe later we can look at an easy way to force the frontend to be 32 bit.

@RobertBColton RobertBColton force-pushed the libpng-util branch from b55f031 to 3086c9f Feb 15, 2019

png_set_tRNS_to_alpha(png);

// These color_type don't have an alpha channel then fill it with 0xff.
if (color_type == PNG_COLOR_TYPE_RGB ||

This comment has been minimized.

Copy link
@JoshDreamland

JoshDreamland Feb 16, 2019

Member

Row pointers are how PNG works, Robert; you're just asking it for one at a time instead of all of them at once, for whatever reason, and it's costing you effort, because if you don't care about supporting images that don't fit in a contiguous block of memory (which you're forcing here, anyway), you can just ask PNG to read the whole image and normalize it for you instead of calling png_set_gray_to_rgb, etc.

But whatever, I guess. You can address bugs about unsupported PNG formats as they come in. 😛

@RobertBColton RobertBColton merged commit 112dc54 into master Feb 16, 2019

4 checks passed

codecov/patch 90.9% of diff hit (target 17.56%)
Details
codecov/project 17.6% (+0.04%) compared to ff78397
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@RobertBColton RobertBColton deleted the libpng-util branch Feb 16, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.