Repository navigation
Change the method of get file extension. - #475
Conversation
|
@renefloor could this be reviewed ? using magic bytes is preferred over a potentially missing extension |
settings.gradle and pubspec.yaml conflicted on lines main has since moved past (kotlin 2.3.20, build_runner ^2.14.0), so main's lines are kept. image_cache_manager.dart reformatted with dart format. Co-Authored-By: Claude <noreply@anthropic.com>
|
|
The resizer decided whether a file could be resized from the extension of the file on disk. That extension is derived from the url, so images served from extension-less urls, or from urls whose extension does not match the payload, were never resized. It also treated any zero-prefixed binary as an ico. - Detect jpeg, png, ico, cur and webp by their magic number, reading 12 bytes instead of 8 (webp needs the RIFF/WEBP marker at offset 8). - Keep an extension fallback for formats without a signature at the start of the file, like tga, which only stores a signature in its footer. - Read the header in a helper that always closes the file handle. - Restore the trailing newline in example/android/build.gradle, which was dropped by an earlier commit in this branch. - Bump to 3.4.6 with a dated changelog entry, as CONTRIBUTING now requires. Tests: cover an image served without an extension and a non-image payload served with an image extension.
|
Pushed a follow-up on top of your merge — thanks for resolving the What changed in
Tests in Also bumped to 3.4.6 with a dated changelog entry per CONTRIBUTING, and restored the trailing newline in
|
Flutter decodes bmp and wbmp as well, so they are resizable too. Bmp has a 'BM' signature and can be detected from the content. Wbmp starts with a two byte header that is too generic to match on, so it joins tga in the extension fallback.
|
Follow-up: added bmp ( Wbmp is in Flutter's list of decodable formats, but it cannot be detected from the content: its header is Gif is intentionally left out: it is decodable, but |
Two bytes (the SOI marker) are enough to tell jpeg apart from the other formats that are checked, which is what SDWebImage and most detectors do. A file that is detected as an image but fails to decode then errors out of getImageFile, as it already did for a mislabeled extension before this change.
The extension that was passed to putFile came from the cached file path. For a url without an extension that path ends in the key itself, so the extension contained a slash and putFile failed to create the file. This is what the new test for an extension-less url caught. The resized image is always encoded as png anyway, so name it png instead of copying the extension of the url.
The resizer always re-encodes the image to png, which keeps a single frame. Now that webp is recognized by its signature, an animated webp would enter the resizer and silently lose its animation. That is the same reason gif is not recognized at all. Read the flags of the VP8X chunk, which is where an extended webp marks itself as animated, and pass those files through untouched. A static webp, with or without a VP8X chunk, is still resized.
A tga without an id field and without a colour map starts with the same four bytes as an ico or a cur, so the signatures cannot be told apart. It does not affect the result, since all of them are resizable, but it looks like a mistake without the note.
rickdijk
left a comment
There was a problem hiding this comment.
Thanks for picking this up. Detecting the format from the content is the right direction. A few things to look at before this can land; details are inline.
Two points that don't fit on a diff line:
shouldResize ignores maxHeight when maxWidth is set (existing bug, _resizeImageFile). The ternary parses as maxWidth != null ? image.width > maxWidth : (...), so the height check only runs when maxWidth is null. getImageFile(url, maxWidth: 200, maxHeight: 100) on a 150x400 image returns the 400px-tall original. It predates this PR, but since the function is being reworked anyway it would be good to fix it here:
final shouldResize = (maxWidth != null && image.width > maxWidth) ||
(maxHeight != null && image.height > maxHeight);PR description. Could you update it to the current PR template? The checklist is missing the 'Version bumped and dated CHANGELOG entry added' item and still mentions develop. main is the only branch now.
Instead of matching magic numbers, the file is offered to the image engine: ImageDescriptor.encoded + instantiateCodec report whether the file decodes at all, how many frames it has and how large it is. A file that fails to decode, or that holds an animation (gif, apng, animated webp), is returned untouched instead of erroring the stream or losing its animation. This covers every format the engine supports, including platform ones like heif, without a list to maintain. Also: - Fix shouldResize only checking maxHeight when maxWidth is null. - Fix the comment on the stored extension: it comes from the Content-Type header, not from the url; storing as png was already correct since the resized image is png-encoded. - Deprecate supportedFileNames, which no longer reflects what gets resized. Tests: an image served with a generic content type is resized and stored as .png, static webp and bmp are resized, a file that fails to decode is returned untouched, and animated gif, apng and webp are passed through.
|
Pushed 02775e7 addressing the review. The detection is now engine based as suggested in the review: Also done:
The workflow run for this push is still sitting at |
# Conflicts: # flutter_cache_manager/CHANGELOG.md
|
Merged current main into the branch (that release also shipped 3.4.6, so this PR is bumped to 3.4.7 now, with a dated CHANGELOG entry on top of the 3.4.6 one). |
rickdijk
left a comment
There was a problem hiding this comment.
Thanks, this looks a lot better. Letting the engine decide resolves all the earlier points, and the tests cover the new cases well. I resolved those threads. There are three more things below. The first one should be fixed before this is merged.
ImageDescriptor.width and height throw on web, so the size is taken from the first frame instead. On IO the buffer is created from the path so the bytes are not copied into memory for files that turn out not to be images. The CHANGELOG wording now refers to the Content-Type header and merges the duplicated bullets.
|
Pushed
|
The test file loaded its fixtures with dart:io, so `flutter test --platform chrome` failed during setup and the web resize path was never exercised. The images are now base64 constants in test_images.dart, which keeps the bytes in the test bundle and works in the browser as well. The fixtures are regenerated as plain 120x120 images (8 colour png, 1 bpp bmp, 3 frame gif/apng/webp) so they stay small: 5.7 KB in total instead of 84 KB.
|
Added The fixtures were read with Results:
I did not touch |
rickdijk
left a comment
There was a problem hiding this comment.
Review of the current head (e077cfa).
| Criterion | Verdict | Reason |
|---|---|---|
| Adds real value | pass | Fixes webp not being resized (#353), images served as application/octet-stream not being resized, and shouldResize ignoring maxHeight when maxWidth is set. |
| Really fixes it | pass | Detection no longer depends on the Content-Type-derived extension; the engine decides, and animated images are passed through. |
| Breaks something | concern | Behaviour changes and a deprecation ship as a patch release (see below). |
| Tests | pass | flutter test passes (128). 6 of the 8 new tests fail against main's image_cache_manager.dart and pass here; the animated gif/webp ones pass on both, as expected. The fromFilePath branch and the web path are not exercised in CI. |
| Contribution guidelines | fail | No pubspec.yaml bump, and the entry is added under the already-dated 3.4.7 (inline). |
dart format --set-exit-if-changed and flutter analyze are clean.
Recommendation: request changes. The approach is right; what's left is the version/changelog and the robustness points inline (mainly the custom FileSystem bypass and errors that can still escape).
- Semver. This deprecates
supportedFileNamesand changes behaviour callers can see: static gif/webp/bmp are now resized to png, resized files are stored as.png, and decode failures now return the original instead of erroring the stream, which reverses what 3.4.5's changelog promised for #400. Per pub's versioning, a deprecation plus these changes fits 3.5.0 better than a patch, with the error-handling change called out in the entry.
…le files Review follow-up: - The resize now decodes straight from the probed descriptor with instantiateCodec(targetWidth:, targetHeight:), so the file is read and decoded once instead of decoding a full-size first frame and then decoding again through FileImage/ResizeImage. The first-frame decode remains only on web, where descriptor.width and height throw. - ImmutableBuffer.fromFilePath is now only used when the file is backed by a LocalFileSystem, so a custom Config.fileSystem is never bypassed; other file systems go through file.readAsBytes(). - Files that cannot be decoded or resized return the original file with a warning logged through cacheLogger instead of erroring the stream, and all of the probing happens inside the try so read errors are handled too. - The not-resizable outcome (not an image, animated, or within the requested size) is remembered per key, so repeated calls do not re-read and re-decode the file every time. - Bumps the version to 3.5.0 with its own dated CHANGELOG entry, since the deprecation and the behaviour changes do not fit a patch release. Verified with flutter test (146 passed) and flutter test --platform chrome test/image_cache_manager_test.dart (17 passed).
|
Pushed
Verification: |
✨ What kind of change does this PR introduce? (Bug fix, feature, docs update...)
Bug fix / improvement of the image resizer.
Whether a cached file can be resized is decided from the extension of the file. That extension comes from the
Content-Typeheader of the response (not from the url), so:application/octet-streamare never resized.00 00 00 00matches any zero-prefixed binary, and tga cannot be decoded by the engine at all.getImageFileand errors the stream (#400).shouldResizeonly checksmaxHeightwhenmaxWidthis null, because the ternary parses asmaxWidth != null ? width > maxWidth : (...).getImageFile(url, maxWidth: 200, maxHeight: 100)on a 150x400 image returns the 400px-tall original.🆕 What is the new behavior (if this is a feature change)?
The image engine decides: the file is offered to
ui.ImageDescriptor.encoded, which reports whether it decodes at all, its frame count and its size. A file that fails to decode is returned untouched instead of erroring the stream, and a file with more than one frame (gif, apng, animated webp) is passed through so the animation is not lost by the png re-encode. No list of signatures or extensions has to be maintained, and every format the engine supports — including platform specific ones like heif — is resizable.Resized images are stored with a
.pngextension, matching the png encoding that is used.supportedFileNamesis deprecated, since it no longer reflects what gets resized; removing it would be breaking, so it is kept for now.Fixes #353 (webp not resized).
💥 Does this PR introduce a breaking change?
No.
supportedFileNamesis deprecated but still exported.🐛 Recommendations for testing
flutter testcovers the resize and caching behaviour, plus new tests for: an image served with a generic content type (stored as.bin) being resized and stored as.png, a url without an extension, static webp and bmp being resized, a non-image payload being returned untouched, and animated gif, apng and webp being passed through.📝 Links to relevant issues/docs
🤔 Checklist before submitting