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

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

Index: third_party/WebKit/Source/platform/graphics/BitmapImageTest.cpp
diff --git a/third_party/WebKit/Source/platform/graphics/BitmapImageTest.cpp b/third_party/WebKit/Source/platform/graphics/BitmapImageTest.cpp
index 44c76f95d9e1669bcc86f3d5d4a0192c4573501f..f5f8e2a3f8a43cfcddad5562a9715b522ee4ed4e 100644
--- a/third_party/WebKit/Source/platform/graphics/BitmapImageTest.cpp
+++ b/third_party/WebKit/Source/platform/graphics/BitmapImageTest.cpp
@@ -38,6 +38,8 @@
 #include "platform/testing/UnitTestHelpers.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
+#define BITMAP_IMAGE_TEST 1

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

Don't add this.

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

Done.

+
 namespace blink {
 
 class BitmapImageTest : public ::testing::Test {
@@ -71,11 +73,11 @@ public:
     }
 
     // Accessors to BitmapImage's protected methods.
-    void destroyDecodedData(bool destroyAll) { m_image->destroyDecodedData(destroyAll); }
+    void destroyDecodedData() { m_image->destroyDecodedData(); }
     size_t frameCount() { return m_image->frameCount(); }
-    void frameAtIndex(size_t index)
+    PassRefPtr<SkImage> frameAtIndex(size_t index)
     {
-        m_image->frameAtIndex(index);
+        return m_image->frameAtIndex(index);
     }
     void setCurrentFrame(size_t frame) { m_image->m_currentFrame = frame; }
     size_t frameDecodedSize(size_t frame) { return m_image->m_frames[frame].m_frameBytes; }
@@ -132,6 +134,16 @@ public:
         return m_image->imageForDefaultFrame();
     }
 
+    void setAnimationCacheSizeForTesting(size_t maxCacheSize, size_t maxAnimationSizeInCache)
+    {
+        BitmapImage::setAnimationCacheSizeForTesting(maxCacheSize, maxAnimationSizeInCache);
+    }
+
+    int lastDecodedSizeChange()
+    {
+        return m_imageObserver->m_lastDecodedSizeChangedDelta;
+    }
+
 protected:
     void SetUp() override
     {
@@ -147,25 +159,13 @@ private:
     bool m_enableDeferredDecoding;
 };
 
-TEST_F(BitmapImageTest, destroyDecodedDataExceptCurrentFrame)
-{
-    loadImage("/LayoutTests/fast/images/resources/animated-10color.gif");
-    size_t totalSize = decodedSize();
-    size_t frame = frameCount() / 2;
-    setCurrentFrame(frame);
-    size_t size = frameDecodedSize(frame);
-    destroyDecodedData(false);
-    EXPECT_LT(m_imageObserver->m_lastDecodedSizeChangedDelta, 0);
-    EXPECT_GE(m_imageObserver->m_lastDecodedSizeChangedDelta, -static_cast<int>(totalSize - size));
-}
-
 TEST_F(BitmapImageTest, destroyAllDecodedData)

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

Nit: Remove "All"?

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

Done.

 {
     loadImage("/LayoutTests/fast/images/resources/animated-10color.gif");
     size_t totalSize = decodedSize();
     EXPECT_GT(totalSize, 0u);
-    destroyDecodedData(true);
-    EXPECT_EQ(-static_cast<int>(totalSize), m_imageObserver->m_lastDecodedSizeChangedDelta);
+    destroyDecodedData();
+    EXPECT_EQ(-static_cast<int>(totalSize), lastDecodedSizeChange());
     EXPECT_EQ(0u, decodedSize());
 }
 
@@ -267,30 +267,23 @@ TEST_F(BitmapImageTest, correctDecodedDataSize)
     loadImage("/LayoutTests/fast/images/resources/anim_none.gif", false);
     frameAtIndex(1);
     int frameSize = static_cast<int>(m_image->size().area() * sizeof(ImageFrame::PixelData));
-    EXPECT_EQ(frameSize * 2, m_imageObserver->m_lastDecodedSizeChangedDelta);
-
-    // Trying to destroy all data except an undecoded frame should cause the
-    // decoder to seek backwards and preserve the most recent previous frame
-    // necessary to decode that undecoded frame, and destroy all other frames.
-    setCurrentFrame(2);
-    destroyDecodedData(false);
-    EXPECT_EQ(-frameSize, m_imageObserver->m_lastDecodedSizeChangedDelta);
+    EXPECT_EQ(frameSize * 2, lastDecodedSizeChange());
 }
 
 TEST_F(BitmapImageTest, recachingFrameAfterDataChanged)
 {
     loadImage("/LayoutTests/fast/images/resources/green.jpg");
     setFirstFrameNotComplete();
-    EXPECT_GT(m_imageObserver->m_lastDecodedSizeChangedDelta, 0);
+    EXPECT_GT(lastDecodedSizeChange(), 0);
     m_imageObserver->m_lastDecodedSizeChangedDelta = 0;
 
     // Calling dataChanged causes the cache to flush, but doesn't affect the
     // source's decoded frames. It shouldn't affect decoded size.
     m_image->dataChanged(true);
-    EXPECT_EQ(0, m_imageObserver->m_lastDecodedSizeChangedDelta);
+    EXPECT_EQ(0, lastDecodedSizeChange());
     // Recaching the first frame also shouldn't affect decoded size.
     m_image->imageForCurrentFrame();
-    EXPECT_EQ(0, m_imageObserver->m_lastDecodedSizeChangedDelta);
+    EXPECT_EQ(0, lastDecodedSizeChange());
 }
 
 class BitmapImageDeferredDecodingTest : public BitmapImageTest {
@@ -298,6 +291,89 @@ public:
     BitmapImageDeferredDecodingTest() : BitmapImageTest(true) { }
 };
 
+class BitmapImageTestEmptyImpl : public BitmapImageTest {
+public:
+    BitmapImageTestEmptyImpl() : BitmapImageTest(true)
+    {
+        SetUp();
+    }
+
+    void TestBody() override
+    {
+    }
+};
+
+TEST_F(BitmapImageDeferredDecodingTest, animationBitmapImageCacheMaxPerAnimation)
+{
+    loadImage("/LayoutTests/fast/images/resources/anim_none.gif", false);
+    uint32_t image0 = frameAtIndex(0)->uniqueID();
+    int frameSize = static_cast<int>(m_image->size().area() * sizeof(ImageFrame::PixelData));

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

Nit: Should be size_t with no cast since that's what you pass to
setAnimationCacheSizeForTesting() (2 places)

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

This method is removed in patch#2 so Done applies to all ot them bellow.

+    setAnimationCacheSizeForTesting(frameSize * 4, frameSize * 2);
+
+    uint32_t image1 = frameAtIndex(1)->uniqueID();
+    uint32_t image0v1 = frameAtIndex(0)->uniqueID();

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

Nit: Inline into next statement

+    EXPECT_EQ(image0, image0v1);
+    uint32_t image2 = frameAtIndex(2)->uniqueID();
+    uint32_t image1v1 = frameAtIndex(1)->uniqueID();

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

Nit: Inline into next statement

+    // It is expected that 0 and 1 is kept in the cache, and 2 is not cached
+    // as only first two frames are cached.
+    EXPECT_EQ(image1, image1v1);
+    uint32_t image2v1 = frameAtIndex(2)->uniqueID();

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

Nit: Inline into next statement

+    EXPECT_NE(image2, image2v1);
+}
+
+TEST_F(BitmapImageDeferredDecodingTest, animationBitmapImageCacheEvict)
+{
+    loadImage("/LayoutTests/fast/images/resources/anim_none.gif", false);
+    uint32_t image0 = frameAtIndex(0)->uniqueID();
+    int frameSize = static_cast<int>(m_image->size().area() * sizeof(ImageFrame::PixelData));
+    setAnimationCacheSizeForTesting(frameSize * 2, frameSize * 2);
+
+    // Load another animation that would evict first animation frames

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

Nit: Trailing period

+    BitmapImageTestEmptyImpl second;
+    second.loadImage("/LayoutTests/fast/images/resources/anim_none.gif", false);
+    uint32_t second0 = second.frameAtIndex(0)->uniqueID();
+
+    uint32_t image0v1 = frameAtIndex(0)->uniqueID();
+    EXPECT_EQ(image0, image0v1);
+
+    uint32_t second0v1 = second.frameAtIndex(0)->uniqueID();
+    EXPECT_EQ(second0, second0v1);
+    second.frameAtIndex(1);
+
+    // Second 0 and second 1 are now in the cache.
+    uint32_t image0v2 = frameAtIndex(0)->uniqueID();
+    EXPECT_NE(image0v1, image0v2);

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

Nit: Compare to |image0| so |image0v1| above can be inlined into that EXPECT

+
+    // Second was evicted as original frame 0 + second 2 frames are above cache
+    // size even second itself is under per animation limit size.

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

This sentence has grammar issues severe enough that I don't understand what it's
trying to say.

+    uint32_t second0v2 = second.frameAtIndex(0)->uniqueID();
+    EXPECT_NE(second0v1, second0v2);

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

Nit: Compare to |second0| so |second0v1| above can be inlined into that EXPECT

+
+    uint32_t image0v3 = frameAtIndex(0)->uniqueID();
+    EXPECT_EQ(image0v2, image0v3);
+    uint32_t second0v3 = second.frameAtIndex(0)->uniqueID();
+    EXPECT_EQ(second0v2, second0v3);
+
+    // By setAnimationCacheSizeForTesting it is defined to keep in cache first
+    // two frames per animation. Other frames, like frame 2 is cached only
+    // temporarily, until access to some other frame.
+    // When making space for frame 2 it in cache, the very same ImageBitmap that
+    // contains this frame will first get cleared and removed from cache (as it
+    // is first for removal in MRU cache), frame index 2 added to it and cached.

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

Similarly this whole comment has significant grammar issues.

+    frameAtIndex(2);
+    uint32_t image0v4 = frameAtIndex(0)->uniqueID();
+    EXPECT_NE(image0v3, image0v4);

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

Nit: Compare to |image0v2| so |image0v3| above can be inlined into that EXPECT

+    uint32_t second0v4 = second.frameAtIndex(0)->uniqueID();
+    EXPECT_EQ(second0v3, second0v4);

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

Nit: Compare to |second0v2| so |second0v3| above can be inlined into that EXPECT

+
+    BitmapImageTestEmptyImpl third;
+    third.loadImage("/LayoutTests/fast/images/resources/anim_none.gif", false);
+    third.frameAtIndex(0); // It should evict image0v4.
+    uint32_t image0v5 = frameAtIndex(0)->uniqueID();

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

Nit: Inline into next statement

+    EXPECT_NE(image0v4, image0v5);
+}
+
 TEST_F(BitmapImageDeferredDecodingTest, correctDecodedDataSize)
 {
     // When deferred decoding is enabled, requesting any one frame shouldn't
@@ -305,14 +381,7 @@ TEST_F(BitmapImageDeferredDecodingTest, correctDecodedDataSize)
     loadImage("/LayoutTests/fast/images/resources/anim_none.gif", false);
     frameAtIndex(1);
     int frameSize = static_cast<int>(m_image->size().area() * sizeof(ImageFrame::PixelData));
-    EXPECT_EQ(frameSize, m_imageObserver->m_lastDecodedSizeChangedDelta);
-    frameAtIndex(0);
-
-    // Trying to destroy all data except an undecoded frame should go ahead and
-    // destroy all other frames.
-    setCurrentFrame(2);
-    destroyDecodedData(false);
-    EXPECT_EQ(-frameSize * 2, m_imageObserver->m_lastDecodedSizeChangedDelta);
+    EXPECT_EQ(frameSize, lastDecodedSizeChange());
 }
 
 template <typename HistogramEnumType>