Add a ratio cap to decoded animated image frames #6310

mklim · 2018-09-21T22:28:02Z

Provide a relative, per-image limit to the amount of memory
that's used to cache decoded image frames. Adds an overridable default
to dart:ui that developers can set to control how much memory images
are allowed to use decoded vs undecoded.

Note that required frames are always cached regardless of the ratio cap,
because they're currently necessary for the GIF to animate. Previously
cached unessential frames are not cleared in response to the cache
hitting or exceeding the cap.

See flutter/flutter#20998 and flutter/flutter#14344

Hixie · 2018-09-27T18:02:46Z

lib/ui/painting.dart

@@ -1567,6 +1567,22 @@ class FrameInfo extends NativeFieldWrapperClass2 {
  Image get image native 'FrameInfo_image';
 }

+/// The default max amount of memory to use for caching decoded animated image
+/// frames compared to total undecoded size. See [decodedCacheRatioCapOverride].
+const double kDefaultDecodedCacheRatioCap = 25.0;


Can we make this private?

Hixie · 2018-09-27T18:03:49Z

lib/ui/painting.dart

+
+/// By default individual frames of animated images are cached into memory to
+/// avoid using CPU to re-decode them for every loop in the animation. This
+/// behavior will result in out of memory crashes when decoding large


nit: "out-of-memory"

Hixie · 2018-09-27T18:04:00Z

lib/ui/painting.dart

+/// By default individual frames of animated images are cached into memory to
+/// avoid using CPU to re-decode them for every loop in the animation. This
+/// behavior will result in out of memory crashes when decoding large
+/// (or large amounts of) animated images. Set this value to limit how much


amounts -> numbers

lib/ui/painting.dart

Hixie · 2018-09-27T18:06:47Z

lib/ui/painting.dart

+/// unessential decoded frames. A setting of `1.0` or less disables all caching
+/// of unessential decoded frames. See [kDefaultDecodedCacheRatioCap] for the
+/// default value.
+double decodedCacheRatioCapOverride;


There's no setter here, meaning that setting the value has no effect. Document how to make it take effect (e.g. "this value only takes effect when instantiateImageCodec is invoked; to change the cache on the fly, the image must be redecoded with a new value).

But also, this is a global value with non-immediate side-effects, which is generally frowned upon. I would recommend making this an argument to instantiateImageCodec rather than a global variable.

(Consider how a framework that wanted to pick the ratio on a per-image basis would have to be written. Or what would happen if two libraries with different opinions were to interact. They'd be clobbering each other's setting of this variable.)

On it being a global: making it a param of the constructor forces this to bubble out pretty far in order for users to actually set it when constructing their images, since Codec isn't being constructed directly and ends up being created from several different top level widgets. From what I can tell AssetImage, ExtractAssetImage, FileImage, NetworkImage, and MemoryImage all end up eventually instantiating Codec and would need the param added to their constructors too in order to pass it through. There also looks like there's static constructors attached to Image itself for each of those classes: Image.asset, Image.file, Image.network, Image.memory. I'm good with adding this new param throughout if you think it's preferable to a global setter? When I first wrote this I was thinking it was a bigger problem to have a param existing across all those signatures in order to flip a setting way over in this class, but I hadn't thought about people wanting to potentially set it on a per-image basis.

Talked offline. It's possible to avoid both of these problems if the multiple instantiateImageCodec calls in image_provider.dart are diverted into a new singular method in binding.dart. A global is more of an accepted practice on the framework side, so the default can be defined and overridden there.

mklim · 2018-09-28T22:27:11Z

Moved the cap into the framework in flutter/flutter#22452.

Hixie · 2018-10-05T23:36:45Z

lib/ui/painting.dart

 ///
-/// The following image formats are supported: {@macro flutter.dart:ui.imageFormats}
+/// [decodedCacheRatioCap] is the default maximum multiple of the compressed


Avoid starting with a lower case letter by using a style like The [foo] parameter is..

lib/ui/painting.dart

Hixie · 2018-10-05T23:39:55Z

lib/ui/painting.dart

+/// setting this to `2.0` means that a 400KB GIF would be allowed at most to use
+/// 800KB of memory caching unessential decoded frames. Caching decoded frames
+/// saves CPU but can result in out-of-memory crashes when decoding large (or
+/// multiple) animated images.


we should add some comment advising about what blow-up factors are common, e.g. mentioning that 2x is pretty much never going to be a reasonable factor.

Hixie · 2018-10-05T23:40:09Z

lib/ui/painting.dart

 ///
 /// The returned future can complete with an error if the image decoding has
 /// failed.
-Future<Codec> instantiateImageCodec(Uint8List list) {
+Future<Codec> instantiateImageCodec(Uint8List list,
+                                   {double decodedCacheRatioCap = double.maxFinite}) {


why maxFinite instead of infinity?

lib/ui/painting.dart

Hixie · 2018-10-05T23:41:31Z

Dart code LGTM modulo comments. I'll let Chinmay or Jason review the C++.

cbracken · 2018-10-09T23:21:14Z

lib/ui/painting/codec.cc

-      SkBitmap& requiredBitmap = frameBitmaps_[requiredFrame];
-      // For simplicity, do not try to cache old frames
+
+      SkBitmap requiredBitmap = *frameBitmaps_[requiredFrame]->bitmap_;


nit: consider extracting a local ptr to frameBitmaps_[requiredFrame]->bitmap_ from the condition in line 408, then using that here.

cbracken · 2018-10-09T23:34:38Z

lib/ui/painting.dart

+/// saves CPU but can result in out-of-memory crashes when decoding large (or
+/// multiple) animated images. Note that GIFs are highly compressed, and it's
+/// unlikely that a factor that low will be sufficient to cache all decoded
+/// frames. The default value is `25.0`.


Note: After talking in person, this will be set in a followup PR.

cbracken · 2018-10-09T23:34:47Z

Provide a relative, per-image limit to the amount of memory that's used to cache decoded image frames. Adds an overridable default to `dart:ui` that developers can set to control how much memory images are allowed to use decoded vs undecoded. Note that required frames are always cached regardless of the ratio cap, because they're currently necessary for the GIF to animate. Previously cached unessential frames are not cleared in response to the cache hitting or exceeding the cap. Fixes #20998, #14344

Sets it as an optional param to `instantiateImageCodec`. Will be passed in by the framework in a followup PR.

flutter/engine@bc6df5b...068d6f8 git log bc6df5b..068d6f8 --no-merges --oneline 068d6f8 Roll src/third_party/skia f40ddfabb7f0..a38ab3b094ba (2 commits) (flutter/engine#6484) 7afbcd9 Add a ratio cap to decoded animated image frames (flutter/engine#6310) f8d8777 Roll src/third_party/skia 42137de2b2b8..f40ddfabb7f0 (7 commits) (flutter/engine#6482) The AutoRoll server is located here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md If the roll is causing failures, please contact the current sheriff, who should be CC'd on the roll, and stop the roller if necessary.

Users can set `PaintingBinding.decodedCacheRatioCap` to control the max amount of memory used per image to avoid decoding frames each animation loop. This depends on flutter/engine#6310. Fixes #20998, and fixes #14344

mklim requested review from chinmaygarde and Hixie September 21, 2018 22:28

googlebot added the cla: yes label Sep 21, 2018

mklim mentioned this pull request Sep 26, 2018

Animated gif is causing my App to crash on some Android devices and iOS 12. Seems fine on iOS 11 and below. flutter/flutter#20998

Closed

Hixie reviewed Sep 27, 2018

View reviewed changes

lib/ui/painting.dart Outdated Show resolved Hide resolved

Hixie reviewed Sep 27, 2018

View reviewed changes

mklim mentioned this pull request Sep 28, 2018

Wire decodedCacheRatioCap from ImageCache to Codec flutter/flutter#22452

Merged

Hixie reviewed Oct 5, 2018

View reviewed changes

lib/ui/painting.dart Show resolved Hide resolved

Hixie reviewed Oct 5, 2018

View reviewed changes

lib/ui/painting.dart Show resolved Hide resolved

mklim requested a review from amirh October 8, 2018 21:00

cbracken approved these changes Oct 9, 2018

View reviewed changes

Michael Klimushyn added 2 commits October 9, 2018 16:35

Move decodedCacheRatioCap out of the engine

476bbae

Sets it as an optional param to `instantiateImageCodec`. Will be passed in by the framework in a followup PR.

mklim merged commit 7afbcd9 into flutter:master Oct 10, 2018

mklim deleted the gif_decode_limit branch October 10, 2018 00:07

engine-flutter-autoroll mentioned this pull request Oct 10, 2018

Roll engine bc6df5bc34dc..068d6f8a6351 (3 commits) flutter/flutter#22898

Closed

mklim mentioned this pull request Jan 10, 2019

Remove in memory decoded frame cache flutter/flutter#26081

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a ratio cap to decoded animated image frames #6310

Add a ratio cap to decoded animated image frames #6310

mklim commented Sep 21, 2018 •

edited

Hixie Sep 27, 2018

Hixie Sep 27, 2018

Hixie Sep 27, 2018

Hixie Sep 27, 2018

Hixie Sep 27, 2018

mklim Sep 28, 2018

mklim Sep 28, 2018

mklim commented Sep 28, 2018

Hixie Oct 5, 2018

Hixie Oct 5, 2018

Hixie Oct 5, 2018

Hixie commented Oct 5, 2018

cbracken Oct 9, 2018

cbracken Oct 9, 2018

cbracken commented Oct 9, 2018

Add a ratio cap to decoded animated image frames #6310

Add a ratio cap to decoded animated image frames #6310

Conversation

mklim commented Sep 21, 2018 • edited

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

mklim commented Sep 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hixie commented Oct 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbracken commented Oct 9, 2018

mklim commented Sep 21, 2018 •

edited