Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
kavimuru opened this issue Nov 21, 2022 · 71 comments
Closed

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

kavimuru opened this issue Nov 21, 2022 · 71 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Monthly KSv2

Comments

@kavimuru
Copy link

kavimuru commented Nov 21, 2022

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 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:

az_recorder_20221121_114833.1.mp4
android-image-blurry.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
@kavimuru kavimuru added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Nov 21, 2022
@melvin-bot
Copy link

melvin-bot bot commented Nov 21, 2022

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

@aldo-expensify
Copy link
Contributor

I remember this same images was causing this a long time ago: #5193

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

@mananjadhav
Copy link
Collaborator

@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 melvin-bot bot added the Overdue label Nov 24, 2022
@melvin-bot
Copy link

melvin-bot bot commented Nov 25, 2022

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

@tjferriss
Copy link
Contributor

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

@melvin-bot melvin-bot bot removed the Overdue label Nov 28, 2022
@mananjadhav
Copy link
Collaborator

@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
Copy link
Contributor

Maybe this will get fixed once the PR #12648 gets re-done and we use fast-image.. maybe

@mananjadhav
Copy link
Collaborator

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
Copy link
Contributor

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
Copy link
Contributor

Reproduced:

Web Android
Screen Shot 2022-11-28 at 2 52 43 PM

@tjferriss
Copy link
Contributor

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

@tjferriss tjferriss added the External Added to denote the issue can be worked on by a contributor label Nov 29, 2022
@melvin-bot
Copy link

melvin-bot bot commented Nov 29, 2022

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

@melvin-bot melvin-bot bot changed the title Large image doesn't zoom well in Android and gets blurry [$1000] Large image doesn't zoom well in Android and gets blurry Nov 29, 2022
@melvin-bot
Copy link

melvin-bot bot commented Nov 29, 2022

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

@melvin-bot
Copy link

melvin-bot bot commented Nov 29, 2022

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Nov 29, 2022
@melvin-bot
Copy link

melvin-bot bot commented Nov 29, 2022

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

@markarmanus
Copy link
Contributor

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

facebook/react-native#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
Copy link
Contributor

aldo-expensify commented Nov 30, 2022

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: #5193 (comment)

Here is the PR that was done to fix it: #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
Copy link
Contributor

markarmanus commented Nov 30, 2022

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.

image
image
image

Behaves exactly the same on ios

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

@tjferriss
Copy link
Contributor

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

@mananjadhav
Copy link
Collaborator

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.

@cristipaval
Copy link
Contributor

Not overdue, @ArekChr is going to open an App PR soon.

@melvin-bot melvin-bot bot removed the Overdue label Mar 17, 2023
@melvin-bot melvin-bot bot added the Overdue label Mar 27, 2023
@cristipaval
Copy link
Contributor

cristipaval commented Mar 27, 2023

We have our fork ready, we now have to add it to the App. @ArekChr is working on it. We're facing some challenges when building the App with the fork. We'll ask for the team's help if we don't figure it out soon.

@melvin-bot melvin-bot bot removed the Overdue label Mar 27, 2023
@ArekChr ArekChr mentioned this issue Mar 27, 2023
55 tasks
@melvin-bot melvin-bot bot added the Reviewing Has a PR in review label Mar 27, 2023
@mananjadhav
Copy link
Collaborator

I should be free by tomorrow or day after. Do let me know if I can help in anyway for building the App with the fork.

@cristipaval
Copy link
Contributor

We just got past that thing. PR is going to be ready soon, @ArekChr is taking screen recordings now.

@ArekChr
Copy link
Contributor

ArekChr commented Mar 30, 2023

There is a blocker to finish QA checklist tests for mWeb safari and chrome #2499

@melvin-bot melvin-bot bot added Monthly KSv2 and removed Weekly KSv2 labels Apr 24, 2023
@MelvinBot
Copy link

This issue has not been updated in over 15 days. @mananjadhav, @tjferriss, @cristipaval, @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!

@cristipaval
Copy link
Contributor

Closing this one, as the fix hit production a few weeks ago.

@mananjadhav
Copy link
Collaborator

@cristipaval @tjferriss I did the C+ review and testing on this one. Payout is pending for that .

@cristipaval cristipaval removed Reviewing Has a PR in review Internal Requires API changes or must be handled by Expensify staff Planning Changes still in the thought process labels Apr 24, 2023
@cristipaval
Copy link
Contributor

cristipaval commented Apr 24, 2023

@cristipaval @tjferriss I did the C+ review and testing on this one. Payout is pending for that .

Oh sorry, I missed that! @tjferriss , should we re-open this one and add Awaiting Payment label?

@tjferriss
Copy link
Contributor

@mananjadhav I've sent you a request via Upworks here: https://www.upwork.com/jobs/~01ae27c2191acb5934. I also included the reporting bonus in the amount.

@tjferriss tjferriss reopened this Apr 24, 2023
@tjferriss tjferriss added the Awaiting Payment Auto-added when associated PR is deployed to production label Apr 24, 2023
@mananjadhav
Copy link
Collaborator

Accepted @tjferriss

@tjferriss
Copy link
Contributor

The payment has been sent.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Monthly KSv2
Projects
None yet
Development

No branches or pull requests