From d94ff4bdbec7f1141d16cf97d3ceed485c9a1d36 Mon Sep 17 00:00:00 2001 From: Dan Field Date: Thu, 6 Feb 2020 13:19:46 -0800 Subject: [PATCH] Fix LRUness of ScrollAwareImageProvider (#50242) --- .../lib/src/painting/image_provider.dart | 12 ++++ .../widgets/scroll_aware_image_provider.dart | 16 +++-- .../test/painting/image_test_utils.dart | 7 +- .../scroll_aware_image_provider_test.dart | 66 +++++++++++++++++++ 4 files changed, 93 insertions(+), 8 deletions(-) diff --git a/packages/flutter/lib/src/painting/image_provider.dart b/packages/flutter/lib/src/painting/image_provider.dart index 8c0ac718854..04ec59f8ea6 100644 --- a/packages/flutter/lib/src/painting/image_provider.dart +++ b/packages/flutter/lib/src/painting/image_provider.dart @@ -411,6 +411,18 @@ abstract class ImageProvider { /// [ImageCache]. @protected void resolveStreamForKey(ImageConfiguration configuration, ImageStream stream, T key, ImageErrorListener handleError) { + // This is an unusual edge case where someone has told us that they found + // the image we want before getting to this method. We should avoid calling + // load again, but still update the image cache with LRU information. + if (stream.completer != null) { + final ImageStreamCompleter completer = PaintingBinding.instance.imageCache.putIfAbsent( + key, + () => stream.completer, + onError: handleError, + ); + assert(identical(completer, stream.completer)); + return; + } final ImageStreamCompleter completer = PaintingBinding.instance.imageCache.putIfAbsent( key, () => load(key, PaintingBinding.instance.instantiateImageCodec), diff --git a/packages/flutter/lib/src/widgets/scroll_aware_image_provider.dart b/packages/flutter/lib/src/widgets/scroll_aware_image_provider.dart index 90f301aec6b..851aa1eff19 100644 --- a/packages/flutter/lib/src/widgets/scroll_aware_image_provider.dart +++ b/packages/flutter/lib/src/widgets/scroll_aware_image_provider.dart @@ -74,12 +74,16 @@ class ScrollAwareImageProvider extends ImageProvider { T key, ImageErrorListener handleError, ) { - // Something managed to complete the stream. Nothing left to do. - if (stream.completer != null) { - return; - } - // Something else got this image into the cache. Return it. - if (PaintingBinding.instance.imageCache.containsKey(key)) { + // Something managed to complete the stream, or it's already in the image + // cache. Notify the wrapped provider and expect it to behave by not + // reloading the image since it's already resolved. + // Do this even if the context has gone out of the tree, since it will + // update LRU information about the cache. Even though we never showed the + // image, it was still touched more recently. + // Do this before checking scrolling, so that if the bytes are available we + // render them even though we're scrolling fast - there's no additional + // allocations to do for texture memory, it's already there. + if (stream.completer != null || PaintingBinding.instance.imageCache.containsKey(key)) { imageProvider.resolveStreamForKey(configuration, stream, key, handleError); return; } diff --git a/packages/flutter/test/painting/image_test_utils.dart b/packages/flutter/test/painting/image_test_utils.dart index c690f762e78..4fbd7a2e0ce 100644 --- a/packages/flutter/test/painting/image_test_utils.dart +++ b/packages/flutter/test/painting/image_test_utils.dart @@ -18,6 +18,7 @@ class TestImageProvider extends ImageProvider { final Completer _completer = Completer.sync(); ImageConfiguration configuration; + int loadCallCount = 0; @override Future obtainKey(ImageConfiguration configuration) { @@ -31,8 +32,10 @@ class TestImageProvider extends ImageProvider { } @override - ImageStreamCompleter load(TestImageProvider key, DecoderCallback decode) => - OneFrameImageStreamCompleter(_completer.future); + ImageStreamCompleter load(TestImageProvider key, DecoderCallback decode) { + loadCallCount += 1; + return OneFrameImageStreamCompleter(_completer.future); + } ImageInfo complete() { final ImageInfo imageInfo = ImageInfo(image: testImage); diff --git a/packages/flutter/test/widgets/scroll_aware_image_provider_test.dart b/packages/flutter/test/widgets/scroll_aware_image_provider_test.dart index e86179ab007..1f2b56cff85 100644 --- a/packages/flutter/test/widgets/scroll_aware_image_provider_test.dart +++ b/packages/flutter/test/widgets/scroll_aware_image_provider_test.dart @@ -327,8 +327,74 @@ void main() { expect(imageCache.containsKey(testImageProvider), true); expect(imageCache.currentSize, 1); + expect(testImageProvider.loadCallCount, 1); expect(stream.completer, null); }); + + testWidgets('ScrollAwareImageProvider does not block LRU updates to image cache', (WidgetTester tester) async { + final int oldSize = imageCache.maximumSize; + imageCache.maximumSize = 1; + + final GlobalKey key = GlobalKey(); + final ScrollController scrollController = ScrollController(); + await tester.pumpWidget(Directionality( + textDirection: TextDirection.ltr, + child: SingleChildScrollView( + physics: ControllablePhysics(), + controller: scrollController, + child: TestWidget(key), + ), + )); + + final DisposableBuildContext context = DisposableBuildContext(key.currentState); + const TestImage testImage = TestImage(width: 10, height: 10); + final TestImageProvider testImageProvider = TestImageProvider(testImage); + final ScrollAwareImageProvider imageProvider = ScrollAwareImageProvider( + context: context, + imageProvider: testImageProvider, + ); + + expect(testImageProvider.configuration, null); + expect(imageCache.containsKey(testImageProvider), false); + + final ControllablePhysics physics = _findPhysics(tester); + physics.recommendDeferredLoadingValue = true; + + final ImageStream stream = imageProvider.resolve(ImageConfiguration.empty); + + expect(testImageProvider.configuration, null); + expect(stream.completer, null); + expect(imageCache.currentSize, 0); + + // Occupy the only slot in the cache with another image. + final TestImageProvider testImageProvider2 = TestImageProvider(const TestImage()); + testImageProvider2.complete(); + await precacheImage(testImageProvider2, context.context); + expect(imageCache.containsKey(testImageProvider), false); + expect(imageCache.containsKey(testImageProvider2), true); + expect(imageCache.currentSize, 1); + + // Complete the original image while we're still scrolling fast. + testImageProvider.complete(); + stream.setCompleter(testImageProvider.load(testImageProvider, PaintingBinding.instance.instantiateImageCodec)); + + // Verify that this hasn't chagned the cache state yet + expect(imageCache.containsKey(testImageProvider), false); + expect(imageCache.containsKey(testImageProvider2), true); + expect(imageCache.currentSize, 1); + expect(testImageProvider.loadCallCount, 1); + + await tester.pump(); + + // After pumping a frame, the original image should be in the cache because + // it took the LRU slot. + expect(imageCache.containsKey(testImageProvider), true); + expect(imageCache.containsKey(testImageProvider2), false); + expect(imageCache.currentSize, 1); + expect(testImageProvider.loadCallCount, 1); + + imageCache.maximumSize = oldSize; + }); } class TestWidget extends StatefulWidget {