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.
| try { | ||
| descriptor = await ui.ImageDescriptor.encoded(buffer); | ||
| codec = await descriptor.instantiateCodec(); | ||
| return (codec.frameCount, descriptor.width, descriptor.height); |
There was a problem hiding this comment.
On web, ImageDescriptor.encoded does not support width / height: the web engine throws UnsupportedError('ImageDescriptor.width is not supported on web.'). The on Object catch turns that into null, so on web every image is now returned without being resized, with no error. The web config uses MemoryCacheSystem, so this code path does run there.
Taking the size from the first frame works on all platforms and costs the same as the full decode main already did:
final frame = await codec.getNextFrame();
try {
return (codec.frameCount, frame.image.width, frame.image.height);
} finally {
frame.image.dispose();
}The tests only run on the VM, so they won't catch this. If it isn't clear whether resizing worked on web before, flutter test --platform chrome for this file would show it.
| 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.
| @@ -1,3 +1,10 @@ | |||
| ## [3.4.7] - 2026-10-27 | |||
|
|
|||
| * Detects whether a downloaded file can be resized by letting the image engine decode it, instead of from the extension of the url. Images served with a generic `Content-Type` such as `application/octet-stream` are now resized, and files that are not images are returned untouched ([#475](https://github.com/Baseflow/flutter_cache_manager/pull/475)) | |||
There was a problem hiding this comment.
This still says "instead of from the extension of the url". As the third bullet and the code comment say, the extension comes from the Content-Type. Something like "instead of from the file extension derived from the Content-Type header" would fit. The first two bullets also say mostly the same thing and could be merged into one.
✨ 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