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

[CameraRoll] Add assetThumbnail prop #1704

Closed
wants to merge 4 commits into from
Closed

[CameraRoll] Add assetThumbnail prop #1704

wants to merge 4 commits into from

Conversation

Iragne
Copy link
Contributor

@Iragne Iragne commented Jun 22, 2015

in reference with this PR
#1406

@Iragne Iragne changed the title FIX crash with Assets [CameraRoll] Add assetThumbnail prop Jun 22, 2015
@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 Jun 22, 2015
nativeProps.imageTag = source.uri;
if (this.props.assetThumbnail === true){
nativeProps.assetThumbnail = source.uri;
}else{
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: } else {

@Iragne
Copy link
Contributor Author

Iragne commented Jun 24, 2015

ok good point ;)

@Iragne
Copy link
Contributor Author

Iragne commented Jun 24, 2015

The modification is done

@Iragne
Copy link
Contributor Author

Iragne commented Jul 6, 2015

@brentvatne and @vjeux that sound ok for you ?

nativeProps.imageTag = source.uri;
if (source.assetThumbnail && source.assetThumbnail === true){
nativeProps.assetThumbnail = source.uri;
}else{
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: should be } else {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

@mrspeaker
Copy link
Contributor

This patch works like a charm, by the way... really looking forward to it landing!

aroth added a commit to aroth/react-native that referenced this pull request Jul 13, 2015
…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.
@Iragne Iragne closed this Sep 2, 2015
@Iragne
Copy link
Contributor Author

Iragne commented Sep 2, 2015

I close because a PR pass with other way to solve the issue
Prefered resize rather than a native Thumb extraction ;)

@facebook-github-bot
Copy link
Contributor

@Iragne updated the pull request.

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.

5 participants