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

use new downsamplemode to disable downsampling #45078

Closed
wants to merge 3 commits into from

Conversation

sunnylqm
Copy link
Contributor

@sunnylqm sunnylqm commented Jun 19, 2024

Summary

Starting from RN version 0.57, a change in Fresco caused the option that was supposed to disable downsampling to lose its effect and become unconfigurable. This resulted in large images appearing blurry on Android devices. The latest Fresco version introduces a new downsample mode, allowing us to completely disable downsampling again (and users can modify the configuration themselves), restoring the behavior prior to version 0.57.

Fixes #21301

Changelog:

[Android] [Changed] - Set fresco DownsampleMode to NEVER to disable downsampling on android

Test Plan

Test a large remote image like facebook/fresco#2397 (comment)
With default config or set downsampleMode to AUTO/ALWAYS, it should be downsampled/blurred like how it behaves today.
Change it to NEVER it should NOT be downsampled.

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels Jun 19, 2024
Copy link

@clytras clytras left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sunnylqm Thank you for your effort putting on resolving this long standing issue.

I'll try to test this on my end.

…react/modules/fresco/FrescoModule.java

Co-authored-by: onetrace-abdullah <117074905+onetrace-abdullah@users.noreply.github.com>
@Abbondanzo
Copy link
Contributor

Abbondanzo commented Jun 20, 2024

I added a comment #21301 (comment) to the issue you linked, but I wanted to call out an undocumented feature I added recently in #44803 that lets you upscale an image before it gets passed to the hardware layer. It's called resizeMultiplier and will upscale the desired image dimensions of the resized image before handing off to Android's hardware layer to do the remainder of scaling. Out-of-curiosity, is downsampling causing issues for you or is changing scale as a whole causing issues? I'd recommend using resizeMethod="scale" if you're losing image quality or resizeMethod="resize" if the image has aliasing issues. If possible, could you provide an Expo snack or screenshots of the issues you're seeing? This way, we can rule out one or the other.

@sunnylqm
Copy link
Contributor Author

@Abbondanzo one example of downsampling issue facebook/fresco#2397 (comment) and it's not even rn code

@onetrace-abdullah
Copy link

I've been able to test this PR, and it seems to work great with PNGs, but JPEGs are still blurry and pixelated compared to iOS:

PNG (before) PNG (after)
before after
JPG (before) JPG (after) JPG (iOS)
before after ios

All images are being loaded from the file system, and not from a remote URL.

Unfortunately I wasn't able to test the resizeMultiplier prop as I couldn't run the app with react-native@next due to errors related to AGP

@onetrace-abdullah
Copy link

Here's a better example:

Android (JPG) iOS (JPG)
android ios

@facebook-github-bot
Copy link
Contributor

@Abbondanzo has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@Abbondanzo
Copy link
Contributor

@sunnylqm could you update the PR body with a changelog and basic test plan? For the test plan, even if it's just code a JS code snippet, that's perfectly fine. Expo snacks are also helpful.

@onetrace-abdullah those are great examples. What are the source image dimensions you are using and/or do you have some basic image assets you could share as part of the test plan? Ideally, we can integrate screenshot tests for behavior this significant and I'd like to replicate that level of blurriness.

@onetrace-abdullah
Copy link

@Abbondanzo here are the exact images I used, the PNG image is from https://github.com/clytras/react-native-fresco

@Abbondanzo
Copy link
Contributor

Abbondanzo commented Jun 24, 2024

@Abbondanzo here are the exact images I used, the PNG image is from https://github.com/clytras/react-native-fresco

@onetrace-abdullah thanks for sharing. While I tested these changes, there was one issue that I kept coming across: bitmap memory exhaustion. The largest of your images, at 6740x4761, tries to consume over 128MB of memory (6740x4761x4bytes per pixel) and this causes the RNTester app to fatally crash, even on newer devices. It didn't matter the resizeMethod, for without downsampling the image during decoding steps, that memory is still exhausted.

This maximum limit is enforced by the canvas, as anything over 100MB will cause a fatal exception to be thrown. Fresco has some experimental settings, namely setMaxBitmapSize and setDownsampleIfLargeBitmap, when applied will only downsample JPEG images.

This behavior is less-than-ideal, but may warrant corrective downsampling in a small handful of cases. I don't want to introduce any regressive behavior so I'm going to reach out to the Fresco team to see if we could potentially enable downsampling if the image size is larger than 100MB. This check is already being made for GIFs, but the image will not be displayed in these cases, and it's probably better to have an image at lower quality than it is to have no image at all (or worse, crashes).

@Abbondanzo
Copy link
Contributor

Hey everyone, just wanted to provide an update here: I've spoken with the Fresco team and I want to expand the experimental setDownsampleIfLargeBitmap flag to support image types other than JPEG before we ship this, making it the default behavior. If anyone is interested expediting this change, I'm open to making this change opt-in by putting it behind a feature flag (documentation here). I don't have a timeline on making changes to Fresco as other priorities have taken hold for me at the moment, but I will do my best to provide updates here as they become applicable.

@zell180
Copy link

zell180 commented Jul 4, 2024

Hey everyone, just wanted to provide an update here: I've spoken with the Fresco team and I want to expand the experimental setDownsampleIfLargeBitmap flag to support image types other than JPEG before we ship this, making it the default behavior. If anyone is interested expediting this change, I'm open to making this change opt-in by putting it behind a feature flag (documentation here). I don't have a timeline on making changes to Fresco as other priorities have taken hold for me at the moment, but I will do my best to provide updates here as they become applicable.

Hi, we're experimenting this issue on our project with Expo 50 and react-native 0.73.2
How we can disable downsampling at this time without waiting for release? there is a release date for the fix?
Thank you

@Abbondanzo
Copy link
Contributor

Hey everyone, just wanted to provide an update here: I've spoken with the Fresco team and I want to expand the experimental setDownsampleIfLargeBitmap flag to support image types other than JPEG before we ship this, making it the default behavior. If anyone is interested expediting this change, I'm open to making this change opt-in by putting it behind a feature flag (documentation here). I don't have a timeline on making changes to Fresco as other priorities have taken hold for me at the moment, but I will do my best to provide updates here as they become applicable.

Hi, we're experimenting this issue on our project with Expo 50 and react-native 0.73.2 How we can disable downsampling at this time without waiting for release? there is a release date for the fix? Thank you

@zell180 you will have to construct a MainPackageConfig (source) with a new Fresco pipeline config fed into that. In your application class, you'd build the MainReactPackage with that new config similarly to the snippet found here: facebook/fresco#2397 (comment). Except, you'd want to use the new downsampling mode from Fresco 3.2.0 so your image pipeline would look like so:

ImagePipelineConfig frescoConfig = ImagePipelineConfig
    .newBuilder(context)
    .setDownsampleMode(DownsampleMode.NEVER)
    .build();

@zell180
Copy link

zell180 commented Jul 28, 2024

Hey everyone, just wanted to provide an update here: I've spoken with the Fresco team and I want to expand the experimental setDownsampleIfLargeBitmap flag to support image types other than JPEG before we ship this, making it the default behavior. If anyone is interested expediting this change, I'm open to making this change opt-in by putting it behind a feature flag (documentation here). I don't have a timeline on making changes to Fresco as other priorities have taken hold for me at the moment, but I will do my best to provide updates here as they become applicable.

Hi, we're experimenting this issue on our project with Expo 50 and react-native 0.73.2 How we can disable downsampling at this time without waiting for release? there is a release date for the fix? Thank you

@zell180 you will have to construct a MainPackageConfig (source) with a new Fresco pipeline config fed into that. In your application class, you'd build the MainReactPackage with that new config similarly to the snippet found here: facebook/fresco#2397 (comment). Except, you'd want to use the new downsampling mode from Fresco 3.2.0 so your image pipeline would look like so:

ImagePipelineConfig frescoConfig = ImagePipelineConfig
    .newBuilder(context)
    .setDownsampleMode(DownsampleMode.NEVER)
    .build();

I’ve try a lot but without results :(
There are some news about a fixed release?

@zell180
Copy link

zell180 commented Aug 30, 2024

Hi everyone, there is some update about this pr?

@Abbondanzo
Copy link
Contributor

I've picked this back up and have made some changes to Fresco recently that I think will strike a safer balance: facebook/fresco#2787. Rather than disable downsampling globally for every application, I am adding a new resizeMethod to Image (likely going to call it never) that can disable downsampling on a per-image basis.

Disabling downsampling by default is risky because it can cause runtime exceptions when trying to load larger images, especially when trying to render images that were captured in full resolution from the camera. This limitation comes from Android: it has a hard-coded 100MB limit on the bitmaps it tries to render. For other use cases, such as galleries or image pickers, loading full resolution images with downsampling disabled frequently causes OOM problems--especially on lower-end devices.

I hope this compromise can solve the issues you all have been running into, and I apologize for the lack of updates. I'll keep posting back here with each of the relevant changes to get this working.

@Abbondanzo
Copy link
Contributor

Hey all, providing another update as promised. The changes from facebook/fresco#2787 have made their way into Fresco v3.3.0 and are being added as part of #46864. Subsequently, I'm adding a new resizeMethod type of never to #46866. This will disable downsampling on a per-image basis as I've shared here before. I'm adding a small test case in RNTester so you can experiment with the different resizeMethod against a large image. Once these land, I'll update the documentation over at reactnative.dev so everyone can take advantage

@Abbondanzo
Copy link
Contributor

Alrighty, last update for a little while (at least until this gets packaged up into the next release):

You can follow along with the release status of this change by checking out the tags associated with 6202319 (right now there are none). When you pick up the change in a new release, it should be as simple as using it like this:

<Image 
  source={require('./assets/my-large-image.png')} 
  resizeMethod="none" 
/>

I'm going to close this PR out and drop this update across some of the linked issues. Thanks again to @sunnylqm for laying the groundwork and @clytras for raising this so dang long ago

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RN 0.57.x Bundled large images have low quality when viewing using <Image/> component with 1:1 AR on Android
6 participants