-
Notifications
You must be signed in to change notification settings - Fork 3
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
Conversation
@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. |
@mountiny After testing the changes in a clean project, the images are displayed correctly. |
@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 |
@mountiny I will continue working on this task |
@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 Unfortunately, I couldn't resolve the issues by combining the current solution with 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.mp4fast-image-with-custom-transformer.mp4 |
Started a thread here |
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! :) |
What's the status on this PR? I think this PR could effectively fix another bug with large images. Expensify/App#24337 (comment) |
@chrispader Hey, it looks like we stopped using this fork in the Expensify app but didn't find a reason yet. |
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) |
@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 |
sorry i meant merging upstream |
@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. |
i'll work on the upstream PR tmrw then! 👍🏼🤠 |
@mountiny i asked in a previous If so, we might not want to put too much effort into fixing this in the upstream repo. |
Anyway, i still created an upstream PR with the changes from @ArekChr 🚀 |
Nice, thank you, given we do not want to maintain the fork, I will close this PR |
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