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

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

Index: third_party/WebKit/Source/platform/graphics/BitmapImage.h
diff --git a/third_party/WebKit/Source/platform/graphics/BitmapImage.h b/third_party/WebKit/Source/platform/graphics/BitmapImage.h
index 2a650a893f4364f6de016512b4c67a9031db798d..7bf2ee2afa3e7975f8e0bee048d94ee22eac7425 100644
--- a/third_party/WebKit/Source/platform/graphics/BitmapImage.h
+++ b/third_party/WebKit/Source/platform/graphics/BitmapImage.h
@@ -44,8 +44,9 @@ namespace blink {
 template <typename T> class Timer;
 
 class PLATFORM_EXPORT BitmapImage final : public Image {
-    friend class GeneratedImage;
+    friend class BitmapImageTest;
     friend class CrossfadeGeneratedImage;
+    friend class GeneratedImage;
     friend class GradientGeneratedImage;
     friend class GraphicsContext;
 public:
@@ -73,7 +74,7 @@ public:
     // This because we start and stop animating lazily. Animation starts when
     // the image is rendered, and automatically pauses once all observers no
     // longer want to render the image.
-    void stopAnimation() override;
+    void stopAnimation();

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

It's confusing that we mix overrides and non-overrides together.  Can we move
them so all the overrides are above or below all the non-overrides?

Hopefully this will make it easier to tell in the future if any of these can be
removed as well (e.g. do the three non-overrides above this all need to exist
still?).

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

Done.
They are all still used (for testing).

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

Still need to move sizeRespectingOrientation(), currentFrameOrientation(), and
createWithOrientationForTesting() out of the overrides.

In the private section, draw() and startAnimation() are overrides mixed in with
everything else.

Also, you should move the implementation in the .cpp file around at the same
time you move the .h file contents, to keep them in the same order.

Feel free to do all this mechnical reordering as a separate patch to avoid
confusion.

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

Thanks, makes sense. I would like to do this together with planned refactoring
RefPtr -> sk_sp.

     void resetAnimation() override;
     bool maybeAnimated() override;
 
@@ -88,6 +89,11 @@ public:
     bool currentFrameIsComplete() override;
     bool currentFrameIsLazyDecoded() override;
 
+    // Called to wipe out the entire frame buffer cache and tell the image
+    // source to destroy everything; this is used when e.g. we want to free
+    // some room in the image cache.

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

Seems like this comment should be on the base class declaration of this function
and not here.

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

Descriptio applies to this class functionality (framebuffer cache) - another
subclass (SVGImage) has it's own.

+    void destroyDecodedData() override;
+
     ImageOrientation currentFrameOrientation();
 
     // Construct a BitmapImage with the given orientation.
@@ -96,11 +102,6 @@ public:
     void advanceAnimationForTesting() override { internalAdvanceAnimation(false); }
 
 private:
-    friend class BitmapImageTest;
-
-    void updateSize() const;
-
-private:
     enum RepetitionCountStatus {
       Unknown,    // We haven't checked the source's repetition count.
       Uncertain,  // We have a repetition count, but it might be wrong (some GIFs have a count after the image data, and will report "loop once" until all data has been decoded).
@@ -110,8 +111,11 @@ private:
     BitmapImage(const SkBitmap &, ImageObserver* = 0);
     BitmapImage(ImageObserver* = 0);
 
+    static void setAnimationCacheSizeForTesting(size_t maxCacheSize, size_t maxAnimationSizeInCache);

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

Please add descriptive comments on new functions you add.  (A lot of Blink
inherits WebKit's policy of not documenting functions, but let's not extend that
to any new code.)

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

Done.
This one is removed.

+
     void draw(SkCanvas*, const SkPaint&, const FloatRect& dstRect, const FloatRect& srcRect, RespectImageOrientationEnum, ImageClampingMode) override;
 
+    void updateSize() const;
     size_t currentFrame() const { return m_currentFrame; }
     size_t frameCount();
 
@@ -123,27 +127,12 @@ private:
     ImageOrientation frameOrientationAtIndex(size_t);
 
     // Decodes and caches a frame. Never accessed except internally.

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

While here: Why is "Never accessed except internally" needed when this is in the
private section?

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

Done. Removed.

-    void cacheFrame(size_t index);
-
-    // Called before accessing m_frames[index]. Returns false on index out of bounds.
-    bool ensureFrameIsCached(size_t index);
+    PassRefPtr<SkImage> cacheFrame(size_t index);
 
     // Returns the total number of bytes allocated for all framebuffers, i.e.
     // the sum of m_source.frameBytesAtIndex(...) for all frames.
     size_t totalFrameBytes();
 
-    // Called to invalidate cached data. When |destroyAll| is true, we wipe out
-    // the entire frame buffer cache and tell the image source to destroy
-    // everything; this is used when e.g. we want to free some room in the image
-    // cache. If |destroyAll| is false, we delete frames except the current
-    // frame; this is used while animating large images to keep memory footprint
-    // low; the decoder should preserve the current frame and may preserve some
-    // other frames to avoid redecoding the whole image on every frame.
-    void destroyDecodedData(bool destroyAll) override;
-
-    // If the image is large enough, calls destroyDecodedData().
-    void destroyDecodedDataIfNecessary();
-
     // Notifies observers that the memory footprint has changed.
     void notifyMemoryChanged();
 
@@ -170,6 +159,9 @@ private:
     size_t m_currentFrame; // The index of the current frame of animation.
     Vector<FrameData, 1> m_frames; // An array of the cached frames of the animation. We have to ref frames to pin them in the cache.
 
+    RefPtr<SkImage> m_frame; // One frame is cached (the most recently accessed) also for animations.

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

"A cached copy of the most recently-accessed frame"?  I don't know what "also
for animations" means.

I would name this and the next member like m_cachedFrame or something.

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

Done.

+    size_t m_frameIndex; // Index of the frame that is cached.
+
     OwnPtr<Timer<BitmapImage>> m_frameTimer;
     int m_repetitionCount; // How many total animation loops we should do.  This will be cAnimationNone if this image type is incapable of animation.
     RepetitionCountStatus m_repetitionCountStatus;