-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Enable zopfli on oxipng #1413
base: dev
Are you sure you want to change the base?
Enable zopfli on oxipng #1413
Conversation
src/shared/missing-preact-types.d.ts
Outdated
| 'sync' | ||
| 'async' | ||
| 'auto' | ||
| SignalLike<'sync' | 'async' | 'auto' | undefined>; |
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.
Should this be here? (it's causing the build to fail)
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.
Without that I get this error: Subsequent property declarations must have the same type. Property 'decoding' must be of type '"sync" | "async" | "auto" | SignalLike<"sync" | "async" | "auto" | undefined> | undefined', but here has type '"sync" | "async" | "auto" | undefined'
.
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.
@jakearchibald After further investigation I found that the error was caused by a package version error. After fixing that, the error is gone, so the code you mentioned
This is exciting! |
@Csabesz07 Your code seems to be working :) If you compress a bigger image, the time it take to compress is hugely different although in this case, the compressed file size is almost identical, which is not completely unexpected :D I’ll do a review in a bit :) |
codecs/oxipng/src/lib.rs
Outdated
@@ -20,6 +24,23 @@ pub fn optimise( | |||
} else { | |||
Interlacing::None | |||
}); | |||
match deflater { |
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.
Would be nice to introduce a type here first, like we have in the resize
module, so we can match on semantically meaningful words rather than numbers.
Also, invalid numbers should be panic()
’d, not just use a default value.
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.
codecs/oxipng/src/lib.rs
Outdated
} | ||
1 => { | ||
options.deflate = Deflaters::Zopfli { | ||
iterations: Option::unwrap(NonZeroU8::new(iterations)), |
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.
iterations: Option::unwrap(NonZeroU8::new(iterations)), | |
iterations: NonZeroU8::new(iterations).unwrap(), |
I'm not seeing any difference when changing the deflater, or the iterations / compress level setting, which feels a bit sus |
Exactly! This is my concern as well, but the way I see, all options are being applied at the moment. |
I rebuilt the codec locally when testing. I wonder if the binaries on the PR are outdated or something. I’ll take another look tomorrow at that. |
This feature lets the user further optimise the compression of the image by adding new options under oxipng.