-
Notifications
You must be signed in to change notification settings - Fork 12
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
Support opaque images with background chunks #54
Conversation
Thanks for providing a hotfix that quickly 💯. |
Odd, because this works: |
Benchmark resultJudge resultBenchmark Report for /home/runner/work/PNGFiles.jl/PNGFiles.jlJob Properties
ResultsA ratio greater than
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfoTarget
Baseline
Target resultBenchmark Report for /home/runner/work/PNGFiles.jl/PNGFiles.jlJob Properties
ResultsBelow is a table of this job's results, obtained by running the benchmarks.
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfo
Baseline resultBenchmark Report for /home/runner/work/PNGFiles.jl/PNGFiles.jlJob Properties
ResultsBelow is a table of this job's results, obtained by running the benchmarks.
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfo
Runtime information
|
Thanks! Will continue investigation tomorrow |
I thought rolling back PNGFiles didn't fix the segfaults? Maybe I'm missing something |
I checked out Correction of #54 (comment): |
Since this seems to break a lot of things and this fix helps, I'm going to merge this today. Not sure how others feel about the warning message when reading an opaque image with a background chunk. My thinking was that if someone is interested in knowing the suggested background color, the warning would at least tell them what it is. But I think in the future we want to provide some utility functions to query this kind of metadata from images and make them separate from loading. |
An update to this: using
👍 since it affects not only |
@IanButterworth I think I must've made a mistake while evaluating the older version of PNGFiles with Plots. Sorry for the noise everyone. |
Just thinking out loud here, maybe we can improve testing by loading / saving back a bunch of png files from a database, so that regressions like these can be caught in CI. I just came across http://www.schaik.com/pngsuite/ with a relatively small db (http://www.schaik.com/pngsuite/PngSuite-2017jul19.tgz) 66kB, which should be relatively easy to integrate in the CI actions. They even provide corrupted .png files for graceful exit testing (ans thus less likely to segfault in c libs). I interpret the LICENSE as being very permissive. |
We already use pngsuite for testing, but I guess our tests are not as extensive as they should... |
background_color = process_background(png_ptr, info_ptr, _adjust_background_bitdepth(background, bit_depth)) | ||
is_transparent || @warn("Background color for non-transparent image: $(background_color)") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the user need to be warned about this? If not, perhaps make it a debug?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just saw your message above. Yeah, I don't think this needs to be generate log noise. Being a debug would at least make it accessible until we expose an api for it
Damn, I should have read the tests first, before speaking 😅. OK, I think I'm going to try to enhance those. |
By mistake, I assumed that opaque PNG image cannot have a background chunk, but they can (to provide suggestion for their surroundings), this lead to a bug where we'd add some padding for alpha values even in case there were none.
I believe this fixes #53.
cc: @t-bltg @IanButterworth