Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.59k stars 2.92k forks source link

[HOLD for payment 2024-07-24] [$500] Chat - Bad performance when opening, closing or zooming big image #38180

Closed lanitochka17 closed 4 months ago

lanitochka17 commented 8 months ago

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Version Number: 1.4.51-0 Reproducible in staging?: Y Reproducible in production?: Y If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/4418462 Email or phone of affected tester (no customers): vdargentotest+web031224@gmail.com Issue reported by: Applause - Internal Team

Action Performed:

Pre-requisite: the user must be logged in

  1. Go to any chat
  2. Send the attached GIF
  3. Send the attached image
  4. Click on the image preview to open it
  5. Verify the opening process is not smooth
  6. Click on the image to zoom in
  7. Verify is not instantly zoomed in, the behavior is slowly
  8. Try to move the zoom view, verify is not smooth
  9. Close the image
  10. Verify the closing process is not smooth

Expected Result:

There should be no performance issues when opening, zooming or closing big images

Actual Result:

The behavior of the app gets slower when opening, zooming or closing the big image

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/78819774/bd11fa5f-47c6-4ba4-b4be-4c83e3fd4701

Bug6411486_1710273547556!Big_image (1)

Bug6411486_1710276381171!typing-fast-typing

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~0184b3ab8ca5055856
  • Upwork Job ID: 1777877652768452608
  • Last Price Increase: 2024-06-27
  • Automatic offers:
    • wildan-m | Contributor | 102956097
Issue OwnerCurrent Issue Owner: @sobitneupane
melvin-bot[bot] commented 8 months ago

Triggered auto assignment to @mallenexpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

lanitochka17 commented 8 months ago

@mallenexpensify FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

lanitochka17 commented 8 months ago

We think that this bug might be related to #vip-vsp CC @quinthar

mallenexpensify commented 8 months ago

Check on in vip-vsb https://expensify.slack.com/archives/C066HJM2CAZ/p1710294208664919

melvin-bot[bot] commented 8 months ago

@mallenexpensify Huh... This is 4 days overdue. Who can take care of this?

mallenexpensify commented 8 months ago

Adding to #vip-vsb since it's a bug that doesn't involve money.

mallenexpensify commented 8 months ago

Bumping to weekly while we await a status update

melvin-bot[bot] commented 7 months ago

Job added to Upwork: https://www.upwork.com/jobs/~0184b3ab8ca5055856

melvin-bot[bot] commented 7 months ago

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

mallenexpensify commented 7 months ago

I think/hope this can be External. @sobitneupane , let me know if you disagree.

sobitneupane commented 7 months ago

@mallenexpensify I do agree. I think this can be handled externally.

ShridharGoel commented 7 months ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

We can see some performance issues with images, like when using high resolution images.

What is the root cause of that problem?

Images which have too high resolution show bad performance since the render time increases and tasks such as zoom also become heavy.

For example, the below image is 9001x9000.

Example image https://private-user-images.githubusercontent.com/78819774/312232438-66b91727-3f02-456a-bc34-f2f0f80622bb.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MTMyNTkwNjEsIm5iZiI6MTcxMzI1ODc2MSwicGF0aCI6Ii83ODgxOTc3NC8zMTIyMzI0MzgtNjZiOTE3MjctM2YwMi00NTZhLWJjMzQtZjJmMGY4MDYyMmJiLnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDA0MTYlMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQwNDE2VDA5MTI0MVomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTBhNjFkNTVmNTkwMGU0NzY5ZDFkYjMzYWNkYzY3YjI4YTNlYWUxYTZmMmU1MWU0ZmZkZDczMzY3MzhjY2QxMjQmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0JmFjdG9yX2lkPTAma2V5X2lkPTAmcmVwb19pZD0wIn0.Dv5Ay334kJNvPjzEruP8UgHyMdv47lpZGhdrPXZLUBM

In the validation logic, we are not checking this.

https://github.com/Expensify/App/blob/ed7029b33c5711736569aea8981e21fc68e12d89/src/components/AttachmentModal.tsx#L314-L356

What changes do you think we should make in order to solve the problem?

Add the below logic after we get updatedFile object in validateAndDisplayFileToUpload inside AttachmentModal. This would help us in displayed a preview with low size, while uploading the original image to the backend.

const img = new Image();
img.src = URL.createObjectURL(updatedFile);

await new Promise((resolve) => {
    img.onload = resolve;
});

let inputSource = URL.createObjectURL(updatedFile);

// If any dimension is above 4096, create a compressed version for display
if (img.width > 4096 || img.height > 4096) {
    const canvas = document.createElement('canvas');
    canvas.width = img.width > 4096 ? 4096 : img.width;
    canvas.height = img.height > 4096 ? 4096 : img.height;

    const ctx = canvas.getContext('2d');
    ctx.drawImage(img, 0, 0, canvas.width, canvas.height);

    inputSource = canvas.toDataURL();
}

Above code can be polished.

Note that we are only changing the inputSource and not the updatedFile. So, only the preview in the modal gets changed.

Then, we can also show a message saying that the preview is of low quality than expected, and original image can be seen by downloading (UX to be decided by design team).

{isImageCompressed && (
    <View style={[styles.flexRow, styles.alignItemsCenter, styles.mt2, styles.mb2]}>
        <Icon src={Info} additionalStyles={[styles.mr2]} fill={theme.placeholderText} />
        <Text style={styles.textNormal} color={theme.placeholderText}>{'This image has been resized for previewing. '}
            <TextLink
                onPress={downloadAttachment}
            >Download</TextLink>
            {' for full resolution.'}</Text>
    </View>
)}

UI above can be polished. Also, we'll need to ensure that the preview fits in the modal.

isImageCompressed is state which will be set to true only if compression happens.

What alternative solution did you explore?

Old solution We can check if the image being uploaded has a high resolution. If yes, then we'll compress it before uploading. Add the below code after checking if it's a valid file: https://github.com/Expensify/App/blob/ed7029b33c5711736569aea8981e21fc68e12d89/src/components/AttachmentModal.tsx#L327-L330 Pseudo code (we can polish it): ``` const cleanAndOpenUploadModal = (fileObject) => { if (fileObject instanceof File) { /** * Cleaning file name, done here so that it covers all cases: * upload, drag and drop, copy-paste */ let updatedFile = fileObject; const cleanName = FileUtils.cleanFileName(updatedFile.name); if (updatedFile.name !== cleanName) { updatedFile = new File([updatedFile], cleanName, {type: updatedFile.type}); } const inputSource = URL.createObjectURL(updatedFile); const inputModalType = getModalType(inputSource, updatedFile); setIsModalOpen(true); setSourceState(inputSource); setFile(updatedFile); setModalType(inputModalType); } else if (newImage.uri) { const inputModalType = getModalType(fileObject.uri, fileObject); setIsModalOpen(true); setSourceState(fileObject.uri); setFile(fileObject); setModalType(inputModalType); } } if (Str.isImage(fileObject.name)) { getImageResolution(fileObject).then( ({ height, width }) => height <= CONST.AVATAR_MAX_HEIGHT_PX && width <= CONST.AVATAR_MAX_WIDTH_PX, ).then((check) => { if (!check) { cropOrRotateImage(fileObject.uri, [], {compress: 0.5}) .then((newImage) => { // Continue with new image cleanAndOpenUploadModal(newImage) }) } else { // Continue with old image cleanAndOpenUploadModal(fileObject) } }) } else { // Continue with the file cleanAndOpenUploadModal(fileObject) } ``` Above logic compresses the image by 50% only if it exceeds 4096 in height/width. This would decrease the image quality, but that should be fine because the image in such cases would mostly already be high quality. Note that I have tested the heavy image with the above compression logic, and it is working well. https://github.com/Expensify/App/assets/35566748/26919c1b-b8f1-4a53-9693-c3cb3787a32e
mallenexpensify commented 7 months ago

@sobitneupane , can you review the above proposal plz?

sobitneupane commented 7 months ago

Thanks for the proposal @ShridharGoel

We would want solution that solves the issue for big image as the one attached here. Thank you for your efforts

ShridharGoel commented 7 months ago

Proposal

Updated

ShridharGoel commented 7 months ago

@sobitneupane Can you have a look at the updated proposal?

sobitneupane commented 7 months ago

@ShridharGoel Can you please add more details to RCA.

In which file does the issue lie? Where are you proposing to make the change? What will be the affect on quality of image after the change? Can you also attach what the output would look like?

ShridharGoel commented 7 months ago

@sobitneupane Updated.

sobitneupane commented 7 months ago

@ShridharGoel I am fine with slight delay when zooming big image rather than decreasing quality as it only happens first time I zoom the image after opening the modal. I believe we can improve the performance issue when opening and closing the modal without decreasing the quality.

@mallenexpensify What's your take?

If we are okay with decreasing the image resolution, rather than compressing it to half, I think we should calculate the aspect ratio to make either height or width equal to max allowed value.

ShridharGoel commented 7 months ago

@sobitneupane I just checked that it works well even if we call cropOrRotateImage with compress as 1, which means we can achieve the goal without changing quality of image. Does this sound good?

ShridharGoel commented 7 months ago

Note that width will be auto-defined based on the aspect ratio (Link). We are going to do the below only for high size images.

cropOrRotateImage(fileObject.uri, [{ resize: { height: CONST.AVATAR_MAX_HEIGHT_PX } }], { compress: 1, name: fileObject.name, type: fileObject.type })
.then((newImage) => {
    // Continue with new image
    cleanAndOpenUploadModal(newImage);
});
melvin-bot[bot] commented 7 months ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

sobitneupane commented 7 months ago

@ShridharGoel We can do that. But won't it degrade the time required to upload file. I mean once I chose a heavy file,how long will it take to open upload modal?

mallenexpensify commented 7 months ago

What's your take?

Awaiting reply from @ShridharGoel on your comment above @sobitneupane . If there's no degradation, that sounds like the way to go. If there is, and we feel we might have to compromise, can you start a discussion in #expensify-open-source to get more 👀? I know nothing about the coding behind the feature and fix so I don't feel comfortable making a guess or assumption.

ghost commented 7 months ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

Chat - Bad performance when opening, closing or zooming big image.

What is the root cause of that problem?

Opening big image: The provided image is png which takes over 10 seconds to loads. After testing other image formats, the delay is specific only to the png format. Other image formats, jpg, web, gif load without locking the UI.

Closing big image: In AttachmentModal.tsx, the HeaderWithBackButton component has onCloseButtonPress={closeModal}, the onModalClose does not have implementation, the onModalHide can be used instead which calls Navigation.dismissModal() of ReportAttachments.tsx. https://github.com/Expensify/App/blob/68e69623a8bca0dce5f384fa351d5cbb2214aeb4/src/components/AttachmentModal.tsx#L505

What changes do you think we should make in order to solve the problem?

For Opening big image: The issue is specific to png file format. I have tested with other image formats (jpg, gif, webp) and the attachment modal does not lock. The backend converts the png to jpeg and makes available 1024px size images in jpg format. To test upload the provided big image and then in the network panel notice the big file uploaded as '.1024.jpg' appended.

/chat-attachments/1911982936538631822/w_2617956ba1f93adf274e31ac20973e77c3ce2f78.png.1024.jpg

/chat-attachments/6665154622286864152/w_196b95a9706a9d0527864963ff054b0ddea49946.jpg.1024.jpg

In ImageView, https://github.com/Expensify/App/blob/9a727de1e67013a8baf2dff018063af03d963ada/src/components/ImageView/index.tsx#L36

Add use state variable imageUrl and check fileExtension fo

const [imageUrl, setImageUrl] = useState(() => {
    const { fileExtension } = FileUtils.splitExtensionFromFileName(url ?? '');
    const isvalid = (fileExtension === "png") ? true : false;
    const u = isvalid ? url + ".1024.jpg" : url;
    return u;
})

The backend is also serving jpg file formats in 1024px, so jpeg can also leverage the use of 1024px.

For closing big image: In AttachmentModal, onModalClose() has no effect due to lack of implementation, instead call onModalHide() which calls Navigation.dismissModal. https://github.com/Expensify/App/blob/68e69623a8bca0dce5f384fa351d5cbb2214aeb4/src/components/AttachmentModal.tsx#L505

Alternatively, in the closeModal callback https://github.com/Expensify/App/blob/305f12c52ef228e7ed4845735a1794942a2b8d87/src/components/AttachmentModal.tsx#L384 call

setShouldLoadAttachment(false);

and then in the AttachmentCarousel block add check for shouldLoadAttachments https://github.com/Expensify/App/blob/305f12c52ef228e7ed4845735a1794942a2b8d87/src/components/AttachmentModal.tsx#L525

!isEmptyObject(report) && shouldLoadAttachment && !isReceiptAttachment

When the setIsModalOpen(false) is called in the closeModal callback, the isVisible property is handles the visibility of the Modal. The AttachmentCarousel is still rendered if the isVisible is set which causes the delay, so adding check for shouldLoadAttachment improves closing the modal. https://github.com/Expensify/App/blob/305f12c52ef228e7ed4845735a1794942a2b8d87/src/components/AttachmentModal.tsx#L484

Zooming big image: The zooming issue is also resolved for png formats when using 1024px jpg.

What alternative solutions did you explore? (Optional)

sobitneupane commented 7 months ago

@jsdev2547 Similar to closing image (in your proposal), can we improve the opening image experience (without converting)? It is okay to show loading indicator by opening the attachment modal. But we have some lag while opening the image. What is the root cause for it?

melvin-bot[bot] commented 7 months ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

ghost commented 7 months ago

@sobitneupane I have revised proposal. I am finding the lag and UI becoming non-responsive when opening the big image is specific only to pngs. To test need to check for non-responsive behavior in UI, i.e. the attachment modal should be able to get closed when the image is loading. When testing, I found the delay in opening and closing the image is considerably more noticeable in pngs than in other formats. Also the zoom issue is also resolved when using backend provided 1024px jpg images for pngs. The advantage for using the 1024px jpg images is the conversion is already done by the backend. Please do help test and confirm the findings if the delay is noticeable only in pngs then the provided 1024px jpg images could provide solution for the issue.

melvin-bot[bot] commented 7 months ago

@mallenexpensify, @sobitneupane Whoops! This issue is 2 days overdue. Let's get this updated quick!

sobitneupane commented 7 months ago

Also the zoom issue is also resolved when using backend provided 1024px jpg images for pngs.

@jsdev2547 I don't think your above statement is true. I can reproduce the issue even with 1024px jpg image (I am quite surprised tbh). I thought the issue was due to the dimension but the issue is being reproduced even with 1024px image (may be due to cache?).

https://github.com/Expensify/App/assets/25876548/694d53e1-d400-46d8-b632-cdf6ca01cdd3

ghost commented 7 months ago

I can reproduce the issue even with 1024px jpg image

@sobitneupane When I reset the closing modal portion ie. remove setShouldLoadAttachment(false); from closeModal callback, then the closing lag occurs even with 1024px jpg image. To resolve the lag issue need to include setShouldLoadAttachment(false) in closeModal. I have prepared branch for testing, try testing zooming on 1024px jpg.

poc.webm

sobitneupane commented 7 months ago

Thanks for the update @jsdev2547. Your proposal will improve the experience but still the screen still freezes while opening the modal. The issue with zoom is still reproducible.

https://github.com/Expensify/App/assets/25876548/19cb5d4a-6eb2-40f1-bee9-6b2e62fbefd7

ghost commented 7 months ago

@sobitneupane I wanted to confirm if the screen freeze and the zoom freeze are related to the StyleUtils.getZoomSizingStyle https://github.com/Expensify/App/blob/64d22534b3a06fb516684198167ab2e263d99a0e/src/components/ImageView/index.tsx#L225 During testing, when I comment out the PressableWithoutFeedback component and leave only the Image component, the screen freeze disappears but so does the zoom. Perhaps the zoom functionality needs to be revised and use gesture handler technique i.e zoom using double tap gesture.

Please test the branch, to confirm if the screen freeze still exists. In the test branch, the image loading for png is using 1024x jpg, while the jpg are loading original size. The jpg still take few seconds to load but the screen does not freeze.

ShridharGoel commented 7 months ago

But won't it degrade the time required to upload file. I mean once I chose a heavy file,how long will it take to open upload modal?

Yes, it takes 5-6 seconds on using those changes. But I also noticed that there is some improvement by just using cropOrRotateImage with compress as 1 and no resizing (it seems to be changing the encoding which is improving the performance), and this opens the modal faster (1-2 seconds delay I think). This will maintain the quality as well.

sobitneupane commented 7 months ago

@jsdev2547 My bad. I cannot reproduce the issue I mentioned here with this changes (initial commit)

I have three major questions about your proposal though.

melvin-bot[bot] commented 7 months ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

melvin-bot[bot] commented 7 months ago

@mallenexpensify, @sobitneupane Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

mvtglobally commented 7 months ago

Issue not reproducible during KI retests. (First week)

sobitneupane commented 6 months ago

Bump @jsdev2547 https://github.com/Expensify/App/issues/38180#issuecomment-2084724901

ghost commented 6 months ago

@sobitneupane I am still trying to find reliable solution for when the user uploads the image attachment.

melvin-bot[bot] commented 6 months ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

melvin-bot[bot] commented 6 months ago

@mallenexpensify, @sobitneupane Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

melvin-bot[bot] commented 6 months ago

@mallenexpensify, @sobitneupane Still overdue 6 days?! Let's take care of this!

mallenexpensify commented 6 months ago

@sobitneupane , what do you propose are the next best steps here?

sobitneupane commented 6 months ago

We are still waiting for the better proposals.

@jsdev2547 Any update?

melvin-bot[bot] commented 6 months ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

sobitneupane commented 6 months ago

Waiting on proposals

melvin-bot[bot] commented 6 months ago

Upwork job price has been updated to $500

mallenexpensify commented 6 months ago

Thanks @sobitneupane , bumped to $500

sobitneupane commented 6 months ago

Waiting on Proposal