react-native
react-native copied to clipboard
facebook
[iOS]:- fixed image resize mode in release mode for iOS
Summary:
On iOS, when attempting to use a local image with a different resizeMode than the default one in a release build, it doesn't work as expected if the image is a local asset. The problem lies in how the image is loaded and processed. In a debug build, the image loads correctly with the desired resizeMode. This is because the method RCTDecodeImageWithData is employed, which handles the resizeMode properly.
https://github.com/facebook/react-native/assets/72331432/763d07db-4345-42f0-add3-ce7c1a606104
In contrast, in a release build, the image does not load with the desired resizeMode. This is due to the use of RCTImageFromLocalAssetURL, which is designed specifically to load an image by URL and does not handle resizeMode.
To address this issue, a new method called RCTDecodeImageWithLocalAssetURL has been introduced. This method replicates the resizeMode handling strategy of RCTDecodeImageWithData but loads the image by URL instead of by data. To ensure backwards compatibility and handle edge cases where the image cannot be loaded using the new method, a fallback to RCTImageFromLocalAssetURL is retained.
Following has been done to fix the same:-
- Created RCTDecodeImageWithLocalAssetURL to handle loading images by URL with proper resizeMode.
- Retained fallback mechanism to use RCTImageFromLocalAssetURL when necessary.
Changelog:
[IOS] [FIXED] - fixed resize mode in release mode for iOS
Test Plan:
tried to implement the changes in a react native scaffold by changing the node_module files and it worked perfectly
https://github.com/facebook/react-native/assets/72331432/7660ef0e-0a82-457c-8a19-cbaff64748fa
cc @NickGerleman @cipolleschi @javache @blakef
| Platform | Engine | Arch | Size (bytes) | Diff |
|---|---|---|---|---|
| android | hermes | arm64-v8a | 19,741,485 | -754,632 |
| android | hermes | armeabi-v7a | n/a | -- |
| android | hermes | x86 | n/a | -- |
| android | hermes | x86_64 | n/a | -- |
| android | jsc | arm64-v8a | 22,946,151 | -747,330 |
| android | jsc | armeabi-v7a | n/a | -- |
| android | jsc | x86 | n/a | -- |
| android | jsc | x86_64 | n/a | -- |
Base commit: 4a8f0ee58a1eaa119d8dc0354d08e531a32bbcbd Branch: main
@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.
@cipolleschi 😅 are the fb internal linter fail related to my change since it is only accessible to meta employees!
@cipolleschi friendly ping 😀 Is this good to go? Or there are some changes to be made?
Is it good. I had to lint it with the internal linter and there were some unrelated tests failing. I rebased it as well, and I hope we can land it before EOW. I appreciate your patience, we are a bit busy preparing the next release.
thanks @cipolleschi , for helping me out with this PR! hoping to make react native more awesome 😊
Some tests were failing internally. I'm fixing them and I hope to get this approved internally soon, so I can land it. Sorry for the delay.
hey @cipolleschi its definitely okay and i understand appreciate the work you are doing 😊.
@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.
Hi @Biki-das! Sorry for this to taking too long, but we are not convinced by the fix. We are a bit scared that the there would be perf implications and this difference between Debug and Release is a bit weird.
The right approach here is to understand why image decoding would fail, and avoid going into the CGImageSourceCreateWithURL path in the first place, as it might be more expensive.
It could also be that the issue is not in the image loading code but in the sequence that is used to set the resizeMode.
If it is not too much of an ask, would you prepare a simple repro, like the one you are using in the videos, starting from this template?
The reason image decoding fails is because in debug only mode it's using the method RCTDecodeImageWithData which is handling the resize mode properly. But in release build it is using RCTImageFromLocalAssetURL which is only made to load an image by url, so the resize mode is just never used. so the difference that you are implying is bound to occur as this is the way the code was implemented when it was written.
the point that i understand as of now is the CGImageSourceCreateWithURL method which seems to be the issue?
am i right
the image decoding does not fail as far as i understand, the resize mode just gets ignored in case of release build while using the RCTImageFromLocalAssetURL
well further ok let me try to create a reproduce repo
cc @cipolleschi
just tinkering and have a added a CFRelease method call to ensure the sourceRef is release one thing i am trying to understand is there a lot of performance diff between calling CGImageSourceCreateWithUrl and CGImageSourceCreateWithData?.
would like to know more as i don't have much idea of the same
https://github.com/facebook/react-native/blob/main/packages/react-native/Libraries/Image/RCTBundleAssetImageLoader.mm#L52
this is the method being used currently to handle the image decoding for the release mode. no resize mode aspects are taken here
I think that the suggestion revolves more around: how can we make RCTImageFromLocalAssetURL honor the resizeMode?
The root difference, IIUC, is that:
- in debug mode, the asset is provided by Metro as a URL. So we need to decode the url and use the
RCTDecodeImageWithData - in release mode, there is no Metro. When building, we package the JS and all the assets in a file and we add the file to the bundle. So we can use the
RCTImageFromLocalAssetURLto read the assets from the bundle directly. This should save performances as we don't need some of the decoding put in place byRCTDecodeImageWithDataand we can leverage the internal cache ofUIImage.
The proper solution would be not to decode everything from scratch, but to extend RCTImageFromLocalAssetURL so that it accept and respect the resizeMode. Or, alternatively, apply the resize mode after RCTImageFromLocalAssetURL returns, if possible.
What do you think? Does it make more sense?
Thanks @cipolleschi for the explanation yeah now it is clear to me what is expected let me take a look at the same during the weekend to come up with something
Hello, I am having this exact same issue on version 0.74.3. Is there any solution to this? Or if I downgrade RN will I still get this error?
@thepdatoi sorry to hear that you are having the same error and yes, even if you downgrade, you'll still have the same issue, sadly.
This PR is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be closed in 7 days.
This PR was closed because it has been stalled for 7 days with no activity.