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

Use customizable or user-supplied BufWriter for Zopfli #521

Open
Pr0methean opened this issue Jun 18, 2023 · 12 comments
Open

Use customizable or user-supplied BufWriter for Zopfli #521

Pr0methean opened this issue Jun 18, 2023 · 12 comments
Labels
T-Feature Requests for a new feature to be added

Comments

@Pr0methean
Copy link
Contributor

Pr0methean commented Jun 18, 2023

I have a program that creates large PNGs (up to 48 MiB of raw pixel data, but usually 12 or 16 MiB) and uses Zopfli as the deflater for PNG encoding. At these sizes, compression ratios are usually between 99.4% and 99.9%. Compression is currently slow because of the splits at every ZOPFLI_MASTER_BLOCK_SIZE (currently 1 million) bytes. When I raised this issue at zopfli-rs/zopfli#18, they suggested to instead use a large BufWriter wrapping a DeflateEncoder. Could we please have the option to do that? The best way would be to have the user supply a mutably borrowed BufWriter, so that we could reuse the buffer for multiple images.

@Pr0methean Pr0methean changed the title Use Zopfli DeflateEncoder with customizable buffer size Use customizable or user-supplied BufWriter for Zopfli Jun 18, 2023
@AlexTMjugador
Copy link
Collaborator

Putting on my OxiPNG contributor hat, I think this might be part of the explanation for #414. I'm busy with other things at the moment, but I feel it's worth taking a look at how performance compares with different master block sizes here.

@Pr0methean
Copy link
Contributor Author

I'll experiment with this in a fork.

@Pr0methean
Copy link
Contributor Author

See Pr0methean@97248f7.

@TPS
Copy link

TPS commented Jun 19, 2023

I'll experiment with this in a fork.

@Pr0methean How are the results from this fork/commit?

@andrews05
Copy link
Collaborator

@AlexTMjugador Afaik, zopflipng doesn't alter the ZOPFLI_MASTER_BLOCK_SIZE - it's a compile time constant which defaults to 1MB. So surely that wouldn't explain the other issue?

@AlexTMjugador
Copy link
Collaborator

AlexTMjugador commented Jun 19, 2023

So surely that wouldn't explain the other issue?

That constant is indeed unchanged, but it might be the case (I'm not sure right now, I may be wrong) that ZopfliPNG exercises a different code path that does not use that constant for segmenting the input data. After all, practical PNG images are not that big, and that master block size was mostly meant to deal with big datasets such as the enwiki corpus.

@Pr0methean
Copy link
Contributor Author

Pr0methean commented Jun 20, 2023

@TPS So far, the speed seems to be about the same, but it has the advantage that Zopfli can choose strategically where to split the file. I currently set it to a maximum of 64 splits for a 4096x4096 32-bit RGBA image, but in practice it chooses 24-36 blocks for the first attempt and up to 54 for the second attempt.

@TPS
Copy link

TPS commented Jun 20, 2023

@Pr0methean That seems encouraging. Why not make this an official PR here (draft, if you're not satisfied &/or done w/ tweaking it), referring this & the above issue, so, whenever the rest are merged by @shssoichiro in preparation for the next version & binary update, your work can benefit all of us? 🙇🏾‍♂️

@TPS
Copy link

TPS commented Jul 29, 2023

  • As noted in the comments, part of the code is "forked" from zopfli-rs. I'm not sure what's different, but surely it would be better to get the required changes submitted to the zopfli library? It doesn't seem like a great idea to keep partially forked code here.

@Pr0methean Per andrews05's comment, did you happen to PR to zopli-rs? I'm curious to find out how it'd do.

@Pr0methean
Copy link
Contributor Author

Yes; my PR was remade as #530 and merged.

@TPS
Copy link

TPS commented Aug 8, 2023

& then reverted, favoring PR @ zopfli-rs/zopfli#27.

@andrews05 andrews05 added the T-Feature Requests for a new feature to be added label Oct 10, 2023
@TPS
Copy link

TPS commented Feb 3, 2024

@Pr0methean Since this is merged @ zopfli-rs/zopfli@21d5521, close FTW?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-Feature Requests for a new feature to be added
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants