App
App copied to clipboard
[$2000][Image] Attachments uploaded via camera lose their rotation, appears sideways.
If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!
Action Performed:
- Go to a DM
- Tap plus in chat
- Choose “add an attachment”
- Chose “take a photo”
- Take your photo, save
Expected Result:
Uploaded image should be in the same orientation
Actual Result:
All uploaded photos lose their orientation and are rotated 90 degrees to the left
Workaround:
unknown
Platform:
Where is this issue occurring?
- iOS
Version Number: 1.2.15-2 Reproducible in staging?: y Reproducible in production?: y Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Notes/Photos/Videos:
https://user-images.githubusercontent.com/43996225/195900439-cb259355-2f1b-45a5-bf87-3096a4b86252.MP4
Expensify/Expensify Issue URL: Issue reported by: @JmillsExpensify Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1665704504033759
Triggered auto assignment to @arielgreen (AutoAssignerTriage
), see https://stackoverflow.com/c/expensify/questions/4749 for more details.
@arielgreen I'm going to take this one from you if you don't mind! 😄
Having trouble accessing Upwork at the moment, though I'll get to this one asap.
Actually I'm getting ahead of myself. I think this should be external but I'll quickly confirm with an Engineer to ensure that others agree.
Triggered auto assignment to @NikkiWines (Engineering
), see https://stackoverflow.com/c/expensify/questions/4319 for more details.
@NikkiWines I imagine we can look for the orientation someone in the image metadata, so it's simply a matter of making sure we respect that orientation post upload. Do you agree?
I'd imagine it varies depending on what time of image but from looking at a couple of different types (.heic, .tiff, .jpg, screenshot) it looks like the orientation is occasionally provided:
.heic
.tiff
.jpeg
taking a screenshot of another image
Wouldn't be a fool proof method to identifying orientation but we could most likely check to see if the orientation is present and modify the image based on that. I think this is a fine issue for External
Current assignee @JmillsExpensify is eligible for the External assigner, not assigning anyone new.
Triggered auto assignment to Contributor-plus team member for initial proposal review - @sobitneupane (External
)
Triggered auto assignment to @mountiny (External
), see https://stackoverflow.com/c/expensify/questions/7972 for more details.
Hmm this is odd issue and definitely feels like this must have worked before, maybe another regression which made it to production 😬
@kavimuru @mvtglobally can you confirm if you have a testRail case for this thing? ie taking a picture and uploading it and making sure all looks correct (resolution and orientation)
Waiting for proposals
@kavimuru @mvtglobally can you confirm if you have a testRail case for this thing? ie taking a picture and uploading it and making sure all looks correct (resolution and orientation)
@kavimuru @mvtglobally I wanted to circle back on @mountiny's comment above. Do either you know if this a potential regression or never worked? I kind of think it never worked, but I'm not certain either.
Still waiting for proposals in any case.
It is old nonetheless so I would not worry about the regression t this point I guess, just gonna have to make sure this is added as regression test to Testrail.
@JmillsExpensify I think we can double the price.
Ok, cool. I think this is pretty important, so I'm going to immediately raise the price to $1000.
@Beamanator Before I raise the price of this again, I kind of think this is an image bug that fits within the scope of your project. What do you think?
@JmillsExpensify thanks for tagging me! I added this bug to my tracker to keep an eye on it, but I don't think it needs to be put on hold since it's not related to caching - I would say go ahead and double and keep this one open to work on 👍
Sounds good!
Contributors - please make sure to post your proposal in this issue. Then when we get there, the Upwork job is here: https://www.upwork.com/jobs/~01a08d6a4623670c83.
Snagged from ya @JmillsExpensify since I'm managing the [Image]
tracking issue.
- https://github.com/Expensify/App/issues/11854
Proposal
I also encountered this problem in my project
https://github.com/react-native-image-picker/react-native-image-picker/blob/7490cc493737cd03f881cddeddbac28d43987534/ios/ImagePickerManager.m#L121-L134
Solution A
Use 4.8.4
, It's work fine in my project
- "react-native-image-picker": "^4.8.5",
+ "react-native-image-picker": "^4.8.4",
Solution B
Wait for https://github.com/react-native-image-picker/react-native-image-picker/pull/2036 to be merged, then we update to the next version 4.11.0
- "react-native-image-picker": "^4.8.5",
+ "react-native-image-picker": "^4.11.0",
Solution C
In the https://github.com/react-native-image-picker/react-native-image-picker/pull/2006, he pointed out that UIImageJPEGRepresentation
and UIImageJPEGRepresentation
return larger files, but this is maybe related to albums
, not related to camera
, should not create a new NSData
, so I want to create a npx patch
to delete it
- CFMutableDataRef imageData = CFDataCreateMutable(NULL, 0);
- CGImageDestinationRef destination = CGImageDestinationCreateWithData(imageData, kUTTypeJPEG, 1, NULL);
- CGImageDestinationAddImage(destination, image.CGImage, NULL);
- CGImageDestinationFinalize(destination);
- CFRelease(destination);
- data = (__bridge NSData *)imageData;
Waiting for your suggestions and choices
Thank you @hellohublot for your findings.
I believe we prefer approach of solving the issue upstream (soultion B of the proposal). But https://github.com/react-native-image-picker/react-native-image-picker/pull/2036 has not received much attention from maintainer.
I am not sure of the process when something has to be solved upstream. Your thoughts @mountiny ?
I agree we would prefer to fix this in the upstream. I have commented in the PR tagging the maintainer. I think in the past, he responded quite quickly but let;s see.
- I think in the mean time if rolling back one minor version to
4.8.4
fixes this, we should go ahead and do that. - Keep an eye/make a follow up tracking issue to wait for the aforementioned fix being released upstream and update the package to that version.
Can we confirm there are not bad consequences of downgrading to 4.8.4? I would assume no, but we need to make sure. cc @sobitneupane @hellohublot
@hellohublot Will you be able to confirm the following?
Can we confirm there are not bad consequences of downgrading to 4.8.4?
Yes, from some of the links below, you can see that we submitted the fix to 4.8.4 in order to fix the image file being too large
https://github.com/Expensify/App/issues/9424
https://github.com/react-native-image-picker/react-native-image-picker/pull/2006
https://github.com/react-native-image-picker/react-native-image-picker/compare/v4.8.4...v4.8.5#diff-e80fbe0919b9194e068e83807bd44a593c476cc4a9b787c284ec3283445d163cL121-L166
https://github.com/Expensify/App/pull/10297
So I also agree that we wait for the https://github.com/react-native-image-picker/react-native-image-picker/pull/2036 to be merged and the release of the new version, or we adopt before then
- "react-native-image-picker": "^4.8.5",
+ "react-native-image-picker": "git+https://github.com/GiovanniVisentiniCasavo/react-native-image-picker#39b3cc24653655e0a0c7b9ec38248cbed2b54112",
So, the issue with downgrading to 4.8.4 is https://github.com/Expensify/App/issues/9424.
I don't think it's good idea to use the following as it is yet to be tested and approved.
- "react-native-image-picker": "^4.8.5",
+ "react-native-image-picker": "git+https://github.com/GiovanniVisentiniCasavo/react-native-image-picker#39b3cc24653655e0a0c7b9ec38248cbed2b54112",
Yes, we'd better do some more testing on this library, I'll also investigate and follow up on this pull request, and then stably upgrade to 4.11.0
We can also make our own fork of that library. Let's see if the maintainer in the repo will respond.
Ok, we might want to either fork this or apply a patch of that PR since this is quite high-value issue and we cannot rely on the maintainer to merge/release this.
Agree with that.
I have reached out to the maintainer, we are happy to support them as the package i very useful for us. I will give this couple of days (maintaining our own fork is quite a bit of overhead so better to avoid this if we can, they have been active in previous months).