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

Low image quality using <Image/> component on RN > = 0.57 (Fresco >= 1.10.0) #2397

Closed
clytras opened this issue Aug 22, 2019 · 75 comments
Closed

Comments

@clytras
Copy link

clytras commented Aug 22, 2019

Description

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

There is low quality when loading large bundled (PNG, GIF and maybe more formats, NOT JPEG) images only on Android:

At the left screenshot we see the exact same code running with RN 0.56.0 and at the right screenshot we see RN 0.57.1. The code is just a simple image <Image source={require('./assets/ELHall1.png')} resizeMethod="resize" /> and the image size is 2111 x 4645 pixels. Both projects are fresh installed using react-native init RN057ImageTest and react-native init --version="0.56.0" RN056ImageTest. This continues to happen from 0.56 and all versions after and latest RN 0.60.x.

This is confirmed to be caused by RN Fresco lib change between 0.56 and 0.57 from 1.9.0 to 1.10.0 facebook/react-native@b6f2aad. Check comment facebook/react-native#21301 (comment).

After some search at Fresco issues, I can see some related issues that it's suggested that large images should be divided and recomposed piece by piece, which it resolves many cases (most map related large images) but it can be very inconvenient especially for dynamic loaded/created images. This was working until RN 0.56 and from 0.57 and after it does not.

Reproduction

RN: It's the initial App.js with an <Image/> component added.

...
type Props = {};
export default class App extends Component<Props> {
  render() {
    return (
      <View style={styles.container}>
        <Image source={require('./assets/ELHall1.png')} resizeMethod="resize" />
      </View>
    );
  }
}
...

Additional Information

At this comment facebook/react-native#21301 (comment), lambdapioneer writes that this maybe is related to scale down (sub-sampling) large images:

I assume it's related to how Fresco scales downs (sub-sampling) large images (which is an important feature for memory and performance concerns). There has been some changes in these area during that time mainly for removing native code dependencies to help reducing the also pressing .so unsatisfied link errors. So to say: it might be a side-effect of another large improvement.

  • Fresco version: >= 1.10.0
  • Platform version: RN >= 0.57, all Android versions
@stale
Copy link

stale bot commented Aug 29, 2019

Hey there, it looks like there has been no activity on this issue recently. Has the issue been fixed, or does it still require the community's attention? This issue may be closed if no further activity occurs. You may also label this issue as "bug" or "enhancement" and I will leave it open. Thank you for your contributions.

@stale stale bot added the stale label Aug 29, 2019
@oprisnik
Copy link
Contributor

oprisnik commented Sep 4, 2019

What Fresco versions are RN 0.56.0 and 0.57.1 using? This narrows down the number of commits that could have caused this.

@stale stale bot removed the stale label Sep 4, 2019
@oprisnik oprisnik added the needs-details This issue or PR is currently not actionable as it misses details (e.g. for reproducing the problem) label Sep 4, 2019
@clytras
Copy link
Author

clytras commented Sep 6, 2019

Hello @oprisnik,

I'll check and see exactly in which version patch got the upgrade when I'll return back to my office, though I think facebook/react-native#21301 (comment) reports exactly from which version got this behaviour change.

@s1rius
Copy link
Contributor

s1rius commented Sep 7, 2019

Hi @clytras, I found the image on screen in this repository.
I download the picture, and find the picture is 2111(width)*4645(height).

Fresco will downsample an image to fit the OpenGL limit, the default image's max width/height is hardcode in this line.

In my opinion,
The easy way is change a ResizeOption's maxBitmapSize field.
The right way is don't use Fresco to display a VERY large image.

Create an other widget, and use this library to display a VERY large Image is many android team's choice.

@clytras
Copy link
Author

clytras commented Sep 7, 2019

Hi @s1rius, I write the image size precisely if you read the open issue post:

The code is just a simple image <Image source={require('./assets/ELHall1.png')} resizeMethod="resize" /> and the image size is 2111 x 4645 pixels

This is about a behaviour change that happened from RN >= 0.57 which changed from Fresco 1.9.0 to 1.10.0. Large images like that one were working and loading in full quality before that version. Contributors from RN repo closed the issue as they state that this is not a RN issue but a Fresco lib issue. If that behavious change is intentionally made, then we can close this issue. Yes there are workarounds but it's that this was working until a version and suddenly, and with no info on why, stopped working.

@stale
Copy link

stale bot commented Sep 14, 2019

Hey there, it looks like there has been no activity on this issue recently. Has the issue been fixed, or does it still require the community's attention? This issue may be closed if no further activity occurs. You may also label this issue as "bug" or "enhancement" and I will leave it open. Thank you for your contributions.

@stale stale bot added the stale label Sep 14, 2019
@stale
Copy link

stale bot commented Sep 21, 2019

Closing this issue after a prolonged period of inactivity. If this issue is still present in the latest release, please feel free to reopen with up-to-date information.

@stale stale bot closed this as completed Sep 21, 2019
@oprisnik oprisnik reopened this Sep 22, 2019
@stale stale bot removed the stale label Sep 22, 2019
@oprisnik
Copy link
Contributor

A potential change that could have caused this is fa71901

Can you verify if that's the offending change?

@clytras
Copy link
Author

clytras commented Sep 25, 2019

Hello again @oprisnik and thanks for investigating this.

I am not so familiar with gradle and how I can change RN to compile fresco directly from source instead of downloading the library. Steps I've taken with no success:

  1. Create RN 0.57 project using react-native-cli
  2. Clone fresco lib to the project root directory
  3. Checkout to v1.10.0 branch inside fresco lib
  4. Add android-ndk-r20 path to <Project>\android\local.properties (ndk.dir=G:\\Dev\\Android\\android-ndk-r20)

Then I found this answer at SO https://stackoverflow.com/a/52861379/1889685 which compiles RN with fresco from source and overrides the library:

cd android
./gradlew assembleDebug --include-build /e/Sandbox/RN057ImageTest/fresco/

but I get the following error:

> Task :fresco:imagepipeline:ndk_build_bitmaps FAILED
A problem was found with the configuration of task ':fresco:imagepipeline:ndk_build_bitmaps'. Registering invalid inputs and outputs via TaskInputs and TaskOutputs methods has been deprecated and is scheduled to be removed in Gradle 5.0.
 - File 'E:\Sandbox\RN057ImageTest\fresco\imagepipeline\src\main\jni\bitmaps' specified for property '$1' is not a file.

Maybe @sunnylqm can test this if he has some time, because he is way more familiar with this process.

If it helps, I can create a repo with what I have so far.

@stale
Copy link

stale bot commented Oct 2, 2019

Hey there, it looks like there has been no activity on this issue recently. Has the issue been fixed, or does it still require the community's attention? This issue may be closed if no further activity occurs. You may also label this issue as "bug" or "enhancement" and I will leave it open. Thank you for your contributions.

@stale stale bot added the stale label Oct 2, 2019
@sunnylqm
Copy link
Contributor

sunnylqm commented Oct 4, 2019

I tried to build but also got the ndk error. Do you guys have nightly builds that we can test on?

@stale stale bot removed the stale label Oct 4, 2019
@sunnylqm
Copy link
Contributor

sunnylqm commented Oct 4, 2019

Since I can compile v2.0.0 without any modification, I tried another way to verify. I disabled downsampling which is enabled by the commit above, and here are the results

test image uri:
"https://wagonsclub.oss-cn-beijing.aliyuncs.com/static/carousel/carousel1_bg.jpg"

fragment_drawee_simple.xml

  <com.facebook.drawee.view.SimpleDraweeView
      android:id="@+id/drawee_view"
      android:layout_width="match_parent"
      android:layout_height="1000dp"
      />

original v2.0.0 showcase
image

disabled downsampling v2.0.0 showcase
image

@clytras
Copy link
Author

clytras commented Oct 10, 2019

@sunnylqm thanks for debugging this. How can I disable downsampling? I've tried the following methods with no success:

1st in MainApplication.java onCreate:

@Override
public void onCreate() {
  super.onCreate();

  ImagePipelineConfig config = ImagePipelineConfig.newBuilder(this)
    .setDownsampleEnabled(false)
    .build();

  Fresco.initialize(this, config);

  SoLoader.init(this, /* native exopackage */ false);
}

With this I get a message at Logcat that Fresco is already initialized.

2nd method I've used by initializing MainReactPackage with MainPackageConfig but it didn't work either:

protected List<ReactPackage> getPackages() {
  Context context = getApplicationContext();
  ImagePipelineConfig pipelineConfig = ImagePipelineConfig
    .newBuilder(context.getApplicationContext())
    .setDownsampleEnabled(false)
    .build();
  MainPackageConfig config = new MainPackageConfig.Builder().setFrescoConfig(pipelineConfig).build();

  return new ArrayList<>(Arrays.<ReactPackage>asList(
    new MainReactPackage(config),
    new ReactNativeFirebaseAppPackage(),
    new FastImageViewPackage(),
    new RNGestureHandlerPackage(),
    new ReanimatedPackage(),
    new SvgPackage()
  ));
}

@sunnylqm
Copy link
Contributor

@clytras I dont know. I disabled it in source code.

@sunnylqm
Copy link
Contributor

ping @oprisnik

@oprisnik
Copy link
Contributor

Looks like this is set via the React Native main package config: https://github.com/facebook/react-native/blob/6c0f73b3223968448bb186b82f06f6819068a21d/ReactAndroid/src/main/java/com/facebook/react/shell/MainPackageConfig.java

Not sure how you have to set that up with RN, never tried that.

@sunnylqm
Copy link
Contributor

@oprisnik Is there any plan to fix this issue?

@clytras
Copy link
Author

clytras commented Oct 12, 2019

I believe this issue still belongs to the RN repo. Image downsampling is something that without a doubt is for reducing memory cost, gain efficiency and having less crasses when loading large images. I think this issues should be handled by RN in a way that it allows end developers to choose if they want to have image downsampling or not in a form of an option/property. For example in my app, where I use large images for presenting some floor plans, I need the high-res when the user zooms in the floor plan. I always check the device capabilities and if it's a low-end device with less RAM or low-res, then I load smaller and lower res images.

There is the resizeMethod property for the <Image/> component which has scale and resize. RN currenlty does not respect the scale value of that property, which I think it should disable image downsampling and only enable it when setting it to resize.

resizeMethod

The mechanism that should be used to resize the image when the image's dimensions differ from the image view's dimensions. Defaults to auto.

resize: A software operation which changes the encoded image in memory before it gets decoded. This should be used instead of scale when the image is much larger than the view.

scale: The image gets drawn downscaled or upscaled. Compared to resize, scale is faster (usually hardware accelerated) and produces higher quality images. This should be used if the image is smaller than the view. It should also be used if the image is slightly bigger than the view.

I'd really like to see your thoughts on this @oprisnik @sunnylqm.

@sunnylqm
Copy link
Contributor

sunnylqm commented Oct 14, 2019

@clytras As you've said, it's a behavior change in fresco without explanation. (The demo I show is the example app in fresco repo) So I don't think it has anything to do with RN.

@clytras
Copy link
Author

clytras commented Oct 14, 2019

@sunnylqm of course it is a behavior change in fresco without explanation but RN should leave the choice of enabling/disabling this fresco feature to the end devs.

Also don't forget that this change of behavior does not of course happen in iOS, so we have to do something about it and since fresco configuration happens in react native, then I think this configuration should be exposed to end RN developers.

BTW I managed to compile Fresco 2.0.0 (master branch) with latest RN 0.61.2 and disabled downsampling by commenting out downsampling condition check at DecodeProducer.java as you did.

I created a local.properties file inside fresco repo directory and used Android NDK Revision 19c x86_64 for windows 64-bit.

ndk.dir=G:\\Dev\\Android\\android-ndk-r19c
org.gradle.daemon=true
org.gradle.parallel=true
org.gradle.configureondemand=true

Fresco binaries compiled successfully and image downsampling has been disabled for the RN app built by running:

cd android
.\gradlew assembleDebug --include-build ..\fresco\

@oprisnik
Copy link
Contributor

yeah, the solution for this would be to disable downsampling for your applications, via the RN initialization logic or when Fresco is initialized.

@pateljoel
Copy link

Unfortunately FastImage doesn't work and the issue still persists for me on the Image component, please take a look my Expo snack for reproducing this issue.

https://snack.expo.dev/@joelpateljp/low-quality-image-android-example

@pateljoel
Copy link

Also is there a timeline for when this patch will be merged?

@ramkumar2098
Copy link

I ended up using webview as it displays images as is.

<WebView source={{ html: `<img src=${image_url} style="width: 100%;" />` }} />

It works for my usecase.

@Mfweb
Copy link

Mfweb commented Dec 15, 2022

any updates on this?

@anandpandey2414
Copy link

Any update on this guys. or any simple solution Fastimage is not working for me.
Don't wanna use patch is there anything else we can do? Please suggest for the same

@timoisalive
Copy link

Hey, is the allowDownscaling prop of the expo-image supposed to be an answer to this issue? From the description, that's what it seems, but I wasn't able to notice any difference in the image quality setting it true or false. 🤔

https://docs.expo.dev/versions/unversioned/sdk/image/#allowdownscaling

@pateljoel
Copy link

Is there any update on this? And also has anyone managed to solve this issue or have a solution? I'm not sure if the latest fresco 3.0.0 solves this issue either.

@sunnylqm
Copy link
Contributor

need someone from fresco team to review this pr #2500
but unfortunately for some reason they rarely review prs from the community

@pateljoel
Copy link

pateljoel commented Sep 11, 2023

@sunnylqm Any luck on contacting other maintainers to review this? It looks like it would / should only take a few hours to review and merge but instead we're 'waiting for godot' here.

@sunnylqm
Copy link
Contributor

maybe they just muted this repo. maybe you should try to find their personal sns accounts to dm.

@clytras
Copy link
Author

clytras commented Sep 15, 2023

The suprising thing to me is that the issue is still opened. I mean, if they didn't consider this to be a bug, they would have closed the issue stating it's expected behavior, if they would recognize that this is a bug, they would have merged the fix PR. It's kind of strange.

@pateljoel
Copy link

@clytras I mean @oprisnik knows about this issue, he has commented on this thread and his last activity on this was on Oct 28, 2019, I don't know about the other maintainers (I believe there are about 3 or 10 of them) but surely they should have looked into even merging the PR if they have access.

@sunnylqm I'm still pinging the PR but I don't think there is any email to contact any of them.

@sunnylqm
Copy link
Contributor

sunnylqm commented Mar 9, 2024

we need to wait for a new fresco release and i'll do the necessary pr on RN side.

@clytras
Copy link
Author

clytras commented Mar 9, 2024

A big thank you to @sunnylqm for making downsample configurable. I'm looking forward for a new version and of course the RN implementation.

Thank you @sunnylqm ❤️

@onetrace-abdullah
Copy link

@sunnylqm Looks like Fresco released v3.2.0 recently, and the version bump has been merged into React Native facebook/react-native@744024b

@sunnylqm
Copy link
Contributor

updated the rn config, but do not have time to build and test.

facebook/react-native#45078

Abbondanzo added a commit to Abbondanzo/react-native that referenced this issue Oct 7, 2024
Summary:
## Summary
Adds a new `resizeMethod` called `never`. It disables downsampling on the image request and disregards the global image pipeline default. This has been a pain point raised when rendering large images on Android in issues like [this one](facebook#21301) and [this one](facebook/fresco#2397).

## Changelog
[Android][Added] - Adds a new `resizeMethod`, `never`, which disables downsampling for an image

Differential Revision: D62393211
Abbondanzo added a commit to Abbondanzo/react-native that referenced this issue Oct 8, 2024
…book#46866)

Summary:

## Summary
Adds a new `resizeMethod` called `none`. It disables downsampling on the image request and disregards the global image pipeline default. This has been a pain point raised when rendering large images on Android in issues like [this one](facebook#21301) and [this one](facebook/fresco#2397).

## Changelog
[Android][Added] - Adds a new `resizeMethod`, `none`, which disables downsampling for an image

Reviewed By: yungsters

Differential Revision: D62393211
facebook-github-bot pushed a commit to facebook/react-native that referenced this issue Oct 8, 2024
Summary:
Pull Request resolved: #46866

## Summary
Adds a new `resizeMethod` called `none`. It disables downsampling on the image request and disregards the global image pipeline default. This has been a pain point raised when rendering large images on Android in issues like [this one](#21301) and [this one](facebook/fresco#2397).

## Changelog
[Android][Added] - Adds a new `resizeMethod`, `none`, which disables downsampling for an image

Reviewed By: yungsters

Differential Revision: D62393211

fbshipit-source-id: d85e3ed098ad502c8edbdfa817c841271ee9e914
@Abbondanzo
Copy link
Contributor

For Fresco

Fresco v3.3.0 now supports downsample overrides on a per-image-request basis following the work from #2787. You can use ImageRequestBuilder.setDownsampleOverride like so to enable/disable downsampling outside of how you have your image pipeline configured.

For React Native

x-posting my update from facebook/react-native#45078

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" 
/>

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