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

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

 /*
  * Copyright (C) 2006 Samuel Weinig (sam.weinig@gmail.com)
  * Copyright (C) 2004, 2005, 2006, 2008 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  * 1. Redistributions of source code must retain the above copyright
  *    notice, this list of conditions and the following disclaimer.
  * 2. Redistributions in binary form must reproduce the above copyright
@@ skipping 51 matching lines @@
     , m_repetitionsComplete(0)
     , m_desiredFrameStartTime(0)
     , m_frameCount(0)
     , m_animationPolicy(ImageAnimationPolicyAllowed)
     , m_animationFinished(false)
     , m_allDataReceived(false)
     , m_haveSize(false)
     , 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.

 {
 }
 
 BitmapImage::BitmapImage(const SkBitmap& bitmap, ImageObserver* observer)
     : Image(observer)
     , m_size(bitmap.width(), bitmap.height())
     , m_currentFrame(0)
     , m_repetitionCount(cAnimationNone)
     , m_repetitionCountStatus(Unknown)
     , m_repetitionsComplete(0)
     , m_frameCount(1)
     , m_animationPolicy(ImageAnimationPolicyAllowed)
     , m_animationFinished(true)
     , m_allDataReceived(true)
     , 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.
     m_sizeRespectingOrientation = m_size;
 
     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;
 }
 
 BitmapImage::~BitmapImage()
 {
     stopAnimation();
 }
 
 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);
+    m_frame.clear();
+    for (size_t i = 0; i < m_frames.size(); ++i)
+        m_frames[i].clear(true);
+    m_source.clearCacheExceptFrame(kNotFound);
     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;
-    for (size_t i = 0; i < m_frames.size(); ++i)
-        allFrameBytes += m_frames[i].m_frameBytes;
-
-    if (allFrameBytes > cLargeAnimationCutoff) {
-        destroyDecodedData(false);
-    }
-}
-
 void BitmapImage::notifyMemoryChanged()
 {
     if (getImageObserver())
         getImageObserver()->decodedSizeChangedTo(this, totalFrameBytes());
 }
 
 size_t BitmapImage::totalFrameBytes()
 {
     const size_t numFrames = frameCount();
     size_t totalBytes = 0;
     for (size_t i = 0; i < numFrames; ++i)
         totalBytes += m_source.frameBytesAtIndex(i);
     return totalBytes;
 }
 
-void BitmapImage::cacheFrame(size_t index)
+PassRefPtr<SkImage> BitmapImage::cacheFrame(size_t index)
 {
     size_t numFrames = frameCount();
     if (m_frames.size() < numFrames)
         m_frames.grow(numFrames);
 
     // 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;
     m_frames[index].m_isComplete = m_source.frameIsCompleteAtIndex(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);
 
     const IntSize frameSize(index ? m_source.frameSizeAtIndex(index) : m_size);
     if (frameSize != m_size)
         m_hasUniformFrameSize = false;
 
     notifyMemoryChanged();
+    return image.release();
 }
 
 void BitmapImage::updateSize() const
 {
     if (!m_sizeAvailable || m_haveSize)
         return;
 
     m_size = m_source.size();
     m_sizeRespectingOrientation = m_source.size(RespectImageOrientation);
     m_haveSize = true;
@@ skipping 38 matching lines @@
     // frame affected by appending new data here. Thus we have to clear all the
     // incomplete frames to be safe.
     for (size_t i = 0; i < m_frames.size(); ++i) {
         // 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)
             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());
     m_source.setData(*data(), allDataReceived);
 
     m_haveFrameCount = false;
     m_hasUniformFrameSize = true;
     return isSizeAvailable();
 }
 
@@ skipping 41 matching lines @@
             // the image, as it was needed for page layout, so we need to reverse it back here.
             adjustedDstRect = FloatRect(adjustedDstRect.x(), adjustedDstRect.y(), adjustedDstRect.height(), adjustedDstRect.width());
         }
     }
 
     SkRect skSrcRect = adjustedSrcRect;
     canvas->drawImageRect(image.get(), skSrcRect, adjustedDstRect, &paint,
         WebCoreClampingModeToSkiaRectConstraint(clampMode));
     canvas->restoreToCount(initialSaveCount);
 
-    if (currentFrameIsLazyDecoded())
+    if (image->isLazyGenerated())
         PlatformInstrumentation::didDrawLazyPixelRef(image->uniqueID());
 
     if (ImageObserver* observer = getImageObserver())
         observer->didDraw(this);
 
     startAnimation();
 }
 
 size_t BitmapImage::frameCount()
 {
@@ skipping 21 matching lines @@
 
     if (m_sizeAvailable && hasVisibleImageSize(size())) {
         BitmapImageMetrics::countDecodedImageType(m_source.filenameExtension());
         if (m_source.filenameExtension() == "jpg")
             BitmapImageMetrics::countImageOrientation(m_source.orientationAtIndex(0).orientation());
     }
 
     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)
 {
     if (index < m_frames.size() && m_frames[index].m_haveMetadata && m_frames[index].m_isComplete)
         return true;
 
     return m_source.frameIsCompleteAtIndex(index);
 }
 
@@ skipping 194 matching lines @@
     // the timer unless all renderers have stopped drawing.
     m_frameTimer.clear();
 }
 
 void BitmapImage::resetAnimation()
 {
     stopAnimation();
     m_currentFrame = 0;
     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()
 {
     if (m_animationFinished)
         return false;
     if (frameCount() > 1)
         return true;
 
     return m_source.repetitionCount() != cAnimationNone;
@@ skipping 37 matching lines @@
         // because it is 0 (see comments on its declaration in ImageAnimation.h).
         if ((repetitionCount(true) != cAnimationLoopInfinite && m_repetitionsComplete > m_repetitionCount)
             || (m_animationPolicy == ImageAnimationPolicyAnimateOnce && m_repetitionsComplete > 0)) {
             m_animationFinished = true;
             m_desiredFrameStartTime = 0;
             --m_currentFrame;
             advancedAnimation = false;
         } 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;
 }
 
 } // namespace blink