High CPU and increased memory usage fix for high-res (GIF, WEBP...) animations.
Patch dependens on skia patch:
https://codereview.chromium.org/1928403002
Addressing issues identified with initial patch:
crrev.com/1857543002: Don't recreate SkImages for high-res (GIF, WEBP...) animations.
by implementing this on different way.
- Out of memory fix
===================
Originally, high res GIF frames are re-created and re-decoded in BitmapImage
during playback , leading to re-creating SkImages and filling up
SoftwareImageDecoderController cache:
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 [1]
[1]
https://bugs.chromium.org/p/chromium/issues/detail?id=165750#c13
Patch here mitigates the issue by using same uniqueId which makes the cache
utilized, whether it is cc SIDC or skia discardable memory cache.
@fmalita's concern from [2], [3] is addressed, too: memory used with page is
now about 3 times lower*.
*) Note:
After rebasing to the latest, memory consumption with page [3] is the same
as in version without the patch (measured on Windows).
Meanwhile, SIDC budget got increased and now I verified that this
patch makes SIDC cache used better - there is no redecoding of every frame
since they are cached in SIDC cache. After some time, depending on number
of GIFs in viewport, it happens that there is no decoding at all while
rendering - all the decoded bitmaps are fetched from SIDC cache.
Without this patch, this isn't a case - there is re-decoding.
Memory consumption (Windows "process working set" [4]) with bug 165750 is:
Original : ~500MB
This patch: ~ 60MB
[2] https://chromiumcodereview.appspot.com/1857543002/#msg10
[3] http://www.androidpolice.com/2014/10/20/animation-bonanza-android-5-0-lollipop-in-gifs/
[4] https://chromiumcodereview.appspot.com/1857543002/#msg13
- Avoid redecoding
==================
Now there is no re-decoding as there is no cache miss when accessing cached
decoded bitmap.
- destroyDecodedData(true) -> destroyDecodedData
================================================
There is no need to have destroyDecodedData(false) and it is removed.
Since original patch [5], 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.
[5] original patch, @pkasting, Dec 2008
https://chromium.googlesource.com/chromium/src/+/f868b0862a903e2a4ffa0fc975080a09ad73cb64
- Remove holding SkImage reference for every frame in BitmapImage
=================================================================
No need to to it also for low size animations - if decoded bitmap is cached in
skia or SIDC then there would be no redecoding. One frame is cached per
BitmapImage (whether it is single or multiple frame) to speedup access.
- destroyDecodedDataIfNecessary replaced by always caching only one frame.
==========================================================================
This removes unnecessary cache layer in BitmapImage::m_frames[].m_frame - if
decoded bitmap is cached in skia or SIDC then there would be no redecoding.
- FrameData -> DeferredFrameData in DeferredImageDecoder
========================================================
DeferredImageDecoder doesn't use m_frame, m_haveMetadata and m_hasAlpha from
FrameData so slimmed down struct introduced with required fields only.
- virtual void stopAnimation() removed.
=======================================
Only called for BitmapImage class. There is no usage for this method in Image
and SVGImage so removed it from there.
- BitmapImage::destroyDecodedData() override private->public
============================================================
virtual Image::destroyDecodedData that this one is overriding is public
from the beginning [6]. instead of downcasting to Image (where the method is
public) and calling it made overrides public in subclasses too.
Purpose of it is unclear in original patch [6] where it replaced invalidateData.
[6] original patch, @hyatt, Mar 2007
https://chromium.googlesource.com/chromium/src/+/d9bbf3787106e2620e8f51f1d371c1441643be6c%5E%21/#F12
BUG=165750
Review-Url: https://codereview.chromium.org/1925533003
Cr-Commit-Position: refs/heads/master@{#395863}
19 files changed
