Slow rendering of CameraRoll images since 0.4.0 #930

Closed
VonD opened this Issue Apr 20, 2015 · 22 comments

Comments

Projects
None yet
10 participants
@VonD
Contributor

VonD commented Apr 20, 2015

Hi,

Images from the CameraRoll are very slow to render since 0.4.0 (up to a minute). Downgrading to 0.3.11 solves the issue.
This can be reproduced in a brand new app, or in the UIExplorer for example. I can attach some sample code to the issue if needed.

Thanks

@nicklockwood

This comment has been minimized.

Show comment
Hide comment
@nicklockwood

nicklockwood Apr 20, 2015

Contributor

That would be helpful - thanks!

Contributor

nicklockwood commented Apr 20, 2015

That would be helpful - thanks!

@VonD

This comment has been minimized.

Show comment
Hide comment
@VonD

VonD Apr 20, 2015

Contributor

To reproduce

react-native init TestImagesLoadingTime

then replace the contents of index.ios.js with this : https://gist.github.com/VonD/a530d21a53499a085080

Below are the captures of the same code with react-native 0.3.11 and 0.4.0 (sorry, the second one is quite heavy)

capture-0 3 11

capture-0 4 0

Contributor

VonD commented Apr 20, 2015

To reproduce

react-native init TestImagesLoadingTime

then replace the contents of index.ios.js with this : https://gist.github.com/VonD/a530d21a53499a085080

Below are the captures of the same code with react-native 0.3.11 and 0.4.0 (sorry, the second one is quite heavy)

capture-0 3 11

capture-0 4 0

@rt2zz

This comment has been minimized.

Show comment
Hide comment
@rt2zz

rt2zz Apr 22, 2015

Contributor

I can confirm the issue - the Image component appears to specifically be slow with CameraRoll images.

EDIT: more specifically Image component takes a long time to display asset-library URI's

Contributor

rt2zz commented Apr 22, 2015

I can confirm the issue - the Image component appears to specifically be slow with CameraRoll images.

EDIT: more specifically Image component takes a long time to display asset-library URI's

@scottdixon

This comment has been minimized.

Show comment
Hide comment
@scottdixon

scottdixon Apr 22, 2015

Contributor

+1 @rt2zz

Contributor

scottdixon commented Apr 22, 2015

+1 @rt2zz

@arypurnomoz

This comment has been minimized.

Show comment
Hide comment

+1

@adriansprod

This comment has been minimized.

Show comment
Hide comment
@scottdixon

This comment has been minimized.

Show comment
Hide comment
Contributor

scottdixon commented Apr 27, 2015

@rt2zz

This comment has been minimized.

Show comment
Hide comment
@rt2zz

rt2zz Apr 29, 2015

Contributor

@scottdixon that may be a seperate issue, but I just tested on 0.4.1 and loading the lower quality image does not solve the delay issue.

I did some debugging through the obvious image code, and everything is returning fast, RCTImageLoader.m loadImageWithTag which returns to RCTStaticImageManager.m RCT_CUSTOM_VIEW_PROPERTY(imageTag, NSString, RCTStaticImage) are are both running with minimal latency, yet there still is a lag of ~5s before the actual image renders to screen.

Contributor

rt2zz commented Apr 29, 2015

@scottdixon that may be a seperate issue, but I just tested on 0.4.1 and loading the lower quality image does not solve the delay issue.

I did some debugging through the obvious image code, and everything is returning fast, RCTImageLoader.m loadImageWithTag which returns to RCTStaticImageManager.m RCT_CUSTOM_VIEW_PROPERTY(imageTag, NSString, RCTStaticImage) are are both running with minimal latency, yet there still is a lag of ~5s before the actual image renders to screen.

@adriansprod

This comment has been minimized.

Show comment
Hide comment
@adriansprod

adriansprod Apr 29, 2015

I have cameraroll on a tab and it is very slow to render the images, but if i click another tab straight away and come back quickly then the images are shown where as they would still not have appeared if i stay on the tab.

I have cameraroll on a tab and it is very slow to render the images, but if i click another tab straight away and come back quickly then the images are shown where as they would still not have appeared if i stay on the tab.

@plredmond

This comment has been minimized.

Show comment
Hide comment
@plredmond

plredmond Apr 30, 2015

possibly related xcode logging about assetsd?

2015-04-30 11:39:42.464 ...[1274:189831] Connection to assetsd was interrupted or assetsd died
2015-04-30 11:39:42.637 ...[1274:190017] Communications error: <OS_xpc_error: <error: 0x19caaba80> { count = 1, contents =
    "XPCErrorDescription" => <string: 0x19caabe78> { length = 22, contents = "Connection interrupted" }
}>

possibly related xcode logging about assetsd?

2015-04-30 11:39:42.464 ...[1274:189831] Connection to assetsd was interrupted or assetsd died
2015-04-30 11:39:42.637 ...[1274:190017] Communications error: <OS_xpc_error: <error: 0x19caaba80> { count = 1, contents =
    "XPCErrorDescription" => <string: 0x19caabe78> { length = 22, contents = "Connection interrupted" }
}>
@aroth

This comment has been minimized.

Show comment
Hide comment
@aroth

aroth May 1, 2015

Contributor

I have developed a quick fix for this issue at aroth@453064b. The slow load times can be resolved by wrapping the asset callback in a dispatch_async call using the default serial queue:

dispatch_async(dispatch_get_main_queue(), ^{
    callback(nil, image);
});

Secondly, loading a large number of images will trigger memory warnings and/or OOM crashes. Using the thumbnail method of the ALAsset class can help remedy this (especially useful when iterating over images from the camera roll). I have updated image loading code to take in a representation string: full, screen or thumbnail. The default is 'full' if not provided.

<Image
  source={asset.node.image}
  representation={'thumbnail'}
  style={imageStyle}
/>

Full pull request pending.

Contributor

aroth commented May 1, 2015

I have developed a quick fix for this issue at aroth@453064b. The slow load times can be resolved by wrapping the asset callback in a dispatch_async call using the default serial queue:

dispatch_async(dispatch_get_main_queue(), ^{
    callback(nil, image);
});

Secondly, loading a large number of images will trigger memory warnings and/or OOM crashes. Using the thumbnail method of the ALAsset class can help remedy this (especially useful when iterating over images from the camera roll). I have updated image loading code to take in a representation string: full, screen or thumbnail. The default is 'full' if not provided.

<Image
  source={asset.node.image}
  representation={'thumbnail'}
  style={imageStyle}
/>

Full pull request pending.

@rt2zz

This comment has been minimized.

Show comment
Hide comment
@rt2zz

rt2zz May 2, 2015

Contributor

@aroth thanks! dispatch_async definitely fixes the load time

Contributor

rt2zz commented May 2, 2015

@aroth thanks! dispatch_async definitely fixes the load time

@brentvatne

This comment has been minimized.

Show comment
Hide comment
@brentvatne

brentvatne May 2, 2015

Collaborator

@aroth - nice one 😄

Collaborator

brentvatne commented May 2, 2015

@aroth - nice one 😄

@VonD

This comment has been minimized.

Show comment
Hide comment
@VonD

VonD May 8, 2015

Contributor

@aroth thanks!

About the image representation property : RN docs mention that the Image component already picks the most efficient representation (http://facebook.github.io/react-native/docs/image.html#best-camera-roll-image) : does your implementation do something different/more about it ?

About your PR : what's the status on that ?

Contributor

VonD commented May 8, 2015

@aroth thanks!

About the image representation property : RN docs mention that the Image component already picks the most efficient representation (http://facebook.github.io/react-native/docs/image.html#best-camera-roll-image) : does your implementation do something different/more about it ?

About your PR : what's the status on that ?

@bparadie

This comment has been minimized.

Show comment
Hide comment
@bparadie

bparadie May 13, 2015

@aroth You might want to remove npm-debug.log from your aroth@453064b commit.

@aroth You might want to remove npm-debug.log from your aroth@453064b commit.

@aroth

This comment has been minimized.

Show comment
Hide comment
@aroth

aroth May 13, 2015

Contributor

I've created a pull request at #1263 to address the slow CameraRoll image load issue only.

As for asset representation (full screen, full resolution, or thumbnail) -- I would like to address that concern separately in another Issue/PR. As is, even with this PR, loading several full resolution CameraRoll images will cause memory to spike and the app will likely crash. Thumbnail versions should be accessible and used when appropriate.

@VonD - I may be wrong, but it appears that the docs are a bit misleading. If you review RCTImageLoader.m you will see that the full resolution image is loaded for any imageTag passed into loadImageWithTag: with an assets-library prefix.

UIImage *image = [UIImage imageWithCGImage:[representation fullResolutionImage] scale:1.0f orientation:(UIImageOrientation)orientation];

My previous implementation did not automatically handle this. Instead, it required passing in a representation string (full, screen or thumbnail). I'm looking into the issue further to see if it can be handled in a more efficient way.

Contributor

aroth commented May 13, 2015

I've created a pull request at #1263 to address the slow CameraRoll image load issue only.

As for asset representation (full screen, full resolution, or thumbnail) -- I would like to address that concern separately in another Issue/PR. As is, even with this PR, loading several full resolution CameraRoll images will cause memory to spike and the app will likely crash. Thumbnail versions should be accessible and used when appropriate.

@VonD - I may be wrong, but it appears that the docs are a bit misleading. If you review RCTImageLoader.m you will see that the full resolution image is loaded for any imageTag passed into loadImageWithTag: with an assets-library prefix.

UIImage *image = [UIImage imageWithCGImage:[representation fullResolutionImage] scale:1.0f orientation:(UIImageOrientation)orientation];

My previous implementation did not automatically handle this. Instead, it required passing in a representation string (full, screen or thumbnail). I'm looking into the issue further to see if it can be handled in a more efficient way.

@VonD

This comment has been minimized.

Show comment
Hide comment
@VonD

VonD May 20, 2015

Contributor

@aroth thanks for the clarification. Have you been able to make progress on making the representation configurable ? That would indeed be very useful. Should I open an issue for that ?
Also, if the reprensentation is configurable, the orientation should be too, to avoid, e.g. displaying an upside down thumbnail.

Contributor

VonD commented May 20, 2015

@aroth thanks for the clarification. Have you been able to make progress on making the representation configurable ? That would indeed be very useful. Should I open an issue for that ?
Also, if the reprensentation is configurable, the orientation should be too, to avoid, e.g. displaying an upside down thumbnail.

@brentvatne

This comment has been minimized.

Show comment
Hide comment
@brentvatne

brentvatne May 30, 2015

Collaborator

The fix for this has been merged!

Collaborator

brentvatne commented May 30, 2015

The fix for this has been merged!

@brentvatne brentvatne closed this May 30, 2015

@brentvatne

This comment has been minimized.

Show comment
Hide comment
@brentvatne

brentvatne May 30, 2015

Collaborator

@VonD - if the fix doesn't solve all of your issues, it would be great if you could open another issue! 😄

Collaborator

brentvatne commented May 30, 2015

@VonD - if the fix doesn't solve all of your issues, it would be great if you could open another issue! 😄

@aroth

This comment has been minimized.

Show comment
Hide comment
@aroth

aroth May 31, 2015

Contributor

@brentvatne Thanks, this was merged internally by @nicklockwood a couple of weeks ago.

@VonD We still need support for configurable representation and orientation of ALAssets.

https://github.com/facebook/react-native/blob/master/Libraries/Image/RCTImageLoader.m#L81
https://github.com/facebook/react-native/blob/master/Libraries/Image/RCTImageLoader.m#L82

Another issue should be opened. I will try to involve myself this week. Thanks again.

Contributor

aroth commented May 31, 2015

@brentvatne Thanks, this was merged internally by @nicklockwood a couple of weeks ago.

@VonD We still need support for configurable representation and orientation of ALAssets.

https://github.com/facebook/react-native/blob/master/Libraries/Image/RCTImageLoader.m#L81
https://github.com/facebook/react-native/blob/master/Libraries/Image/RCTImageLoader.m#L82

Another issue should be opened. I will try to involve myself this week. Thanks again.

@bparadie

This comment has been minimized.

Show comment
Hide comment
@bparadie

bparadie Jun 9, 2015

@brentvatne @VonD CameraRoll in 0.6.0 is still dog slow for me.

Any thoughts on this note from the ALAssetRepresentation documentation?

IMPORTANT

The Assets Library framework is deprecated as of iOS 9.0. Instead, use the Photos framework instead, which in iOS 8.0 and later provides more features and better performance for working with a user’s photo library. For more information, see Photos Framework Reference.

In the Photos framework, the PHAsset and PHImageManager classes provide functionality for fetching an asset’s image or video data.

bparadie commented Jun 9, 2015

@brentvatne @VonD CameraRoll in 0.6.0 is still dog slow for me.

Any thoughts on this note from the ALAssetRepresentation documentation?

IMPORTANT

The Assets Library framework is deprecated as of iOS 9.0. Instead, use the Photos framework instead, which in iOS 8.0 and later provides more features and better performance for working with a user’s photo library. For more information, see Photos Framework Reference.

In the Photos framework, the PHAsset and PHImageManager classes provide functionality for fetching an asset’s image or video data.

@VonD

This comment has been minimized.

Show comment
Hide comment
@VonD

VonD Jun 9, 2015

Contributor

The issue about the image representation can be found here : #1567

Contributor

VonD commented Jun 9, 2015

The issue about the image representation can be found here : #1567

aroth added a commit to aroth/react-native that referenced this issue Jul 13, 2015

Automatically scale iOS assets based on the target dimensions specifi…
…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 #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 facebook locked as resolved and limited conversation to collaborators May 30, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.