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

Implement Bitmap Transformation with Size Limit #6

Closed
wants to merge 1 commit into from

Conversation

ArekChr
Copy link

@ArekChr ArekChr commented May 16, 2023

This pull request introduces a new transformation method for handling Bitmap objects with a size limit. It provides an implementation that scales down the input Bitmap if its byte count exceeds the specified threshold.

This pull request addresses an issue encountered when dealing with large images on Android devices with hardware acceleration enabled. The issue manifests as a java.lang.RuntimeException: Canvas: trying to draw too large(xxx,xxx,xxx bytes) bitmap error. By introducing a new transformation method, this pull request offers a solution to display large images while still accommodating the specified size limit.

Changes Made:

Added a new class ResizeTransformation, transform, to perform the Bitmap transformation.
The transform method uses a BitmapPool, the transform Bitmap, and output width and height as parameters.
If the transform Bitmap's byte count exceeds the predefined MAX_BYTES threshold:
A scaling factor is calculated based on the square root of the ratio between MAX_BYTES and the transform Bitmap's byte count.
The new width and height are computed by multiplying the output width and height by the scaling factor.
The Bitmap.createScaledBitmap() method creates a scaled Bitmap with new dimensions.
The scaled Bitmap is returned as the result of the transformation.
If the transform Bitmap's byte count is within the MAX_BYTES threshold:
The original toTransform Bitmap is returned as it is.

Overall, this pull request aims to provide a reusable and efficient Bitmap transformation implementation that handles size limitations, allowing for optimized memory usage when working with large images.

Related issues:
Expensify/App#17427
Expensify/App#18963
Expensify/App#24337

@mountiny
Copy link

@ArekChr is there anyone specific you would want to ask for a review and advice on this PR? We can ask in SWM too if there is anyone who you think could help.

@ArekChr
Copy link
Author

ArekChr commented May 22, 2023

@mountiny After testing the changes in a clean project, the images are displayed correctly.
But the main problem I have with the integration in NewDot is that I can not display the image correctly.
Specifically, the current sizing calculation logic and the image pan zoom framework are hindering the proper display of the image.

@mountiny
Copy link

@ArekChr Whats the latest on this one then, should you focus on resolving this issue before continuing with the performance tests? I believe we have this PR which is on hold for this PR Expensify/App#18997

@ArekChr
Copy link
Author

ArekChr commented May 23, 2023

@mountiny I will continue working on this task

@ArekChr
Copy link
Author

ArekChr commented May 24, 2023

@mountiny I have implemented the PoC of the custom pan and pinch view using Reanimated and Gesture Handler. The images retain their quality and work well with the changes I made for react-native-fast-image (in this PR) and the react-native-fast-image upstream. The zoom functionality is working, but it currently zooms based on the top of the image instead of the centre. Additionally, there is no limit to moving outside the container, regardless of where the pinching gesture is made.

Unfortunately, I couldn't resolve the issues by combining the current solution with react-native-image-pan-zoom, used in NewDot. I also tried combining it with react-native-reanimated-zoom, and my attempts have not yielded any positive results so far. I must admit that I could be more of an expert in handling gestures with Reanimated.

It would be a good idea to include someone from SWM to implement a custom image zoom component specifically designed for large images or investigate if that's needed if the current solution is possible to fix.

I'm sending the results of the fast-image upstream and with image transform for reference.

fast-image-upstream.mp4
fast-image-with-custom-transformer.mp4

@mountiny
Copy link

Started a thread here

@chrispader
Copy link

chrispader commented Jul 12, 2023

How is this number determined? What is the threshold we want have? cc @ArekChr @Julesssss

private final double MAX_BYTES = 25000000.0;

@ArekChr
Copy link
Author

ArekChr commented Jul 20, 2023

How is this number determined? What is the threshold we want have? cc @ArekChr @Julesssss

private final double MAX_BYTES = 25000000.0;

The MAX_BYTES threshold is determined based on the maximum size of Bitmap Android can handle without throwing an error, which generally depends on the device's capabilities. This value, however, is not an absolute limit, and it can vary between different devices.

The threshold value of 25,000,000 bytes (about 24 MB) because it's a safe average size that should work well on all devices. This helps us avoid an error when an image is too big but still allows us to use clear, high-quality images.

We can change this number, if needed, based on what a specific device can handle or what our app needs. It's about finding a balance between how clear we can make an image and how much memory we are okay with using to process that image.

@chrispader
Copy link

How is this number determined? What is the threshold we want have? cc @ArekChr @Julesssss

private final double MAX_BYTES = 25000000.0;

The MAX_BYTES threshold is determined based on the maximum size of Bitmap Android can handle without throwing an error, which generally depends on the device's capabilities. This value, however, is not an absolute limit, and it can vary between different devices.

The threshold value of 25,000,000 bytes (about 24 MB) because it's a safe average size that should work well on all devices. This helps us avoid an error when an image is too big but still allows us to use clear, high-quality images.

We can change this number, if needed, based on what a specific device can handle or what our app needs. It's about finding a balance between how clear we can make an image and how much memory we are okay with using to process that image.

ah okay, thanks for the explanation! :)

@chrispader
Copy link

Hey @ArekChr @mountiny ! :)

What's the status on this PR?

I think this PR could effectively fix another bug with large images. Expensify/App#24337 (comment)

@ArekChr
Copy link
Author

ArekChr commented Nov 15, 2023

@chrispader Hey, it looks like we stopped using this fork in the Expensify app but didn't find a reason yet.
edit:
I think we can merge this PR and test large images over 9k pixels on android devices.

@mountiny
Copy link

I think in general we just prefer to not use forks as its time consuming to manage them. Would you like to test it in this branch only and then raise a PR upstream if we get a fix?

@chrispader
Copy link

I think in general we just prefer to not use forks as its time consuming to manage them. Would you like to test it in this branch only and then raise a PR upstream if we get a fix?

@mountiny do you mean @ArekChr or me? @ArekChr if you have time to work on this upstream? I can also take over if you're busy.

We're gonna have to HOLD another PR on this change, so we'd like to merge this as fast as possible. Expensify/App#28159 (comment)

@mountiny
Copy link

@chrispader We can merge this but we do not use this fork anymore, do you plan to reintroduce the fork in that PR?

@chrispader
Copy link

@chrispader We can merge this but we do not use this fork anymore, do you plan to reintroduce the fork in that PR?

no, if we don't use the fork anymore we should rather wait for the fix upstream.

The problem with the other PR is, that we're switching over to the image transformation component i implemented a while back for the attachment carousel, but it crashes when the images are too large...

and we don't want to use the component for the "send attachment preview", when it still crashes

@chrispader
Copy link

We're gonna have to HOLD another PR on this change, so we'd like to merge this as fast as possible.

sorry i meant merging upstream

@mountiny
Copy link

@chrispader Arek is a C+ I am not sure if he has time for this to get it done urgently. So if you or Margelo has bandwidth to create upstream PR that would be great.

@chrispader
Copy link

i'll work on the upstream PR tmrw then! 👍🏼🤠

@chrispader
Copy link

@mountiny i asked in a previous expensify-open-source thread if we are still pursuing to switch over to expo-image.

If so, we might not want to put too much effort into fixing this in the upstream repo.

@chrispader
Copy link

Anyway, i still created an upstream PR with the changes from @ArekChr 🚀

@mountiny
Copy link

Nice, thank you, given we do not want to maintain the fork, I will close this PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants