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 78511f31d29142eb28e38b71f05c9731c1ab2495..9ff61ac3a4ea0efbf4124cb449c4af3cb116adc0 100644
--- a/third_party/WebKit/Source/platform/graphics/BitmapImage.cpp
+++ b/third_party/WebKit/Source/platform/graphics/BitmapImage.cpp
@@ -69,6 +69,7 @@ BitmapImage::BitmapImage(ImageObserver* observer)
     , m_sizeAvailable(false)
     , m_hasUniformFrameSize(true)
     , m_haveFrameCount(false)
+    , m_frameIndex(0)

[Peter Kasting, 2016/05/04 03:01:06]

Here and below: Initialize members in the same order they were declared.

[aleksandar.stojiljkovic, 2016/05/07 19:50:52]

Done.

 {
 }
 
@@ -86,6 +87,8 @@ BitmapImage::BitmapImage(const SkBitmap& bitmap, ImageObserver* observer)
     , m_haveSize(true)
     , m_sizeAvailable(true)
     , m_haveFrameCount(true)
+    , m_frameIndex(0)
+    , m_frame(adoptRef(SkImage::NewFromBitmap(bitmap)))
 {
     // Since we don't have a decoder, we can't figure out the image orientation.
     // Set m_sizeRespectingOrientation to be the same as m_size so it's not 0x0.
@@ -93,7 +96,6 @@ BitmapImage::BitmapImage(const SkBitmap& bitmap, ImageObserver* observer)
 
     m_frames.grow(1);
     m_frames[0].m_hasAlpha = !bitmap.isOpaque();
-    m_frames[0].m_frame = adoptRef(SkImage::NewFromBitmap(bitmap));
     m_frames[0].m_haveMetadata = true;
 }
 
@@ -107,31 +109,13 @@ 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
-        // save the memory for the framebuffer data), so we don't need to clear
-        // the metadata.
-        m_frames[i].clear(false);
-    }
-
-    m_source.clearCacheExceptFrame(destroyAll ? kNotFound : m_currentFrame);
-    notifyMemoryChanged();
-}
-
-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;
+    m_frame.clear();
     for (size_t i = 0; i < m_frames.size(); ++i)
-        allFrameBytes += m_frames[i].m_frameBytes;
-
-    if (allFrameBytes > cLargeAnimationCutoff) {
-        destroyDecodedData(false);
-    }
+        m_frames[i].clear(true);
+    m_source.clearCacheExceptFrame(kNotFound);
+    notifyMemoryChanged();
 }
 
 void BitmapImage::notifyMemoryChanged()
@@ -149,7 +133,7 @@ size_t BitmapImage::totalFrameBytes()
     return totalBytes;
 }
 
-void BitmapImage::cacheFrame(size_t index)
+PassRefPtr<SkImage> BitmapImage::cacheFrame(size_t index)
 {
     size_t numFrames = frameCount();
     if (m_frames.size() < numFrames)
@@ -157,7 +141,9 @@ 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);

[Peter Kasting, 2016/05/04 03:01:06]

Note that Skia is currently switching to sk_sp and I don't know whether
RefPtr<SkImage> and the like are still desirable in that world.  Make sure you
understand whether this is how the code is supposed to be going forward before
adding this.

[aleksandar.stojiljkovic, 2016/05/04 20:56:29]

I found several patches related to switch to sk_sp on other places in blink,
none of them yet in FrameData and BitmapImage or tests using this.
Change would be significant and make this review more laborious - I prefer to do
refactoring sk_sp -> RefPtr in follow up patch.

+    m_frame = image;
+    m_frameIndex = index;
 
     m_frames[index].m_orientation = m_source.orientationAtIndex(index);
     m_frames[index].m_haveMetadata = true;
@@ -172,6 +158,7 @@ void BitmapImage::cacheFrame(size_t index)
         m_hasUniformFrameSize = false;
 
     notifyMemoryChanged();
+    return image.release();
 }
 
 void BitmapImage::updateSize() const
@@ -230,6 +217,9 @@ bool BitmapImage::dataChanged(bool allDataReceived)
             m_frames[i].clear(true);
     }
 
+    if (m_frameIndex < m_frames.size() && m_frames[m_frameIndex].m_haveMetadata && !m_frames[m_frameIndex].m_isComplete)
+        m_frame.clear();

[Peter Kasting, 2016/05/04 03:01:06]

I think this would be more clearly associated with the loop above if you instead
changed that loop body to:

    if (m_frames[i].m_haveMetadata && !m_frames[i].m_isComplete) {
        m_frames[i].clear(true);
        if (i == m_frameIndex)
            m_frame.clear();
    }

[aleksandar.stojiljkovic, 2016/05/04 20:56:29]

Done.

+
     // Feed all the data we've seen so far to the image decoder.
     m_allDataReceived = allDataReceived;
     ASSERT(data());
@@ -291,7 +281,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())
@@ -333,23 +323,19 @@ bool BitmapImage::isSizeAvailable()
     return m_sizeAvailable;
 }
 
-bool BitmapImage::ensureFrameIsCached(size_t index)
-{
-    if (index >= frameCount())
-        return false;
-
-    if (index >= m_frames.size() || !m_frames[index].m_frame)
-        cacheFrame(index);
-
-    return true;
-}
-
 PassRefPtr<SkImage> BitmapImage::frameAtIndex(size_t index)
 {
-    if (!ensureFrameIsCached(index))
+    if (index >= frameCount())
         return nullptr;
 
-    return m_frames[index].m_frame;
+    RefPtr<SkImage> image;
+    if (index == m_frameIndex)
+        image = m_frame;
+
+    if (image) {
+        return image.release();
+    }
+    return cacheFrame(index);

[Peter Kasting, 2016/05/04 03:01:06]

It seems like this is a long-winded way of writing:

    if (index == m_frameIndex && m_frame) {
        RefPtr<SkImage> image = m_frame;
        return image.release();
    }

    return cacheFrame(index);

[aleksandar.stojiljkovic, 2016/05/04 20:56:29]

Done.

 }
 
 bool BitmapImage::frameIsCompleteAtIndex(size_t index)
@@ -565,9 +551,6 @@ void BitmapImage::resetAnimation()
     m_repetitionsComplete = 0;
     m_desiredFrameStartTime = 0;
     m_animationFinished = false;

[Peter Kasting, 2016/05/04 03:01:06]

Should we clear m_frame here?

[aleksandar.stojiljkovic, 2016/05/04 20:56:29]

Done.

[Peter Kasting, 2016/05/07 01:58:29]

For clarity, I wasn't very sure whether we should or not.  Perhaps keeping
m_cachedFrame would save time once we start animating again?  Though if we don't
start again, perhaps it's a waste of memory unless it's a cache of the first
frame?  So should we clear it iff it is not the first frame?

In any case I'd add a comment on why we should clear this (and, if necessary,
when).

[aleksandar.stojiljkovic, 2016/05/07 19:50:52]

Sorry, the way I handled this clear() was obscure - original code cleared frame
reference from BitmapImage by calling destroyDecodedDataIfNecessary().
It is correct to clear it since this code is called e.g. when all ImageResource
clients get removed to cleanup. 
This is only clearing BitmapImage reference while copy of current frame is still
kept in decoder (decoder is disposed on ~BitmapImage ...
~ImageFrameGenerator()).
Thanks.

[1]https://gist.github.com/astojilj/fbecf7a901a46d774eb9c0253a18eb4c

-
-    // For extremely large animations, when the animation is reset, we just throw everything away.
-    destroyDecodedDataIfNecessary();
 }
 
 bool BitmapImage::maybeAnimated()
@@ -625,12 +608,12 @@ 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;
 }