-
Notifications
You must be signed in to change notification settings - Fork 125
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
Minor reduction improvements #502
Conversation
1c477e3
to
3f936a0
Compare
Well, this turned into a larger refactor than I had planned 😆 @kornelski I ended up removing the extra 4-bit evaluation. I managed to find some images which did prefer 4-bit to 2-bit, but (perhaps due to improvements since that code was added) they were actually best at 8-bit (or even rgb). So I think it's not providing any benefit anymore, but do let me know if you disagree. |
a2a5993
to
63f91b9
Compare
src/reduction/mod.rs
Outdated
if let Some(reduced) = reduce_to_palette(&png) { | ||
png = Arc::new(reduced); | ||
// Make sure the palette gets sorted (ideally, this should be done within reduce_to_palette) | ||
if let Some(reduced) = reduced_palette(&png, opts.optimize_alpha) { |
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.
can reduce_to_palette
be moved before line 80 to try reduced_palette
once?
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.
The two calls to reduced_palette
are mutually exclusive: The earlier one will run if the image is already indexed (the function will simply return if it isn't), while this one will run if we've just converted to indexed.
Thanks, looks good, just needs a rebase 👍 |
@shssoichiro Thanks, done 🙂 |
This started off as minor a bug fix but then expanded into a larger refactor.
The bug occurred when a GrayscaleAlpha image with a
bKGD
chunk tried to reduce to Indexed. The bKGD conversion was assuming RGB and would fail the reduction entirely if the chunk wasn't 6 bytes. It now correctly reduces a Grayscale bKGD and thereby allows the reduction to Indexed. (My fault, since I added the GrayAlpha to Indexed support)However, due to the new found ability to reduce such images to indexed, "issue-60.png" ended up regressing slightly as the indexed version was actually larger than the gray alpha. Ultimately I ended up completely refactoring the reduction and evaluation sequence, combining the
perform_reductions
function withreduce_color_type
(I've put the code in the reduction mod, mainly to reduce the amount of code inlib
).The result is a bit tidier and has a few notable outcomes:
The end result of all this is... "issue-52-04.png" now compresses 3 bytes smaller 😂
Performance is essentially unchanged (I did measure a fractional improvement at level 0).