[$1000] [HOLD for payment 2023-08-28] [HOLD for payment 2023-08-16] Find solution to low resolution large images on Android, and the Canvas crash

[Julesssss]

Problem

While attempting to solve an issue with low-quality Android images, we disabled hardware acceleration and introduced a major performance regression.

@terrysahaidak noticed this while running performance testing and outlined a plan for re-enabling and trying an alternative fix.

Solution

• Revert the PR for an immediate performance gain
• Investigate the possibility of disabling hardware acceleration for images
• Figure our our long term solution: Either accept lower quality images, or find an alternative

Example big image

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01fe4797a997c161ea
• Upwork Job ID: 1696867422937661440
• Last Price Increase: 2023-08-30

[mountiny]

cc @fabioh8010 @ArekChr

[ArekChr]

Hi, I'm working on an image transform that will allow the display of large images that will allow re-enabling hardware acceleration in this PR

[Julesssss]

Awesome, look forward to this @ArekChr 👍

[Julesssss]

My first step is to revert the original PR, which I'll do this afternoon after finishing higher priority work.

[terrysahaidak]

@ArekChr I also looking into this, and i found that if we render the image as is with fast image and use scale it down with transform we can get pretty decent performance. I used Reanimated zoom component, fast image (our Image component) and just use scale as scale.value * 0.1:

gallery_long_image_test.mov

[Julesssss]

First step was taken here: revert the PR that disabled hardware acceleration

[ArekChr]

@terrysahaidak looks like downscale is working great in your example 👌 What was the reason for changing zoom-in to the custom reanimated component? Asking because I have some issues with displaying large images with my changes in react-native-fast-image I can't get it working well with react-native-image-pan-zoom

[terrysahaidak]

@ArekChr react-native-image-pan-zoom is built with PanResponder and is deprecated by the authors, the performance is not great and it feels very janky on android, even pinch gesture is not registered all the time for me. We had this bug #17720 we had to disable switching pages because of it. The new approach will have better performance, we will be able to implement all our own interactions with all the animations running on UI thread.

But we still need to downscale the image, because when a number of pixels is too high when you transform scale image down it can also lag a bit, because basically, we try to render too many pixels.

[ArekChr]

Does the scale-down that you mention resolve that issue? How does it work with small images? The factor of scale 0.1 in the example will be calculated automatically?

Regarding downscale, in the main branch, the' fast-image' framework has disabled transformations to display the image's original size. What size of the image are you trying to display?

Maybe reverting these changes will reduce lags while transforming large images.

Also, I implemented a custom image resizer for fast-image, I'm not sure, but maybe this could help with reducing lags when connecting it with your reanimated component for zooming-in Expensify/react-native-fast-image#6

[terrysahaidak]

I tried the image zoom component I implemented with Reanimated/Gesture handler + @ArekChr Fast Image PR. In this example, i render the image as is and scale it down using transform: [{ scale }]

It works pretty well on low-end Samsung a12 ($150 phone):

REC_20230525_110417.mov

The logic in this zoom component is currently implemented to handle regular-size images, but i needs just a few adjustments to make it work properly with these long images - so you'll be able to zoom the image, and only pan it when edges are outside the viewport.

[mountiny]

Thats great, thanks Terry!

[melvin-bot]

This issue has not been updated in over 15 days. @Julesssss, @ArekChr eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

[Julesssss]

Hey @terrysahaidak, you r work looks promising. Is this something we can continue working on?

[mountiny]

Arek is ooo for couple of weeks now I believe.

@terrysahaidak would you be able to provide an update, thanks!

[chrispader]

Currently working on finishing @terrysahaidak 's work. Created a draft PR here

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.3.55-8 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• fix: invoke useWindowDimensions #24353

If no regressions arise, payment will be issued on 2023-08-28. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

• External issue reporter
• Contributor that fixed the issue
• Contributor+ that helped on the issue and/or PR

For reference, here are some details about the assignees on this issue:

• @chrispader does not require payment (Contractor)
• @ArekChr does not require payment (Contractor)

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

• Merged PR within 3 business days of assignment - 50% bonus
• Merged PR more than 9 business days after assignment - 50% penalty

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~01fe4797a997c161ea

[melvin-bot]

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

[melvin-bot]

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

[Julesssss]

Hey @jliexpensify, please complete the BZ checklist. I think no payment is required here.

[jliexpensify]

@ArekChr @chrispader - this wasn't a regression/no linked PR's or discussions right?

Any QA steps you'd like to recommend, so I can get a GH created?

[Julesssss]

this wasn't a regression/no linked PR's or discussions right?

@jliexpensify nah, not a regression

[jliexpensify]

@Julesssss am I going crazy or did the checklist not appear in this GH?

[Julesssss]

Yeah, I only see this one...

[jliexpensify]

Really weird - the only other thing from the checklist (from memory) are the QA steps, if anyone wants to recommend some? I can make a GH for testrail if needed.

Otherwise, we can close this!

[jliexpensify]

Bump @chrispader @ArekChr - any QA steps for me to document in a GH? Cheers.

[chrispader]

I'd say the same as in the fixing PR:

On iOS/Android:

1. Go to a report/chat with a large attachments in it (example image)
2. Open an attachment image
3. Make sure the app doesn't crash
4. Make sure gestures are smooth and easy to use
5. Make sure low resolution large images are displayed correctly and zooming works.
6. Make sure other attachment types (PDF, etc.) still work

[jliexpensify]

Cheers, GH created. No payment needed so closing!

[sobitneupane]

I did review the associated PR. Requested $1000 for PR review in newDot.

cc: @jliexpensify

[jliexpensify]

Just had a look and you are correct cc @Julesssss

Payment Summary for Jason Mills:

• C+: @sobitneupane $1000 (New.Dot): reviewed PR

[JmillsExpensify]

$1,000 payment approved via NewDot based on BZ summary.