Image.network's caching strategy is problematic

[dnfield]

Currently, Image.network will cache the decoded version of the image in the ImageCache under the following circumstances:

• The cache max allowed size is > 0
• The cache is not already full

One problem we have is that if the network image is > cache size, we automatically resize it to make the cache bigger and try again. @gaaclarke has been working on fixing that.

Another problem is that we only provide ways to cache the decoded image, which may be much much larger than the encoded data. A developer may want to cache the encoded bytes to avoid a network call, but not cache the decoded image (which might be very large). We should make it easier to do the right thing here.

/cc @gaaclarke @goderbauer @jonahwilliams @liyuqian @zmtzawqlp @cbracken

[goderbauer]

Is the gist of this bug that we should make it configurable wether the encoded or decoded image is cached? Would that be something you want to configure on a per-image or per-app base?

[dnfield]

@goderbauer - yes. I don't have full details worked out, but didn't want to lose track of this.

[gaaclarke]

I suggest we create an on-disk caching mechanism for encoded network images, something akin to what web browsers do.

[jonahwilliams]

I think that overall, the issue with Image.network is that it is too much like <img /> and therefore magic, as opposed to a layered abstraction like the rest of flutter. It should be trivial for someone to layer a caching strategy on top of it (compositionally) without ripping out its guts.

[dnfield]

Maybe we could come up with some kind of inherited widget that deals with caching decisions for images. That could also help with the sliver list case - a user could choose to exclude that subtree from image caching. It could also help with network images where the caching widget could cache the encoded bytes instead of the raw.

[dnfield]

I think for this, rather than something specific for images, it would be good to have a widget that can handle caching decisions for network requests.

I'm imagining an inherited widget that would be looked for by other widgets that make network calls, and if it's there they're expected to check if it has cached response data first. Whether to cache the image would then be a separate decision, and possibly handled as in the situation described in #47380 (comment)

[ignatz]

I'm all for solving the generic case, however I would suggest to focus, understand and ideally improve the image-related issues first. Hopefully we'll learn something along the way :).

IMHO there are two mostly orthogonal issues:

1. Picking the right domain-specific caching policy and caching scope. For example, don't cache when you don't expect any hits or picking a cache with the right network, memory and cpu trade-offs for a given environment.
2. The current, tight coupling between public APIs and the caching behavior.

Let me expand a little more on #2, since I believe this to be the more fundamental issue. Today, the ImageProvider itself is "stateless". ImageProvider::resolve() depends on the cache to avoid redundant resolutions. In practice, this means, for example:

• that precacheImage (

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

  Line 77 in fabf4e3

  Future<void> precacheImage(

  ) is racy. Holding on the ImageProvider will not guarantee that image is available by the time we want to render it. Instead, we might have prefetched the image and since gotten rid of it again. If the cache size is 0, the pre-cache will actually download the image and throw it away.
• It's hard to even clear() the cache at the moment. Even if I hold on to the ImageProvider, I might have to re-download and decode the image.

If I had to guess, the image provider being stateless might have been the reason for growing the size limits of a limited cache in the first place because not-caching really isn't an option.

My suggestion would be to:

1. make the ImageProvider stateful
2. remove the expansion code from the image cache and simply not cache images that don't fit into the cache.
3. think about how we can make caching policies and scopes more explicit.

Curious to hear your thoughts.

[dnfield]

@ignatz - I'm not entirely sure how making it stateful would help things. Can you elaborate on that?

What would you expect the behavior of precacheImage to be when cachesize is 0? Perhaps it should throw instead of actually decoding the image?

One of the problems I see with NetworkImage in particular is that we're assuming that you want to hit the network again any time you drop the image from your cache.

Another problem I see is that we assume there can be one global static ImageCache that all providers interact with in the same way rather than image caches specific to specific parts of the tree (perhaps this is what you're getting at by state/stateless, I'm not sure - but I don't think we need to make providers stateful to fix that).

[ignatz]

What would you expect the behavior of precacheImage to be when cachesize is 0? Perhaps it should throw instead of actually decoding the image?

As a developer when calling precacheImage I would expect that by the time I commit to rendering the Image I can either join into the ongoing fetch or am guaranteed to not fetch again whenever the fetch has completed. It's not clear to me why re-fetching would be intended behavior.

Let me make one more example based a concrete problem I've encountered in our code-base. We're using the ImageProvider::resolve API to get an ImageStream on which we're registering an onImage callback to update the widget tree, once the image is downloaded. However, when I set the cache size to zero or use a cache that doesn't cache large images, I end up downloading the image at least twice (once to fetch it for the render and once to fetch it to get an ImageStream). Ironically, the meaning of the onImage signal changes, it's no longer a signal when the image is ready to display.

From an image or image provider API point of view it's hard to reason about the behavior, since the cache is rather internal and the semantics of resolve and ImageStream heavily depend on the properties and implementation details of said cache. Naively, I would expect caching to be more a transparent optimization rather than driving the behavior.

Personally, I would find it more intuitive if the ImageProvider has a lifecycle and cycles from referencing the url to referencing the encoded image or even the decoded image? (If there are memory concerns the provider could still have explicit lifecycle methods to e.g. reset it into an earlier lifecycle stage)

[dnfield]

I think we could get what you're looking for by distinguishing between how the encoded bytes are loaded and how the image is actually decoded, right?

For example, if there was some layer responsible for loading the network bytes separate from a layer responsible for decoding.

The whole point of ImageProvider is to manage interaction with the cache during image decoding AFAICT. Your case sounds like you'd rather not use ImageProvider at all, and instead handle making the dart:ui calls to decode the image yourself and eventually create widgets once that's done.

[ignatz]

I think we could get what you're looking for by distinguishing between how the encoded bytes are loaded and how the image is actually decoded, right?

I'm not sure. Are you suggesting a multi-layer caching approach?

For example, if there was some layer responsible for loading the network bytes separate from a layer responsible for decoding.

Today we get two calls to ImageProvider::resolve:

1. from our code to register a onDownloadFinished callback
2. from the framework through _ImageState._resolveImage.

Something should likely be stateful, it could be the network ImageProvider, it could be something else .

The whole point of ImageProvider is to manage interaction with the cache during image decoding AFAICT. Your case sounds like you'd rather not use ImageProvider at all, and instead handle making the dart:ui calls to decode the image yourself and eventually create widgets once that's done.

Interesting, I hadn't thought of ImageProvider as an API to merely interact with the cache. FWIW, having the image provider "own" the downloaded image would also help with, e.g.

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

Line 586 in 68d0c89

// TODO(ianh): Find some way to honor cache headers to the extent that when the

Generally, I'm sure we could change the APIs, my suggestions were mostly targeted towards making the existing APIs work even if the underlying cache behavior changes, e.g. disabled, don't cache large images, clear the cache, ...

[vanlooverenkoen]

@dnfield This is what my memory consumption looks like. The highlighted section is the normal app use. Doing some API calls with Dio. After the highlighted section the app displays 20 network images. With the size of a couple kb.

This is done on a iPhone 7 plus. But we are having this issue on all our test devices. android & iOS

This triggers crashes after some use of the app. Because the app spikes again once new pictures are reloaded

[vanlooverenkoen]

@violet-dev created a repo to reproduce the bug:
https://github.com/violet-dev/mmmmmmmm (inappropriate images)

@TahaTesser any idea why this is happening?

main.dart example:

import 'dart:async';

import 'package:cached_network_image/cached_network_image.dart';
import 'package:flutter/material.dart';

void main() {
  runApp(MyApp());
}

class MyApp extends StatelessWidget {
  @override
  Widget build(BuildContext context) {
    return MaterialApp(
      title: 'Flutter Demo',
      home: Screen1(),
    );
  }
}

class Screen1 extends StatelessWidget {
  @override
  Widget build(BuildContext context) {
    return Scaffold(
      appBar: AppBar(
        title: Text('Screen1'),
      ),
      body: Center(
        child: MaterialButton(
          child: Text('Go to screen 2'),
          onPressed: () => Navigator.of(context).push(MaterialPageRoute(builder: (context) => Screen2())),
        ),
      ),
    );
  }
}

class Screen2 extends StatefulWidget {
  @override
  _Screen2State createState() => _Screen2State();
}

class _Screen2State extends State<Screen2> {
  Timer _clearTimer;

  @override
  void initState() {
    super.initState();

//    _clearTimer = Timer.periodic(Duration(seconds: 1), (timer) {
//      imageCache.clearLiveImages();
//      imageCache.clear();
//    });
  }

  @override
  void dispose() {
    _clearTimer?.cancel();
    super.dispose();
  }

  @override
  Widget build(BuildContext context) {
    return Scaffold(
      appBar: AppBar(
        title: Text('Screen2'),
      ),
      body: ListView.builder(
        itemBuilder: (context, index) {
          return Container(
            width: 100,
            height: 150,
            child: Image.network(
              _getUrl(index),
              fit: BoxFit.cover,
              width: 100,
              height: 150,
            ),
          );
//
//          return CachedNetworkImage(
//            imageUrl: _getUrl(index),
//            fit: BoxFit.cover,
//            fadeInDuration: Duration(microseconds: 500),
//            fadeInCurve: Curves.easeIn,
//            // memCacheWidth: width.toInt(),
//            progressIndicatorBuilder: (context, string, progress) {
//              return SizedBox(
//                height: 300,
//                child: Center(
//                  child: SizedBox(
//                    child: CircularProgressIndicator(value: progress.progress),
//                    width: 30,
//                    height: 30,
//                  ),
//                ),
//              );
//            },
//          );
        },
      ),
    );
  }

  String _getUrl(int index) => "https://picsum.photos/id/$index/2500/2000";
}

pubspec.yaml

name: flutter_long_list
description: A new Flutter application.

version: 1.0.0+1

environment:
  sdk: ">=2.7.0 <3.0.0"

dependencies:
  flutter:
    sdk: flutter
  cupertino_icons: 0.1.3
  cached_network_image: 2.3.2+1


dev_dependencies:
  flutter_test:
    sdk: flutter

flutter:
  uses-material-design: true

Using the clear images without a cacheWidth & cacheHeight:

Using the clear images with a cacheWidth & cacheHeight

[kf6gpe]

@dnfield @liyuqian Do we want to give this a priority fro the remaining work, or migrate remaining work to other bugs?

[dnfield]

Marking this as P5 - I think this problem is better solved in some community packages, but we could still ake it better in the framework.

[esDotDev]

Another aspect to think about is the desktop application that does want to cache the decoded bytes because we have plenty of RAM and just want the best UX possible. If you look at something like slack, Epic Games Store, Steam none of them noticeably reload an image that was just recently loaded. For example:

It would be really nice for the framework to provide more developer control here. For example, on desktop we might do setImageCollectionDelay(Duration(seconds: 180)) and maybe setMaxImageCache(1024 * 1024 * 4), which means hold all images for 3m, unless you hit the max, then start cleaning up oldest ones first. That will give us really excellent UX and help effectively use the resources of the host machine.

Currently it seems as those the RAM is collected instantly when an image goes off screen. This might make sense for mobile, but does not really on Desktop where we have RAM to spare and no aggressive OS trying to collect us. As implemented, even when using cached_network_image, we still see loading/flicker images constantly, since loading from disk is async, especially when flying through a grid of hi-resolution images, and the ram cached images are super-aggressively cleaned up when scrolling or switching pages.

[dnfield]

Images stay in memory until they're evicted from the ImageCache.

There's no time based component for images, although it might be interesting to clean up images that haven't been touched in the last N seconds.

[esDotDev]

But aren't they evicted from ImageCache near-instantly when there is no listeners? In practice this very aggressive, and leads to something re-loading when you switch pages, even though you may have looked at it 5 seconds ago.

My ask is basically to add a time component, so listener count is not the only deciding factor. But maybe I mis-understand, I can't find a clear explenation of ImageCache's evictiona rationale, but the behavior I'm seeing in-app seems to reflect this.

Seems like the time-based method would work most elegantly with Flutters lists that rely so much on virtualization to be performant. Here's an example of the type of UI that is coming to mind:
http://screens.gskinner.com/shawn/2020-12-11_16-44-30.mp4

It just works of the rationale that if you viewed something recently, there's a high change you will view it again momentarily, and just optimizing for that high % use case, and also the fact that we are on very powerful machines, and if we can use the RAM to improve the performance, we should.

Trying to build something like this, where you just want smooth scrolling, and no visible loading on the images, just feels like you're fighting the framework the entire time, as it's making these sorta mobile-centric opinionated decisions about resource management.

[dnfield]

They are not evicted from the image cache until something else needs to take their place, and then yes they are evicted immediately.

[esDotDev]

I suppose I need to up my cache limits then!
Thanks,

[Edit] Works perfectly now, sorry for the noise.