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

[Image] Automatic scaling/thumbnailing of iOS images; resolves "Best Camera Roll Image" documentation discrepancy. #1969

Closed
wants to merge 1 commit into from

Conversation

aroth
Copy link
Contributor

@aroth aroth commented Jul 13, 2015

Currently, documentation under "Image > Best Camera Roll Image" is
misleading:

“iOS saves multiple sizes for the same image in your Camera Roll, it is
very important to pick the one that's as close as possible for
performance reasons. You wouldn't want to use the full quality
3264x2448 image as source when displaying a 200x200 thumbnail. If
there's an exact match, React Native will pick it, otherwise it's going
to use the first one that's at least 50% bigger in order to avoid blur
when resizing from a close size. All of this is done by default so you
don't have to worry about writing the tedious (and error prone) code to
do it yourself.”

https://facebook.github.io/react-native/docs/image.html

This does not occur. Instead, React Native loads the full resolution
image no matter the desired size. This means an 8MP photo will require
32MB of memory to display even if displayed in a 100x100 thumbnail.
Loading a series of large images will spike memory and likely crash
your app.

This commit resolves the discrepancy and brings the codebase inline
with current documentation.

Example: Consider a 3264x2448 image loaded on an iPhone 6 with the
following tag:

<Image src={{ uri: 'assets-library://... }} style={{ width: 320,
height: 240 }}/>

The image will automatically be scaled to 640x320 (320x240 target
dimensions * a Retina scale of 2.0). This uses considerably less memory
than rendering the full resolution asset. It also happens automatically.

To force the loading of full resolution, the assetUseMaximumSize option
can be passed in:

<Image src={{ uri: 'assets-library://...', options: {
assetUseMaximumSize: true } }} style={{ width: 320, height: 240 }}/>

Additionally, RCTImageLoader has been updated to handle automatic
scaling for images from both the Assets Library and Photos Framework.

Added an example to UIExplorer. Tap an image in the .

Issues touched:
#1567
#1845
#1579
#930

Note:

Pull Request #1704 (#1704)
takes an alternate approach by allowing an assetThumbnail prop
(boolean) to be passed in to the Image tag's source hash. IE:

<Image src={{ uri: ..., assetThumbnail: true }} />

If true, the asset's thumbnail representation will be used. The
resolution of this thumbnail is non-configurable and does not scale
well.

…ed in the style property of the <Image... /> tag.

Currently, documentation under "Image > Best Camera Roll Image" is
misleading:

“iOS saves multiple sizes for the same image in your Camera Roll, it is
very important to pick the one that's as close as possible for
performance reasons. You wouldn't want to use the full quality
3264x2448 image as source when displaying a 200x200 thumbnail. If
there's an exact match, React Native will pick it, otherwise it's going
to use the first one that's at least 50% bigger in order to avoid blur
when resizing from a close size. All of this is done by default so you
don't have to worry about writing the tedious (and error prone) code to
do it yourself.”

https://facebook.github.io/react-native/docs/image.html

This does not occur. Instead, React Native loads the full resolution
image no matter the desired size. This means an 8MP photo will require
32MB of memory to display even if displayed in a 100x100 thumbnail.
Loading a series of large images will spike memory and likely crash
your app.

This commit resolves the discrepancy and brings the codebase inline
with current documentation.

Example: Consider a 3264x2448 image loaded on an iPhone 6 with the
following tag:

   <Image src={{ uri: 'assets-library://... }} style={{ width: 320,
height: 240 }}/>

The image will automatically be scaled to 640x320 (320x240 target
dimensions * a Retina scale of 2.0). This uses considerably less memory
than rendering the full resolution asset. It also happens automatically.

To force the loading of full resolution, the assetUseMaximumSize option
can be passed in:

	<Image src={{ uri: 'assets-library://...', options: {
assetUseMaximumSize: true } }} style={{ width: 320, height: 240 }}/>

Additionally, RCTImageLoader has been updated to handle automatic
scaling for images from both the Assets Library and Photos Framework.

Added an example to UIExplorer. Tap an image in the <CameraRollView>.

Issues touched:
	facebook#1567
	facebook#1845
	facebook#1579
	facebook#930

Note:

Pull Request facebook#1704 (facebook#1704)
takes an alternate approach by allowing an assetThumbnail prop
(boolean) to be passed in to the Image tag's source hash. IE:

<Image src={{ uri: ..., assetThumbnail: true }} />

If true, the asset's thumbnail representation will be used. The
resolution of this thumbnail is non-configurable and does not scale
well.
@facebook-github-bot facebook-github-bot added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Jul 13, 2015
@sahrens
Copy link
Contributor

sahrens commented Jul 14, 2015

Hmm, I thought the documentation was pretty accurate, and the implementation used the actual layout dimensions of the image to grab the optimal size, although I don't think it did any dynamic resizing, just picked the best option the OS has available, but perhaps something broke, or there is a discrepancy with our internal modules...

Anyway, we definitely don't want to be loading giant images when using thumbnails.

@sahrens
Copy link
Contributor

sahrens commented Jul 14, 2015

Nick: can you take a look at this since you're refactoring some of our image stuff right now?

@nicklockwood
Copy link
Contributor

@sahrens, we were using different implementations internally and in OSS. The OSS one wasn't doing any scaling, but the internal one was, and I've now merged them.

This PR is going to conflict pretty heavily with the changes I landed today, but the examples look nice, and there are some features exposed on PhotoKit that we don't have yet.

@aroth can you try to rebase after my changes land on github later today? If it turns out to be too hard, or you don't have time, I'll see if I can manually pick in the stuff that's worth keeping. Thanks!

@aroth
Copy link
Contributor Author

aroth commented Jul 14, 2015

@nicklockwood Thanks. I suspected differences between internal & OSS implementations. I'll attempt a rebase once the changes land.

aroth added a commit to aroth/react-native that referenced this pull request Jul 15, 2015
Enhanced version of changes from
facebook#1969
---

Recent improvements allow RCTImageLoader to select a more appropriate
sized image based on the layout dimensions. Sizes:

	- asset.thumbnail
	- asset.aspectRatioThumbnail
	- asset.defaultRepresentation.fullScreenImage
	- asset.defaultRepresentation.fullResolutionImage

Prior, only the fullResolutionImage was used. This was memory intensive
and resulted in crashes when loading several large images at once. The
updated implementation works well, but can be made more efficient:

Consider loading 10 8MP (3264x2448) images in 150x150 pixel containers.
The target size (150x150) is larger than asset.thumbnail (approx
100x100), therefore the fullScreenImage representation is used instead
(approx 1334x1000).

This commit will scale the asset to the minimum size required while
taking into account original aspect ratio and device scale. Memory
usage is considerably lower and many more images can be loaded in
sequence without having to worry about memory warnings/crashes.

Memory comparison:
																				master				  this commit
8mp image to 150x150 thumb			   			5.2mb					  468kb
8mp image to 320x240 thumb							5.2mb					  1.2mb
100 images from camera roll 150x150			176.2mb spike   138.3mb spike

Additional reading:
http://www.mindsea.com/2012/12/downscaling-huge-alassets-without-fear-of
-sigkill/

Support has been added for both ALAssets and Photos Framework.

Also added additional example: tap an image from the Camera Roll
Example to see multiple versions of the image at different resolutions.
@aroth
Copy link
Contributor Author

aroth commented Jul 15, 2015

@nicklockwood Closed, created #2008 with enhancements.

@aroth aroth closed this Jul 15, 2015
sahrens pushed a commit to sahrens/react-native that referenced this pull request Jul 22, 2015
Summary:
Update to facebook#1969

--
Recent improvements allow RCTImageLoader to select a more appropriate sized image based on the layout dimensions. Sizes:

	- asset.thumbnail
	- asset.aspectRatioThumbnail
	- asset.defaultRepresentation.fullScreenImage
	- asset.defaultRepresentation.fullResolutionImage

Prior, only the fullResolutionImage was used. This was memory intensive and resulted in crashes when loading several large images at once. The updated implementation works well, but can be made more efficient:

Consider loading 10 8MP (3264x2448) images in 150x150 pixel containers. The target size (150x150) is larger than asset.thumbnail (approx 100x100), therefore the fullScreenImage representation is used instead (approx 1334x1000).

This commit will scale the asset to the minimum size required while taking into account original aspect ratio and device scale. Memory usage is considerably lower and many more images can be loaded in
sequence without having to worry
Closes facebook#2008
Github Author: Adam Roth <adamjroth@gmail.com>
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants