react-native icon indicating copy to clipboard operation
react-native copied to clipboard

use new downsamplemode to disable downsampling

Open sunnylqm opened this issue 1 year ago • 11 comments

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 https://github.com/facebook/react-native/issues/21301

Changelog:

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

Test Plan

Test a large remote image like https://github.com/facebook/fresco/issues/2397#issuecomment-538223936 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.

sunnylqm avatar Jun 19 '24 23:06 sunnylqm

I added a comment https://github.com/facebook/react-native/issues/21301#issuecomment-2180997341 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.

Abbondanzo avatar Jun 20 '24 15:06 Abbondanzo

@Abbondanzo one example of downsampling issue https://github.com/facebook/fresco/issues/2397#issuecomment-538223936 and it's not even rn code

sunnylqm avatar Jun 21 '24 08:06 sunnylqm

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 avatar Jun 21 '24 10:06 onetrace-abdullah

Here's a better example:

Android (JPG) iOS (JPG)
android ios

onetrace-abdullah avatar Jun 21 '24 11:06 onetrace-abdullah

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

facebook-github-bot avatar Jun 21 '24 14:06 facebook-github-bot

@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.

Abbondanzo avatar Jun 21 '24 15:06 Abbondanzo

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

onetrace-abdullah avatar Jun 21 '24 16:06 onetrace-abdullah

@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 avatar Jun 24 '24 17:06 Abbondanzo

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.

Abbondanzo avatar Jul 02 '24 21:07 Abbondanzo

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 avatar Jul 04 '24 14:07 zell180

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: https://github.com/facebook/fresco/issues/2397#issuecomment-576399299. 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();

Abbondanzo avatar Jul 07 '24 19:07 Abbondanzo

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 avatar Jul 28 '24 18:07 zell180

Hi everyone, there is some update about this pr?

zell180 avatar Aug 30 '24 07:08 zell180

I've picked this back up and have made some changes to Fresco recently that I think will strike a safer balance: https://github.com/facebook/fresco/pull/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 avatar Sep 09 '24 22:09 Abbondanzo

Hey all, providing another update as promised. The changes from https://github.com/facebook/fresco/pull/2787 have made their way into Fresco v3.3.0 and are being added as part of https://github.com/facebook/react-native/pull/46864. Subsequently, I'm adding a new resizeMethod type of never to https://github.com/facebook/react-native/pull/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 avatar Oct 07 '24 18:10 Abbondanzo

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

  • https://github.com/facebook/react-native/pull/46866 was merged and adds a new resizeMethod called none. This maps to the NEVER downsample mode in Fresco on a per-image basis.
  • I've added documentation regarding this new resize method to https://reactnative.dev/docs/next/image#resizemethod-android with a brief disclaimer about what I've shared here: it can be a footgun that causes runtime exceptions
  • https://github.com/facebook/react-native/pull/46873 adds a showcase of each of the four resizeMethod to the RNTester app. I'm setting up tests to run against these images to ensure that downsampling is always correctly turned off when this prop is supplied

You can follow along with the release status of this change by checking out the tags associated with 6202319ed544e4d23f1f327ff5334c480af3819d (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

Abbondanzo avatar Oct 08 '24 22:10 Abbondanzo