Skip to content
This repository has been archived by the owner on Feb 22, 2023. It is now read-only.

[image_picker] Image picker fix metadata #3873

Merged
merged 9 commits into from May 13, 2021

Conversation

ydag
Copy link
Contributor

@ydag ydag commented May 10, 2021

This PR fixes a rotation problem where "Select Photos" limited access is chosen but the image that is picked is not included in selected photos and the image is scaled.

List which issues are fixed by this PR. You must list at least one issue.

If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides. (Note that unlike the flutter/flutter repo, the flutter/plugins repo does use dart format. See plugin_tool format)
  • I signed the CLA.
  • The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]
  • I listed at least one issue that this PR fixes in the description above.
  • I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy.
  • I updated CHANGELOG.md to add a description of the change.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test exempt.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@cpboyd
Copy link
Contributor

cpboyd commented May 11, 2021

I've noticed that if you try to select this image:
Screen Shot 2021-05-11 at 10 32 15 AM
on the iOS simulators without permission, it fails even with these changes. None of the other default images cause the same issue.

Per #3835 (comment), image is nil.

loadObjectOfClass fails with this error:

2021-05-11 10:33:10.432504-0400 Runner[18240:9676219] [claims] Upload preparation for claim 13C254A4-B664-453E-B725-E927126DECE8 completed with error: Error Domain=NSCocoaErrorDomain Code=260 "The file “version=1&uuid=CC95F08C-88C3-4012-9D6D-64A413D254B3&mode=compatible.jpeg” couldn’t be opened because there is no such file." UserInfo={NSURL=file://~/Library/Developer/CoreSimulator/Devices/B68DDD0B-98C1-483B-875C-F025235CC303/data/Containers/Shared/AppGroup/F638EBF0-36D2-4E47-ADC5-9270B11D1CDF/File%20Provider%20Storage/photospicker/version=1&uuid=CC95F08C-88C3-4012-9D6D-64A413D254B3&mode=compatible.jpeg, NSFilePath=~/Library/Developer/CoreSimulator/Devices/B68DDD0B-98C1-483B-875C-F025235CC303/data/Containers/Shared/AppGroup/F638EBF0-36D2-4E47-ADC5-9270B11D1CDF/File Provider Storage/photospicker/version=1&uuid=CC95F08C-88C3-4012-9D6D-64A413D254B3&mode=compatible.jpeg, NSUnderlyingError=0x6000013e3fc0 {Error Domain=NSPOSIXErrorDomain Code=2 "No such file or directory"}}

UIImagePicker doesn't exhibit the same issue.

@cpboyd
Copy link
Contributor

cpboyd commented May 11, 2021

The specific issue I'm seeing seems related to https://developer.apple.com/forums/thread/654021

At the very bottom of the thread, someone notes:

In the Simulator example images, they are helpful in providing multiple image formats. One includes a ".heic" file, that's not a JPEG. And this is the one that usually borks.

He gave a workaround:

let supportedRepresentations = [UTType.rawImage.identifier,
                                UTType.tiff.identifier,
                                UTType.bmp.identifier,
                                UTType.png.identifier,
                                UTType.heif.identifier,
                                UTType.heic.identifier,
                                UTType.jpeg.identifier,
                                UTType.webP.identifier,
                                UTType.gif.identifier,
]
for representation in supportedRepresentations {
    if result.itemProvider.hasRepresentationConforming(toTypeIdentifier: representation, fileOptions: .init()) {
        result.itemProvider.loadInPlaceFileRepresentation(forTypeIdentifier: representation) { (originalUrl, inPlace, error) in

Have you tested with HEIC and other (non-JPEG) image formats on device?

@ydag
Copy link
Contributor Author

ydag commented May 11, 2021

The specific issue I'm seeing seems related to https://developer.apple.com/forums/thread/654021

At the very bottom of the thread, someone notes:

In the Simulator example images, they are helpful in providing multiple image formats. One includes a ".heic" file, that's not a JPEG. And this is the one that usually borks.

He gave a workaround:

let supportedRepresentations = [UTType.rawImage.identifier,
                                UTType.tiff.identifier,
                                UTType.bmp.identifier,
                                UTType.png.identifier,
                                UTType.heif.identifier,
                                UTType.heic.identifier,
                                UTType.jpeg.identifier,
                                UTType.webP.identifier,
                                UTType.gif.identifier,
]
for representation in supportedRepresentations {
    if result.itemProvider.hasRepresentationConforming(toTypeIdentifier: representation, fileOptions: .init()) {
        result.itemProvider.loadInPlaceFileRepresentation(forTypeIdentifier: representation) { (originalUrl, inPlace, error) in

Have you tested with HEIC and other (non-JPEG) image formats on device?

Hi @cpboyd , thanks for the review!

Yes, I have tested with the HEIC format. But as you mentioned above there is a known issue related to PHPicker that I guess we need to wait until it is solved by Apple and I explained here why I changed the UITest as well.

@cpboyd
Copy link
Contributor

cpboyd commented May 11, 2021

@ydag Ah, so the JPEG transcoding is simulator only?

Is the transcoding expected from PHPicker or does this only occur when not given full permissions to the image?

@ydag
Copy link
Contributor Author

ydag commented May 11, 2021

@cpboyd This is not about the permissions. Even with full permission, it will fail on the simulator.

@@ -1,3 +1,8 @@
## 0.7.6
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since there's no publish API change, it should be 0.7.5+1

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

@@ -20,7 +20,8 @@ NS_ASSUME_NONNULL_BEGIN

+ (UIImage *)scaledImage:(UIImage *)image
maxWidth:(NSNumber *)maxWidth
maxHeight:(NSNumber *)maxHeight;
maxHeight:(NSNumber *)maxHeight
metadataAvailability:(BOOL)metadataAvailability;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since it's a bool, the name should be more like a bool, maybe isMetadataAvailable

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

[FLTImagePickerImageUtil scaledImage:localImage
maxWidth:maxWidth
maxHeight:maxHeight
metadataAvailability:originalAsset == nil ? NO : YES];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can simply be metadataAvailability:originalAsset != nil];

ditto below

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

UIImage *newImage = [FLTImagePickerImageUtil scaledImage:image
maxWidth:@3
maxHeight:@2
metadataAvailability:YES];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you also test the scenario with no meta data?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

@ydag ydag mentioned this pull request May 12, 2021
11 tasks
Copy link
Contributor

@cyanglaz cyanglaz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@cyanglaz cyanglaz added the waiting for tree to go green (Use "autosubmit") This PR is approved and tested, but waiting for the tree to be green to land. label May 13, 2021
@fluttergithubbot
Copy link

This pull request is not suitable for automatic merging in its current state.

  • This commit has empty status or empty checks. Please check the Google CLA status is present and Flutter Dashboard application has multiple checks.

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 19, 2021
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 19, 2021
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 19, 2021
fotiDim pushed a commit to fotiDim/plugins that referenced this pull request Sep 13, 2021
This PR fixes a rotation problem where "Select Photos" limited access is chosen but the image that is picked is not included in selected photos and the image is scaled.
@mvanbeusekom mvanbeusekom deleted the image_picker_fix_metadata branch September 21, 2021 09:33
jolmiracle pushed a commit to miracle-as/webview_flutter that referenced this pull request Sep 24, 2021
This PR fixes a rotation problem where "Select Photos" limited access is chosen but the image that is picked is not included in selected photos and the image is scaled.
# Conflicts:
#	packages/image_picker/image_picker/pubspec.yaml
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
5 participants