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

Make reductions return a new uncompressed image #158

Merged
merged 10 commits into from
Jan 27, 2019

Conversation

kornelski
Copy link
Contributor

@kornelski kornelski commented Jan 14, 2019

Continues making reductions immutable, and partially fixes #145 and #152

@kornelski
Copy link
Contributor Author

Ouch, dropping worse-compressing reductions has broken tests that expected these reductions

@kornelski
Copy link
Contributor Author

The test failure on Windows is surprising. I've tested on my Windows machine, with both rayon and without, and it worked fine.

I think it could happen if multiple reductions compress to the exact same size, and then the (non-deterministically) last one to compress wins. But a tie in compression size seems quite unlikely.

@shssoichiro
Copy link
Owner

shssoichiro commented Jan 18, 2019

It might make sense to make the reduction deterministic, e.g. we should prefer lower bit-depth if there's otherwise a tie.

@kornelski
Copy link
Contributor Author

Another source of difference was zlib implementation. On very small files (~15 bytes long) file sizes from miniz and cfzlib differ by 1 byte, and cause a different depth to be chosen.

@kornelski
Copy link
Contributor Author

In the end I've skipped problematic test with miniz. It's an unusual file that compresses over 100KB of data to 890 bytes, and the difference between various modes is just 20 bytes, so it's a very close call.

@kornelski
Copy link
Contributor Author

Finally ready :)

Copy link
Owner

@shssoichiro shssoichiro 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, thanks!

@shssoichiro shssoichiro merged commit 9af874d into shssoichiro:master Jan 27, 2019
@kornelski kornelski deleted the pngimage branch January 27, 2019 17:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dual purpose and implicit state in PngData
2 participants