-
Notifications
You must be signed in to change notification settings - Fork 9.7k
[image_picker] Do not resize GIF images #2821
Changes from all commits
b1c0e17
7670c96
aeb0a3a
a93b935
ac8de30
8a07d76
e6d9fc8
2abb07f
cef9a60
bcf05d1
38f5cfe
d4badb0
ff51d61
26930f8
5a8f350
6080f0d
9873b3c
60c4a5f
76e5364
e48fe36
4b4cfd4
e709bed
e9f5296
246d688
c3677fb
f82882a
a19f248
de13769
bcd85ee
40e4268
888f5b8
b1f6cb4
161ded1
bb82d8d
3038282
8df136e
5129308
aef3422
e9d23a2
2d2cdf0
93b6615
2133a7f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,8 +12,15 @@ | |
| import java.io.File; | ||
| import java.io.FileOutputStream; | ||
| import java.io.IOException; | ||
| import java.util.Arrays; | ||
| import java.util.HashSet; | ||
| import java.util.Set; | ||
|
|
||
| class ImageResizer { | ||
| /** Extensions that the ImageResizer cannot resize. */ | ||
| private static final Set<String> nonResizeableExtensions = | ||
| new HashSet<>(Arrays.asList("gif", "svg", "apng")); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Applying this to all
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hm, I'll look into implementing this; however I think the header doesn't contain info about animation, the file needs to be scanned to look for "local image descriptors" (> 1)
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There's a promising "Movie" class that might be useful to see if an image is animated or not. I'll use it to check both GIF and PNG (potentially animated), and leave the extensions for APNG (definitely animated) and SVG (vector).
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Adding to what Stuart has commented: For non-animated gif, we should scale it as it works as is. We can make this PR to exclude file extensions that cannot be scaled, which doesn't include gifs. And create another PR to scale gif.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @cyanglaz in theory there is more than 1 animated file format available on Android: Animated GIFs, WebP and HEIF via list of Android formats IMHO, the MR currently resolves a issue in that the GIF file looses animation, which is the No. 1 purpose of using a GIF file. I understand your point to "do it right", but think this would delay the product iteration by a lot. Why not merging this change with determined behaviour (as breaking change) and consider the next step after community feedback?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
And the only reason to pass max width/height is to cause the image to be resized. You are proposing breaking something that currently works in order to fix something that is sometimes currently broken. That is not a clear improvement.
Doing things correctly sometimes takes longer, yes. Rapidly moving between different broken states isn't something clients of software generally view as a feature.
If we are going to do an interim fix, it would be something along the lines of my non-breaking suggestion, so that we do not:
Breaking changes are disruptive; we do not want to set ourselves up to make several when there's a perfectly good way not to.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @stuartmorgan OK understood. |
||
|
|
||
| private final File externalFilesDirectory; | ||
| private final ExifDataCopier exifDataCopier; | ||
|
|
||
|
|
@@ -33,15 +40,21 @@ String resizeImageIfNeeded( | |
| @Nullable Double maxWidth, | ||
| @Nullable Double maxHeight, | ||
| @Nullable Integer imageQuality) { | ||
| Bitmap bmp = decodeFile(imagePath); | ||
| if (bmp == null) { | ||
| return null; | ||
| } | ||
| boolean shouldScale = | ||
| maxWidth != null || maxHeight != null || isImageQualityValid(imageQuality); | ||
| if (!shouldScale) { | ||
| return imagePath; | ||
cyanglaz marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
| // This class cannot resize certain extensions, so skip those. | ||
| String extension = imagePath.substring(imagePath.lastIndexOf(".") + 1).toLowerCase(); | ||
| boolean canScale = !nonResizeableExtensions.contains(extension); | ||
| if (!canScale) { | ||
| return imagePath; | ||
| } | ||
| Bitmap bmp = decodeFile(imagePath); | ||
| if (bmp == null) { | ||
| return null; | ||
| } | ||
| try { | ||
| String[] pathParts = imagePath.split("/"); | ||
| String imageName = pathParts[pathParts.length - 1]; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -25,13 +25,15 @@ public class ImageResizerTest { | |
|
|
||
| ImageResizer resizer; | ||
| File imageFile; | ||
| File gifImageFile; | ||
| File externalDirectory; | ||
| Bitmap originalImageBitmap; | ||
|
|
||
| @Before | ||
| public void setUp() throws IOException { | ||
| MockitoAnnotations.initMocks(this); | ||
| imageFile = new File(getClass().getClassLoader().getResource("pngImage.png").getFile()); | ||
| gifImageFile = new File(getClass().getClassLoader().getResource("dash-fainting.gif").getFile()); | ||
| originalImageBitmap = BitmapFactory.decodeFile(imageFile.getPath()); | ||
| TemporaryFolder temporaryFolder = new TemporaryFolder(); | ||
| temporaryFolder.create(); | ||
|
|
@@ -40,34 +42,40 @@ public void setUp() throws IOException { | |
| } | ||
|
|
||
| @Test | ||
| public void onResizeImageIfNeeded_WhenQualityIsNull_ShoultNotResize_ReturnTheUnscaledFile() { | ||
| String outoutFile = resizer.resizeImageIfNeeded(imageFile.getPath(), null, null, null); | ||
| assertThat(outoutFile, equalTo(imageFile.getPath())); | ||
| public void onResizeImageIfNeeded_WhenQualityIsNull_ShouldNotResize_ReturnTheUnscaledFile() { | ||
| String outputFile = resizer.resizeImageIfNeeded(imageFile.getPath(), null, null, null); | ||
| assertThat(outputFile, equalTo(imageFile.getPath())); | ||
| } | ||
|
|
||
| @Test | ||
| public void onResizeImageIfNeeded_WhenQualityIsNotNull_ShoulResize_ReturnResizedFile() { | ||
| String outoutFile = resizer.resizeImageIfNeeded(imageFile.getPath(), null, null, 50); | ||
| assertThat(outoutFile, equalTo(externalDirectory.getPath() + "/scaled_pngImage.png")); | ||
| public void onResizeImageIfNeeded_WhenQualityIsNotNull_ShouldResize_ReturnResizedFile() { | ||
| String outputFile = resizer.resizeImageIfNeeded(imageFile.getPath(), null, null, 50); | ||
| assertThat(outputFile, equalTo(externalDirectory.getPath() + "/scaled_pngImage.png")); | ||
| } | ||
|
|
||
| @Test | ||
| public void onResizeImageIfNeeded_WhenWidthIsNotNull_ShoulResize_ReturnResizedFile() { | ||
| String outoutFile = resizer.resizeImageIfNeeded(imageFile.getPath(), 50.0, null, null); | ||
| assertThat(outoutFile, equalTo(externalDirectory.getPath() + "/scaled_pngImage.png")); | ||
| public void onResizeImageIfNeeded_WhenImageIsGif_ShouldNotResize_ReturnUnscaledFile() { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (This is the test covering the new feature of not resizing .gif files) |
||
| String outputFilePath = resizer.resizeImageIfNeeded(gifImageFile.getPath(), null, null, 50); | ||
| assertThat(outputFilePath, equalTo(gifImageFile.getPath())); | ||
| } | ||
|
|
||
| @Test | ||
| public void onResizeImageIfNeeded_WhenHeightIsNotNull_ShoulResize_ReturnResizedFile() { | ||
| String outoutFile = resizer.resizeImageIfNeeded(imageFile.getPath(), null, 50.0, null); | ||
| assertThat(outoutFile, equalTo(externalDirectory.getPath() + "/scaled_pngImage.png")); | ||
| public void onResizeImageIfNeeded_WhenWidthIsNotNull_ShouldResize_ReturnResizedFile() { | ||
| String outputFile = resizer.resizeImageIfNeeded(imageFile.getPath(), 50.0, null, null); | ||
| assertThat(outputFile, equalTo(externalDirectory.getPath() + "/scaled_pngImage.png")); | ||
| } | ||
|
|
||
| @Test | ||
| public void onResizeImageIfNeeded_WhenHeightIsNotNull_ShouldResize_ReturnResizedFile() { | ||
| String outputFile = resizer.resizeImageIfNeeded(imageFile.getPath(), null, 50.0, null); | ||
| assertThat(outputFile, equalTo(externalDirectory.getPath() + "/scaled_pngImage.png")); | ||
| } | ||
|
|
||
| @Test | ||
| public void onResizeImageIfNeeded_WhenParentDirectoryDoesNotExists_ShouldNotCrash() { | ||
| File nonExistentDirectory = new File(externalDirectory, "/nonExistent"); | ||
| ImageResizer invalidResizer = new ImageResizer(nonExistentDirectory, new ExifDataCopier()); | ||
| String outoutFile = invalidResizer.resizeImageIfNeeded(imageFile.getPath(), null, 50.0, null); | ||
| assertThat(outoutFile, equalTo(nonExistentDirectory.getPath() + "/scaled_pngImage.png")); | ||
| String outputFile = invalidResizer.resizeImageIfNeeded(imageFile.getPath(), null, 50.0, null); | ||
| assertThat(outputFile, equalTo(nonExistentDirectory.getPath() + "/scaled_pngImage.png")); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -44,6 +44,8 @@ class ImagePicker { | |
| /// image types such as JPEG and on Android PNG and WebP, too. If compression is not supported for the image that is picked, | ||
| /// a warning message will be logged. | ||
| /// | ||
| /// GIF images picked from the gallery will only be scaled on iOS. They'll be returned as-is on Android and the Web. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a breaking change, and would need to be versioned accordingly. I think a better option would probably be a new, optional boolean to apply resizing only if we're confident that we can do it without destroying information, defaulting to false. That makes this a non-breaking change (and would make things like the animated vs non-animated gif issue something we could punt on).
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So, given the current The new boolean would be called
WDYT, @stuartmorgan (naming variables is not easy ;-)). Side note: When you said:
.. I started to wonder what happens to the EXIF metadata on resize. It was on the radar previously https://github.com/flutter/flutter/issues?q=is%3Aissue+exif+image_picker+is%3Aopen so may warrant another check in the future.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
That sounds good, except s/would/could/, and I would explicitly say that it's a heuristic that may be adjusted over time. I think your wording would be misleading about, e.g., the initial planned handling of GIFs (where it will fail to resize them even when not animated).
You could say something like "fundamentally change the image" instead of "incur a loss of information", to make it clearer that it's not just any loss of information. That's probably a good idea anyway, because as currently written any resize down would actually be forbidden because it loses information.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Whatever the change ends up being, the doc change will need to be applied to all the methods that take a size. |
||
| /// | ||
| /// Use `preferredCameraDevice` to specify the camera to use when the `source` is [ImageSource.camera]. | ||
| /// The `preferredCameraDevice` is ignored when `source` is [ImageSource.gallery]. It is also ignored if the chosen camera is not supported on the device. | ||
| /// Defaults to [CameraDevice.rear]. Note that Android has no documented parameter for an intent to specify if | ||
|
|
||
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.
This should be updated to reflect the actual set of extensions in the PR.