Defer image decoding when scrolling fast. by dnfield · Pull Request #48536 · flutter/flutter

dnfield · 2020-01-09T22:52:57Z

Description

A List or GridView of images can end up consuming vast amounts of memory, particularly when the user "flings" the list causing the scrollable to go ballistic.

The basic problem is that we push a ton of decoding work onto the IO thread of the engine, and then end up not even needing those images because the children aren't in the tree anymore. This causes thrash on the image cache, wasted CPU load for decoding those images, and in some cases OOM death.

I've resolved this by adding a new ImageProvider implementation that allows the caller to defer or cancel loading the image. The ImageState now uses this to wrap its incoming image provider, and defers loading until scrolling has slowed down enough that we would be seen on screen (defined as scrolling velocity < the physical number of pixels on the longest side of the screen). If we're no longer in the tree at that point, we cancel the load. This provider is cache-aware, and will just return the image in the cache if at any point during its cycle the image appears in the cache.

/cc @hugosantos @ignatz @alml @cvarvara
/cc @liyuqian @chinmaygarde

Related Issues

#32143
#44510
b/144232910

#48305
Fixes #48775

Tests

I added the following tests:

Tests that the image cache can tell us whether it has a pending or completed image for a given key
Tests that the DeferringImageProvider can cancel, defer, or resolve an image
Tests that the DeferringImageProvider will check the cache (in case someone pre-cached the image, or in case another widget managed to cache it for us since we last checked).
Tests that a large grid view of images driven by a high velocity ballistic simulation do not try to load images during the high velocity part of the simulation.

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I signed the CLA.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I updated/added relevant documentation (doc comments with ///).
All existing and new tests are passing.
The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
I am willing to follow-up on review comments in a timely manner.

Breaking Change

Right now this is - I'm open to feedback suggesting ways to make it not be.

Will add link to design doc shortly - will add link to migration guide if this remains an API break before landing.

Did any tests fail when you ran them? Please read Handling breaking changes.

No, no existing tests failed, so this is not a breaking change.
Yes, this is a breaking change. If not, delete the remainder of this section.
- I wrote a design doc: https://flutter.dev/go/deferred-image-decoding
- I got input from the developer relations team, specifically from: Replace with the names of who gave advice
- I wrote a migration guide: Replace with a link to your migration guide

dnfield · 2020-01-09T22:53:29Z


  @override
  void dispose() {
-    assert(_imageStream != null);


We might not have ever gotten an _imageStream before being disposed of if we were scrolling fast.

How can this happen? Isn't provider.resolve in line 1036 always synchronously returning a stream? I also don't see code that changed when we call resolve on the provider...

I'll put this back. This was from a different experiment

dnfield · 2020-01-09T22:57:34Z

  }

+  /// Returns whether this [key] has been previously added by [putIfAbsent].
+  bool containsKey(Object key) {


This is breaking (we have internal customers that implement ImageCache). There are ugly ways to make it not be breaking (e.g. make putIfAbsent tolerate having a loader that returns null, which is documented as being bad but not tested against, and we have internal implementers that tolerate it).

I think it's worth the break but am willing to hear other ideas.

Don't hesitate to make breaking changes if they are valuable. Just make sure to go through the process (which involves getting feedback about whether it's valuable).

dnfield · 2020-01-09T22:58:09Z

-  /// method.
+  /// method. If they need to manage the actual resolution of the image, they
+  /// should override [resolveForStream] instead.
+  @nonVirtual


This is a bigger break that I need to make the way I'm doing DeferringImageProvider work. I don't to call resolve from there and end up decoding work before I'm ready.

Interestingly, this would not break any internal customers or existing customer tests - it only breaks our own test implementations.

jonahwilliams · 2020-01-09T23:10:21Z

+  int get hashCode => hashValues(imageProvider, getNextAction);
+
+  @override
+  String toString() => '$runtimeType(imageProvider: $imageProvider)';


please don't use runtimeType.toString()

it is incredibly slow

This is our standard toString template right?

(we do this everywhere)

That doesn't make it fast

Fair - how about we handle this in a separate PR and upate the style guide?

(or file a bug against Dart SDK to make it be fast?)

it's fine to use runtimeType.toString, just don't use it in release builds.

This is done.

Co-Authored-By: Jonah Williams <[email protected]>

dnfield · 2020-01-09T23:57:16Z

Related: #32156

gaaclarke · 2020-01-10T00:56:24Z

+    return stream;
+  }
+
+  /// Subclasses that need to manage resolution more finely should override


The documentation should probably emphasize what this method actually does. Then as a note add information about overriding. It's hard to overrride if you don't know the contract of the method.

gaaclarke · 2020-01-10T01:03:17Z

+        return;
+      }
+      switch (getNextAction()) {
+        case DeferringImageProviderAction.defer:


Do we really need defer? Seems like the cancelling work you did could be sufficient for your problem.

Yes. Without this our only options are to either immediately cancel or immediately resolve. We want to be able to defer until a condition is met (scrolling has slowed)

We also need an image provider here so we can see if we even should bother waiting or not - if the image is already in the cache we shouldn't wait.

gaaclarke · 2020-01-10T01:05:56Z

+        // to see if we've slowed down or disappeared.
+        // It's safe to use these calls as point-in-time - do not subscribe to
+        // updates of MediaQuery or Scrollable.
+        final double maxPhysicalDimension = WidgetsBinding.instance.window.physicalSize.longestSide;


Nit: being explicit on units here can help verify the math.

Are you asking for a different variable name or something else?

Yep, putting the units in the variable name is the easiest change.

Ahh sure. That makes sense.

Just to be a bit more obvious. I've seen velocity calculations get messed up when people mix logical pixels and hardware pixels.

I am intentionally doing that but I'll add some explanation.

Basically, we want something a factor bigger than logical pixels, and I'm assuming this is safe because higher dpi should mean more memory available on device.

Pushed up some changes to the docs and this property name.

gaaclarke · 2020-01-10T01:06:47Z

  ScrollActivity _activity;

+  /// The current [ScrollActivity.velocity] of the current activity, if any.
+  double get currentScrollVelocity => activity?.velocity;


Document units in docstring and/or property name.

This is still outstanding =)

Hm. I thought I did this one, I think I did the similar one instead. Will update.

GitHub is messed up here. The text acutally reads:

/// /// This value is in logical pixels per second. It may be positive, negative, /// 0, or null.

In addition to what's showing right now. You can see it at: f732fbe

Cool thanks, what's a velocity of null mean?

Hm. I might be wrong about this. I was thinking there was some code path somewhere that could set it to null or set the activity to null. I'm having trouble finding it. @goderbauer may know if this is accurate.

Oh, derp.

This can return null if you're not in a scrollable context :)

It's not clear to me why this should return null.

This can return null if you're not in a scrollable context :)

How would you have a ScrollPosition if you're not in a scrollable context?

Also: What does it mean from an API point of view when your velocity is null? Shouldn't this always return negative, 0 or positive numbers?

You are right, I think I copied this over from scrollable.dart, which is where it can be null. Removed the bit about null here.

gaaclarke · 2020-01-15T00:57:45Z

+        // It's safe to use these calls as point-in-time - do not subscribe to
+        // updates of MediaQuery or Scrollable.
+        final double maxPhysicalPixels = WidgetsBinding.instance.window.physicalSize.longestSide;
+        if ((Scrollable.scrollingVelocityOfContext(context) ?? 0).abs() > maxPhysicalPixels) {


You are mixing units here, velocity versus physical pixels. I'd compare velocity to velocity.

It's intentional though. The idea is the DPI is a good heuristic to use for this case.

That's fine, i'd document it with another variable like.
double fullScreenPerSecond = maxPhysicalPixels;

Is there some scenario where we could end up with enough velocity to meet this condition indefinitely? For example, some sort of absurdly pixel scaling combined with a looping animation?

In such a situation we'd have to be scrolling at high velocity indefinitely. Or, we'd have to be in some kind of crazy scroll activity that's lying to us about how fast it's scrolling indefinitely.

I suppose the most pathological case I can think of is an animation that loops at high velocity such that it scrolls up and down very very fast. If you wanted the image to ever show up in that case, you'd have to precache it.

And it couldn't be just any animation, it would have to be an animation that drove a scroll controller such that the activity used was a ballistic activity scrolling at high velocity.

gaaclarke

+1 . I'd like to get another lgtm. The documentation and testing is good and I don't see any obvious bugs. I still don't 100% grok the necessity of the defer and I'm not too keen on the usage of the DPI as a proxy to fudge the heuristics.

jonahwilliams · 2020-01-15T03:12:14Z

+            scheduleMicrotask(() => deferredResolve(key));
+          });
+          return;
+        case DeferringImageProviderAction.cancel:


cancel seems like it could stop an image from being loaded, but not once it starts loading? It should be possible to cancel NetworkRequests too, though I'm not sure if HttpClient exposes an API for it

We can look at cancelling once a load starts, but this PR isn't trying to solve that one.

Part of the problem there is that it will be very hard once we actually start the machinery on the IO thread in the engine to cancel it before allocations have happened.

Network request handling could use some love but I think that's orthogonal to this issue. This is trying to stop us from even making the network request to begin with if we won't need it.

I think it would be unfortunate if we had to come up with an entirely separate solution for network requests. I might propose a slightly more general API:

The deferring image provider accepts a controller/token instead of a callback. This controller maintains a state flag similar to what the callback is providing now.

The controller state can be modified via setters/methods like cancel() or resume().

The cancellation could be propagated up through the network image provider, by exposing some functionality on ImageStream perhaps? Ideally this could eventually be wired up the HttpClient and others.

My sense is that places we've introduced controllers (e.g. TextEditing) have really only caused more confusion and API challenges than they resolve. I think it will also become very confusing if cancel behaves differently in different contexts - did I cancel before making the request? Did I cancel the active request? Did I cancel it early enough to have effect? Did I call cancel after the whole thing was already done?

I think for NetworkRequests made by the framework in general, we should create another InheritedWidget that would handle things like this (caching requests, cancelling requests, etc). WDYT?

I don't really agree but I'm not involved enough in the problem, so I'll defer to the rest of the framework team

goderbauer · 2020-01-16T20:52:59Z

  /// Subclasses should implement [obtainKey] and [load], which are used by this
-  /// method.
+  /// method. If they need to manage the actual resolution of the image, they
+  /// should override [resolveForStream] instead.


nit: instead of what?

Added a couple words (of this method)

goderbauer · 2020-01-16T20:54:54Z

+/// never fire and the cached image is reused.
+@immutable
+class DeferringImageProvider<T> extends ImageProvider<T> {
+


nit: remove extra blank line

goderbauer · 2020-01-16T20:56:31Z

+  /// This callback is never called if the [ImageCache] already has the image
+  /// present, such as from [precacheImage] or another provider successfully
+  /// loading it.
+  final DeferringImageProviderAction Function() getNextAction;


Use a typedef

goderbauer · 2020-01-16T20:59:45Z

  String toString() => '${objectRuntimeType(this, 'ExactAssetImage')}(name: "$keyName", scale: $scale, bundle: $bundle)';
 }

+/// The action a [DeferringImageProvider] should take currently.


Maybe expand on this a little and mention that the getNextAction callback on DeferringImageProvider is supposed to return these.

goderbauer · 2020-01-16T21:01:05Z

+  /// its [DeferringImageProvider.getNextAction] callback anymore.
+  cancel,
+
+  /// Indicates that the caller would like to defer the image and be called


Maybe clearer:

Suggested change

/// Indicates that the caller would like to defer the image and be called

/// Indicates that the caller would like to defer resolving the image and be called

goderbauer · 2020-01-16T21:27:29Z


  @override
  void dispose() {
-    assert(_imageStream != null);


How can this happen? Isn't provider.resolve in line 1036 always synchronously returning a stream? I also don't see code that changed when we call resolve on the provider...

goderbauer · 2020-01-16T21:27:48Z


  void _listenToStream() {
-    if (_isListeningToStream)
+    if (_isListeningToStream || _imageStream == null)


Same here (and below): how can this be null?

Removed - this was from a previous experiment that I forgot to clean up.

goderbauer · 2020-01-16T21:29:06Z

+        // It's safe to use these calls as point-in-time - do not subscribe to
+        // updates of MediaQuery or Scrollable.
+        final double maxPhysicalPixels = WidgetsBinding.instance.window.physicalSize.longestSide;
+        if ((Scrollable.scrollingVelocityOfContext(context) ?? 0).abs() > maxPhysicalPixels) {


What if I very quickly and continuously scroll up and down: Does this block loading images forever?

You would have to precache the image in such a case.

And for most cases, you'd eventually lose velocity long enough at some point between frames (during a direction change) that it should still end up working out.

goderbauer · 2020-01-16T21:31:28Z

  ScrollActivity _activity;

+  /// The current [ScrollActivity.velocity] of the current activity, if any.
+  double get currentScrollVelocity => activity?.velocity;


It's not clear to me why this should return null.

This can return null if you're not in a scrollable context :)

How would you have a ScrollPosition if you're not in a scrollable context?

Also: What does it mean from an API point of view when your velocity is null? Shouldn't this always return negative, 0 or positive numbers?

goderbauer · 2020-01-16T21:33:10Z

+  /// is only safe to use as a single point in time reference.
+  ///
+  /// This value is in logical pixels per second. It may be positive, negative,
+  /// 0, or null.


Document what null means?

(here it's correct what you said above: You're not in a scrollable context)

goderbauer · 2020-01-16T21:56:57Z

+        // It's safe to use these calls as point-in-time - do not subscribe to
+        // updates of MediaQuery or Scrollable.
+        final double maxPhysicalPixels = WidgetsBinding.instance.window.physicalSize.longestSide;
+        if ((Scrollable.scrollingVelocityOfContext(context) ?? 0).abs() > maxPhysicalPixels) {


It is a little surprising to tie our Image implementation to our implementation of Scrollable. If somebody were to write a new Scrolling lib (because they don't like our sliver-based on or whatever) they would not be able to make uses of this feature for images shown in their scrolling container... This my be more hypothetical, though.

Yes, I agree this is a problem. I'm not sure what we can do about it at the moment. I think we should punt on it for now and deal with it if it comes up - we could rework this so that you could find other ways to notify widgets about how fast you are scrolling.

Hixie · 2020-01-22T18:52:40Z

+    return stream;
+  }
+
+  /// This method does the acutal work of [resolve], both decoding the image


typo actual

Hixie · 2020-01-22T18:54:19Z

+  ///
+  /// While it returns [DeferringImageProviderAction.defer], it schedules a new
+  /// call during the [SchedulerPhase.midFrameMicrotasks] of the next frame.
+  final DeferringImageProviderActionCallback handleDeferringImageProviderAction;


per our style guide this should be called onSomething.

Hixie · 2020-01-22T18:55:15Z

+  @override
+  void resolveForStream(ImageConfiguration configuration, ImageStream stream) {
+    void deferredResolve(Object key) {
+      if (PaintingBinding.instance.imageCache.containsKey(key)) {


we should document that this image provider only works with other image providers that use the cache correctly

Hixie · 2020-01-22T18:58:16Z

+        // Did this element get disposed of (scrolled too far out of view before
+        // scrolling slowed down enough to catch up)?
+        if (!mounted) {
+          return DeferringImageProviderAction.cancel;


it really would be better if we could do that in dispose...

Hixie · 2020-01-22T19:03:17Z

+          return;
+      }
+    }
+    obtainKey(configuration).then(deferredResolve);


this line drops errors

Hixie · 2020-01-22T19:04:53Z

+  }
+
+  /// This method does the acutal work of [resolve], both decoding the image
+  /// and making sure it ends up in the [ImageCache].


"Decode the image and add it to the [ImageCache] if necessary".

Hixie · 2020-01-22T19:31:46Z

@goderbauer @dnfield and I discussed this a bit. Some thoughts:

the heuristic probably belongs on the physics, that way it can be overridden in crazy cases involving weird transforms, etc.
we could call this something more general, like "shouldDeferLoads", to make it clearer that this can be reused in other context, e.g. whether to load news reports when scrolling through a news feed.

dnfield · 2020-01-22T19:59:42Z

Talked offline a bit more -

It seems like this logic would be more comfortable living more in the widgets layer. Going to experiment with that to see if I can clean up some of the rough edges here.

Hixie · 2020-01-22T20:17:23Z

Further discussion with @dnfield suggested that maybe what we should do is move the new image provider into the widgets layer, so it can know about BuildContext and Scrollable, and have it create a new ImageStream or ImageStreamCompleter that figures out whether to cancel based on when it runs out of listeners.

dnfield · 2020-01-23T17:08:31Z

I'm going to close this - the approach I'm going to take has some similarities to this but should be an improved API. At this point, there's lots of out of date comments on this PR and the approach is different enough that I'd like to not lose all the context here. Will follow up with the next PR after #49319 lands (second PR will depend on that one)

dnfield · 2020-01-24T00:48:35Z

Continued in #49389

dnfield added 3 commits January 9, 2020 09:29

start

91aebd5

Merge remote-tracking branch 'upstream/master' into image_scrolling

f6d8fa0

tests

c038a73

dnfield added c: API break Backwards-incompatible API changes f: scrolling Viewports, list views, slivers, etc. a: images Loading, displaying, rendering images perf: memory Performance issues related to memory labels Jan 9, 2020

dnfield requested review from Hixie, cbracken, gaaclarke and goderbauer January 9, 2020 22:52

fluttergithubbot added the framework flutter/packages/flutter repository. See also f: labels. label Jan 9, 2020

googlebot added the cla: yes label Jan 9, 2020

dnfield commented Jan 9, 2020

View reviewed changes

jonahwilliams reviewed Jan 9, 2020

View reviewed changes

Comment thread packages/flutter/lib/src/painting/image_provider.dart Outdated

jonahwilliams reviewed Jan 9, 2020

View reviewed changes

Update packages/flutter/lib/src/painting/image_provider.dart

dbc7dd7

Co-Authored-By: Jonah Williams <[email protected]>

fix bad merge

75d3091

This was referenced Jan 9, 2020

Memory Leak flutter framework iOS - gridview, listview - it IS a bug... #32143

Closed

Flutter Image list Crashing #32820

Closed

gaaclarke reviewed Jan 10, 2020

View reviewed changes

dnfield added 3 commits January 9, 2020 19:54

aaron review

f732fbe

update tests for new scheduling logic

0522fb7

delete hashcode/==

f074ee1

This was referenced Jan 13, 2020

memory in ios increase so fast that cause crash #36753

Closed

How to stop image load when scrolling list ? #48775

Closed

It should be possible to exclude an image from the ImageCache #47380

Open

dnfield requested a review from gaaclarke January 14, 2020 23:54

dnfield added 2 commits January 14, 2020 16:43

Merge remote-tracking branch 'upstream/master' into image_scrolling

7cc16ed

runtimeType

83653ff

gaaclarke reviewed Jan 15, 2020

View reviewed changes

gaaclarke approved these changes Jan 15, 2020

View reviewed changes

jonahwilliams reviewed Jan 15, 2020

View reviewed changes

goderbauer reviewed Jan 16, 2020

View reviewed changes

dnfield added 2 commits January 16, 2020 14:23

goderbauer review

75b9109

remove rescheduling

656ef9c

dnfield mentioned this pull request Jan 22, 2020

[OOM] memory consumption is too high #48305

Closed

Hixie reviewed Jan 22, 2020

View reviewed changes

dnfield mentioned this pull request Jan 22, 2020

recommendDeferredLoading #49319

Merged

9 tasks done

dnfield closed this Jan 23, 2020

dnfield mentioned this pull request Jan 23, 2020

Defer image decoding when scrolling fast #49389

Merged

11 of 13 tasks

dnfield deleted the image_scrolling branch February 7, 2020 06:52

dnfield mentioned this pull request Feb 7, 2020

Memory leaks on iOS when working with images #45391

Closed

dnfield mentioned this pull request Feb 22, 2020

Flutter abort issue resulted from large memory use of pictures in low-memory iPhones. #51046

Closed

Piinks mentioned this pull request Feb 16, 2021

Scrolling over a large delta in a very short amount of time in ListView.builder is significantly slower #75399

Open

github-actions Bot locked as resolved and limited conversation to collaborators Aug 1, 2021

	/// Indicates that the caller would like to defer the image and be called
	/// Indicates that the caller would like to defer resolving the image and be called

Conversation

dnfield commented Jan 9, 2020 • edited by goderbauer Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Tests

Checklist

Breaking Change

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnfield commented Jan 9, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

dnfield commented Jan 9, 2020 •

edited by goderbauer

Loading

gaaclarke Jan 15, 2020 •

edited

Loading