There is a dependency from this to  Issue 165750  - fix depends on this issue:

https://codereview.chromium.org/1857543002/
 
"
The patch here fixes the same issue in
http://www.classicdoom.com/maps/plusecs/plu08.htm - this 3 frame GIF used to
grow working set from 70 to more than 450MB (filling up the
ImageDecodeController cache with duplicates).

However, situation is opposite with
http://www.androidpolice.com/2014/10/20/animation-bonanza-android-5-0-lollipo...
The patch here increases working set from 350 to 700MB, as you said.

I did some debugging. All works fine (no increased working set) if having this
change (and ImageDecodeController (IDC) is active and patch here applied):

bool SoftwareImageDecodeController::CanHandleImage(const ImageKey& key) {
  // TODO(vmpstr): Start handling medium filter quality as well.
-  return key.filter_quality() != kMedium_SkFilterQuality;
+  return true;
}
"

In other words, IDC (Image decode controller) now fills up cache with duplicates and could lead to out of memory. Potential fix depends on getting medium image quality implemented.

Interesting. What is defined as a working set here? Returning false here means that Skia will be responsible for decoding and caching this image. However, Skia also doesn't keep any memory locked (aside for when it draws). So if the working set includes unlocked discardable memory, then I was under the impression that this is not a big deal (ie, the discardable system will purge memory as needed).

However, if it's a locked discardable memory, then I think we need to investigate why this increases the working set this much.

The memory is not locked, AFAICT we're observing increased resource cache pressure with large animated images.

Setting ImageDecodeController aside for a moment, before Aleksandar's patch Blink would keep the number of cached frames for a given image capped (artificially, based on size heuristics).  As frame SkImages got recycled/deleted, Skia would purge any associated resources.

With the patch, Blink no longer limits the number of cached SkImage frames => Skia is no longer purging their resource cache entries => the only cap is discardable mem limits and backpressure => increased resource cache footprint for animated GIF abusers.

ImageDecodeController takes over decoding/scaling/caching and seems to mitigate this problem (I don't fully understand how), except for kMedium_SkFilterQuality which is pass-through and hits Skia's resource cache as described.

One could argue that increased cache utilization is not necessarily a bad thing, particularly with unlocked discardable mem: that's what caches are for, WAI, see Linux' page cache, etc.  Still, a 2X working set is likely to get noticed and may have other ramifications (what else are we purging when we hit discardable mem limits?).

#2
> What is defined as a working set here?
https://codereview.chromium.org/1857543002/#msg13

Instead of RSS, I used Windows Resource monitor (launch from Task Manager->Performance tab->Open Resource monitor) and for each for process memory there is "working set" column. There are also Commit and Shareable memory (this one grows when caches - skia or IDC grow).

Memory pressure happens both with current code or with my patch, but for different reasons.

1) Current code - with artificial memory cap (Issue 165750#13)
IDC caches SkImages. When BitmapImage removes the reference, SkImage uniqueID information is lost for BitmapImage but entry remains in IDC cache. Next loop, BitmapImage decodes the same thing again and adds new SkImage copy (but with different uniqueID) to IDC cache.

2) With the patch and no support for kMedium_SkFilterQuality
BitmapImage holds the references of SkImages with pixel refs in SkResourceCache.
 
#3
>One could argue that increased cache utilization is not necessarily a bad thing

You are right. Problem with this implementation (artificial memory cap) is that it is removing frames for large gifs when they are needed immediately after removal for rendering. It removes for those that are in viewport (those that are drawn - it seems though that advance animation happens during scrolling even for animateds gifs that are not visible and this needs to be checked too). It could e.g. first remove frames of those that are not needed (e.g. not recently used, out of viewport).

I didn't want to make this task here a fix for https://codereview.chromium.org/1857543002/ - I was just pointing to dependency to the way how IDC handles cache - maybe it shouldn't cache SkImages...

Anyway, important that this task gets developed on it own pace - no need to hurry and make complicated dependencies.
Instead, I'll work on different fix for memory pressure issue 1) - so that there is still a cap in BitmapImage (whatever the logic is, we can make it better later) and that BitmapImage holds the uniqueID so that it can fetch existing IDC copy (by recreating another SkImage with the same unique ID; now, that might be against the rules, to have multiple instances of SkImage with the same uniqueID,... need to check.

There are other problems (like decoder cache holding one copy and IDC/skia cache another).
Finally, after all code gets in place, images would get cached once (one copy) and should be released from cache when e.g. not in viewport - and not like now, based on size during rendering as those big ones (subjective, 2 frame 720p GIF also causes problems) often cost tens of milliseconds to decode.
 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4c4a73d71ba30eca9fd9b8d17f8175f462631689

commit 4c4a73d71ba30eca9fd9b8d17f8175f462631689
Author: cblume <cblume@chromium.org>
Date: Thu Apr 14 21:18:45 2016

Refactor SoftwareImageDecodeController to make adding support for
medium quality more readable and natural.

BUG= 594839 
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Review URL: https://codereview.chromium.org/1801933004

Cr-Commit-Position: refs/heads/master@{#387426}

[modify] https://crrev.com/4c4a73d71ba30eca9fd9b8d17f8175f462631689/cc/tiles/software_image_decode_controller.cc
[modify] https://crrev.com/4c4a73d71ba30eca9fd9b8d17f8175f462631689/cc/tiles/software_image_decode_controller.h

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8b9662d8d9563da3ad545f1e7717d134238c8961

commit 8b9662d8d9563da3ad545f1e7717d134238c8961
Author: cblume <cblume@chromium.org>
Date: Tue May 03 21:20:44 2016

Add medium image quality to software predecode.

Software predecode has low and high qualities supported. This CL adds
support for medium quality.

BUG= 594839 
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Review-Url: https://codereview.chromium.org/1839833003
Cr-Commit-Position: refs/heads/master@{#391357}

[modify] https://crrev.com/8b9662d8d9563da3ad545f1e7717d134238c8961/cc/tiles/software_image_decode_controller.cc
[modify] https://crrev.com/8b9662d8d9563da3ad545f1e7717d134238c8961/cc/tiles/software_image_decode_controller.h
[modify] https://crrev.com/8b9662d8d9563da3ad545f1e7717d134238c8961/cc/tiles/software_image_decode_controller_unittest.cc