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

Avoid re-encoding images when uploading local files #31457

Closed

Conversation

arthuralee
Copy link
Contributor

Summary

Fixes #27099

When you upload a local file using XHR + the FormData API, RN uses RCTNetworkTask to retrieve the image file data from the local filesystem (request URL is a file:// URL) (code pointer). As a result, if you are uploading a local image file that is in the app's directory RCTNetworkTask will end up using RCTLocalAssetImageLoader to load the image, which reads the image into a UIImage and then re-encodes it using UIImageJPEGRepresentation with a compression quality of 1.0, which is the higest (code pointer). Not only is this unnecessary, it ends up inflating the size of the jpg if it had been previously compressed to a lower quality.

With this PR, this issue is fixed by forcing the RCTFileRequestHandler to be used when retrieving local files for upload, regardless of whether they are images or not. As a result, any file to be uploaded gets read into NSData which is the format needed when appending to the multipart body.
I considered fixing this by modifying the behavior of how the handlers were chosen, but this felt like a safer fix since it will be scoped to just uploads and wont affect image fetching.

Changelog

[iOS] [Fixed] - Avoid re-encoding images when uploading local files

Test Plan

The repro for this is a bit troublesome, especially because this issue doesn't repro in RNTester. There is some code that is to be overriding the handlers that will be used, excluding the RCTImageLoader. I had to repro this in a fresh new RN app.

  1. Create a blank RN app
  2. Put an image in the folder of the app's install location. This would be similar to where files might be placed after an app downloads or captures an image.
  3. Set up a quick express server that accepts multipart form uploads and stores the files
  4. Trigger an upload via react native
const data = new FormData();
data.append('image', {
  uri:
    '/Users/arthur.lee/Library/Developer/CoreSimulator/Devices/46CDD981-9164-4925-9025-1A76C0D9F0F5/data/Containers/Bundle/Application/B1E8A764-6221-4EA9-BE9A-2CB1699FD218/test.app/test.bundle/compressed.jpg',
  type: 'image/jpeg',
  name: 'image.jpeg',
});

fetch(`http://localhost:3000/upload`, {
  method: 'POST',
  headers: {'Content-Type': 'multipart/form-data'},
  body: data,
}).then(console.log);
  1. Trigger the upload with and without this patch

Original file:

$ ls -lh
total 448
-rw-r--r--  1 arthur.lee  staff   223K Apr 29 17:08 compressed.jpg

Uploaded file (with and without patch):

$ ls -lh
total 1624
-rw-r--r--@ 1 arthur.lee  staff   584K Apr 29 17:11 image-nopatch.jpeg
-rw-r--r--@ 1 arthur.lee  staff   223K Apr 29 17:20 image-withpatch.jpeg

Would appreciate pointers on whether this needs to be tested more extensively

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Apr 30, 2021
@analysis-bot
Copy link

Platform Engine Arch Size (bytes) Diff
ios - universal n/a --

Base commit: 9e020ef

@analysis-bot
Copy link

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 8,863,161 -112,362
android hermes armeabi-v7a 8,385,629 -102,339
android hermes x86 9,320,841 -107,023
android hermes x86_64 9,264,584 -105,288
android jsc arm64-v8a 10,592,267 -192
android jsc armeabi-v7a 10,097,819 -126
android jsc x86 10,611,444 -89
android jsc x86_64 11,195,458 +11

Base commit: 9e020ef

@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

@PeteTheHeat merged this pull request in f78526c.

@facebook-github-bot facebook-github-bot added the Merged This PR has been merged. label Jun 15, 2021
facebook-github-bot pushed a commit that referenced this pull request Jun 16, 2021
Summary:
In D28630805 (f78526c) ([github link](#31457)) I added a setter to workaround a bug I perceived with `moduleRegistry`. Turns out - the proper fix was to remove the `synthesize` line. See conversation on linked diff for more context.

Changelog: [Internal]

Reviewed By: RSNara

Differential Revision: D29144717

fbshipit-source-id: aa95b670b540b9007eed76769c9babc10ea399ce
@arthuralee arthuralee deleted the improve-local-image-loading branch September 10, 2021 16:59
@chrisbobbe
Copy link

It looks like this fix was reverted in e83feff and the bug is active again, tracked in #33760 and #31641.

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. Merged This PR has been merged. Needs: React Native Team Attention
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Images posted to server via fetch get their size inflated significantly
5 participants