[$2000] Large image doesn't zoom well in Android and gets blurry

[kavimuru]

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

---

Action Performed:

1. Go to any chat and upload a very long/large image. Example image
2. In the Attachment preview, tap the image to zoom
3. Keep zooming until you'll see that the image is blurry

Expected Result:

Image should zoom to show a clear image instead of a blurry one

Actual Result:

Image content is not visible, and it is blurry.

Workaround:

unknown

Platform:

Where is this issue occurring?

• Android

Version Number: 1.2.29-6 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/203113972-1f4bb867-186f-4d5c-89e7-cd2761fa2c6b.mp4

https://user-images.githubusercontent.com/43996225/203113166-fb40d7b0-6bb3-418c-ad80-6bb22280e9ba.mov

Expensify/Expensify Issue URL: Issue reported by: @mananjadhav Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1668928921788569

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0100609c253c2769f4
• Upwork Job ID: 1597637165026803712
• Last Price Increase: 2022-12-08

[melvin-bot[bot]]

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

[aldo-expensify]

I remember this same images was causing this a long time ago: https://github.com/Expensify/App/issues/5193

I wonder if it ever got fixed and if this is a regression.

[mananjadhav]

@aldo-expensify I mentioned this in the slack too, it is a regression. I had tested this earlier and it did work at some point when working on another issue.

[melvin-bot[bot]]

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

[tjferriss]

I don't have an Android device. How would you suggest reproducing the bug?

[mananjadhav]

@tjferriss do you have an Android emulator or say Browserstack login? You can then reproduce this. This occurs only on Android. Second video on the issue body is from my Android device.

[aldo-expensify]

Maybe this will get fixed once the PR https://github.com/Expensify/App/pull/12648 gets re-done and we use fast-image.. maybe

[mananjadhav]

Well I tested this when #12648 was worked upon and it wasn't resolved. In fact I found this issue, while testing the same PR. We can still put this on hold, and come back again if you think that's the best option.

[aldo-expensify]

come back again if you think that's the best option.

No, given that you already saw that this doesn't get fixed, ignore my previous comment :P

[aldo-expensify]

Reproduced:

Web	Android

[tjferriss]

Thank you @aldo-expensify. This looks ready to go so I'm going to add the External tag.

[melvin-bot[bot]]

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

[melvin-bot[bot]]

Job added to Upwork: https://www.upwork.com/jobs/~0100609c253c2769f4

[melvin-bot[bot]]

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

[melvin-bot[bot]]

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

[markarmanus]

It seems that this is a common issue on android only

here is a few links of people complaining about it.

https://stackoverflow.com/questions/53402308/react-native-0-57-x-image-large-images-low-quality

https://github.com/facebook/react-native/issues/10470

You can consider it a Fresco problem or a react native problem, but regardless the solution is not feasable for a ticket of this size.

I spent the last 5 hours debugging this I hope my explanation helped !

[aldo-expensify]

Thanks @markarmanus

I was also having a look, what you said matches with explanation on what was happening when it was fixed in the past: https://github.com/Expensify/App/issues/5193#issuecomment-974397005

Here is the PR that was done to fix it: https://github.com/Expensify/App/pull/6442

Haven't looked further into why now it is not working, but I'm leaving the links here in case they are of use to someone.

[markarmanus]

Proposal

Problem:

The issue is happening on android, when you render an image that the original resolution is massive but you render it with a smaller resolution

Example:

// original image is 14500x3000
<Image style={{width: 300, height: 300, }} resize="contain" />

On IOS this image is going to stay at the original resolution when zooming in.

On Android the image gets downscaled to the resolution sepefied to the component so that results in a blury picture on zoom in.

As i stated above this is a recurring issue in the react native community. There is currently no solution for the issue.

I was able to create a solution. its somewhat hacky but it works. When rendering the image we render the image in its full resolution. then create a container of the image that scales it down to fit the screen.

Here is how to achieve this.

const imageTransformScale = Math.min(containerHeight / imageHeight, containerWidth / imageWidth);
this.setState({imageHeight, imageWidth, imageTransformScale});
// calculate a imageTransformScale

We also git rid of this code which is attempting to make the resolution the size of the screen.

const aspectRatio = Math.min(containerHeight / imageHeight, containerWidth / imageWidth);

if (imageHeight > imageWidth) {
       imageHeight *= aspectRatio;
} else {
       imageWidth *= aspectRatio;
}

Then Contain the image in a view that uses that scale

<View style={{transform: [{scale: this.state.imageTransformScale}]}}>
     <ImageZoom
         ...
     </ImageZoom>
</View>

We also need to extend ImageZoom with these props

cropWidth={this.props.windowWidth / this.state.imageTransformScale}
cropHeight={this.state.containerHeight / this.state.imageTransformScale}

Here is the result in screen shots.

Behaves exactly the same on ios

The final solution includes a few other line changes but this is mostly it.

[tjferriss]

@marcaaron and @mananjadhav what do we think of @markarmanus proposal?

[mananjadhav]

I am not 100% convinced with @markarmanus's proposal.

When rendering the image we render the image in its full resolution. then create a container of the image that scales it down to fit the screen.

Also I didn't understand this statement. @markarmanus if you check this earlier, it was solved and it worked fine. I think with multiple refactors to the image component, we've lost/remove the change.

[markarmanus]

@mananjadhav If you notice in the last PR we had to fix this issue https://github.com/Expensify/App/pull/6442

In the code It has this in the calculate image size (resolution)

Notice how it does not affect much of the height of the image. Which is why it redners at a really high resolution as in the picture he has in the PR

But if you notice the picture renders zoomed in already. That because when you pass a big resolution the component it tries to resize the image to a fit a 6000 pixel heigh window. and the phone is obv not 6000 so you see a very zoomed in picture. if you now go into the code base and manually change the resolution in line 161 in ImageView/index.native.js this will happen.

looking back at the history of this i cant seem to find any time where we had a solution that does not have a high resolution render.

Also if you take a look at the links i provide above it shows other people having the same issue.

I hope that helps !

[marcaaron]

@markarmanus thanks for the investigation, but I think we moved into the "workaround" proposal a bit prematurely.

I'm a little concerned about these comments:

As i stated above this is a recurring issue in the react native community. There is currently no solution for the issue. I was able to create a solution. its somewhat hacky but it works.

Our general approach to fixing bugs is to address issues at their root cause. Workarounds are sometimes accepted (usually not) but never without a clear explanation of the problem.

So at a high level, it's possible we might accept this solution (I haven't reviewed it) though unlikely given the lack of explanation into the root cause and focus on the workaround. Let's maybe pause until we have a clear explanation of why it happens. If it's a problem with react native then we should be starting with any open community issues, links to problem code, etc. I see you've added some links here - but I think it would be best to summarize what the exact problem is before proceeding and make sure everyone is on the same page.

[markarmanus]

@marcaaron I cant seem to dig out the cause reason for it. but its a common issue on android. If you create a react native project from scratch. Render the image above and then zoom in it will have the same problem.

A good solution would be to use a Webview component. and we get then git rid of the zoom library complelty which is out dated

[melvin-bot[bot]]

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

[mananjadhav]

Waiting for proposals.

[melvin-bot[bot]]

Upwork job price has been updated to $2000

[markarmanus]

@mananjadhav @marcaaron I finally was able to dig out the root cause. Basiaclly Android uses Fresco to render images. Now fresco has multiple methods to render an image https://frescolib.org/docs/scaletypes.html But there is a hard limit on the resolution of a picture to render when resizing. its hard coded right here

Here is a library that is written in Native code (for android) that attempts to solve the issue by removing Fresco all together https://github.com/davemorrissey/subsampling-scale-image-view

Here is an old request to modify the value https://github.com/facebook/fresco/issues/916

But they dont allow it because it could cause memory issues.

Now to solve the issue we have 2 options:

Solution 1: (i made a more detailed proposal above on it)

Render the image in full resolution and scale it down to fit within the container. (easy but somewhat hacky solution) we also need to be careful to not render image too big to crash phone (code already exists in the app to prevent that)

Solution 2: (What i think is better)

Replace the component with a Webview component. This basically uses a browser engine to render the image which would allow zoom in much better preformance and consistent experience on both devices.

[markarmanus]

Let me know if you want me to demo Solution 2 or have any questions on it.

[marcaaron]

Thanks. No need to demo anything yet. I need to take this to the team and discuss in Slack before we can proceed further. @mananjadhav since you're the assigned C+ do you want to take a stab at summarizing the proposals?

[marcaaron]

@mananjadhav what do you think about my previous comment? Are you able to summarize this so we can take it to Slack?

[mananjadhav]

@marcaaron I'll summarize this and post it on Slack today.

[tjferriss]

@mananjadhav did you start a discussion in Slack today? I don't see anything. Can you tag everyone there when you do so we can follow along?

[mananjadhav]

Apologies for the delay. A bit under the weather. I am going to post it in a few hours.

[aswin-s]

This bug is caused due to an incorrect image size calculation on

https://github.com/Expensify/App/blame/main/src/components/ImageView/index.native.js#L75

const aspectRatio = Math.min(containerHeight / imageHeight, containerWidth / imageWidth);

imageHeight *= aspectRatio;
imageWidth *= aspectRatio;

For the test image (875px x 14894px ) above lines will max out the height of the image at window height, as it uses Math.min. Resizing an extra long image like this to window height will obviously reduce the image detail, resulting in a blurred out view. I think above modification was made to fix an issue where smaller images were not getting fit to window width. ( Related issues https://github.com/Expensify/App/pull/8804, https://github.com/Expensify/App/pull/8285)

That fix seems to be wrong as window width and height shouldn't influence the calculation of the native image dimensions. If we really want to scale the image preview to fit the window bounds, it should be performed via ImageZoom component which has a centerOn prop.

My proposal is to revert the changes to calculateImageSize so that it is not dependent on windowSize, but still limit the dimensions to be within maxDimensions.

calculateImageSize() {
        if (!this.props.url) {
            return;
        }
        ImageSize.getSize(this.props.url).then(({width, height}) => {
            let imageWidth = width;
            let imageHeight = height;
            const containerWidth = Math.round(this.props.windowWidth);
            const containerHeight = Math.round(this.state.containerHeight);

            // Resize the image to max dimensions possible on the Native platforms to prevent crashes on Android. To keep the same behavior, apply to IOS as well.
            const maxDimensionsScale = 11;
            const maxWidth = this.props.windowWidth * maxDimensionsScale;
            const maxHeight = this.props.windowHeight * maxDimensionsScale;
            if (imageWidth > maxWidth || imageHeight > maxHeight) {
                const scale = Math.min(maxWidth / imageWidth, maxHeight / imageHeight);
                imageWidth *= scale;
                imageHeight *= scale;
            }

            this.setState({imageHeight, imageWidth});
        });
    }

Then use centerOn prop in ImageZoom component to scale and position the original image so that it fits the window on initial load.

Also note that we had switched from Fresco to Glide in #5193, so issue is not related to Fresco downsampling.

[mananjadhav]

Started a thread here

[tjferriss]

@mananjadhav will be summarizing the conclusions from the Slack discussion tomorrow.

[cristipaval]

Do we really want to handle very big images? What is the benefit? It is well known that Whatsapp resizes the images and makes them smaller. They don't want to handle big images, there are other original sized photo transfer services like WeTransfer, Airdrop etc. cc @JmillsExpensify for this

If we still want to support very big images, I think we should wait for this PR to get merged and then try to solve this issue. I know you @mananjadhav tested already with fast-image integrated (as you mentioned here). But I wouldn't dismiss Glide wrapped up by fast-image yet. I would put this issue on hold until fast-image is reintegrated, and then try solving this issue by leveraging Glide library with different configs.

[marcaaron]

Gonna take this one internal. I think the complexity is fairly high and does not feel like a super urgent bug as it is only affecting very large images (unless I am mistaken on that). There are a lot of good ideas floating around, but I think we need to step back and decide what kind of solution we want.

[JmillsExpensify]

Circling back to this issue, I think we should decide what the internal next steps are, as this bug just hit the 4 weeks outstanding mark. Here's my two cents: Given that this bug only seems to affect large images, and related, we have several updates in the pipeline for image handling in general, I favor putting a hold on this until we're through @Beamanator's image improvements. Those are much higher value anyway. At that point we can circle back to this consideration and decide what the best next step is. Thoughts?

[marcaaron]

That seems reasonable to me. I don't think we have a clear idea of how to proceed here at the moment.

[JmillsExpensify]

Filed this issue in the N7 project, along with the larger image improvement issues. More context in this Slack thread.

[cristipaval]

Not overdue, Melvin.

[marcaaron]

Still on HOLD

[tjferriss]

Still on HOLD. Should this go to monthly at this point or do we expect it to come off hold in a few weeks?

[JmillsExpensify]

The core image improvements are starting to wind down. I would keep it weekly for a couple more weeks before changing to monthly.

[marcaaron]

Seems like this one is still on HOLD. Though - once off HOLD, I think it would be good to do a quick sync on the root cause. I haven't looked at this in a while and still lacking generally clarity on what the solution could be.

[tjferriss]

We're still on hold. @JmillsExpensify what exactly should I be tracking to indicate this can be taken off hold?

[JmillsExpensify]

Good question. I would think we might even be able to take this off hold now? cc @Beamanator @trjExpensify

[trjExpensify]

I think we were specifically holding on image caching, and the mobile side of that is done, it's only web that remains. This is an Android issue, so I think it can come off-hold. Agree with @marcaaron sentiment here as a next step.

[tjferriss]

Thanks for the context. I've taken the issue off hold. @marcaaron could you kick of this sync on the root causes? Perhaps we can jump back into the original thread for this discussion.