Fix : android crash when attaching a large size image #7209

ahmdshrif · 2022-01-13T18:19:53Z

Details

fixe App crashes when attaching a large size image in android chat

Fixed Issues

Tests

Verify that no errors appear in the JS console

QA Steps

1- Open app and login
2- Open any chat
3- Tap on + > Add Attachment > Choose from galery
4- Select a very long image
5- Wait a moment
6- Verify that app not crash

Verify that no errors appear in the JS console

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Screen.Recording.2022-01-14.at.8.02.44.PM.mov

Android

Screen.Recording.2021-12-21.at.9.25.56.PM.mov

github-actions · 2022-01-13T18:20:11Z

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

ahmdshrif · 2022-01-13T20:32:35Z

I have read the CLA Document and I hereby sign the CLA

parasharrajat · 2022-01-14T08:45:03Z

@ahmdshrif Could you please add videos for all platforms? Videos should cover Image zooming.
Also, please update the issue description.

Should be
$ https://github.com/Expensify/App/issues/6836

parasharrajat · 2022-01-14T08:46:50Z

src/components/AttachmentPicker/index.native.js

@@ -27,11 +27,14 @@ const propTypes = {
 /**
  * See https://github.com/react-native-image-picker/react-native-image-picker/#options
  * for ImagePicker configuration options
+  * maxWidth and maxHeight to avoid android crash when with large images


Please add more details. What is the effect of adding these?

Please add more details. What is the effect of adding these?

done,

@ahmdshrif Could you please add videos for all platforms? Videos should cover Image zooming. Also, please update the issue description.

Should be $ https://github.com/Expensify/App/issues/6836

I update android and ios videos with zoom.
note I only change files with extension .native.js
so it only affects android and ios.
also, this issue is only on android.

Well, the description is still wrong. The template must be saying to add only the link to the issue.

Could you please replace $ GH_LINK with $ https://github.com/Expensify/App/issues/6836

ok, I change the link,
and describe the changes more.

src/components/AttachmentPicker/index.native.js

parasharrajat

Ok. I tested it. But results are not favorable. During the proposal review, you said that image quality is not decreased but I see pixilated image. Although, it prevents app crashes but makes the feature literally unusable.

Can you please do some research and share that with us How did you end up deciding the resizing is necessary? I didn't get the answer to this question during the proposal review. We will try to find a solution together.

Also, If resizing is the only way. How can we do that without compromising the image quality? If we can't resize with the same quality, then what do you suggest as an alternative so that we can

Prevent the crash.
User is able to use the feature properly.

ahmdshrif · 2022-01-15T11:27:40Z

Ok. I tested it. But results are not favorable. During the proposal review, you said that image quality is not decreased but I see pixilated image. Although, it prevents app crashes but makes the feature literally unusable.

@parasharrajat
I mention it will not affect small images .but affect the large image relative to its size.

I think @Jag96 is mentioning in review it's acceptable since it does not affect the small images.

Agreed, and if there isn't any quality degradation for images smaller than the maxHeight/maxWidth values we set then I think that should be fine.

parasharrajat · 2022-01-15T12:46:05Z

Ok. Thanks for pointing. It's ok to have resizing. But may be there is a better alternative so that user can still interpret the information that he is sharing.

ahmdshrif · 2022-01-15T12:49:40Z

i also get new solution which adding android:hardwareAccelerated="false to AndroidManifest.xml
it works great without any resizing.

ss.mov

ahmdshrif · 2022-01-15T12:53:41Z

Ok. Thanks for pointing. It's ok to have resizing. But may be there is a better alternative so that user can still interpret the information that he is sharing.

@parasharrajat yes the new solution is working fine without any resizing if you agree I will push it now.

ahmdshrif · 2022-01-16T18:34:52Z

@parasharrajat after debugging I find this error happens in react-native-pan-zoom . when I disable it I can use large images without any issue.

after tracking the native error its comes from android canvas which is used in react-native-pan-zoom .
and the only solution I find for this is to disable android:hardwareAccelerated
https://stackoverflow.com/questions/39111248/canvas-trying-to-draw-too-large-bitmap-when-android-n-display-size-set-larger

so we have 2 solutions now :
The first is to disable android:hardwareAccelerated and I prefer this one and have already pushed it so you can test it.

second :
is resize the image only in react-native -zoom not in the image picker. so the user will have some quality dropping but it can download the image with full quality.

parasharrajat · 2022-01-16T19:18:04Z

Thanks, @ahmdshrif. I will check about the usages of hardware acceleration. Although, it seems a small change it affects the whole app. This decision can't be taken alone. But we are getting close. Now you have traced the error to react-native-pan-zoom. We may have better chances of resolving this. Let's see if we can fix this in react-native-pan-zoom or try to get some information on why this crash happened on that lib. I will look into this too.

ahmdshrif · 2022-01-18T09:06:36Z

hi, @parasharrajat is this acceptable quality with resizing?
since resizing from zoom pan (not image picker )to max quality can be like that

parasharrajat · 2022-01-18T09:15:12Z

That works. As said #7209 (comment), this is a big decision and it could affect performance badly.
While zooming in a very large image, we can hit the lag. I have not tested it yet but this is why hardware acceleration is used.

But I will give you a suggestion.

Disable the hardware acceleration on the Image modal screen only. And I think there is an easier way of doing it. It is just a matter of looking.
Conditionally turn the hardware acceleration on-off.

ahmdshrif · 2022-01-18T09:24:42Z

@parasharrajat
this image is resizing with hardware accelerator enabled! just resizing to max possible quality

parasharrajat · 2022-01-18T10:01:33Z

I didn't get it. Could you please rephrase it?

ahmdshrif · 2022-01-18T14:50:17Z

@parasharrajat
i mean the screenshote in #7209 (comment) is taken without adding
android:hardwareAccelerated="false"

it's just optimization for the previous resizing code.

parasharrajat · 2022-01-18T14:57:50Z

Ok. yeah, it looks good. Let me know when can I test.

ahmdshrif · 2022-01-18T18:34:36Z

@parasharrajat I push the code.
I can't confirm that's a perfect solution but it's working fine

I just redefine the fixed number( maxWidth, maxHeight) by ratio, and again no document for that but the small screen crashes with small dimension numbers, and big screens crash with big numbers for image dimension.
by test I find these ration working with all. without any issue.

also, I get the image from the image picker with full quality and just resize it for image zoom View.
but the user still can download the image with the full quality.

I will add check to resize only if it's android. but what do u think about this?

Jag96 · 2022-01-18T22:53:41Z

@ahmdshrif it looks like your commits are not signed, please see https://github.com/Expensify/App/blob/main/CONTRIBUTING.md#begin-coding-your-solution-in-a-pull-request and ensure your commits are signed. If you are unable to sign the commits in this PR please open a new one with signed commits.

parasharrajat · 2022-01-19T19:26:15Z

@ahmdshrif Could you please merge the main and test it to see if the app still crashes without your solution? It may be possible it is fixed based on QA team.

ahmdshrif · 2022-01-19T20:13:40Z

@parasharrajat I test the main branch and App still crashes.
Maybe they test on large screen devices or with a small image.

Signed-off-by: AHMED <a.s1171111@gmail.com>

Co-authored-by: Rajat Parashar <parasharrajat@users.noreply.github.com> Signed-off-by: AHMED <a.s1171111@gmail.com>

Signed-off-by: AHMED <a.s1171111@gmail.com>

ahmdshrif · 2022-01-19T20:44:30Z

@parasharrajat I rebase with main so you can test with and without my code. also, apply changes we discuss.

parasharrajat

LGTM. I don't think we need to export maxDimensionsScale to CONST as this is a very specific constant. But leaving this to @Jag96.

cc: @Jag96
🎀 👀 🎀 C+ reviewed

ahmdshrif · 2022-01-21T05:54:51Z

I think to make maxDimensionsScale as const. make code more readable.

Julesssss

The app no longer crashes, so it's technically a pass. But I am not seeing a preview on an older device (Nexus 5X). It shows a tiny zoomed-out version briefly, then disappears altogether., with the following native exception:

W/OpenGLRenderer: Bitmap too large to be uploaded into a texture (580x19734, max=16384x16384)

Any suggestions for resolving this?

AndroidBitmapError.mp4

parasharrajat · 2022-01-25T14:42:24Z

I think we should use FastImage and this issue should be resovled. This is already a dependency of the project.

ahmdshrif · 2022-01-25T15:07:36Z

The app no longer crashes, so it's technically a pass. But I am not seeing a preview on an older device (Nexus 5X). It shows a tiny zoomed-out version briefly, then disappears altogether., with the following native exception:
W/OpenGLRenderer: Bitmap too large to be uploaded into a texture (580x19734, max=16384x16384)
Any suggestions for resolving this?

@Julesssss
for me, I test the app in google play and still crash. also, I test the main before and still crash.
note: you should have a device with a small screen also you should wait some time so it's not crash directly on all devices.

also, this pr solves the issue you talking about. we resize the image to the max size that the device can load.

Screen_Recording_20220125-171257_New.Expensify.mp4

Julesssss · 2022-01-25T15:27:33Z

Ah okay, good point. I guess we should move the following suggestion into a new issue in that case.

@parasharrajat, what do you think? Should your suggestion be separated?

I think we should use FastImage and this issue should be resovled. This is already a dependency of the project.

parasharrajat · 2022-01-25T15:37:49Z

Yeah, we can do that @Julesssss

Julesssss

So the Bitmap too large exception was already occuring. As this PR solves the stated problem I'm going to merge.

OSBotify · 2022-01-25T15:41:50Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

OSBotify · 2022-01-25T20:33:41Z

🚀 Deployed to staging by @Julesssss in version: 1.1.32-1 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

OSBotify · 2022-02-01T18:04:45Z

🚀 Deployed to production by @roryabraham in version: 1.1.33-3 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

ahmdshrif force-pushed the fix/android-update-large-image branch from c107ecf to fcfea51 Compare January 13, 2022 20:30

ahmdshrif marked this pull request as ready for review January 13, 2022 21:00

ahmdshrif requested a review from a team as a code owner January 13, 2022 21:00

botify requested review from Jag96 and parasharrajat January 13, 2022 21:00

MelvinBot removed the request for review from a team January 13, 2022 21:00

parasharrajat suggested changes Jan 14, 2022

View reviewed changes

ahmdshrif changed the title ~~add maxwidth and maxheight to image picker~~ Fix : android crash when attaching a large size image Jan 14, 2022

ahmdshrif requested a review from parasharrajat January 14, 2022 18:14

parasharrajat suggested changes Jan 14, 2022

View reviewed changes

src/components/AttachmentPicker/index.native.js Outdated Show resolved Hide resolved

ahmdshrif requested a review from parasharrajat January 14, 2022 18:48

parasharrajat suggested changes Jan 15, 2022

View reviewed changes

ahmdshrif requested a review from parasharrajat January 16, 2022 18:36

ahmdshrif and others added 8 commits January 19, 2022 22:18

add maxwidth and maxheight to image picker

67e3817

Signed-off-by: AHMED <a.s1171111@gmail.com>

update comment

de6664d

Signed-off-by: AHMED <a.s1171111@gmail.com>

Apply suggestions from code review

92f7617

Co-authored-by: Rajat Parashar <parasharrajat@users.noreply.github.com> Signed-off-by: AHMED <a.s1171111@gmail.com>

delete resize && disable hardwareAccelerated

71ffd3b

Signed-off-by: AHMED <a.s1171111@gmail.com>

add resize delete hardware acceleration

26b8ff6

Signed-off-by: AHMED <a.s1171111@gmail.com>

delete hardwareAccelerated comment

6288d76

Signed-off-by: AHMED <a.s1171111@gmail.com>

delete white spaces

3d2848d

Signed-off-by: AHMED <a.s1171111@gmail.com>

apply change from review

cb2a618

ahmdshrif force-pushed the fix/android-update-large-image branch from 096c979 to cb2a618 Compare January 19, 2022 20:36

refactor code

644c995

ahmdshrif requested a review from parasharrajat January 19, 2022 20:44

parasharrajat approved these changes Jan 20, 2022

View reviewed changes

Julesssss reviewed Jan 25, 2022

View reviewed changes

Julesssss approved these changes Jan 25, 2022

View reviewed changes

Julesssss merged commit 7075d8f into Expensify:main Jan 25, 2022

OSBotify mentioned this pull request Jan 25, 2022

Deploy Checklist: New Expensify 2022-01-25 #7395

Closed

46 tasks

parasharrajat mentioned this pull request Jan 26, 2022

Attachments - Borders are displayed around the attachment #7407

Closed

botify mentioned this pull request Feb 1, 2022

[HOLD for payment 2022-02-08] Android - Chat - App crashes when attaching a large size image #6836

Closed

ahmdshrif deleted the fix/android-update-large-image branch March 11, 2022 09:05

Fix : android crash when attaching a large size image #7209

Fix : android crash when attaching a large size image #7209

Conversation

ahmdshrif commented Jan 13, 2022 • edited Loading

Details

Fixed Issues

Tests

QA Steps

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

github-actions bot commented Jan 13, 2022 • edited Loading

ahmdshrif commented Jan 13, 2022

parasharrajat commented Jan 14, 2022

parasharrajat Jan 14, 2022

Choose a reason for hiding this comment

ahmdshrif Jan 14, 2022

Choose a reason for hiding this comment

parasharrajat Jan 14, 2022

Choose a reason for hiding this comment

ahmdshrif Jan 14, 2022

Choose a reason for hiding this comment

parasharrajat left a comment • edited Loading

Choose a reason for hiding this comment

ahmdshrif commented Jan 15, 2022 • edited Loading

parasharrajat commented Jan 15, 2022

ahmdshrif commented Jan 15, 2022

ahmdshrif commented Jan 15, 2022

ahmdshrif commented Jan 16, 2022

parasharrajat commented Jan 16, 2022

ahmdshrif commented Jan 18, 2022

parasharrajat commented Jan 18, 2022

ahmdshrif commented Jan 18, 2022

parasharrajat commented Jan 18, 2022

ahmdshrif commented Jan 18, 2022

parasharrajat commented Jan 18, 2022

ahmdshrif commented Jan 18, 2022

Jag96 commented Jan 18, 2022

parasharrajat commented Jan 19, 2022

ahmdshrif commented Jan 19, 2022

ahmdshrif commented Jan 19, 2022

parasharrajat left a comment • edited Loading

Choose a reason for hiding this comment

ahmdshrif commented Jan 21, 2022

Julesssss left a comment

Choose a reason for hiding this comment

parasharrajat commented Jan 25, 2022

ahmdshrif commented Jan 25, 2022 • edited Loading

Julesssss commented Jan 25, 2022

parasharrajat commented Jan 25, 2022

Julesssss left a comment

Choose a reason for hiding this comment

OSBotify commented Jan 25, 2022

OSBotify commented Jan 25, 2022

OSBotify commented Feb 1, 2022

ahmdshrif commented Jan 13, 2022 •

edited

Loading

github-actions bot commented Jan 13, 2022 •

edited

Loading

parasharrajat left a comment •

edited

Loading

ahmdshrif commented Jan 15, 2022 •

edited

Loading

parasharrajat left a comment •

edited

Loading

ahmdshrif commented Jan 25, 2022 •

edited

Loading