From 419a2853a8e4ac846524afeb49b866d4cdaa7d9d Mon Sep 17 00:00:00 2001 From: Dan Field Date: Tue, 25 Feb 2020 16:52:47 -0800 Subject: [PATCH] Revert "Live image cache" (#51441) * Revert "Live image cache (#51249)" This reverts commit e2dcdb60e327f80d414d3d1e72e2863bf4c9252c. * fix tests for other commit --- .../test/image_cache_tracing_test.dart | 7 +- .../flutter/lib/src/painting/binding.dart | 1 - .../flutter/lib/src/painting/image_cache.dart | 248 ++---------------- .../lib/src/painting/image_provider.dart | 99 ++----- .../lib/src/painting/image_stream.dart | 22 -- packages/flutter/lib/src/widgets/image.dart | 33 +-- .../flutter/test/painting/binding_test.dart | 87 ------ .../test/painting/image_cache_test.dart | 109 +------- .../test/painting/image_provider_test.dart | 51 +--- packages/flutter/test/widgets/image_test.dart | 218 +-------------- 10 files changed, 66 insertions(+), 809 deletions(-) diff --git a/dev/tracing_tests/test/image_cache_tracing_test.dart b/dev/tracing_tests/test/image_cache_tracing_test.dart index 6eac37689e..e4d786d6c7 100644 --- a/dev/tracing_tests/test/image_cache_tracing_test.dart +++ b/dev/tracing_tests/test/image_cache_tracing_test.dart @@ -20,7 +20,7 @@ void main() { final developer.ServiceProtocolInfo info = await developer.Service.getInfo(); if (info.serverUri == null) { - fail('This test _must_ be run with --enable-vmservice.'); + throw TestFailure('This test _must_ be run with --enable-vmservice.'); } await timelineObtainer.connect(info.serverUri); await timelineObtainer.setDartFlags(); @@ -58,8 +58,7 @@ void main() { 'name': 'ImageCache.clear', 'args': { 'pendingImages': 1, - 'keepAliveImages': 0, - 'liveImages': 1, + 'cachedImages': 0, 'currentSizeInBytes': 0, 'isolateId': isolateId, } @@ -150,7 +149,7 @@ class TimelineObtainer { Future close() async { expect(_completers, isEmpty); - await _observatorySocket?.close(); + await _observatorySocket.close(); } } diff --git a/packages/flutter/lib/src/painting/binding.dart b/packages/flutter/lib/src/painting/binding.dart index d5f00a981d..cbdb882dbc 100644 --- a/packages/flutter/lib/src/painting/binding.dart +++ b/packages/flutter/lib/src/painting/binding.dart @@ -96,7 +96,6 @@ mixin PaintingBinding on BindingBase, ServicesBinding { void evict(String asset) { super.evict(asset); imageCache.clear(); - imageCache.clearLiveImages(); } /// Listenable that notifies when the available fonts on the system have diff --git a/packages/flutter/lib/src/painting/image_cache.dart b/packages/flutter/lib/src/painting/image_cache.dart index 6f03b6ffdd..63034dbba5 100644 --- a/packages/flutter/lib/src/painting/image_cache.dart +++ b/packages/flutter/lib/src/painting/image_cache.dart @@ -3,7 +3,6 @@ // found in the LICENSE file. import 'dart:developer'; -import 'dart:ui' show hashValues; import 'package:flutter/foundation.dart'; @@ -16,24 +15,18 @@ const int _kDefaultSizeBytes = 100 << 20; // 100 MiB /// /// Implements a least-recently-used cache of up to 1000 images, and up to 100 /// MB. The maximum size can be adjusted using [maximumSize] and -/// [maximumSizeBytes]. -/// -/// The cache also holds a list of "live" references. An image is considered -/// live if its [ImageStreamCompleter]'s listener count has never dropped to -/// zero after adding at least one listener. The cache uses -/// [ImageStreamCompleter.addOnLastListenerRemovedCallback] to determine when -/// this has happened. +/// [maximumSizeBytes]. Images that are actively in use (i.e. to which the +/// application is holding references, either via [ImageStream] objects, +/// [ImageStreamCompleter] objects, [ImageInfo] objects, or raw [dart:ui.Image] +/// objects) may get evicted from the cache (and thus need to be refetched from +/// the network if they are referenced in the [putIfAbsent] method), but the raw +/// bits are kept in memory for as long as the application is using them. /// /// The [putIfAbsent] method is the main entry-point to the cache API. It /// returns the previously cached [ImageStreamCompleter] for the given key, if /// available; if not, it calls the given callback to obtain it first. In either /// case, the key is moved to the "most recently used" position. /// -/// A caller can determine whether an image is already in the cache by using -/// [containsKey], which will return true if the image is tracked by the cache -/// in a pending or compelted state. More fine grained information is available -/// by using the [statusForKey] method. -/// /// Generally this class is not used directly. The [ImageProvider] class and its /// subclasses automatically handle the caching of images. /// @@ -78,11 +71,6 @@ const int _kDefaultSizeBytes = 100 << 20; // 100 MiB class ImageCache { final Map _pendingImages = {}; final Map _cache = {}; - /// ImageStreamCompleters with at least one listener. These images may or may - /// not fit into the _pendingImages or _cache objects. - /// - /// Unlike _cache, the [_CachedImage] for this may have a null byte size. - final Map _liveImages = {}; /// Maximum number of entries to store in the cache. /// @@ -162,27 +150,20 @@ class ImageCache { int get currentSizeBytes => _currentSizeBytes; int _currentSizeBytes = 0; - /// Evicts all pending and keepAlive entries from the cache. + /// Evicts all entries from the cache. /// /// This is useful if, for instance, the root asset bundle has been updated /// and therefore new images must be obtained. /// /// Images which have not finished loading yet will not be removed from the /// cache, and when they complete they will be inserted as normal. - /// - /// This method does not clear live references to images, since clearing those - /// would not reduce memory pressure. Such images still have listeners in the - /// application code, and will still remain resident in memory. - /// - /// To clear live references, use [clearLiveImages]. void clear() { if (!kReleaseMode) { Timeline.instantSync( 'ImageCache.clear', arguments: { 'pendingImages': _pendingImages.length, - 'keepAliveImages': _cache.length, - 'liveImages': _liveImages.length, + 'cachedImages': _cache.length, 'currentSizeInBytes': _currentSizeBytes, }, ); @@ -193,23 +174,11 @@ class ImageCache { } /// Evicts a single entry from the cache, returning true if successful. - /// /// Pending images waiting for completion are removed as well, returning true - /// if successful. When a pending image is removed the listener on it is - /// removed as well to prevent it from adding itself to the cache if it - /// eventually completes. + /// if successful. /// - /// If this method removes a pending image, it will also remove - /// the corresponding live tracking of the image, since it is no longer clear - /// if the image will ever complete or have any listeners, and failing to - /// remove the live reference could leave the cache in a state where all - /// subsequent calls to [putIfAbsent] will return an [ImageStreamCompleter] - /// that will never complete. - /// - /// If this method removes a completed image, it will _not_ remove the live - /// reference to the image, which will only be cleared when the listener - /// count on the completer drops to zero. To clear live image references, - /// whether completed or not, use [clearLiveImages]. + /// When a pending image is removed the listener on it is removed as well to + /// prevent it from adding itself to the cache if it eventually completes. /// /// The `key` must be equal to an object used to cache an image in /// [ImageCache.putIfAbsent]. @@ -229,19 +198,13 @@ class ImageCache { }); } pendingImage.removeListener(); - // Remove from live images - the cache will not be able to mark - // it as complete, and it might be getting evicted because it - // will never complete, e.g. it was loaded in a FakeAsync zone. - // In such a case, we need to make sure subsequent calls to - // putIfAbsent don't return this image that may never complete. - _liveImages.remove(key); return true; } final _CachedImage image = _cache.remove(key); if (image != null) { if (!kReleaseMode) { Timeline.instantSync('ImageCache.evict', arguments: { - 'type': 'keepAlive', + 'type': 'completed', 'sizeiInBytes': image.sizeBytes, }); } @@ -256,34 +219,6 @@ class ImageCache { return false; } - /// Updates the least recently used image cache with this image, if it is - /// less than the [maximumSizeBytes] of this cache. - /// - /// Resizes the cache as appropriate to maintain the constraints of - /// [maximumSize] and [maximumSizeBytes]. - void _touch(Object key, _CachedImage image, TimelineTask timelineTask) { - assert(timelineTask != null); - if (image.sizeBytes != null && image.sizeBytes <= maximumSizeBytes) { - _currentSizeBytes += image.sizeBytes; - _cache[key] = image; - _checkCacheSize(timelineTask); - } - } - - void _trackLiveImage(Object key, _CachedImage image) { - // Avoid adding unnecessary callbacks to the completer. - _liveImages.putIfAbsent(key, () { - // Even if no callers to ImageProvider.resolve have listened to the stream, - // the cache is listening to the stream and will remove itself once the - // image completes to move it from pending to keepAlive. - // Even if the cache size is 0, we still add this listener. - image.completer.addOnLastListenerRemovedCallback(() { - _liveImages.remove(key); - }); - return image; - }); - } - /// Returns the previously cached [ImageStream] for the given key, if available; /// if not, calls the given callback to obtain it first. In either case, the /// key is moved to the "most recently used" position. @@ -317,32 +252,17 @@ class ImageCache { } // Remove the provider from the list so that we can move it to the // recently used position below. - // Don't use _touch here, which would trigger a check on cache size that is - // not needed since this is just moving an existing cache entry to the head. final _CachedImage image = _cache.remove(key); if (image != null) { if (!kReleaseMode) { - timelineTask.finish(arguments: {'result': 'keepAlive'}); + timelineTask.finish(arguments: {'result': 'completed'}); } - // The image might have been keptAlive but had no listeners (so not live). - // Make sure the cache starts tracking it as live again. - _trackLiveImage(key, image); _cache[key] = image; return image.completer; } - final _CachedImage liveImage = _liveImages[key]; - if (liveImage != null) { - _touch(key, liveImage, timelineTask); - if (!kReleaseMode) { - timelineTask.finish(arguments: {'result': 'keepAlive'}); - } - return liveImage.completer; - } - try { result = loader(); - _trackLiveImage(key, _CachedImage(result, null)); } catch (error, stackTrace) { if (!kReleaseMode) { timelineTask.finish(arguments: { @@ -362,37 +282,21 @@ class ImageCache { if (!kReleaseMode) { listenerTask = TimelineTask(parent: timelineTask)..start('listener'); } - // If we're doing tracing, we need to make sure that we don't try to finish - // the trace entry multiple times if we get re-entrant calls from a multi- - // frame provider here. bool listenedOnce = false; - - // We shouldn't use the _pendingImages map if the cache is disabled, but we - // will have to listen to the image at least once so we don't leak it in - // the live image tracking. - // If the cache is disabled, this variable will be set. - _PendingImage untrackedPendingImage; void listener(ImageInfo info, bool syncCall) { // Images that fail to load don't contribute to cache size. final int imageSize = info?.image == null ? 0 : info.image.height * info.image.width * 4; - final _CachedImage image = _CachedImage(result, imageSize); - if (!_liveImages.containsKey(key)) { - assert(syncCall); - result.addOnLastListenerRemovedCallback(() { - _liveImages.remove(key); - }); - } - _liveImages[key] = image; - final _PendingImage pendingImage = untrackedPendingImage ?? _pendingImages.remove(key); + final _PendingImage pendingImage = _pendingImages.remove(key); if (pendingImage != null) { pendingImage.removeListener(); } - // Only touch if the cache was enabled when resolve was initially called. - if (untrackedPendingImage == null) { - _touch(key, image, listenerTask); - } + if (imageSize <= maximumSizeBytes) { + _currentSizeBytes += imageSize; + _cache[key] = image; + _checkCacheSize(listenerTask); + } if (!kReleaseMode && !listenedOnce) { listenerTask.finish(arguments: { 'syncCall': syncCall, @@ -405,58 +309,20 @@ class ImageCache { } listenedOnce = true; } - - final ImageStreamListener streamListener = ImageStreamListener(listener); if (maximumSize > 0 && maximumSizeBytes > 0) { + final ImageStreamListener streamListener = ImageStreamListener(listener); _pendingImages[key] = _PendingImage(result, streamListener); - } else { - untrackedPendingImage = _PendingImage(result, streamListener); + // Listener is removed in [_PendingImage.removeListener]. + result.addListener(streamListener); } - // Listener is removed in [_PendingImage.removeListener]. - result.addListener(streamListener); - return result; } - /// The [ImageCacheStatus] information for the given `key`. - ImageCacheStatus statusForKey(Object key) { - return ImageCacheStatus._( - pending: _pendingImages.containsKey(key), - keepAlive: _cache.containsKey(key), - live: _liveImages.containsKey(key), - ); - } - /// 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 keepAlive images. - int get liveImageCount => _liveImages.length; - - /// The number of images being tracked as pending in the [ImageCache]. - /// - /// Compare with [ImageCache.currentSize] for keepAlive images. - int get pendingImageCount => _pendingImages.length; - - /// Clears any live references to images in this cache. - /// - /// An image is considered live if its [ImageStreamCompleter] has never hit - /// zero listeners after adding at least one listener. The - /// [ImageStreamCompleter.addOnLastListenerRemovedCallback] is used to - /// determine when this has happened. - /// - /// This is called after a hot reload to evict any stale references to image - /// data for assets that have changed. Calling this method does not relieve - /// memory pressure, since the live image caching only tracks image instances - /// that are also being held by at least one other object. - void clearLiveImages() { - _liveImages.clear(); - } - // Remove images from the cache until both the length and bytes are below // maximum, or the cache is empty. void _checkCacheSize(TimelineTask timelineTask) { @@ -488,76 +354,6 @@ class ImageCache { } } -/// Information about how the [ImageCache] is tracking an image. -/// -/// A [pending] image is one that has not completed yet. It may also be tracked -/// as [live] because something is listening to it. -/// -/// A [keepAlive] image is being held in the cache, which uses Least Recently -/// Used semantics to determine when to evict an image. These images are subject -/// to eviction based on [ImageCache.maximumSizeBytes] and -/// [ImageCache.maximumSize]. It may be [live], but not [pending]. -/// -/// A [live] image is being held until its [ImageStreamCompleter] has no more -/// listeners. It may also be [pending] or [keepAlive]. -/// -/// An [untracked] image is not being cached. -/// -/// To obtain an [ImageCacheStatus], use [ImageCache.statusForKey] or -/// [ImageProvider.obtainCacheStatus]. -class ImageCacheStatus { - const ImageCacheStatus._({ - this.pending = false, - this.keepAlive = false, - this.live = false, - }) : assert(!pending || !keepAlive); - - /// An image that has been submitted to [ImageCache.putIfAbsent], but - /// not yet completed. - final bool pending; - - /// An image that has been submitted to [ImageCache.putIfAbsent], has - /// completed, fits based on the sizing rules of the cache, and has not been - /// evicted. - /// - /// Such images will be kept alive even if [live] is false, as long - /// as they have not been evicted from the cache based on its sizing rules. - final bool keepAlive; - - /// An image that has been submitted to [ImageCache.putIfAbsent] and has at - /// least one listener on its [ImageStreamCompleter]. - /// - /// Such images may also be [keepAlive] if they fit in the cache based on its - /// sizing rules. They may also be [pending] if they have not yet resolved. - final bool live; - - /// An image that is tracked in some way by the [ImageCache], whether - /// [pending], [keepAlive], or [live]. - bool get tracked => pending || keepAlive || live; - - /// An image that either has not been submitted to - /// [ImageCache.putIfAbsent] or has otherwise been evicted from the - /// [keepAlive] and [live] caches. - bool get untracked => !pending && !keepAlive && !live; - - @override - bool operator ==(Object other) { - if (other.runtimeType != runtimeType) { - return false; - } - return other is ImageCacheStatus - && other.pending == pending - && other.keepAlive == keepAlive - && other.live == live; - } - - @override - int get hashCode => hashValues(pending, keepAlive, live); - - @override - String toString() => '${objectRuntimeType(this, 'ImageCacheStatus')}(pending: $pending, live: $live, keepAlive: $keepAlive)'; -} - class _CachedImage { _CachedImage(this.completer, this.sizeBytes); diff --git a/packages/flutter/lib/src/painting/image_provider.dart b/packages/flutter/lib/src/painting/image_provider.dart index 87c29fff53..a0e65a0bfc 100644 --- a/packages/flutter/lib/src/painting/image_provider.dart +++ b/packages/flutter/lib/src/painting/image_provider.dart @@ -17,12 +17,6 @@ import 'binding.dart'; import 'image_cache.dart'; import 'image_stream.dart'; -/// Signature for the callback taken by [_createErrorHandlerAndKey]. -typedef _KeyAndErrorHandlerCallback = void Function(T key, ImageErrorListener handleError); - -/// Signature used for error handling by [_createErrorHandlerAndKey]. -typedef _AsyncKeyErrorHandler = Future Function(T key, dynamic exception, StackTrace stack); - /// Configuration information passed to the [ImageProvider.resolve] method to /// select a specific image. /// @@ -324,28 +318,7 @@ abstract class ImageProvider { final ImageStream stream = createStream(configuration); // Load the key (potentially asynchronously), set up an error handling zone, // and call resolveStreamForKey. - _createErrorHandlerAndKey( - configuration, - (T key, ImageErrorListener errorHandler) { - resolveStreamForKey(configuration, stream, key, errorHandler); - }, - (T key, dynamic exception, StackTrace stack) async { - await null; // wait an event turn in case a listener has been added to the image stream. - final _ErrorImageCompleter imageCompleter = _ErrorImageCompleter(); - stream.setCompleter(imageCompleter); - imageCompleter.setError( - exception: exception, - stack: stack, - context: ErrorDescription('while resolving an image'), - silent: true, // could be a network error or whatnot - informationCollector: () sync* { - yield DiagnosticsProperty('Image provider', this); - yield DiagnosticsProperty('Image configuration', configuration); - yield DiagnosticsProperty('Image key', key, defaultValue: null); - }, - ); - }, - ); + _createErrorHandlerAndKey(configuration, stream); return stream; } @@ -359,66 +332,30 @@ abstract class ImageProvider { return ImageStream(); } - /// Returns the cache location for the key that this [ImageProvider] creates. - /// - /// The location may be [ImageCacheStatus.untracked], indicating that this - /// image provider's key is not available in the [ImageCache]. - /// - /// The `cache` and `configuration` parameters must not be null. If the - /// `handleError` parameter is null, errors will be reported to - /// [FlutterError.onError], and the method will return null. - /// - /// A completed return value of null indicates that an error has occurred. - Future obtainCacheStatus({ - @required ImageConfiguration configuration, - ImageErrorListener handleError, - }) { + void _createErrorHandlerAndKey(ImageConfiguration configuration, ImageStream stream) { assert(configuration != null); - final Completer completer = Completer(); - _createErrorHandlerAndKey( - configuration, - (T key, ImageErrorListener innerHandleError) { - completer.complete(PaintingBinding.instance.imageCache.statusForKey(key)); - }, - (T key, dynamic exception, StackTrace stack) async { - if (handleError != null) { - handleError(exception, stack); - } else { - FlutterError.onError(FlutterErrorDetails( - context: ErrorDescription('while checking the cache location of an image'), - informationCollector: () sync* { - yield DiagnosticsProperty('Image provider', this); - yield DiagnosticsProperty('Image configuration', configuration); - yield DiagnosticsProperty('Image key', key, defaultValue: null); - }, - exception: exception, - stack: stack, - )); - completer.complete(null); - } - }, - ); - return completer.future; - } - - /// This method is used by both [resolve] and [obtainCacheStatus] to ensure - /// that errors thrown during key creation are handled whether synchronous or - /// asynchronous. - void _createErrorHandlerAndKey( - ImageConfiguration configuration, - _KeyAndErrorHandlerCallback successCallback, - _AsyncKeyErrorHandler errorCallback, - ) { + assert(stream != null); T obtainedKey; bool didError = false; Future handleError(dynamic exception, StackTrace stack) async { if (didError) { return; } - if (!didError) { - errorCallback(obtainedKey, exception, stack); - } didError = true; + await null; // wait an event turn in case a listener has been added to the image stream. + final _ErrorImageCompleter imageCompleter = _ErrorImageCompleter(); + stream.setCompleter(imageCompleter); + imageCompleter.setError( + exception: exception, + stack: stack, + context: ErrorDescription('while resolving an image'), + silent: true, // could be a network error or whatnot + informationCollector: () sync* { + yield DiagnosticsProperty('Image provider', this); + yield DiagnosticsProperty('Image configuration', configuration); + yield DiagnosticsProperty('Image key', obtainedKey, defaultValue: null); + }, + ); } // If an error is added to a synchronous completer before a listener has been @@ -447,7 +384,7 @@ abstract class ImageProvider { key.then((T key) { obtainedKey = key; try { - successCallback(key, handleError); + resolveStreamForKey(configuration, stream, key, handleError); } catch (error, stackTrace) { handleError(error, stackTrace); } diff --git a/packages/flutter/lib/src/painting/image_stream.dart b/packages/flutter/lib/src/painting/image_stream.dart index 5db8bacc05..7b5c263bcd 100644 --- a/packages/flutter/lib/src/painting/image_stream.dart +++ b/packages/flutter/lib/src/painting/image_stream.dart @@ -203,11 +203,6 @@ class ImageChunkEvent extends Diagnosticable { /// /// ImageStream objects are backed by [ImageStreamCompleter] objects. /// -/// The [ImageCache] will consider an image to be live until the listener count -/// drops to zero after adding at least one listener. The -/// [addOnLastListenerRemovedCallback] method is used for tracking this -/// information. -/// /// See also: /// /// * [ImageProvider], which has an example that includes the use of an @@ -397,23 +392,6 @@ abstract class ImageStreamCompleter extends Diagnosticable { break; } } - if (_listeners.isEmpty) { - for (final VoidCallback callback in _onLastListenerRemovedCallbacks) { - callback(); - } - _onLastListenerRemovedCallbacks.clear(); - } - } - - final List _onLastListenerRemovedCallbacks = []; - - /// Adds a callback to call when [removeListener] results in an empty - /// list of listeners. - /// - /// This callback will never fire if [removeListener] is never called. - void addOnLastListenerRemovedCallback(VoidCallback callback) { - assert(callback != null); - _onLastListenerRemovedCallbacks.add(callback); } /// Calls all the registered listeners to notify them of a new image. diff --git a/packages/flutter/lib/src/widgets/image.dart b/packages/flutter/lib/src/widgets/image.dart index 12a947e0e3..f373af5cca 100644 --- a/packages/flutter/lib/src/widgets/image.dart +++ b/packages/flutter/lib/src/widgets/image.dart @@ -8,7 +8,6 @@ import 'dart:typed_data'; import 'package:flutter/foundation.dart'; import 'package:flutter/painting.dart'; -import 'package:flutter/scheduler.dart'; import 'package:flutter/services.dart'; import 'package:flutter/semantics.dart'; @@ -66,30 +65,7 @@ ImageConfiguration createLocalImageConfiguration(BuildContext context, { Size si /// If the image is later used by an [Image] or [BoxDecoration] or [FadeInImage], /// it will probably be loaded faster. The consumer of the image does not need /// to use the same [ImageProvider] instance. The [ImageCache] will find the image -/// as long as both images share the same key, and the image is held by the -/// cache. -/// -/// The cache may refuse to hold the image if it is disabled, the image is too -/// large, or some other criteria implemented by a custom [ImageCache] -/// implementation. -/// -/// The [ImageCache] holds a reference to all images passed to [putIfAbsent] as -/// long as their [ImageStreamCompleter] has at least one listener. This method -/// will wait until the end of the frame after its future completes before -/// releasing its own listener. This gives callers a chance to listen to the -/// stream if necessary. A caller can determine if the image ended up in the -/// cache by calling [ImageProvider.obtainCacheStatus]. If it is only held as -/// [ImageCacheStatus.live], and the caller wishes to keep the resolved -/// image in memory, the caller should immediately call `provider.resolve` and -/// add a listener to the returned [ImageStream]. The image will remain pinned -/// in memory at least until the caller removes its listener from the stream, -/// even if it would not otherwise fit into the cache. -/// -/// Callers should be cautious about pinning large images or a large number of -/// images in memory, as this can result in running out of memory and being -/// killed by the operating system. The lower the avilable physical memory, the -/// more susceptible callers will be to running into OOM issues. These issues -/// manifest as immediate process death, sometimes with no other error messages. +/// as long as both images share the same key. /// /// The [BuildContext] and [Size] are used to select an image configuration /// (see [createLocalImageConfiguration]). @@ -115,12 +91,7 @@ Future precacheImage( if (!completer.isCompleted) { completer.complete(); } - // Give callers until at least the end of the frame to subscribe to the - // image stream. - // See ImageCache._liveImages - SchedulerBinding.instance.addPostFrameCallback((Duration timeStamp) { - stream.removeListener(listener); - }); + stream.removeListener(listener); }, onError: (dynamic exception, StackTrace stackTrace) { if (!completer.isCompleted) { diff --git a/packages/flutter/test/painting/binding_test.dart b/packages/flutter/test/painting/binding_test.dart index f216b1ade8..0c0e7dc24a 100644 --- a/packages/flutter/test/painting/binding_test.dart +++ b/packages/flutter/test/painting/binding_test.dart @@ -3,10 +3,7 @@ // found in the LICENSE file. import 'dart:typed_data' show Uint8List; -import 'dart:ui'; -import 'package:flutter/foundation.dart'; -import 'package:flutter/services.dart'; import 'package:flutter_test/flutter_test.dart'; import 'package:flutter/widgets.dart'; import 'package:flutter/painting.dart'; @@ -27,88 +24,4 @@ void main() { }); expect(binding.instantiateImageCodecCalledCount, 1); }); - - test('evict clears live references', () async { - final TestPaintingBinding binding = TestPaintingBinding(); - expect(binding.imageCache.clearCount, 0); - expect(binding.imageCache.liveClearCount, 0); - - binding.evict('/path/to/asset.png'); - expect(binding.imageCache.clearCount, 1); - expect(binding.imageCache.liveClearCount, 1); - }); } - -class TestBindingBase implements BindingBase { - @override - void initInstances() {} - - @override - void initServiceExtensions() {} - - @override - Future lockEvents(Future Function() callback) async {} - - @override - bool get locked => throw UnimplementedError(); - - @override - Future performReassemble() { - throw UnimplementedError(); - } - - @override - void postEvent(String eventKind, Map eventData) {} - - @override - Future reassembleApplication() { - throw UnimplementedError(); - } - - @override - void registerBoolServiceExtension({String name, AsyncValueGetter getter, AsyncValueSetter setter}) {} - - @override - void registerNumericServiceExtension({String name, AsyncValueGetter getter, AsyncValueSetter setter}) {} - - @override - void registerServiceExtension({String name, ServiceExtensionCallback callback}) {} - - @override - void registerSignalServiceExtension({String name, AsyncCallback callback}) {} - - @override - void registerStringServiceExtension({String name, AsyncValueGetter getter, AsyncValueSetter setter}) {} - - @override - void unlocked() {} - - @override - Window get window => throw UnimplementedError(); -} - -class TestPaintingBinding extends TestBindingBase with ServicesBinding, PaintingBinding { - - @override - final FakeImageCache imageCache = FakeImageCache(); - - @override - ImageCache createImageCache() => imageCache; -} - -class FakeImageCache extends ImageCache { - int clearCount = 0; - int liveClearCount = 0; - - @override - void clear() { - clearCount += 1; - super.clear(); - } - - @override - void clearLiveImages() { - liveClearCount += 1; - super.clearLiveImages(); - } -} \ No newline at end of file diff --git a/packages/flutter/test/painting/image_cache_test.dart b/packages/flutter/test/painting/image_cache_test.dart index 2c6bc0f103..39f3a2a8bc 100644 --- a/packages/flutter/test/painting/image_cache_test.dart +++ b/packages/flutter/test/painting/image_cache_test.dart @@ -9,14 +9,13 @@ import '../rendering/rendering_tester.dart'; import 'mocks_for_image_cache.dart'; void main() { - group('ImageCache', () { + group(ImageCache, () { setUpAll(() { TestRenderingFlutterBinding(); // initializes the imageCache }); tearDown(() { imageCache.clear(); - imageCache.clearLiveImages(); imageCache.maximumSize = 1000; imageCache.maximumSizeBytes = 10485760; }); @@ -170,14 +169,7 @@ void main() { return completer1; }) as TestImageStreamCompleter; - expect(imageCache.statusForKey(testImage).pending, true); - expect(imageCache.statusForKey(testImage).live, true); imageCache.clear(); - expect(imageCache.statusForKey(testImage).pending, false); - expect(imageCache.statusForKey(testImage).live, true); - imageCache.clearLiveImages(); - expect(imageCache.statusForKey(testImage).pending, false); - expect(imageCache.statusForKey(testImage).live, false); final TestImageStreamCompleter resultingCompleter2 = imageCache.putIfAbsent(testImage, () { return completer2; @@ -248,106 +240,7 @@ void main() { expect(resultingCompleter1, completer1); expect(imageCache.containsKey(testImage), true); - }); - test('putIfAbsent updates LRU properties of a live image', () async { - imageCache.maximumSize = 1; - const TestImage testImage = TestImage(width: 8, height: 8); - const TestImage testImage2 = TestImage(width: 10, height: 10); - - final TestImageStreamCompleter completer1 = TestImageStreamCompleter()..testSetImage(testImage); - final TestImageStreamCompleter completer2 = TestImageStreamCompleter()..testSetImage(testImage2); - - completer1.addListener(ImageStreamListener((ImageInfo info, bool syncCall) {})); - - final TestImageStreamCompleter resultingCompleter1 = imageCache.putIfAbsent(testImage, () { - return completer1; - }) as TestImageStreamCompleter; - - expect(imageCache.statusForKey(testImage).pending, false); - expect(imageCache.statusForKey(testImage).keepAlive, true); - expect(imageCache.statusForKey(testImage).live, true); - expect(imageCache.statusForKey(testImage2).untracked, true); - final TestImageStreamCompleter resultingCompleter2 = imageCache.putIfAbsent(testImage2, () { - return completer2; - }) as TestImageStreamCompleter; - - - expect(imageCache.statusForKey(testImage).pending, false); - expect(imageCache.statusForKey(testImage).keepAlive, false); // evicted - expect(imageCache.statusForKey(testImage).live, true); - expect(imageCache.statusForKey(testImage2).pending, false); - expect(imageCache.statusForKey(testImage2).keepAlive, true); // took the LRU spot. - expect(imageCache.statusForKey(testImage2).live, false); // no listeners - - expect(resultingCompleter1, completer1); - expect(resultingCompleter2, completer2); - }); - - test('Live image cache avoids leaks of unlistened streams', () async { - imageCache.maximumSize = 3; - - const TestImageProvider(1, 1)..resolve(ImageConfiguration.empty); - const TestImageProvider(2, 2)..resolve(ImageConfiguration.empty); - const TestImageProvider(3, 3)..resolve(ImageConfiguration.empty); - const TestImageProvider(4, 4)..resolve(ImageConfiguration.empty); - const TestImageProvider(5, 5)..resolve(ImageConfiguration.empty); - const TestImageProvider(6, 6)..resolve(ImageConfiguration.empty); - - // wait an event loop to let image resolution process. - await null; - - expect(imageCache.currentSize, 3); - expect(imageCache.liveImageCount, 0); - }); - - test('Disabled image cache does not leak live images', () async { - imageCache.maximumSize = 0; - - const TestImageProvider(1, 1)..resolve(ImageConfiguration.empty); - const TestImageProvider(2, 2)..resolve(ImageConfiguration.empty); - const TestImageProvider(3, 3)..resolve(ImageConfiguration.empty); - const TestImageProvider(4, 4)..resolve(ImageConfiguration.empty); - const TestImageProvider(5, 5)..resolve(ImageConfiguration.empty); - const TestImageProvider(6, 6)..resolve(ImageConfiguration.empty); - - // wait an event loop to let image resolution process. - await null; - - expect(imageCache.currentSize, 0); - expect(imageCache.liveImageCount, 0); - }); - - test('Evicting a pending image clears the live image', () async { - const TestImage testImage = TestImage(width: 8, height: 8); - - final TestImageStreamCompleter completer1 = TestImageStreamCompleter(); - - imageCache.putIfAbsent(testImage, () => completer1); - expect(imageCache.statusForKey(testImage).pending, true); - expect(imageCache.statusForKey(testImage).live, true); - expect(imageCache.statusForKey(testImage).keepAlive, false); - - imageCache.evict(testImage); - expect(imageCache.statusForKey(testImage).untracked, true); - }); - - test('Evicting a completed image does not clear the live image', () async { - const TestImage testImage = TestImage(width: 8, height: 8); - - final TestImageStreamCompleter completer1 = TestImageStreamCompleter() - ..testSetImage(testImage) - ..addListener(ImageStreamListener((ImageInfo info, bool syncCall) {})); - - imageCache.putIfAbsent(testImage, () => completer1); - expect(imageCache.statusForKey(testImage).pending, false); - expect(imageCache.statusForKey(testImage).live, true); - expect(imageCache.statusForKey(testImage).keepAlive, true); - - imageCache.evict(testImage); - expect(imageCache.statusForKey(testImage).pending, false); - expect(imageCache.statusForKey(testImage).live, true); - expect(imageCache.statusForKey(testImage).keepAlive, false); }); }); } diff --git a/packages/flutter/test/painting/image_provider_test.dart b/packages/flutter/test/painting/image_provider_test.dart index 47371be5e8..148912ca68 100644 --- a/packages/flutter/test/painting/image_provider_test.dart +++ b/packages/flutter/test/painting/image_provider_test.dart @@ -36,7 +36,6 @@ void main() { tearDown(() { FlutterError.onError = oldError; PaintingBinding.instance.imageCache.clear(); - PaintingBinding.instance.imageCache.clearLiveImages(); }); group('ImageProvider', () { @@ -53,18 +52,15 @@ void main() { const ImageProvider provider = ExactAssetImage('does-not-exist'); final Object key = await provider.obtainKey(ImageConfiguration.empty); - expect(imageCache.statusForKey(provider).untracked, true); - expect(imageCache.pendingImageCount, 0); + expect(imageCache.containsKey(provider), false); provider.resolve(ImageConfiguration.empty); - expect(imageCache.statusForKey(key).pending, true); - expect(imageCache.pendingImageCount, 1); + expect(imageCache.containsKey(key), true); await error.future; - expect(imageCache.statusForKey(provider).untracked, true); - expect(imageCache.pendingImageCount, 0); + expect(imageCache.containsKey(provider), false); }, skip: isBrowser); test('AssetImageProvider - evicts on null load', () async { @@ -75,18 +71,15 @@ void main() { final ImageProvider provider = ExactAssetImage('does-not-exist', bundle: TestAssetBundle()); final Object key = await provider.obtainKey(ImageConfiguration.empty); - expect(imageCache.statusForKey(provider).untracked, true); - expect(imageCache.pendingImageCount, 0); + expect(imageCache.containsKey(key), false); provider.resolve(ImageConfiguration.empty); - expect(imageCache.statusForKey(key).pending, true); - expect(imageCache.pendingImageCount, 1); + expect(imageCache.containsKey(key), true); await error.future; - expect(imageCache.statusForKey(provider).untracked, true); - expect(imageCache.pendingImageCount, 0); + expect(imageCache.containsKey(key), false); }, skip: isBrowser); test('ImageProvider can evict images', () async { @@ -158,17 +151,6 @@ void main() { expect(await caughtError.future, true); }); - test('obtainKey errors will be caught - check location', () async { - final ImageProvider imageProvider = ObtainKeyErrorImageProvider(); - final Completer caughtError = Completer(); - FlutterError.onError = (FlutterErrorDetails details) { - caughtError.complete(true); - }; - await imageProvider.obtainCacheStatus(configuration: ImageConfiguration.empty); - - expect(await caughtError.future, true); - }); - test('resolve sync errors will be caught', () async { bool uncaught = false; final Zone testZone = Zone.current.fork(specification: ZoneSpecification( @@ -218,26 +200,24 @@ void main() { test('File image with empty file throws expected error and evicts from cache', () async { final Completer error = Completer(); FlutterError.onError = (FlutterErrorDetails details) { + print(details.exception); error.complete(details.exception as StateError); }; final MemoryFileSystem fs = MemoryFileSystem(); final File file = fs.file('/empty.png')..createSync(recursive: true); final FileImage provider = FileImage(file); - expect(imageCache.statusForKey(provider).untracked, true); - expect(imageCache.pendingImageCount, 0); + expect(imageCache.containsKey(provider), false); provider.resolve(ImageConfiguration.empty); - expect(imageCache.statusForKey(provider).pending, true); - expect(imageCache.pendingImageCount, 1); + expect(imageCache.containsKey(provider), true); expect(await error.future, isStateError); - expect(imageCache.statusForKey(provider).untracked, true); - expect(imageCache.pendingImageCount, 0); + expect(imageCache.containsKey(provider), false); }); - group('NetworkImage', () { + group(NetworkImage, () { MockHttpClient httpClient; setUp(() { @@ -262,13 +242,11 @@ void main() { final Completer caughtError = Completer(); final ImageProvider imageProvider = NetworkImage(nonconst(requestUrl)); - expect(imageCache.pendingImageCount, 0); - expect(imageCache.statusForKey(imageProvider).untracked, true); + expect(imageCache.containsKey(imageProvider), false); final ImageStream result = imageProvider.resolve(ImageConfiguration.empty); - expect(imageCache.pendingImageCount, 1); - expect(imageCache.statusForKey(imageProvider).pending, true); + expect(imageCache.containsKey(imageProvider), true); result.addListener(ImageStreamListener((ImageInfo info, bool syncCall) { }, onError: (dynamic error, StackTrace stackTrace) { @@ -277,8 +255,7 @@ void main() { final dynamic err = await caughtError.future; - expect(imageCache.pendingImageCount, 0); - expect(imageCache.statusForKey(imageProvider).untracked, true); + expect(imageCache.containsKey(imageProvider), false); expect( err, diff --git a/packages/flutter/test/widgets/image_test.dart b/packages/flutter/test/widgets/image_test.dart index 6279d13139..cd4a342134 100644 --- a/packages/flutter/test/widgets/image_test.dart +++ b/packages/flutter/test/widgets/image_test.dart @@ -8,7 +8,6 @@ import 'dart:ui' as ui; import 'package:flutter/foundation.dart'; import 'package:flutter/rendering.dart'; -import 'package:flutter/scheduler.dart'; import 'package:flutter/widgets.dart'; import 'package:flutter_test/flutter_test.dart'; @@ -24,18 +23,6 @@ Future createTestImage([List bytes = kTransparentImage]) async { } void main() { - int originalCacheSize; - - setUp(() { - originalCacheSize = imageCache.maximumSize; - imageCache.clear(); - imageCache.clearLiveImages(); - }); - - tearDown(() { - imageCache.maximumSize = originalCacheSize; - }); - testWidgets('Verify Image resets its RenderImage when changing providers', (WidgetTester tester) async { final GlobalKey key = GlobalKey(); final TestImageProvider imageProvider1 = TestImageProvider(); @@ -776,7 +763,7 @@ void main() { expect(isSync, isTrue); }); - testWidgets('Precache removes original listener immediately after future completes, does not crash on successive calls #25143', (WidgetTester tester) async { + testWidgets('Precache remove listeners immediately after future completes, does not crash on successive calls #25143', (WidgetTester tester) async { final TestImageStreamCompleter imageStreamCompleter = TestImageStreamCompleter(); final TestImageProvider provider = TestImageProvider(streamCompleter: imageStreamCompleter); @@ -1377,197 +1364,6 @@ void main() { expect(imageProviders.skip(309 - 15).every(loadCalled), true); expect(imageProviders.take(309 - 15).every(loadNotCalled), true); }); - - testWidgets('Same image provider in multiple parts of the tree, no cache room left', (WidgetTester tester) async { - imageCache.maximumSize = 0; - - final ui.Image image = await tester.runAsync(createTestImage); - final TestImageProvider provider1 = TestImageProvider(); - final TestImageProvider provider2 = TestImageProvider(); - - expect(provider1.loadCallCount, 0); - expect(provider2.loadCallCount, 0); - expect(imageCache.liveImageCount, 0); - - await tester.pumpWidget(Column( - children: [ - Image(image: provider1), - Image(image: provider2), - Image(image: provider1), - Image(image: provider1), - Image(image: provider2), - ], - )); - - expect(imageCache.liveImageCount, 2); - expect(imageCache.statusForKey(provider1).live, true); - expect(imageCache.statusForKey(provider1).pending, false); - expect(imageCache.statusForKey(provider1).keepAlive, false); - expect(imageCache.statusForKey(provider2).live, true); - expect(imageCache.statusForKey(provider2).pending, false); - expect(imageCache.statusForKey(provider2).keepAlive, false); - - expect(provider1.loadCallCount, 1); - expect(provider2.loadCallCount, 1); - - provider1.complete(image); - await tester.idle(); - - provider2.complete(image); - await tester.idle(); - - expect(imageCache.liveImageCount, 2); - expect(imageCache.currentSize, 0); - - await tester.pumpWidget(Image(image: provider2)); - await tester.idle(); - expect(imageCache.statusForKey(provider1).untracked, true); - expect(imageCache.statusForKey(provider2).live, true); - expect(imageCache.statusForKey(provider2).pending, false); - expect(imageCache.statusForKey(provider2).keepAlive, false); - expect(imageCache.liveImageCount, 1); - - await tester.pumpWidget(const SizedBox()); - await tester.idle(); - expect(provider1.loadCallCount, 1); - expect(provider2.loadCallCount, 1); - expect(imageCache.liveImageCount, 0); - }); - - testWidgets('precacheImage does not hold weak ref for more than a frame', (WidgetTester tester) async { - imageCache.maximumSize = 0; - final TestImageProvider provider = TestImageProvider(); - Future precache; - await tester.pumpWidget( - Builder( - builder: (BuildContext context) { - precache = precacheImage(provider, context); - return Container(); - } - ) - ); - provider.complete(); - await precache; - - // Should have ended up with only a weak ref, not in cache because cache size is 0 - expect(imageCache.liveImageCount, 1); - expect(imageCache.containsKey(provider), false); - - final ImageCacheStatus providerLocation = await provider.obtainCacheStatus(configuration: ImageConfiguration.empty); - - expect(providerLocation, isNotNull); - expect(providerLocation.live, true); - expect(providerLocation.keepAlive, false); - expect(providerLocation.pending, false); - - // Check that a second resolve of the same image is synchronous. - expect(provider._lastResolvedConfiguration, isNotNull); - final ImageStream stream = provider.resolve(provider._lastResolvedConfiguration); - bool isSync; - final ImageStreamListener listener = ImageStreamListener((ImageInfo image, bool syncCall) { isSync = syncCall; }); - - // Still have live ref because frame has not pumped yet. - await tester.pump(); - expect(imageCache.liveImageCount, 1); - - SchedulerBinding.instance.scheduleFrame(); - await tester.pump(); - // Live ref should be gone - we didn't listen to the stream. - expect(imageCache.liveImageCount, 0); - expect(imageCache.currentSize, 0); - - stream.addListener(listener); - expect(isSync, true); // because the stream still has the image. - - expect(imageCache.liveImageCount, 0); - expect(imageCache.currentSize, 0); - - expect(provider.loadCallCount, 1); - }); - - testWidgets('precacheImage allows time to take over weak refernce', (WidgetTester tester) async { - final TestImageProvider provider = TestImageProvider(); - Future precache; - await tester.pumpWidget( - Builder( - builder: (BuildContext context) { - precache = precacheImage(provider, context); - return Container(); - } - ) - ); - provider.complete(); - await precache; - - // Should have ended up in the cache and have a weak reference. - expect(imageCache.liveImageCount, 1); - expect(imageCache.currentSize, 1); - expect(imageCache.containsKey(provider), true); - - // Check that a second resolve of the same image is synchronous. - expect(provider._lastResolvedConfiguration, isNotNull); - final ImageStream stream = provider.resolve(provider._lastResolvedConfiguration); - bool isSync; - final ImageStreamListener listener = ImageStreamListener((ImageInfo image, bool syncCall) { isSync = syncCall; }); - - // Should have ended up in the cache and still have a weak reference. - expect(imageCache.liveImageCount, 1); - expect(imageCache.currentSize, 1); - expect(imageCache.containsKey(provider), true); - - stream.addListener(listener); - expect(isSync, true); - - expect(imageCache.liveImageCount, 1); - expect(imageCache.currentSize, 1); - expect(imageCache.containsKey(provider), true); - - SchedulerBinding.instance.scheduleFrame(); - await tester.pump(); - - expect(imageCache.liveImageCount, 1); - expect(imageCache.currentSize, 1); - expect(imageCache.containsKey(provider), true); - stream.removeListener(listener); - - expect(imageCache.liveImageCount, 0); - expect(imageCache.currentSize, 1); - expect(imageCache.containsKey(provider), true); - expect(provider.loadCallCount, 1); - }); - - testWidgets('evict an image during precache', (WidgetTester tester) async { - // This test checks that the live image tracking does not hold on to a - // pending image that will never complete because it has been evicted from - // the cache. - // The scenario may arise in a test harness that is trying to load real - // images using `tester.runAsync()`, and wants to make sure that widgets - // under test have not also tried to resolve the image in a FakeAsync zone. - // The image loaded in the FakeAsync zone will never complete, and the - // runAsync call wants to make sure it gets a load attempt from the correct - // zone. - final Uint8List bytes = Uint8List.fromList(kTransparentImage); - final MemoryImage provider = MemoryImage(bytes); - - await tester.runAsync(() async { - final List> futures = >[]; - await tester.pumpWidget(Builder(builder: (BuildContext context) { - futures.add(precacheImage(provider, context)); - imageCache.evict(provider); - futures.add(precacheImage(provider, context)); - return const SizedBox.expand(); - })); - await Future.wait(futures); - expect(imageCache.statusForKey(provider).keepAlive, true); - expect(imageCache.statusForKey(provider).live, true); - - // Schedule a frame to get precacheImage to stop listening. - SchedulerBinding.instance.scheduleFrame(); - await tester.pump(); - expect(imageCache.statusForKey(provider).keepAlive, true); - expect(imageCache.statusForKey(provider).live, false); - }); - }); } class ConfigurationAwareKey { @@ -1609,9 +1405,8 @@ class TestImageProvider extends ImageProvider { ImageStreamCompleter _streamCompleter; ImageConfiguration _lastResolvedConfiguration; - bool get loadCalled => _loadCallCount > 0; - int get loadCallCount => _loadCallCount; - int _loadCallCount = 0; + bool get loadCalled => _loadCalled; + bool _loadCalled = false; @override Future obtainKey(ImageConfiguration configuration) { @@ -1626,13 +1421,12 @@ class TestImageProvider extends ImageProvider { @override ImageStreamCompleter load(Object key, DecoderCallback decode) { - _loadCallCount += 1; + _loadCalled = true; return _streamCompleter; } - void complete([ui.Image image]) { - image ??= TestImage(); - _completer.complete(ImageInfo(image: image)); + void complete() { + _completer.complete(ImageInfo(image: TestImage())); } void fail(dynamic exception, StackTrace stackTrace) {