feat: use rust native webp en/decoding #31
Conversation
crate resolution locked it to 1.72 before
Reviewer's Guide by SourceryThis pull request introduces native Rust WebP encoding and decoding support using the File-Level Changes
Tips
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @nekowinston - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟡 Testing: 2 issues found
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
| } | ||
| } | ||
|
|
||
| impl<P: Pixel> Iterator for WebPSequenceDecoder<P> { |
There was a problem hiding this comment.
suggestion (performance): Consider optimizing the Iterator implementation for WebPSequenceDecoder
The current implementation allocates a new buffer and converts pixels for each frame. Consider pre-allocating the buffer and reusing it across frames, and look into ways to optimize the pixel conversion process. This could significantly improve performance for animated WebP images.
impl<P: Pixel> Iterator for WebPSequenceDecoder<P> {
type Item = crate::Result<crate::Frame<P>>;
fn next(&mut self) -> Option<Self::Item> {
// Pre-allocate buffer here
let mut buffer = Vec::with_capacity(self.width * self.height * 4);
// Implement frame decoding and pixel conversion logic here
// Reuse buffer for subsequent frames
}
}
| let mut image_buf: Vec<u8> = create_image_buffer(&decoder); | ||
| decoder | ||
| .read_image(&mut image_buf) | ||
| .map_err(|e| crate::Error::DecodingError(e.to_string()))?; |
There was a problem hiding this comment.
suggestion: Consider implementing a more structured error handling approach
Instead of converting errors to strings, consider defining custom error types that wrap the underlying errors from the image_webp library. This would preserve more information about the errors and make it easier for users of your library to handle them appropriately.
.map_err(|e| crate::Error::DecodingError(WebpDecodingError::from(e)))?;
// In the error module:
#[derive(Debug)]
pub enum WebpDecodingError {
ImageError(image_webp::Error),
// Add other specific error variants as needed
}
impl From<image_webp::Error> for WebpDecodingError {
fn from(err: image_webp::Error) -> Self {
WebpDecodingError::ImageError(err)
}
}
| fn test_animated_webp_decode() -> ril::Result<()> { | ||
| for (frame, ref color) in ImageSequence::<Rgb>::open("tests/animated_sample.webp")?.zip(COLORS) |
There was a problem hiding this comment.
suggestion (testing): Improved test specificity for lossless WebP
The new test is more specific and checks against reference images, which is a good improvement. However, consider adding assertions for frame count and dimensions to ensure the overall structure of the animated WebP is correct.
| fn test_animated_webp_decode() -> ril::Result<()> { | |
| for (frame, ref color) in ImageSequence::<Rgb>::open("tests/animated_sample.webp")?.zip(COLORS) | |
| fn test_animated_webp_lossless() -> ril::Result<()> { | |
| let sequence = ImageSequence::<Rgb>::open("tests/animated_lossless.webp")?; | |
| let frame_count = sequence.len(); | |
| assert_eq!(frame_count, 3, "Expected 3 frames in the animated WebP"); | |
| let first_frame = sequence.get(0)?.into_image(); | |
| assert_eq!(first_frame.width(), 400, "Unexpected frame width"); | |
| assert_eq!(first_frame.height(), 400, "Unexpected frame height"); |
| fn test_static_webp_encode() -> ril::Result<()> { | ||
| let image = Image::from_fn(256, 256, |x, _| L(x as u8)); | ||
|
|
||
| image.save_inferred("tests/out/webp_pure_encode_output.webp") |
There was a problem hiding this comment.
issue (testing): Static WebP encoding test lacks assertions
The test encodes an image but doesn't verify the result. Consider adding assertions to check the encoded file's properties or contents.
Still a WIP, based on the recently released (and even more recently fixed)
image-webpcrate.Couple of notes:
webp-pureas a mutually exclusive feature rather than a full replacement, asimage-webpcurrently only supports lossless encoding.COLORSstatic in./tests/test_png.rsto a common module, since three files are now using the same contant, and usingmod png;would also always run png tests when just testing withcargo test --test test_webp-pure --features all-purefor me.We should probably wait for the release of image-webp v0.1.2/v0.2.0; I mainly wrote this PR for catppuccin/toolbox#147, to compile that project to WASM with pure Rust webp support, without having to rely on ImageMagick.
Summary by Sourcery
Add support for Rust-native WebP encoding and decoding with the
image-webpcrate, introducing awebp-purefeature for lossless encoding. Update the MSRV to v1.67.1 and refactor tests to use a common module for shared constants. Update documentation and add new tests for WebP functionality.New Features:
image-webpcrate, with a newwebp-purefeature for lossless encoding.Enhancements:
Build:
image-webpcrate.Documentation:
webp-purefeature alongside existing image encoding features.Tests:
webp-purefeature.Chores:
rust-toolchain.tomlfile to specify the Rust toolchain version and components required for the project.