Don't recreate SkImages for high-res (GIF, WEBP...) animations.

Don't recreate SkImages for high-res (GIF) animations.

Since original patch [1], pixel data caching got moved to skia discardable
memory and recently to SoftwareImageDecodeController.
As ImageDecoder already caches only one (2 in some cases) frame, this code's
effect was to remove SkImage for animations where all combined frames data is
over 5 MB.

[1] original patch, @pkasting, Dec 2008
https://chromium.googlesource.com/chromium/src/+/f868b0862a903e2a4ffa0fc975080a09ad73cb64

The problem caused here (re-decoding and increased memory allocation) is that
SkImage::uniqueID() is used as SoftwareImageDecodeController::decoded_images_
cache key - clearing it causes that cache is filled for every loop and every
frame rendered (always decoding and adding to cache). Next loop's SkImages
have different uniqueId and keep filling up the cache with duplicates of
same frames, evicting useful content. Detailed information about the issue
is in comment here [2]

[2]
https://bugs.chromium.org/p/chromium/issues/detail?id=165750#c13

BUG=165750

[aleksandar.stojiljkovic, 2016-04-02 14:26:32]

Description was changed from

==========
Don't recreate SkImages for high-res (GIF) animations.

Since original patch [1], pixel data caching got moved to skia discardable
memory and recently to SoftwareImageDecodeController.
As ImageDecoder already caches only one (2 in some cases) frame, this code's
effect was to remove SkImage for animations where all combined frames data is
over 5 MB.

[1] original patch, @pkasting, Dec 2008
https://chromium.googlesource.com/chromium/src/+/f868b0862a903e2a4ffa0fc97508...

The problem caused here (re-decoding and increased memory allocation) is that
SkImage::uniqueID() is used as SoftwareImageDecodeController::decoded_images_
cache key - clearing it causes that cache is filled for every loop and every
frame rendered (always decoding and adding to cache). Next loop's SkImages
have different uniqueId and keep filling up the cache with duplicates of
same frames, evicting useful content. Detailed information about the issue
is in comment here [2]

[2]
https://bugs.chromium.org/p/chromium/issues/detail?id=165750#c13

BUG=165750
==========

to

==========
Don't recreate SkImages for high-res (GIF) animations.

Since original patch [1], pixel data caching got moved to skia discardable
memory and recently to SoftwareImageDecodeController.
As ImageDecoder already caches only one (2 in some cases) frame, this code's
effect was to remove SkImage for animations where all combined frames data is
over 5 MB.

[1] original patch, @pkasting, Dec 2008
https://chromium.googlesource.com/chromium/src/+/f868b0862a903e2a4ffa0fc97508...

The problem caused here (re-decoding and increased memory allocation) is that
SkImage::uniqueID() is used as SoftwareImageDecodeController::decoded_images_
cache key - clearing it causes that cache is filled for every loop and every
frame rendered (always decoding and adding to cache). Next loop's SkImages
have different uniqueId and keep filling up the cache with duplicates of
same frames, evicting useful content. Detailed information about the issue
is in comment here [2]

[2]
https://bugs.chromium.org/p/chromium/issues/detail?id=165750#c13

BUG=165750
==========

[aleksandar.stojiljkovic, 2016-04-02 14:32:34]

aleksandar.stojiljkovic@intel.com changed reviewers:
+ pkasting@chromium.org, senorblanco@chromium.org, vmpstr@chromium.org

[aleksandar.stojiljkovic, 2016-04-02 14:32:34]

Review needed from
@senorblanco, as the owner.

Cc-ing
@pkasting, as author of original patch,
@vmpstr, as the issue is related to recently developed blink decode controller
and caching.

Thanks.

[Peter Kasting, 2016-04-03 09:14:52]

It seems like this eliminates any need for destroyDecodedData().  I don't think
that's used outside of tests after this change.

Note that I'm not signing off on this change yet -- I haven't thought about it
deeply enough -- I just think you need to remove more stuff in this CL.

[Peter Kasting, 2016-04-03 09:18:45]

Also, from your description, it sounds as if this is a workaround for some
suboptimal caching behavior.  It may be good to remove this anyway, but are you
sure there don't need to be changes to the lower-level caching stuff as well?

[aleksandar.stojiljkovic, 2016-04-03 10:35:02]

On 2016/04/03 09:14:52, Peter Kasting wrote:
> It seems like this eliminates any need for destroyDecodedData().  I don't
think
> that's used outside of tests after this change.
> 
> Note that I'm not signing off on this change yet -- I haven't thought about it
> deeply enough -- I just think you need to remove more stuff in this CL.

There is a call destroyDecodedData(destroyAll = true) in virtual void
ImageResource::destroyDecodedDataIfPossible that I didn't want to remove as part
of this patch - that is, at least not until discussing with you here.
Anyway, I could remove destroyDecodedData(destroyAll = false) branch and tests
first.

[aleksandar.stojiljkovic, 2016-04-03 11:17:15]

On 2016/04/03 09:18:45, Peter Kasting wrote:
> Also, from your description, it sounds as if this is a workaround for some
> suboptimal caching behavior.  It may be good to remove this anyway, but are
you
> sure there don't need to be changes to the lower-level caching stuff as well?

Initially, i thought it looked like a workaround. But then after checking usage
of SkImage::uniqueId() in  ImageDecodeControllerKey::FromDrawImage and in
SkImageCacherator (previous caching using discardable memory), started to think
opposite - BitmapImage instantiates frames and advances animation. Since it
doesn't control cache evicting strategy and cannot control caching in GIF
animation decoder (where last frames are cached), it looked better to stop
clearing uniqueID for a subset of animations with combined frames size > 5.2MB
(and this way disabling caching).

There is no change needed in layers bellow - it works fine; same way as other
animations with < 5.2 MBs for frames pixel information. First loop gets cached
and then just taken from cache. Debugger verified.

On a related note, in next patches the plan is to join ImageDecoder cache with
SoftwareImageDecodeCotnroller cache; there seems to be no need to keep same
frame cached in both places and do memcpy in ImageFrameGenerator.

[aleksandar.stojiljkovic, 2016-04-03 12:55:01]

Patch Set 2 : review #4 fix. Thanks @pkasting.

[f(malita), 2016-04-04 15:33:16]

fmalita@chromium.org changed reviewers:
+ fmalita@chromium.org

[f(malita), 2016-04-04 15:33:16]

I think this change makes sense, but do you mind checking the impact on memory?

One thing the removed logic provides is an (artificial) cap on
degenerate/abusive image resource cache utilization (think videos masquerading
as GIFs).

I tried a similar experiment a few months back (http://crrev.com/1359953002,
before ImageDecodeController IINM), and abandoned it because it increased the
footprint significantly: IIRC RSS went from ~400MB to ~900MB for something like
http://www.androidpolice.com/2014/10/20/animation-bonanza-android-5-0-lollipo...
on my Linux workstation (not sure whether we have any suitable tests, I was just
loading that URL, scrolling around a bit and then observing the process RES size
in top).

Without ImageDecodeController, more live frames meant we were keeping more
pixels in Skia's resource cache => given enough frames we were exhausting the
whole budget.

With ImageDecodeController in place, Skia is no longer managing the lifetime of
decoded SkImages and IDC is responsible for enforcing its own budget.  But
having more lazy decoded frames may still increase the pressure on IDC and its
cache utilization.  And maybe that's perfectly fine (the purist in me would
argue that's what the cache is for, so WAI), but then it's a CPU vs. memory
trade-off we need to be aware of.

[f(malita), 2016-04-04 15:33:50]

fmalita@chromium.org changed reviewers:
- fmalita@chromium.org

[Peter Kasting, 2016-04-05 01:55:58]

LGTM on the code itself.  fmalita's concerns about overall behavior are stuff
I'm not as qualified to think about.

https://codereview.chromium.org/1857543002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/BitmapImageTest.cpp (right):

https://codereview.chromium.org/1857543002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/BitmapImageTest.cpp:288:
frameAtIndex(0);
This frameAtIndex() call should be removed as well.

[aleksandar.stojiljkovic, 2016-04-05 17:27:17]

On 2016/04/04 15:33:16, f(malita) wrote:
> I think this change makes sense, but do you mind checking the impact on
memory?
> ... 
>
http://www.androidpolice.com/2014/10/20/animation-bonanza-android-5-0-lollipo...

Thanks for the link.
Instead of RSS, I used Windows Resource monitor (launch from Task Manager) and
process "working set" column
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;
}

@vmpstr - why medium quality handling is different from low quality?
Is it so that if CanHandleImage returns false - those pixelrefs are cached to
skia's DM? And if CanHandleImage returns true then SkImages are cached in IDC?

> 
> One thing the removed logic provides is an (artificial) cap on
> degenerate/abusive image resource cache utilization (think videos masquerading
> as GIFs).
> 
> I tried a similar experiment a few months back (http://crrev.com/1359953002,
> before ImageDecodeController IINM), and abandoned it because it increased the
> footprint significantly: IIRC RSS went from ~400MB to ~900MB for something
like
>
http://www.androidpolice.com/2014/10/20/animation-bonanza-android-5-0-lollipo...
> on my Linux workstation (not sure whether we have any suitable tests, I was
just
> loading that URL, scrolling around a bit and then observing the process RES
size
> in top).
I have debugged your patch a bit - on the page
http://www.androidpolice.com/2014/10/20/animation-bonanza-android-5-0-lollipo...,
now there are only lazy decoded frames.

> 
> Without ImageDecodeController, more live frames meant we were keeping more
> pixels in Skia's resource cache => given enough frames we were exhausting the
> whole budget.
Yes. Shared memory allocation in SkResourceCache wasn't pinned - and even fresh
content would get evicted. Need to analyze where is this allocation.

> 
> With ImageDecodeController in place, Skia is no longer managing the lifetime
of
> decoded SkImages and IDC is responsible for enforcing its own budget.  But
> having more lazy decoded frames may still increase the pressure on IDC and its
> cache utilization.  And maybe that's perfectly fine (the purist in me would
> argue that's what the cache is for, so WAI), but then it's a CPU vs. memory
> trade-off we need to be aware of.


I have tried to use SkImage::unique() and only clear them (always, not only for
> 5MB) in BitmapImage::destroyDecodedDataIfNecessary.
This would fix issue with
http://www.androidpolice.com/2014/10/20/animation-bonanza-android-5-0-lollipo...
(as those SkImages are outside IDC cache - if in SkResourceCache then SkImages
are not referenced)
However, it won't fix http://www.classicdoom.com/maps/plusecs/plu08.htm, in
BitmapImage::destroyDecodedDataIfNecessary SkImages are not yet referenced by
IDC cache.

@vmpstr The issue with increased "working set > 500MB per page" is most likely
happening with the latest code. I'm running code from 12.03 here (didn't check
with the latest code).

[aleksandar.stojiljkovic, 2016-04-27 15:49:33]

On 2016/04/05 17:27:17, aleksandar.stojiljkovic wrote:
> On 2016/04/04 15:33:16, f(malita) wrote:
> > I think this change makes sense, but do you mind checking the impact on
> memory?
> > ... 
> >
>
http://www.androidpolice.com/2014/10/20/animation-bonanza-android-5-0-lollipo...
> 
> Thanks for the link.
> Instead of RSS, I used Windows Resource monitor (launch from Task Manager) and
> process "working set" column
> 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;
> }
> 
> @vmpstr - why medium quality handling is different from low quality?
> Is it so that if CanHandleImage returns false - those pixelrefs are cached to
> skia's DM? And if CanHandleImage returns true then SkImages are cached in IDC?
> 
> > 
> > One thing the removed logic provides is an (artificial) cap on
> > degenerate/abusive image resource cache utilization (think videos
masquerading
> > as GIFs).
> > 
> > I tried a similar experiment a few months back (http://crrev.com/1359953002,
> > before ImageDecodeController IINM), and abandoned it because it increased
the
> > footprint significantly: IIRC RSS went from ~400MB to ~900MB for something
> like
> >
>
http://www.androidpolice.com/2014/10/20/animation-bonanza-android-5-0-lollipo...
> > on my Linux workstation (not sure whether we have any suitable tests, I was
> just
> > loading that URL, scrolling around a bit and then observing the process RES
> size
> > in top).
> I have debugged your patch a bit - on the page
>
http://www.androidpolice.com/2014/10/20/animation-bonanza-android-5-0-lollipo...,
> now there are only lazy decoded frames.
> 
> > 
> > Without ImageDecodeController, more live frames meant we were keeping more
> > pixels in Skia's resource cache => given enough frames we were exhausting
the
> > whole budget.
> Yes. Shared memory allocation in SkResourceCache wasn't pinned - and even
fresh
> content would get evicted. Need to analyze where is this allocation.
> 
> > 
> > With ImageDecodeController in place, Skia is no longer managing the lifetime
> of
> > decoded SkImages and IDC is responsible for enforcing its own budget.  But
> > having more lazy decoded frames may still increase the pressure on IDC and
its
> > cache utilization.  And maybe that's perfectly fine (the purist in me would
> > argue that's what the cache is for, so WAI), but then it's a CPU vs. memory
> > trade-off we need to be aware of.
> 
> 
> I have tried to use SkImage::unique() and only clear them (always, not only
for
> > 5MB) in BitmapImage::destroyDecodedDataIfNecessary.
> This would fix issue with
>
http://www.androidpolice.com/2014/10/20/animation-bonanza-android-5-0-lollipo...
> (as those SkImages are outside IDC cache - if in SkResourceCache then SkImages
> are not referenced)
> However, it won't fix http://www.classicdoom.com/maps/plusecs/plu08.htm, in
> BitmapImage::destroyDecodedDataIfNecessary SkImages are not yet referenced by
> IDC cache.
> 
> @vmpstr The issue with increased "working set > 500MB per page" is most likely
> happening with the latest code. I'm running code from 12.03 here (didn't check
> with the latest code).

Closing this issue since I'm working on different fix and have opened another
review thread: https://crrev.com/1925533003/