Load local assets synchronously to prevent image flicker #8102

janicduplessis · 2016-06-14T09:06:59Z

This uses [UIImage imageNamed:] to load local assets that are bundled using require('../image/path.png') and makes sure it is done synchronously on the main queue to prevent images from flickering. This improves user experience a lot when using large local images and prevents icon flickers to match the behaviour of most native apps.

This adds to methods to the ImageLoader protocol, one to tell if the image loader must be executed on the url cache queue and one to tell if the result of the image loader should be cached. I then use these to make the LocalImageLoader bypass the url cache queue and avoid caching images twice.

Note that this doesn't affect debug builds since images are loaded from the packager.

I'm not sure if we want to still support async loading of local images as I'm not sure how much of a perf difference this will make. Maybe someone at fb can benchmark this see how it affects your apps but there wasn't a noticeable one in mine. Also I only enabled this for loading png and jpg images, not sure if we want to add more there.

Test plan
Tested my branch on an app that makes heavy use of local images and made sure there is no flicker and that the code is executed synchronously. Also did some testing in UIExplorer to make sure everything still looked fine.

facebook-github-bot · 2016-06-14T09:07:44Z

By analyzing the blame information on this pull request, we identified @nicklockwood and @rigdern to be potential reviewers.

janicduplessis · 2016-06-14T09:15:19Z

Related issue #2570

javache · 2016-06-14T09:19:51Z

Libraries/Image/RCTImageLoader.m

@@ -480,6 +470,18 @@ - (RCTImageLoaderCancellationBlock)loadImageWithURLRequest:(NSURLRequest *)image
  __block void(^cancelLoad)(void) = nil;
  __weak RCTImageLoader *weakSelf = self;

+  // Load handlers are executed before the generic caching logic so they are responsible
+  // for caching the images if necessary.
+  id<RCTImageURLLoader> loadHandler = [self imageURLLoaderForURL:imageURLRequest.URL];


Moving this here means that all photo loaders implementing this API will now skip caching. This includes for example the RCTPhotoLibraryImageLoader and RCTImageLoaderHelpers.

Good catch, I did a search for the protocol in xcode but it probably only searched in the current project. Does this still seems like the right approach if there are other loaders that depend on the decoded image cache?

Maybe passing an additional parameter to the completion block of RCTImageURLLoader so loaders can decide if the result will be cached or not.

We probably need another approach. To clarify: your issue is that we first have to schedule this on the _URLCacheQueue and then we go to the main queue right? Keep in mind that we also do this for memory reasons, if you start loading a ton of images on the main thread, skipping our queueing, you'll run into OOMs as well.

I'd rather expose an (optional) API on the loadHandler asking for things like shouldCacheImageForURL? Then if you want to, you can skip that part. That doesn't solve the threading approach though.

Yes we have to skip the URLCache queue and also avoid caching the resulting image as this is already handled by UIImage imageNamed, we definetly don't want to cache then twice. Afaik memory should not be an issue since it only applies to local images and they are usually small icons or a few large images.

We could always add a prop like async to the image component to control this behaviour but i think it should default to loading synchronously.

Actually isn't this approach less memory intensive but potentially slower? We are loading images one by one on the main thread so there should only ever be one in memory at a given time. The url queue is useful for loading images asynchronously since there may be many image decoding happening at the same time so huge peaks of memory usage. The only thing that could happen is that many images are loaded on the url queue at the same time while images are loading on the main thread but since the main thread is loading only one image it should not be an issue.

Let's add 2 optional methods to the URLLoader protocol:

shouldCacheLoadedImages (i.e. does the loader have its own image cache, defaults to YES)

requiresScheduling (i.e. do we need to manage the invocation of this loader on our URL queue, defaults to YES)

javache · 2016-06-14T09:20:47Z

Please add to your test plan that network images and photo library images are still properly cached.

janicduplessis · 2016-06-14T18:08:30Z

@javache Update as per our discussion, also tested the photo library images behave the same as before so the loader is executed on the url cache queue and the result image is cached. Also tested that network images are still cached properly.

javache · 2016-06-14T18:25:17Z

Libraries/Image/RCTImageLoader.m

 {
  __block volatile uint32_t cancelled = 0;
  __block void(^cancelLoad)(void) = nil;
  __weak RCTImageLoader *weakSelf = self;
+  // Find suitable image URL loader
+  __block id<RCTImageURLLoader> loadHandler = [self imageURLLoaderForURL:imageURLRequest.URL];
+  __block BOOL cacheResult = [loadHandler respondsToSelector:@selector(shouldCacheLoadedImages)] ?


You only need to mark variables as __block if you plan to re-assign them from inside a block. That doesn't seem to be the case here.

janicduplessis · 2016-06-14T19:56:57Z

@javache Fixed!

javache · 2016-06-14T22:02:48Z

@facebook-github-bot import

ghost · 2016-06-14T22:03:35Z

Thanks for importing. If you are an FB employee go to Phabricator to review.

javache · 2016-06-20T16:32:20Z

Update: testing this a bit more thoroughly internally

janicduplessis · 2016-07-04T18:17:25Z

@javache Any updates on this?

ghost · 2016-07-15T17:00:43Z

It's been a while since the last commit was reviewed and the labels show this pull request needs review. Based on the blame information for the files in this pull request we identified @nicklockwood as a potential reviewer. Could you take a look please or cc someone with more context?

javache · 2016-07-19T17:41:40Z

Should land this week. Sorry for the delay.

Summary: This uses `[UIImage imageNamed:]` to load local assets that are bundled using `require('../image/path.png')` and makes sure it is done synchronously on the main queue to prevent images from flickering. This improves user experience a lot when using large local images and prevents icon flickers to match the behaviour of most native apps. This adds to methods to the ImageLoader protocol, one to tell if the image loader must be executed on the url cache queue and one to tell if the result of the image loader should be cached. I then use these to make the LocalImageLoader bypass the url cache queue and avoid caching images twice. Note that this doesn't affect debug builds since images are loaded from the packager. I'm not sure if we want to still support async loading of local images as I'm not sure how much of a perf difference this will make. Maybe someone at fb can benchmark this see how it affects your apps but there wasn't a noticeable one in mine. Also I only enabled this for loading png and jpg im Closes facebook/react-native#8102 Reviewed By: bnham Differential Revision: D3433647 Pulled By: javache fbshipit-source-id: 37bd6aff20c0465c163db3cdbcaeaedff55f7b1f

Summary: This uses `[UIImage imageNamed:]` to load local assets that are bundled using `require('../image/path.png')` and makes sure it is done synchronously on the main queue to prevent images from flickering. This improves user experience a lot when using large local images and prevents icon flickers to match the behaviour of most native apps. This adds to methods to the ImageLoader protocol, one to tell if the image loader must be executed on the url cache queue and one to tell if the result of the image loader should be cached. I then use these to make the LocalImageLoader bypass the url cache queue and avoid caching images twice. Note that this doesn't affect debug builds since images are loaded from the packager. I'm not sure if we want to still support async loading of local images as I'm not sure how much of a perf difference this will make. Maybe someone at fb can benchmark this see how it affects your apps but there wasn't a noticeable one in mine. Also I only enabled this for loading png and jpg im Closes facebook#8102 Reviewed By: bnham Differential Revision: D3433647 Pulled By: javache fbshipit-source-id: 37bd6aff20c0465c163db3cdbcaeaedff55f7b1f

AlexDM0 · 2016-09-06T15:34:46Z

Hi, is this in RN yet? The flickering issue still persists...

janicduplessis · 2016-09-06T16:55:12Z

This is in version 0.32+. Dont forget that it only affects when building in release mode and loading the bundle from disk.

AlexDM0 · 2016-09-06T19:47:08Z

Great, thank you!

I was still on 0.31 and didn't see it in the change log. Must have missed it.

Cheers!

On 06 Sep 2016, at 18:56, Janic Duplessis notifications@github.com wrote:

This is in version 0.32+. Dont forget that it only affects when building in release mode and loading the bundle from disk.

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or mute the thread.

waqas19921 · 2016-09-19T06:00:43Z

I experienced this still in 0.32.0

JetA2 · 2021-01-28T09:06:10Z

Is there a similar fix for Android (using RN 0.63.2)?

Load local assets synchronously to prevent image flicker

654778c

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 Jun 14, 2016

javache reviewed Jun 14, 2016
View reviewed changes

Add cache and scheduling parameters to image loaders

e837958

janicduplessis force-pushed the image-flicker-ios branch from 40dbc26 to e837958 Compare June 14, 2016 17:55

javache reviewed Jun 14, 2016
View reviewed changes

Remove useless __blocks

019b191

javache mentioned this pull request Jun 20, 2016

Change RCTImageLoader's Cache System to default NSURLRequest's cache system #8235

Closed

ghost 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 Jul 12, 2016

ghost 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 Jul 15, 2016

ghost closed this in 5903949 Jul 21, 2016

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Load local assets synchronously to prevent image flicker #8102

Load local assets synchronously to prevent image flicker #8102

janicduplessis commented Jun 14, 2016 •

edited

facebook-github-bot commented Jun 14, 2016

janicduplessis commented Jun 14, 2016

javache Jun 14, 2016

janicduplessis Jun 14, 2016

janicduplessis Jun 14, 2016

javache Jun 14, 2016

javache Jun 14, 2016

janicduplessis Jun 14, 2016

janicduplessis Jun 14, 2016

janicduplessis Jun 14, 2016 •

edited

javache Jun 14, 2016

javache commented Jun 14, 2016

janicduplessis commented Jun 14, 2016 •

edited

javache Jun 14, 2016

janicduplessis commented Jun 14, 2016

javache commented Jun 14, 2016

ghost commented Jun 14, 2016

javache commented Jun 20, 2016

janicduplessis commented Jul 4, 2016

ghost commented Jul 15, 2016

javache commented Jul 19, 2016

AlexDM0 commented Sep 6, 2016

janicduplessis commented Sep 6, 2016

AlexDM0 commented Sep 6, 2016

waqas19921 commented Sep 19, 2016

JetA2 commented Jan 28, 2021

Load local assets synchronously to prevent image flicker #8102

Load local assets synchronously to prevent image flicker #8102

Conversation

janicduplessis commented Jun 14, 2016 • edited

facebook-github-bot commented Jun 14, 2016

janicduplessis commented Jun 14, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janicduplessis Jun 14, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javache commented Jun 14, 2016

janicduplessis commented Jun 14, 2016 • edited

Choose a reason for hiding this comment

janicduplessis commented Jun 14, 2016

javache commented Jun 14, 2016

ghost commented Jun 14, 2016

javache commented Jun 20, 2016

janicduplessis commented Jul 4, 2016

ghost commented Jul 15, 2016

javache commented Jul 19, 2016

AlexDM0 commented Sep 6, 2016

janicduplessis commented Sep 6, 2016

AlexDM0 commented Sep 6, 2016

waqas19921 commented Sep 19, 2016

JetA2 commented Jan 28, 2021

janicduplessis commented Jun 14, 2016 •

edited

janicduplessis Jun 14, 2016 •

edited

janicduplessis commented Jun 14, 2016 •

edited