Live image cache #50318

dnfield · 2020-02-07T06:33:23Z

WIP - this should not land until #50316; design doc for this is at https://flutter.dev/go/widget-tree-image-cache, although the approach has changed a bit since writing the doc. /cc @goderbauer @ds84182 @liyuqian @gaaclarke @ignatz @alml

Description

Related Issues

fixes #48731
fixes #49456
fixes #45406

Tests

I added the following tests:

Assert that the cache can weakly hold an image even if it's otherwise too big to fit in the cache or the cache is disabled
Assert that the cache does not weakly hold an image that has no listeners.
Test that an ImageProvider can handle errors when trying to find its cache location
Test that an ImageProvider can find its cache location
Test that precacheImage weakly holds an image reference for a frame (but no longer)
Test that this is enough time for someone to grab the stream and hold the image in memory even if it is not in strongly in the cache.

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

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

I don't believe this is a breaking change, but once this is cleaned up a bit I'll manually run it through google's internal test suites.

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/template Replace this with a link to your design doc's short link
- 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-02-07T06:33:43Z

Argh. I meant to open this as a draft. Oh well.

dnfield · 2020-02-07T06:35:11Z

packages/flutter/test/widgets/image_test.dart

-    expect(imageStreamCompleter.listeners.length, 0);
+    // Make sure the second listener can be called re-entrantly.
+    listeners[0].onImage(null, null);
+    listeners[0].onImage(null, null);


This is meant to land in #50316

dnfield · 2020-02-07T06:57:11Z

packages/flutter/lib/src/painting/image_cache.dart

+///
+/// To obtain an [ImageCacheLocation], use [FlutterImageCache.locationForKey] or
+/// [ImageProvider.findCacheLocation].
+class ImageCacheLocation {


/cc @jamesderlin who was looking for a way to know if an image was pending in the cache.

dnfield · 2020-02-07T08:22:27Z

packages/flutter/test/painting/image_cache_test.dart

+      final FlutterImageCache flutterImageCache = imageCache as FlutterImageCache;
+      expect(flutterImageCache.locationForKey(testImage).pending, true);
+      expect(flutterImageCache.locationForKey(testImage).weak, true);
+      flutterImageCache.clear();
+      expect(flutterImageCache.locationForKey(testImage).pending, false);
+      expect(flutterImageCache.locationForKey(testImage).weak, true);
+      flutterImageCache.clearWeakImages();
+      expect(flutterImageCache.locationForKey(testImage).pending, false);
+      expect(flutterImageCache.locationForKey(testImage).weak, false);


clear does remove the pending image, but this test no longer captures how to check if an image is pending or not.

This test could be independently rewritten without these changes to use the existing containsKey API, although the new API offers more robust ways to check if an image is pending.

Hixie · 2020-02-11T19:02:17Z

packages/flutter/lib/src/painting/image_cache.dart

+  /// This method exists so that [FlutterImageCache] can add to its weak cache
+  /// once an [ImageStreamCompleter] has been created by this cache.
+  @protected
+  void _onImageStreamCompleterCreated(Object key, ImageStreamCompleter completer) {}


_handleImageStreamCompleterCreated per style guide

Removing this since we're just going to break the API

Hixie · 2020-02-11T19:03:18Z

packages/flutter/lib/src/painting/image_cache.dart

+
+  /// Clears any weak references to images in this cache.
+  ///
+  /// This should not ordinarily be called by applications.


Rather than saying it shouldn't be called, say when it should be called.

Hixie · 2020-02-11T19:15:11Z

packages/flutter/lib/src/painting/image_cache.dart

+/// This cache also exposes internal dimensions of the implementation, such as
+/// checking whether an image for a given key is pending, weak, or completed.
+/// See [ImageCacheLocation] for more details.
+class FlutterImageCache extends ImageCache {


in the api design review meeting we discussed just merging FlutterImageCache into ImageCache, and make the test-only features be methods called debugFoo... with their bodies in asserts.

I'm leaning towards making these non-debug/assert methods at this point - this could be valuable information at runtime (e.g. if an application wants to know how many live images are on the screen before decoding more). I'm also documenting [precacheImage] as relying on this behavior now.

Hixie · 2020-02-11T19:18:12Z

packages/flutter/lib/src/widgets/image.dart

-      stream.removeListener(listener);
+      // Give callers at least one frame to subscribe to the image stream
+      // so that FlutterImageCache can weakly hold it without losing it.
+      SchedulerBinding.instance.scheduleFrameCallback((Duration timeStamp) {


should probably be schedulePostFrameCallback so that you don't actually schedule a frame

Done, and updated tests relevant to this to actually schedule a frame where needed.

Hixie · 2020-02-11T19:18:35Z

packages/flutter/lib/src/painting/binding.dart

@@ -65,9 +65,11 @@ mixin PaintingBinding on BindingBase, ServicesBinding {



please avoid the word "weak" throughout

dnfield · 2020-02-14T01:13:09Z

I've merged in the image tracing tests. This should be fully ready for review and landing on approval.

dnfield · 2020-02-14T18:34:31Z

(Fixed some bugs in my last push - I had forgotten to delete some code and had a bad assert in there)

goderbauer · 2020-02-14T18:31:27Z

packages/flutter/lib/src/painting/image_cache.dart

  /// Returns whether this `key` has been previously added by [putIfAbsent].
  bool containsKey(Object key) {
    return _pendingImages[key] != null || _cache[key] != null;
  }

+  /// The number of live images being held by the [ImageCache].
+  ///
+  /// Compare with [ImageCache.currentSize] for completed held images.


Not sure what "completed held images" is?

Missed a text replacement. done.

goderbauer · 2020-02-14T18:32:25Z

packages/flutter/lib/src/painting/image_cache.dart

+///
+/// A [pending] image is one that has not completed yet. It may also be tracked
+/// as [live] because something is listening to it.
+///


nit: remove one blank line.

goderbauer · 2020-02-14T18:37:03Z

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

+    };
+
+
+    // If an error is added to a synchronous completer before a listener has been


Wait, isn't the other occurence also in this file? Could this be just be refactored into a private method? I don't think there's any danger of anybody accessing a private method from here?

goderbauer · 2020-02-14T18:48:05Z

packages/flutter/lib/src/painting/image_cache.dart

+      return;
+    }
+    _liveImages[key] = image;
+    image.completer.addOnLastListenerRemovedCallback(() {


What if nobody ever listens to the stream (and hence this callback never fires)? It appears that we'd never remove the image from the live cache?

dnfield · 2020-02-14T18:55:43Z

Ahh I got mixed up about the dangerzone stuff - I thought that one was on ImageCache. I'll refactor.

goderbauer

LGTM

dnfield · 2020-02-15T00:29:02Z

Quick update on this:

When I opened the PR, it was a breaking change.

During the course of work on this PR, the tests it broke no longer were breaking (in fact, they disappeared entirely!).

On landing it, it no longer breaks our compatibility guidelines (https://flutter.dev/docs/resources/compatibility). I'll still send an email to Flutter-announce explaining this.

dnfield · 2020-02-15T00:40:09Z

And if anyone would really like to see a migration guide for this, I can certainly write one :)

This reverts commit 1602be6.

This reverts commit 2f09d60.

* Revert "Revert "Live image cache (#50318)" (#51131)" This reverts commit 2f09d60. * Fix eviction of a pending image

lifelikejuly · 2020-05-11T08:09:47Z

Please I want to know version1.17.0 ImageCache add liveImage how to useful。

dnfield · 2020-05-11T13:41:40Z

It is automatic

lifelikejuly · 2020-05-11T14:04:12Z

It is automatic

Sorry, actually I mean want to ask what it does

dnfield · 2020-05-11T14:31:33Z

The design doc and linked PRs have more details, but in a nutshell this makes sure that once you resolve an image, it continues to be reachable as long as its listener count has not dropped to zero, even if it did not otherwise fit in the image cache. This is important especially for large images or cache configurations that do not allow for many images.

dnfield added 5 commits February 6, 2020 19:45

temp

c2adcff

Make onImage re-entrant safe

31a04ec

merge

da98159

Docs, tests

4580ec5

better test

4c5ae7d

fluttergithubbot added framework flutter/packages/flutter repository. See also f: labels. team Infra upgrades, team productivity, code health, technical debt. See also team: labels. labels Feb 7, 2020

googlebot added the cla: yes label Feb 7, 2020

dnfield added the waiting for tree to go green label Feb 7, 2020

bad import

f9bf360

dnfield commented Feb 7, 2020

View reviewed changes

dnfield added 2 commits February 6, 2020 22:35

unintended pubspec change

cda0d6b

docs and comparisons

1dba7d2

dnfield commented Feb 7, 2020

View reviewed changes

dnfield added 2 commits February 6, 2020 23:42

merge

02aae6e

comment

45f9770

dnfield added work in progress; do not review and removed waiting for tree to go green labels Feb 7, 2020

update test

257ef91

dnfield force-pushed the weak_image_cache branch from d953dda to 257ef91 Compare February 7, 2020 08:20

dnfield commented Feb 7, 2020

View reviewed changes

This was referenced Feb 7, 2020

precacheImage should let you preserve the image it loads even if it doesn't fit in the cache #48731

Closed

Image cache eviction policy is too aggressive #45406

Closed

dnfield added 3 commits February 7, 2020 12:24

merge

6e7b93c

merge

d619811

merge

3cd8ee7

Hixie reviewed Feb 11, 2020

View reviewed changes

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

9b65965

dnfield changed the title ~~Weak image cache~~ Live image cache Feb 11, 2020

dnfield added 4 commits February 13, 2020 10:39

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

b64c1f2

doc fixes, touch, edge cases for live images and sync calls

1cb06c4

status

f565da3

merge

827911e

test

d60d64e

dnfield force-pushed the weak_image_cache branch from 611b5a8 to d60d64e Compare February 14, 2020 17:13

goderbauer reviewed Feb 14, 2020

View reviewed changes

dnfield added 2 commits February 14, 2020 12:35

factor out common code

a0bd901

tests, listen even if disabled

01e4b25

goderbauer approved these changes Feb 14, 2020

View reviewed changes

dnfield merged commit 1602be6 into flutter:master Feb 14, 2020

dnfield deleted the weak_image_cache branch February 14, 2020 23:03

dnfield added the a: images Loading, displaying, rendering images label Feb 18, 2020

dnfield added a commit that referenced this pull request Feb 20, 2020

Revert "Live image cache (#50318)"

d506171

This reverts commit 1602be6.

dnfield mentioned this pull request Feb 20, 2020

Revert "Live image cache" #51131

Merged

dnfield added a commit that referenced this pull request Feb 20, 2020

Revert "Live image cache (#50318)" (#51131)

2f09d60

This reverts commit 1602be6.

dnfield added a commit to dnfield/flutter that referenced this pull request Feb 22, 2020

Revert "Revert "Live image cache (flutter#50318)" (flutter#51131)"

542c547

This reverts commit 2f09d60.

dnfield mentioned this pull request Feb 22, 2020

Live image cache #51249

Merged

dnfield added a commit that referenced this pull request Feb 24, 2020

Live image cache (#51249)

e2dcdb6

* Revert "Revert "Live image cache (#50318)" (#51131)" This reverts commit 2f09d60. * Fix eviction of a pending image

github-actions bot locked as resolved and limited conversation to collaborators Jul 31, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Live image cache #50318

Live image cache #50318

dnfield commented Feb 7, 2020 •

edited

Loading

dnfield commented Feb 7, 2020

dnfield Feb 7, 2020

dnfield Feb 7, 2020

dnfield Feb 7, 2020

Hixie Feb 11, 2020

dnfield Feb 11, 2020

Hixie Feb 11, 2020

dnfield Feb 11, 2020

Hixie Feb 11, 2020

dnfield Feb 11, 2020

Hixie Feb 11, 2020

dnfield Feb 11, 2020

Hixie Feb 11, 2020

dnfield commented Feb 14, 2020

dnfield commented Feb 14, 2020

goderbauer Feb 14, 2020

dnfield Feb 14, 2020

goderbauer Feb 14, 2020

dnfield Feb 14, 2020

goderbauer Feb 14, 2020

goderbauer Feb 14, 2020

dnfield commented Feb 14, 2020

goderbauer left a comment

dnfield commented Feb 15, 2020

dnfield commented Feb 15, 2020

lifelikejuly commented May 11, 2020

dnfield commented May 11, 2020

lifelikejuly commented May 11, 2020

dnfield commented May 11, 2020

		@@ -65,9 +65,11 @@ mixin PaintingBinding on BindingBase, ServicesBinding {

		};


		// If an error is added to a synchronous completer before a listener has been

Live image cache #50318

Live image cache #50318

Conversation

dnfield commented Feb 7, 2020 • edited Loading

Description

Related Issues

Tests

Checklist

Breaking Change

dnfield commented Feb 7, 2020

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

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

dnfield commented Feb 14, 2020

dnfield commented Feb 14, 2020

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

dnfield commented Feb 14, 2020

goderbauer left a comment

Choose a reason for hiding this comment

dnfield commented Feb 15, 2020

dnfield commented Feb 15, 2020

lifelikejuly commented May 11, 2020

dnfield commented May 11, 2020

lifelikejuly commented May 11, 2020

dnfield commented May 11, 2020

dnfield commented Feb 7, 2020 •

edited

Loading