App icon indicating copy to clipboard operation
App copied to clipboard

[$2000][Image] Attachments uploaded via camera lose their rotation, appears sideways.

Open kavimuru opened this issue 2 years ago • 34 comments

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:

  1. Go to a DM
  2. Tap plus in chat
  3. Choose “add an attachment”
  4. Chose “take a photo”
  5. 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

View all open jobs on GitHub

kavimuru avatar Oct 14 '22 16:10 kavimuru

Triggered auto assignment to @arielgreen (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

melvin-bot[bot] avatar Oct 14 '22 16:10 melvin-bot[bot]

@arielgreen I'm going to take this one from you if you don't mind! 😄

JmillsExpensify avatar Oct 14 '22 17:10 JmillsExpensify

Having trouble accessing Upwork at the moment, though I'll get to this one asap.

JmillsExpensify avatar Oct 17 '22 11:10 JmillsExpensify

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.

JmillsExpensify avatar Oct 17 '22 14:10 JmillsExpensify

Triggered auto assignment to @NikkiWines (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

melvin-bot[bot] avatar Oct 17 '22 14:10 melvin-bot[bot]

@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?

JmillsExpensify avatar Oct 17 '22 14:10 JmillsExpensify

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 image

.tiff image

.jpeg image

taking a screenshot of another image 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

NikkiWines avatar Oct 17 '22 17:10 NikkiWines

Current assignee @JmillsExpensify is eligible for the External assigner, not assigning anyone new.

melvin-bot[bot] avatar Oct 17 '22 17:10 melvin-bot[bot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @sobitneupane (External)

melvin-bot[bot] avatar Oct 17 '22 17:10 melvin-bot[bot]

Triggered auto assignment to @mountiny (External), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

melvin-bot[bot] avatar Oct 17 '22 17:10 melvin-bot[bot]

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)

mountiny avatar Oct 17 '22 18:10 mountiny

Waiting for proposals

mountiny avatar Oct 18 '22 15:10 mountiny

@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.

JmillsExpensify avatar Oct 26 '22 17:10 JmillsExpensify

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.

mountiny avatar Oct 27 '22 12:10 mountiny

Ok, cool. I think this is pretty important, so I'm going to immediately raise the price to $1000.

JmillsExpensify avatar Oct 27 '22 20:10 JmillsExpensify

@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 avatar Nov 01 '22 19:11 JmillsExpensify

@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 👍

Beamanator avatar Nov 02 '22 10:11 Beamanator

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.

JmillsExpensify avatar Nov 02 '22 14:11 JmillsExpensify

Snagged from ya @JmillsExpensify since I'm managing the [Image] tracking issue.

  • https://github.com/Expensify/App/issues/11854

mallenexpensify avatar Nov 02 '22 20:11 mallenexpensify

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

hellohublot avatar Nov 03 '22 08:11 hellohublot

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 ?

sobitneupane avatar Nov 03 '22 12:11 sobitneupane

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.

  1. 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.
  2. 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

mountiny avatar Nov 03 '22 12:11 mountiny

@hellohublot Will you be able to confirm the following?

Can we confirm there are not bad consequences of downgrading to 4.8.4?

sobitneupane avatar Nov 04 '22 04:11 sobitneupane

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",

hellohublot avatar Nov 04 '22 04:11 hellohublot

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",

sobitneupane avatar Nov 04 '22 05:11 sobitneupane

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

hellohublot avatar Nov 04 '22 05:11 hellohublot

We can also make our own fork of that library. Let's see if the maintainer in the repo will respond.

mountiny avatar Nov 04 '22 09:11 mountiny

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.

mountiny avatar Nov 07 '22 10:11 mountiny

Agree with that.

JmillsExpensify avatar Nov 08 '22 05:11 JmillsExpensify

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).

mountiny avatar Nov 08 '22 11:11 mountiny