Repository navigation
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
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
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.
| if (!await file.exists()) { | ||
| return null; | ||
| } | ||
| final bytes = await file.readAsBytes(); |
There was a problem hiding this comment.
Small one: before, a file whose extension wasn't on the list was skipped without being read. Now every file passed to getImageFile with a size limit is read fully into memory, then copied once more into the ImmutableBuffer, before we know whether it is an image at all. That matters for something like a large video or PDF fetched through getImageFile. It isn't a blocker, but ui.ImmutableBuffer.fromFilePath (on IO), or skipping files above a size threshold, would avoid it.
There was a problem hiding this comment.
Fair point, changed in 2b58ba8: on IO the buffer is now created with ui.ImmutableBuffer.fromFilePath, so the bytes are not copied into memory unless the path cannot be read. The tests use a mocked file system whose paths do not exist on the real one, so they take the readAsBytes fallback — that fallback is deliberate, not the main path. On web there is no path and the cache is in memory already, so fromUint8List stays there.
I left out a size threshold on purpose: a large image still has to be inspected before it can be resized, so a cap would change behaviour for legitimate large images rather than just skipping non-images. If reading from the path turns out to be a problem on some platform, a cap can be added on top.
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
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.
| @@ -1,5 +1,8 @@ | |||
| ## [3.4.7] - 2026-10-28 | |||
There was a problem hiding this comment.
These bullets go under ## [3.4.7], which main has since dated as its release day (2026-10-31, #545), and pubspec.yaml is still 3.4.7. AGENTS.md asks each PR to bump version: and add its own dated ## [x.y.z] - YYYY-MM-DD entry. As is, this change would ship inside 3.4.7's notes, and this header line conflicts with main. Could you rebase and move these bullets to a new entry (3.5.0, see the review body)?
| final originalFileName = originalFile.file.path; | ||
| final fileExtension = originalFileName.split('.').last; | ||
| if (!supportedFileNames.contains(fileExtension)) { | ||
| final info = await _imageInfo(originalFile.file); |
There was a problem hiding this comment.
When the file is not decodable or is animated, the original is returned and nothing is stored under resizedKey. So every later getImageFile(url, maxWidth: ...) misses getFileFromCache(resizedKey) and runs _imageInfo again: the whole file is read and its first frame fully decoded on every call. An animated gif shown with a resize limit pays that on every build, where main skipped .gif without reading it. Remembering the "not resizable" outcome, for example an in-memory set of keys or a cache entry that points at the original, would avoid repeating the work.
| // (they throw), so the size is taken from the first frame instead. This | ||
| // fully decodes that frame, which costs the same as the decode the | ||
| // resize performs anyway. | ||
| final frame = await codec.getNextFrame(); |
There was a problem hiding this comment.
This decodes the first frame at full resolution only to read the size, and _decodeImage then decodes the file again to resize it. For a 6000x4000 photo that is about 96 MB of RGBA for two ints. On IO, descriptor.width/height work, and descriptor.instantiateCodec(targetWidth:, targetHeight:) can produce the resized frame from the same descriptor, which is one decode and no FileImage/ResizeImage. The first-frame read is only needed on web.
| } finally { | ||
| frame.image.dispose(); | ||
| } | ||
| } on Object { |
There was a problem hiding this comment.
This hides every failure (corrupt data, out-of-memory on a huge image, an unsupported format) with no trace. 3.4.5 deliberately started propagating decode errors (#400), so this is a visible change for callers: a truncated jpeg now silently comes back at full size. Keeping the fallback is fine, but could it log a warning through cacheLogger so a failed resize can be diagnosed?
| if (!await file.exists()) { | ||
| return null; | ||
| } | ||
| final buffer = await _bufferFor(file); |
There was a problem hiding this comment.
_bufferFor runs before the try, so if fromFilePath and the readAsBytes fallback both throw, the error still escapes getImageFile. That happens, for example, when cache cleanup deletes the file between exists() and the read. Moving this line inside the try gives the "returned untouched" behaviour the PR describes.
| return ui.ImmutableBuffer.fromUint8List(await file.readAsBytes()); | ||
| } | ||
| try { | ||
| return await ui.ImmutableBuffer.fromFilePath(file.path); |
There was a problem hiding this comment.
ImmutableBuffer.fromFilePath reads file.path from the real disk and bypasses the configured FileSystem. Config.fileSystem is a supported override. With a FileSystem that stores encrypted or transformed bytes at real paths, this read succeeds but the bytes don't decode, so the image is silently never resized. With a memory-backed FileSystem whose path also exists on disk, it reads an unrelated file. Could this branch be limited to the package's own IOFileSystem (or a LocalFileSystem), with every other FileSystem going through file.readAsBytes()?
| maxHeight = (imageHeight / resizeFactor).round(); | ||
| } | ||
|
|
||
| final resized = await _decodeImage( |
There was a problem hiding this comment.
Even after _imageInfo succeeds, the resize can still throw here (FileImage reads the path directly too) and in toByteData(...)! below when it returns null. The "non-images are returned untouched instead of erroring the stream" guarantee only covers the probe. Resizing with the probed descriptor's codec, or giving this step the same fallback, would close the gap.
| /// The returned record holds the frame count of the image, which is how | ||
| /// animated images are recognized, and its width and height. | ||
| Future<(int frameCount, int width, int height)?> _imageInfo(File file) async { | ||
| if (!await file.exists()) { |
There was a problem hiding this comment.
Nit: once _bufferFor is inside the try, this exists() check is redundant. A missing file throws and is handled, the extra stat goes away, and the race it seems to guard (the file disappearing right after) is still covered.
✨ 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