High CPU and increased memory usage fix for high-res (GIF, WEBP...) animations.

third_party/WebKit/Source/platform/graphics/BitmapImage.cpp

Index: third_party/WebKit/Source/platform/graphics/BitmapImage.cpp
diff --git a/third_party/WebKit/Source/platform/graphics/BitmapImage.cpp b/third_party/WebKit/Source/platform/graphics/BitmapImage.cpp
index fad71475e10d2082229bafd35d958a839138e19c..be7d007001675171d26b91a9861a95177b6b0d0e 100644
--- a/third_party/WebKit/Source/platform/graphics/BitmapImage.cpp
+++ b/third_party/WebKit/Source/platform/graphics/BitmapImage.cpp
@@ -26,6 +26,7 @@
 
 #include "platform/graphics/BitmapImage.h"
 
+#include "base/containers/mru_cache.h"
 #include "platform/PlatformInstrumentation.h"
 #include "platform/Timer.h"
 #include "platform/TraceEvent.h"
@@ -36,9 +37,81 @@
 #include "platform/graphics/StaticBitmapImage.h"
 #include "platform/graphics/skia/SkiaUtils.h"
 #include "third_party/skia/include/core/SkCanvas.h"
+#include "wtf/CheckedNumeric.h"
 #include "wtf/PassRefPtr.h"
 #include "wtf/text/WTFString.h"
 
+namespace {
+
+const size_t kMaxAnimationCacheSize = 1024 * 1024 * 96;
+const size_t kMaxCacheSizeForAnimation = 1024 * 1024 * 24;

[Peter Kasting, 2016/04/28 23:07:54]

These limits seem not only arbitrary but rather alarmingly large for e.g. mobile
devices, especially if they're per-renderer-process.

[aleksandar.stojiljkovic, 2016/04/29 17:17:43]

Done.
Second patch removes this code and also reduces holding references in
BitmapImages (frames are not cached anymore) but only the most recently accessed
frame. 

+
+using ImageMRUCache = base::HashingMRUCache<blink::BitmapImage*, size_t>;
+
+class AnimatedBitmapImageCache : public ImageMRUCache {
+public:
+    static AnimatedBitmapImageCache& AnimatedBitmapImageCache::instance()
+    {
+        DEFINE_THREAD_SAFE_STATIC_LOCAL(AnimatedBitmapImageCache, cache, new AnimatedBitmapImageCache);
+        return cache;
+    }
+
+    void EraseBitmap(blink::BitmapImage* pBitmapImage)

[Peter Kasting, 2016/04/28 23:07:54]

Nit: Use Google style for param naming, no Hungarian notation (multiple places)

[aleksandar.stojiljkovic, 2016/04/29 17:17:43]

Done.
removed the code in second patch.

+    {
+        auto image_it = Peek(pBitmapImage);
+        if (image_it != end()) {
+            m_cacheSize -= image_it->second;
+            Erase(image_it);
+        }
+    }
+
+    void makeSpaceInCache(size_t bytesAboutToAdd)
+    {
+        size_t maxSize = m_maxCacheSize - std::min(bytesAboutToAdd, m_maxCacheSize);
+        if (m_cacheSize <= maxSize)
+            return;
+        for (auto it = rbegin(); m_cacheSize > maxSize && it != rend(); it = rbegin())
+            it->first->destroyDecodedData();
+    }
+
+    void touch(blink::BitmapImage* pBitmapImage)
+    {
+        // Mark that bitmap image was used in MRU cache.
+        Get(pBitmapImage);
+    }
+
+    void cachedSizeChanged(blink::BitmapImage* pBitmapImage, int delta)
+    {
+        auto image_it = Peek(pBitmapImage);
+        if (image_it != end()) {
+            base::CheckedNumeric<intptr_t> signedFramesSize(image_it->second);

[Peter Kasting, 2016/04/28 23:07:54]

intptr_t is not an appropriate type to use for a signed count of bytes.  And why
do you want a signed type here anyway?  It looks as if the idea of this is to
add a (possibly negative) delta to a size_t and ensure you get a (positive)
size_t out.  In that case you should do all the math with checked size_ts.

And do you even need the checked numeric stuff?  When can these sorts of
functions be called with bogus values?  Isn't normal unchecked math good enough?

[aleksandar.stojiljkovic, 2016/04/29 17:17:43]

Done.
Removed the code in second patch.
Needs to be fixed in other places in chromium where I copied the approach from:
e.g. void ImageBuffer::updateGPUMemoryUsage() const

+            signedFramesSize += delta;
+            image_it->second = safeCast<size_t>(signedFramesSize.ValueOrDie());
+        }
+        base::CheckedNumeric<intptr_t> signedCacheSize(m_cacheSize);
+        signedCacheSize += delta;
+        m_cacheSize = safeCast<size_t>(signedCacheSize.ValueOrDie());
+    }
+
+    size_t m_cacheSize = 0;
+    size_t m_maxCacheSize = kMaxAnimationCacheSize;
+    size_t m_maxCacheSizeForAnimation = kMaxCacheSizeForAnimation;
+
+private:
+    AnimatedBitmapImageCache()
+    : ImageMRUCache(ImageMRUCache::NO_AUTO_EVICT)
+    {
+    }
+
+    ~AnimatedBitmapImageCache() override
+    {
+    }
+
+    DISALLOW_COPY_AND_ASSIGN(AnimatedBitmapImageCache);
+};
+
+}
+
 namespace blink {
 
 PassRefPtr<BitmapImage> BitmapImage::createWithOrientationForTesting(const SkBitmap& bitmap, ImageOrientation orientation)
@@ -57,6 +130,7 @@ PassRefPtr<BitmapImage> BitmapImage::createWithOrientationForTesting(const SkBit
 BitmapImage::BitmapImage(ImageObserver* observer)
     : Image(observer)
     , m_currentFrame(0)
+    , m_disposeLaterFrame(kNotFound)
     , m_repetitionCount(cAnimationNone)
     , m_repetitionCountStatus(Unknown)
     , m_repetitionsComplete(0)
@@ -76,6 +150,7 @@ BitmapImage::BitmapImage(const SkBitmap& bitmap, ImageObserver* observer)
     : Image(observer)
     , m_size(bitmap.width(), bitmap.height())
     , m_currentFrame(0)
+    , m_disposeLaterFrame(kNotFound)
     , m_repetitionCount(cAnimationNone)
     , m_repetitionCountStatus(Unknown)
     , m_repetitionsComplete(0)
@@ -100,6 +175,7 @@ BitmapImage::BitmapImage(const SkBitmap& bitmap, ImageObserver* observer)
 BitmapImage::~BitmapImage()
 {
     stopAnimation();
+    AnimatedBitmapImageCache::instance().EraseBitmap(this);
 }
 
 bool BitmapImage::currentFrameHasSingleSecurityOrigin() const
@@ -107,7 +183,7 @@ bool BitmapImage::currentFrameHasSingleSecurityOrigin() const
     return true;
 }
 
-void BitmapImage::destroyDecodedData(bool destroyAll)
+void BitmapImage::destroyDecodedData()
 {
     for (size_t i = 0; i < m_frames.size(); ++i) {
         // The underlying frame isn't actually changing (we're just trying to
@@ -115,25 +191,12 @@ void BitmapImage::destroyDecodedData(bool destroyAll)
         // the metadata.
         m_frames[i].clear(false);
     }
+    AnimatedBitmapImageCache::instance().EraseBitmap(this);
 
-    size_t frameBytesCleared = m_source.clearCacheExceptFrame(destroyAll ? kNotFound : m_currentFrame);
+    size_t frameBytesCleared = m_source.clearCacheExceptFrame(kNotFound);
     notifyMemoryChanged(-safeCast<int>(frameBytesCleared));
 }
 
-void BitmapImage::destroyDecodedDataIfNecessary()
-{
-    // Animated images >5MB are considered large enough that we'll only hang on
-    // to one frame at a time.
-    static const size_t cLargeAnimationCutoff = 5242880;
-    size_t allFrameBytes = 0;
-    for (size_t i = 0; i < m_frames.size(); ++i)
-        allFrameBytes += m_frames[i].m_frameBytes;
-
-    if (allFrameBytes > cLargeAnimationCutoff) {
-        destroyDecodedData(false);
-    }
-}
-
 void BitmapImage::notifyMemoryChanged(int delta)
 {
     if (delta && getImageObserver())
@@ -149,7 +212,7 @@ int BitmapImage::totalFrameBytes()
     return safeCast<int>(totalBytes);
 }
 
-void BitmapImage::cacheFrame(size_t index)
+PassRefPtr<SkImage> BitmapImage::cacheFrame(size_t index)
 {
     size_t numFrames = frameCount();
     if (m_frames.size() < numFrames)
@@ -159,7 +222,19 @@ void BitmapImage::cacheFrame(size_t index)
 
     // We are caching frame snapshots.  This is OK even for partially decoded frames,
     // as they are cleared by dataChanged() when new data arrives.
-    m_frames[index].m_frame = m_source.createFrameAtIndex(index);
+    RefPtr<SkImage> image = m_source.createFrameAtIndex(index);
+    size_t frameBytes = m_source.frameBytesAtIndex(index);
+
+    if (frameCount() > 1) {
+        // Making space in cache might change and notify on memory change and
+        // it is needed to notify on any change before and after the call.

[Peter Kasting, 2016/04/28 23:07:54]

Sorry, I don't understand what this comment is saying.

+        notifyMemoryChanged(totalFrameBytes() - deltaBytes);
+        AnimatedBitmapImageCache::instance().makeSpaceInCache(frameBytes);
+        deltaBytes = totalFrameBytes();
+    }
+
+    m_frames[index].m_frame = image;
+    m_frames[index].m_frameBytes = frameBytes;
 
     m_frames[index].m_orientation = m_source.orientationAtIndex(index);
     m_frames[index].m_haveMetadata = true;
@@ -167,7 +242,9 @@ void BitmapImage::cacheFrame(size_t index)
     if (repetitionCount(false) != cAnimationNone)
         m_frames[index].m_duration = m_source.frameDurationAtIndex(index);
     m_frames[index].m_hasAlpha = m_source.frameHasAlphaAtIndex(index);
-    m_frames[index].m_frameBytes = m_source.frameBytesAtIndex(index);
+
+    if (frameCount() > 1)
+        onAnimationFrameCached(index);
 
     const IntSize frameSize(index ? m_source.frameSizeAtIndex(index) : m_size);
     if (frameSize != m_size)
@@ -178,6 +255,7 @@ void BitmapImage::cacheFrame(size_t index)
     // decoding multiple frames to decode the current frame.
     deltaBytes = totalFrameBytes() - deltaBytes;
     notifyMemoryChanged(deltaBytes);
+    return image.release();
 }
 
 void BitmapImage::updateSize() const
@@ -232,8 +310,11 @@ bool BitmapImage::dataChanged(bool allDataReceived)
         // NOTE: Don't call frameIsCompleteAtIndex() here, that will try to
         // decode any uncached (i.e. never-decoded or
         // cleared-on-a-previous-pass) frames!
-        if (m_frames[i].m_haveMetadata && !m_frames[i].m_isComplete)
+        if (m_frames[i].m_haveMetadata && !m_frames[i].m_isComplete) {
+            if (m_frames.size() > 1)
+                AnimatedBitmapImageCache::instance().cachedSizeChanged(this, -static_cast<int>(m_frames[i].m_frameBytes));
             m_frames[i].clear(true);
+        }
     }
 
     // Feed all the data we've seen so far to the image decoder.
@@ -297,7 +378,7 @@ void BitmapImage::draw(SkCanvas* canvas, const SkPaint& paint, const FloatRect&
         WebCoreClampingModeToSkiaRectConstraint(clampMode));
     canvas->restoreToCount(initialSaveCount);
 
-    if (currentFrameIsLazyDecoded())
+    if (image->isLazyGenerated())
         PlatformInstrumentation::didDrawLazyPixelRef(image->uniqueID());
 
     if (ImageObserver* observer = getImageObserver())
@@ -339,23 +420,30 @@ bool BitmapImage::isSizeAvailable()
     return m_sizeAvailable;
 }
 
-bool BitmapImage::ensureFrameIsCached(size_t index)
+PassRefPtr<SkImage> BitmapImage::frameAtIndex(size_t index)
 {
     if (index >= frameCount())
-        return false;
+        return nullptr;
 
-    if (index >= m_frames.size() || !m_frames[index].m_frame)
-        cacheFrame(index);
+    // Limit cache size; remove previously cached frame from this bitmap that
+    // shouldn't be cached anymore.
+    if (index != m_disposeLaterFrame && m_disposeLaterFrame != kNotFound
+        && m_frames[m_disposeLaterFrame].m_frame && !shouldCacheFrame(m_disposeLaterFrame)) {
+        AnimatedBitmapImageCache::instance().cachedSizeChanged(this, -static_cast<int>(m_frames[m_disposeLaterFrame].m_frameBytes));
+        m_frames[m_disposeLaterFrame].clear(false);
+        m_disposeLaterFrame = kNotFound;
+    }
 
-    return true;
-}
+    RefPtr<SkImage> image;
+    if (index < m_frames.size())
+        image = m_frames[index].m_frame;
 
-PassRefPtr<SkImage> BitmapImage::frameAtIndex(size_t index)
-{
-    if (!ensureFrameIsCached(index))
-        return nullptr;
-
-    return m_frames[index].m_frame;
+    if (image) {
+        if (m_frames.size() > 1)
+            AnimatedBitmapImageCache::instance().touch(this);
+        return image.release();
+    }
+    return cacheFrame(index);
 }
 
 bool BitmapImage::frameIsCompleteAtIndex(size_t index)
@@ -571,9 +659,6 @@ void BitmapImage::resetAnimation()
     m_repetitionsComplete = 0;
     m_desiredFrameStartTime = 0;
     m_animationFinished = false;
-
-    // For extremely large animations, when the animation is reset, we just throw everything away.
-    destroyDecodedDataIfNecessary();
 }
 
 bool BitmapImage::maybeAnimated()
@@ -631,13 +716,57 @@ bool BitmapImage::internalAdvanceAnimation(bool skippingFrames)
         } else
             m_currentFrame = 0;
     }
-    destroyDecodedDataIfNecessary();
 
     // We need to draw this frame if we advanced to it while not skipping, or if
     // while trying to skip frames we hit the last frame and thus had to stop.
     if (skippingFrames != advancedAnimation)
         getImageObserver()->animationAdvanced(this);
+
     return advancedAnimation;
 }
 
+bool BitmapImage::shouldCacheFrame(size_t index)
+{
+    if (m_frames.size() <= 1)
+        return true;
+    // For large animations it doesnt make sense to cache the most recently used
+    // frames since when looping this would lead to continuous evict, re-decode,
+    // re-cache for every frame rendered. Instead, cache only first
+    // (cache->m_maxCacheSizeForAnimation / frameSize) frames - we can prevent
+    // re-decode and re-caching for them.

[Peter Kasting, 2016/04/28 23:07:54]

This doesn't seem like a good policy.

If you wanted to cache a fixed set of frames per image, caching the first frames
makes the least sense since they're basically guaranteed to be the cheapest to
redecode.

What you want to do is adjust the caching policy based on the frame disposal
methods (which are analyzed to create the "required previous frame" fields). 
Basically, if every frame can be decoded with no required previous frames, then
the only benefit to caching is in saving the decode of the particular frames
which get cached; you can pick any frames you want from the image, or none (if
this allows you to spend your cache budget elsewhere).

Similarly, if you never skip decoding any frames, then as long as you've always
preserved the required previous frame for the next frames, every frame decode
basically occurs in isolation and you can again just cache whatever.

But when frames are built atop previous frames, and you might end up skipping
frames when decoding, then you basically want to cache "keyframes" such that you
limit the maximum length of such sequences to small numbers.  This ensures that
seeking through the image will never cause you to have to decode lots and lots
of frames.

+    // Asuming all frames are of the same size and 32bit per pixel is good
+    // enough for purpose here - controlling cache size.
+    assert(isSizeAvailable());
+    return ((index + 1) * sizeof(ImageFrame::PixelData) * size().width() * size().height()
+        <= AnimatedBitmapImageCache::instance().m_maxCacheSizeForAnimation);
+}
+
+void BitmapImage::onAnimationFrameCached(size_t index)
+{
+    // calculate cached frame bytes for all frames.
+    size_t cachedFrameBytes = 0;
+    for (size_t i = 0; i < m_frames.size(); ++i)
+        cachedFrameBytes += m_frames[i].m_frameBytes;
+
+    AnimatedBitmapImageCache* cache = &AnimatedBitmapImageCache::instance();
+    bool shouldCache = shouldCacheFrame(index);
+    // If the frame shouldn't be cached, don't change order in MRU cache.
+    auto image_it = shouldCache ? cache->Get(this) : cache->Peek(this);
+    if (image_it != cache->end()) {
+        // image_it->second - frame bytes of cached BitmapImage frames.
+        cache->m_cacheSize -= image_it->second;
+        image_it->second = cachedFrameBytes;
+    } else {
+        cache->Put(this, cachedFrameBytes);
+    }
+    cache->m_cacheSize += cachedFrameBytes;
+    m_disposeLaterFrame = shouldCache ? kNotFound : index;
+}
+
+void BitmapImage::setAnimationCacheSizeForTesting(size_t maxCacheSize, size_t maxAnimationSizeInCache)
+{
+    AnimatedBitmapImageCache::instance().m_maxCacheSize = maxCacheSize;
+    AnimatedBitmapImageCache::instance().m_maxCacheSizeForAnimation = maxAnimationSizeInCache;
+}
+
 } // namespace blink