Improve APNG support #668
Improve APNG support #668
Conversation
| @@ -8,7 +8,7 @@ pub enum PngError { | |||
| DeflatedDataTooLong(usize), | |||
| TimedOut, | |||
| NotPNG, | |||
| APNGNotSupported, | |||
| APNGOutOfOrder, | |||
There was a problem hiding this comment.
Hm, is this a BC break? APNGNotSupported should have been removed in v9 but it wasn’t.
There was a problem hiding this comment.
Yes, it technically is, as it's a publicly viewable enum.
cargo semver-checks is a really useful tool to answer this sort of questions:
There was a problem hiding this comment.
Nice, I've added APNGNotSupported back in and it's not showing that issue. However, it is showing a bunch of issues to do with the recently-added #[must_use] to a number of functions, should we worry about this?
I wonder if we should add this tool to our workflow...
There was a problem hiding this comment.
Personally, I'd ignore the lints related to #[must_use], as they are too pedantic to matter in practice: they only matter under non-default linter configurations that are a bit of a bad idea to blindly set up. Nevertheless, it's interesting that the tool finds out and let us know about these, just in case my opinion is wrong 🙂
Adding this tool to our workflow would be a good idea. I didn't read too much on how it computes a baseline for the semver checks though, so making sure that it always picks something sane in a workflow (even if it runs from a PR, or from a release tag...) would be great.
There was a problem hiding this comment.
Great, that’s kinda what I figured. Are we good to go with this PR then?
There was a problem hiding this comment.
Before that, I want to give your seemingly good changes a more proper, in-depth look. Please allow for a bit of time until I get around doing it 😄
There was a problem hiding this comment.
Ah sorry, I just assumed you already had. No urgency, I appreciate the scrutiny 🙂
src/lib.rs
Outdated
| let mut ihdr = png.raw.ihdr.clone(); | ||
| ihdr.width = frame.width; | ||
| ihdr.height = frame.height; | ||
| if let Ok(data) = PngImage::new(ihdr, &frame.data).and_then(|image| { |
There was a problem hiding this comment.
Should we error here if decompression fails?
There was a problem hiding this comment.
I've now made this throw any error from PngImage::new, which I think this is the safest thing to do for now.
src/lib.rs
Outdated
| // Use the same filter chosen for the main image | ||
| // No filter means we failed to optimise the main image and we shouldn't bother trying here | ||
| let Some(filter) = png.filter else { | ||
| return; | ||
| }; |
There was a problem hiding this comment.
So I'd be interested in opinions on this. As the comment says, we're unlikely to be able to optimise the fdATs if we failed to optimise the IDAT. The exception is if the image had been previously optimised with the current version of oxipng (before this PR): We will fail to optimise the IDAT any further, but could still optimise the fdATs. The problem is that we don't know what filter to use for the fdATs and guessing would not be ideal.
There is a workaround though: using the --force option will force the IDAT optimisation, thus providing the necessary filter for the fdATs. As the problem will only occur for images that have already been run through oxipng, this may be an acceptable answer.
|
@andrews05 - exciting to see some effort around APNG. Thanks! 🙏 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Two improvements to APNG support:
Reductions are still disabled.
Results on a set of 32 apngs, optimised with
-sao6:(There was a particularly large and poorly compressed file in the set which may be skewing the results a little.)
*Note that apngopt does not guard against writing a larger file than the input. The results here could be slightly smaller if I retained the originals where they were better.
[edit] A second set: