Defer image decoding when scrolling fast #49389

dnfield · 2020-01-23T23:45:01Z

Description

Continuation of #48536

Dependds on #49319

Basically an API cleanup of #48536

Related Issues

#32143
fixes #44510
b/144232910

#48305
Fixes #48775

Tests

I added the following tests:

Tests for new provider, new method on image cache (containsKey), image behavior in a scrolling list.

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.

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: Migration guide for flutter/flutter#49389 website#3622

dnfield · 2020-01-23T23:45:49Z

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

@@ -227,6 +227,11 @@ class ImageCache {
    return result;
  }

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


This is a break.

dnfield · 2020-01-23T23:46:02Z

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

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


This is a break.

why must it be nonVirtual? I mean, I get that it's useful for catching people who are using the API the old way, but is it strictly necessary? What if someone wants to take an entirely different approach?

Because if some other provider overrides it, its new logic will never get called by the new kind of provider I'm making that would wrap it.

I imagine other providers would have this problem. Plus the logic in it is very complicated.

Alternatively, we could update docs to say "if you override this, composed providers may not work and you should consider overriding useKey instead" - but I'm inclined to think people are more likely to see an analyzer warning than a doc comment.

…dget

Hixie · 2020-01-24T01:04:26Z

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

+    return stream;
+  }
+
+  void _safeObtainKey(ImageConfiguration configuration, ImageStream stream) {


"_safeObtainKey" doesn't return a key, which is weird naming

Hixie · 2020-01-24T01:05:52Z

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

@@ -270,10 +270,19 @@ abstract class ImageProvider<T> {
  /// This is the public entry-point of the [ImageProvider] class hierarchy.
  ///
  /// 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 of this method.


docs are out of date

Hixie · 2020-01-24T01:06:52Z

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

@@ -270,10 +270,19 @@ abstract class ImageProvider<T> {
  /// This is the public entry-point of the [ImageProvider] class hierarchy.


please add a section to the class docs (## foo) that discusses the lifecycle of this class and the order in which the various methods are called, by whom, when, and so on. The class is complicated enough now that an overview is necessary.

Hixie · 2020-01-24T01:07:34Z

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

+  ///
+  /// Subclasses should avoid calling [obtainKey] directly and instead override
+  /// this method, which makes sure appropriate error handling is in place to
+  /// notify callers and the framework.


this paragraph is confusing given the first paragraph

Hixie · 2020-01-24T01:08:02Z

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

+  /// this method, which makes sure appropriate error handling is in place to
+  /// notify callers and the framework.
+  ///
+  /// It is safe to call [handleError] multiple times if multiple errors occur.


Does this mean "It is safe for the implementation of this method to call..." ?

Hixie · 2020-01-24T01:08:16Z

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

+  ///
+  /// The default implementation uses the key to interact with the [ImageCache],
+  /// calling [ImageCache.putIfAbsent] and notifying listeners of the [stream].
+  /// Implementers that do not call super should


Hixie · 2020-01-24T01:10:07Z

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

+  /// The [State] must not be null, and [State.mounted] must be true.
+  ScrollAwareContextProvider(this._state)
+      : assert(_state != null),
+        assert(_state.mounted, 'A ScrollAwareContextProvider was given a BuildContext for an Element that was never mounted.');


was never -> is not

Hixie · 2020-01-24T01:11:00Z

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

+    return true;
+  }
+
+  /// Do not touch this.  Use [_context].


Not clear who this doc line is for. It's a private, so presumably it's for us. Bet we touch it 7 lines lower.

It's meant to say don't touch it from the provider. But it's probably not necessary, I'll just remove it.

Hixie · 2020-01-24T01:12:53Z

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

+    assert(key != null);
+    assert(handleError != null);
+
+    void deferredResolve([bool firstCall = false]) {


do you need this to be a nested function? looks like you could get the same results with a regular method, which would be less confusing to follow

dnfield · 2020-01-24T22:16:09Z

I've updated the docs and moved the build context enclosing class to a new class called DisposableBuildContext.

Still waiting for the dependent PR to land so I can rebase it into this one and then mark this ready for review.

dnfield · 2020-01-25T16:11:05Z

Ready for review now!

goderbauer

LGTM after comments are resolved.

goderbauer · 2020-01-29T19:36:46Z

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

@@ -227,6 +227,11 @@ class ImageCache {
    return result;
  }

+  /// Returns whether this [key] has been previously added by [putIfAbsent].


nit: key should be surrounded in `` instead of [] (it's referring a method parameter that does not exist as a field on the sounding context).

goderbauer · 2020-01-29T20:04:15Z

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

@@ -182,6 +182,42 @@ typedef DecoderCallback = Future<ui.Codec> Function(Uint8List bytes, {int cacheW
 ///
 /// The following image formats are supported: {@macro flutter.dart:ui.imageFormats}
 ///
+/// ## Image resolving


nit: Maybe just make this heading "Lifecycle"? Or "Lifecycle of resolving an image" or some such?

goderbauer · 2020-01-29T20:06:05Z

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

+///      This stream will be used to communicate back to the caller when the
+///      image is decoded and ready to display, or when an error occurs.
+///   2. Obtain the key for the image using [obtainKey].
+///      Since this method is asynchronous, but also can return synchronously,


This sentence is strange. I assume you mean it's an asychnronous method, but it may throw exceptions synchronously? There is no way it can actually return anything useful synchronous?

We use SynchronousFutures for these, which look async but are actually return synchronously.

Reworded a bit.

goderbauer · 2020-01-29T20:07:48Z

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

-  /// method.
+  /// method. If they need to change the implementation of [ImageStream] used,
+  /// they should override [createStream]. If they need to manage the actual
+  /// resolution of the image, they should override [resolveStreamForKey].


Maybe add a link to the lifecycle explanation of this method in the class comment?

Done - I think.

goderbauer · 2020-01-29T20:10:24Z

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

-    return stream;
+  }
+
+  /// Called when [resolve] has finished calling [obtainKey].


Maybe: Called by [resolve] with the key returned by [obtainKey].

goderbauer · 2020-01-29T20:17:27Z

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

+  }) : assert(context != null),
+       assert(imageProvider != null);
+
+  /// The context that may or may not be enclosed by a scrollable.


nit: by a [Scrollable]?

goderbauer · 2020-01-29T20:21:02Z

packages/flutter/test/widgets/scroll_aware_image_provider_test.dart

+    return Scrollable.of(find.byType(TestWidget).evaluate().first).position;
+  }
+
+  testWidgets('DisposableBuildContext asserts on disposed state', (WidgetTester tester) async {


Maybe move the DisposableBuildContext tests into their own file since the impl has its own home as well.

goderbauer · 2020-01-29T20:22:46Z

packages/flutter/test/widgets/scroll_aware_image_provider_test.dart

+    expect(state.mounted, true);
+
+    final DisposableBuildContext context = DisposableBuildContext(state);
+    expect(context.debugValidate(), true);


Instead of marking debugValidate visible for tests, could we just test the same by actually accessing the context property and checking that we do or don't get an assert?

(That would also prevent us from ever removing that assert from the context getter)

goderbauer · 2020-01-29T20:23:29Z

packages/flutter/test/widgets/scroll_aware_image_provider_test.dart

+    velocities.add(velocity);
+    return super.recommendDeferredLoading(velocity, metrics, context);
+  }
+}


nit: add a newline

goderbauer · 2020-01-29T20:24:08Z

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

+  /// Always returns true, but will assert if [dispose] has not been called
+  /// but the state this is tracking is unmounted.
+  @visibleForTesting
+  bool debugValidate() {


as pointed out in a comment on the test: Can we keep this private and instead test if accessing context throws?

…dget

dnfield

Thanks for the review!

We should not land this until cl/289271973 is landed internally so it's a soft break. I'll work on a migration guide.

dnfield · 2020-01-29T22:47:44Z

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

@@ -182,6 +182,42 @@ typedef DecoderCallback = Future<ui.Codec> Function(Uint8List bytes, {int cacheW
 ///
 /// The following image formats are supported: {@macro flutter.dart:ui.imageFormats}
 ///
+/// ## Image resolving


dnfield · 2020-01-29T22:47:49Z

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

@@ -227,6 +227,11 @@ class ImageCache {
    return result;
  }

+  /// Returns whether this [key] has been previously added by [putIfAbsent].


dnfield · 2020-01-29T22:49:21Z

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

+  /// Called by [resolve] to create the [ImageStream] it returns.
+  ///
+  /// Subclasses should override this instead of [resolve] if they need to
+  /// return some subclass of [ImageStream].


dnfield · 2020-01-29T22:49:49Z

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

-    return stream;
+  }
+
+  /// Called when [resolve] has finished calling [obtainKey].


dnfield · 2020-01-29T22:50:46Z

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

+  /// Always returns true, but will assert if [dispose] has not been called
+  /// but the state this is tracking is unmounted.
+  @visibleForTesting
+  bool debugValidate() {


dnfield · 2020-01-29T22:51:24Z

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

+  }) : assert(context != null),
+       assert(imageProvider != null);
+
+  /// The context that may or may not be enclosed by a scrollable.


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

packages/flutter/test/widgets/scroll_aware_image_provider_test.dart

+    expect(state.mounted, true);
+
+    final DisposableBuildContext context = DisposableBuildContext(state);
+    expect(context.debugValidate(), true);


dnfield · 2020-01-29T22:54:13Z

packages/flutter/test/widgets/scroll_aware_image_provider_test.dart

+    return Scrollable.of(find.byType(TestWidget).evaluate().first).position;
+  }
+
+  testWidgets('DisposableBuildContext asserts on disposed state', (WidgetTester tester) async {


dnfield · 2020-01-29T22:54:32Z

packages/flutter/test/widgets/scroll_aware_image_provider_test.dart

+    velocities.add(velocity);
+    return super.recommendDeferredLoading(velocity, metrics, context);
+  }
+}


dnfield · 2020-01-29T23:02:48Z

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

-  /// method.
+  /// method. If they need to change the implementation of [ImageStream] used,
+  /// they should override [createStream]. If they need to manage the actual
+  /// resolution of the image, they should override [resolveStreamForKey].


Done - I think.

dnfield · 2020-01-30T17:52:27Z

Internal CL has landed.

* Migration guide for flutter/flutter#49389 * .. * typo * Update image-cache-and-provider.md * Update index.md

Gperez88 · 2020-04-30T20:47:13Z

when they move this to stable branch?

* Improve: Merge from [Defer image decoding when scrolling fast](flutter/flutter#49389). * Flutter sdk minimum version limit to 1.17.0.

dnfield added 6 commits January 22, 2020 14:04

recommendDeferredLoading

2b21d0a

newline

e830b2c

another test

c36c86f

typo

b2f2017

Merge branch 'physics_fast' into image_provider_widget

b281914

start

81df156

dnfield added framework flutter/packages/flutter repository. See also f: labels. c: API break Backwards-incompatible API changes f: scrolling Viewports, list views, slivers, etc. a: images Loading, displaying, rendering images labels Jan 23, 2020

fluttergithubbot added the work in progress; do not review label Jan 23, 2020

dnfield requested a review from goderbauer January 23, 2020 23:45

googlebot added the cla: yes label Jan 23, 2020

dnfield requested review from gaaclarke and Hixie January 23, 2020 23:45

dnfield commented Jan 23, 2020

View reviewed changes

dnfield changed the title ~~Image provider widget~~ Defer image decoding when scrolling fast Jan 23, 2020

dnfield mentioned this pull request Jan 24, 2020

Defer image decoding when scrolling fast. #48536

Closed

13 tasks

dnfield added 2 commits January 23, 2020 16:54

Merge remote-tracking branch 'upstream/master' into image_provider_wi…

a7b14c3

…dget

fixes

616d829

Hixie reviewed Jan 24, 2020

View reviewed changes

docs, refactor, review

7501ec3

dnfield force-pushed the image_provider_widget branch from 572a727 to 7501ec3 Compare January 24, 2020 22:12

merge

bf5d693

dnfield marked this pull request as ready for review January 25, 2020 16:10

dnfield removed the work in progress; do not review label Jan 25, 2020

dnfield added 2 commits January 25, 2020 11:22

fix bad import

874c324

bad merge

cc06576

dnfield force-pushed the image_provider_widget branch from 6df2db7 to cc06576 Compare January 25, 2020 22:11

Hixie mentioned this pull request Jan 29, 2020

[OOM] memory consumption is too high #48305

Closed

goderbauer approved these changes Jan 29, 2020

View reviewed changes

dnfield added 2 commits January 29, 2020 14:41

Merge remote-tracking branch 'upstream/master' into image_provider_wi…

1212dbd

…dget

review

15cc44c

dnfield commented Jan 29, 2020

View reviewed changes

dnfield added a commit to dnfield/website that referenced this pull request Jan 29, 2020

Migration guide for flutter/flutter#49389

490c63e

dnfield added the waiting for tree to go green label Jan 30, 2020

fluttergithubbot merged commit 169529c into flutter:master Jan 30, 2020

dnfield deleted the image_provider_widget branch January 30, 2020 22:11

This was referenced Feb 3, 2020

Flutter needs a way to handle memory constraints around image decoding. #50024

Closed

Fix LRUness of ScrollAwareImageProvider #50242

Merged

sfshaza2 pushed a commit to flutter/website that referenced this pull request Feb 6, 2020

Migration guide for flutter/flutter#49389 (#3622)

edb4ff8

* Migration guide for flutter/flutter#49389 * .. * typo * Update image-cache-and-provider.md * Update index.md

dnfield mentioned this pull request Feb 7, 2020

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

Open

dnfield mentioned this pull request Feb 25, 2020

Loading & displaying images from files causes crash #21571

Closed

dnfield mentioned this pull request Mar 4, 2020

Introduce memory cap for so that scrolling past many images does not spike memory in an unbounded fashion #49316

Closed

fredlee12345678 mentioned this pull request Jun 5, 2020

Defer Image Decode unavailable on OPPO A57 when scroll fast #58762

Closed

zmtzawqlp added a commit to fluttercandies/extended_image that referenced this pull request Aug 4, 2020

## [1.0.0]

f574ea3

* Improve: Merge from [Defer image decoding when scrolling fast](flutter/flutter#49389). * Flutter sdk minimum version limit to 1.17.0.

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

		@@ -270,10 +270,19 @@ abstract class ImageProvider<T> {
		/// This is the public entry-point of the [ImageProvider] class hierarchy.

Defer image decoding when scrolling fast #49389

Defer image decoding when scrolling fast #49389

Conversation

dnfield commented Jan 23, 2020 • edited Loading

Description

Related Issues

Tests

Checklist

Breaking Change

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnfield commented Jan 24, 2020

dnfield commented Jan 25, 2020

goderbauer left a 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

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 left a 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 Jan 30, 2020

Gperez88 commented Apr 30, 2020

dnfield commented Jan 23, 2020 •

edited

Loading