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

Allow APNG with reductions disabled #511

Merged
merged 5 commits into from
Jul 5, 2023
Merged

Conversation

andrews05
Copy link
Collaborator

@andrews05 andrews05 commented May 23, 2023

This is a bit of a hack, not sure if it's something that should actually be added, but it provides rudimentary support for APNG files by disabling all reductions and preserving original chunk order. See #79.
The savings to the test file apng_file.png are indeed minimal, achieving only 0.19% with default options and 0.41% with -o6 -a.

Note the required chunks are retained with strip safe, though you could still opt to strip all to turn it into a standard png.

@TPS
Copy link

TPS commented May 23, 2023

The greatest advantage to this is, when wholesale running over large #s of PNGs, is to avoid seemingly absolutely destroying APNGs in the process (for those not aware that it already won't). That, in & of itself, should increase OxiPNG's uptake & acceptance immediately.

@javiergutierrezchamorro 🙇🏾‍♂️ Comments, please?

@andrews05
Copy link
Collaborator Author

@TPS Oxipng currently skips APNGs entirely, so not sure what destruction you're referring to?

I've just added recompression of fdATs which is pretty straightforward and gets us up 0.74% savings on the apng test file. This is still very limited though as reductions are still disabled and it won't re-filter the fdATs. This is all possible in theory and could come some day, but again this is more of a quick hack to get something working.

@TPS
Copy link

TPS commented May 23, 2023

@andrews05 I clarified my meaning above; sorry for the confusion.

I mean that, much of the implicit premise of #79 ("Supported" ≠ "Optimized") is that OxiPNG lives in a world of static PNGs. I'm not aware of anything in the docs that mentions it's APNG-aware, much less that it's already safe for such. When did that happen?

This PR takes it to the next step: the very tiniest beginning of APNG optimization. That, combined w/ a note in the ReadMe & probably a brief mention in the built-in help text, is really welcome. 🙇🏾‍♂️

@andrews05
Copy link
Collaborator Author

@TPS Ah, looks like it happened after that issue was created (#88), so understandable that people weren't aware.

Yeah, it could definitely do with a note in the Read Me. There's a bunch of other updates that should be made to the Read Me as well but it's tied in with a benchmark script that I've been reluctant to delve into. I might check it out after #495 lands, which will affect benchmarks.

@TPS
Copy link

TPS commented May 24, 2023

@andrews05 I love the iterative nature of how you put together your PRs. Did 070c736 crack 1% savings?

@andrews05
Copy link
Collaborator Author

@TPS Ha, most of the time it's because I keep making mistakes that I need to fix 🤣. In this case the CI failed with the parallel feature disabled because I used a function that hadn't been shimmed for non-parallel yet. The new commit doesn't affect output.

@shssoichiro
Copy link
Owner

shssoichiro commented May 30, 2023

This looks like a nice improvement, even though the compression savings are relatively small. How safe is this to run in bulk? The primary reason why oxipng has continued to skip APNG files is because there has not been a way to verify the resulting image is identical to the input--above all, I want to ensure oxipng never causes damage to a file (unless the user is explicitly using non-default settings that are noted as possibly altering the image). Though if the fdat compressions being done here are fundamentally safe, I'm good with that.

@andrews05
Copy link
Collaborator Author

andrews05 commented May 30, 2023

The sanity checks will cover the main IDAT. I'm not sure how you would verify the rest of the animation though. I tested it on about a dozen images and am reasonably sure it's doing everything right, but hard to be certain 🤷‍♂️

[edit] Oh I see, the image crate can read the frames. I'll see if I can get it to verify them all...

@andrews05 andrews05 force-pushed the apng branch 2 times, most recently from a3d6a30 to e1e7a17 Compare May 30, 2023 06:35
@andrews05
Copy link
Collaborator Author

andrews05 commented May 30, 2023

Right, I've added validation of all frames 🙂

[edit] Er, keeping in mind the validation isn’t enabled in production anymore, so I guess this doesn’t really make it safer 🫤
Maybe I should find lots more images just to test. I only tried about a dozen while making this. (They’re not easy to find!)

@javiergutierrezchamorro

Cool. Looking forward to updated binaries and see if ready for prime time!

@andrews05
Copy link
Collaborator Author

I've now tested on 32 files - still not a huge amount but I am pretty confident that it's safe.
I've also added some warnings about stripping APNGs with --strip all.

@TPS
Copy link

TPS commented Jun 3, 2023

I've now tested on 32 files - still not a huge amount but I am pretty confident that it's safe.

@andrews05 Here's a bunch you could try: https://commons.wikimedia.org/wiki/Category:Animated_PNG_files

@andrews05
Copy link
Collaborator Author

@TPS Oh thanks, that's really helpful. I've now tested on around 350MB of APNGs, all successful 🙂

@TPS
Copy link

TPS commented Jun 4, 2023

In your tests on these, did it still hold to <1% reduction? Anything noteworthy?

@andrews05
Copy link
Collaborator Author

Overall still <1% but I think some individual files got up to 8%.

@ace-dent
Copy link

ace-dent commented Jun 4, 2023

@andrews05 - you probably have this already, but a APNG test corpus is here: https://philip.html5.org/tests/apng/tests.html

@andrews05
Copy link
Collaborator Author

@ace-dent Thanks, I did come across that. That seems to be more to do with correct decoding and rendering though. I think it will be helpful when full support eventually gets implemented but for now we aren't actually decoding the acTL/fcTL chunks.

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.

5 participants