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

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

 /*
  * Copyright (C) 2006 Samuel Weinig (sam.weinig@gmail.com)
  * Copyright (C) 2004, 2005, 2006 Apple Computer, Inc.  All rights reserved.
  * Copyright (C) 2008-2009 Torch Mobile, Inc.
  *
  * 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.
@@ skipping 26 matching lines @@
 #include "platform/graphics/ImageSource.h"
 #include "platform/image-decoders/ImageAnimation.h"
 #include "wtf/Forward.h"
 #include "wtf/OwnPtr.h"
 
 namespace blink {
 
 template <typename T> class Timer;
 
 class PLATFORM_EXPORT BitmapImage final : public Image {
+    friend class BitmapImageTest;
+    friend class CrossfadeGeneratedImage;
     friend class GeneratedImage;
-    friend class CrossfadeGeneratedImage;
     friend class GradientGeneratedImage;
     friend class GraphicsContext;
 public:
     static PassRefPtr<BitmapImage> create(ImageObserver* observer = 0)
     {
         return adoptRef(new BitmapImage(observer));
     }
 
     ~BitmapImage() override;
 
     bool isBitmapImage() const override { return true; }
 
     bool currentFrameHasSingleSecurityOrigin() const override;
 
     IntSize size() const override;
     IntSize sizeRespectingOrientation() const;
     bool getHotSpot(IntPoint&) const override;
     String filenameExtension() const override;
     bool dataChanged(bool allDataReceived) override;
 
     bool isAllDataReceived() const { return m_allDataReceived; }
     bool hasColorProfile() const;
 
     // It may look unusual that there's no start animation call as public API.
     // 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;
 
     void setAnimationPolicy(ImageAnimationPolicy policy) override { m_animationPolicy = policy; }
     ImageAnimationPolicy animationPolicy() override { return m_animationPolicy; }
     void advanceTime(double deltaTimeInSeconds) override;
 
     PassRefPtr<SkImage> imageForCurrentFrame() override;
     PassRefPtr<Image> imageForDefaultFrame() override;
 
     bool currentFrameKnownToBeOpaque(MetadataMode = UseCurrentMetadata) override;
     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.
     static PassRefPtr<BitmapImage> createWithOrientationForTesting(const SkBitmap&, ImageOrientation);
     // Advance the image animation by one frame.
     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).
       Certain     // The repetition count is known to be correct.
     };
 
     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();
 
     PassRefPtr<SkImage> frameAtIndex(size_t);
 
     bool frameIsCompleteAtIndex(size_t);
     float frameDurationAtIndex(size_t);
     bool frameHasAlphaAtIndex(size_t);
     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();
 
     // Whether or not size is available yet.
     bool isSizeAvailable();
 
     // Animation.
     int repetitionCount(bool imageKnownToBeComplete);  // |imageKnownToBeComplete| should be set if the caller knows the entire image has been decoded.
     bool shouldAnimate();
     void startAnimation(CatchUpAnimation = CatchUp) override;
     void advanceAnimation(Timer<BitmapImage>*);
 
     // Function that does the real work of advancing the animation.  When
     // skippingFrames is true, we're in the middle of a loop trying to skip over
     // a bunch of animation frames, so we should not do things like decode each
     // one or notify our observers.
     // Returns whether the animation was advanced.
     bool internalAdvanceAnimation(bool skippingFrames);
 
     ImageSource m_source;
     mutable IntSize m_size; // The size to use for the overall image (will just be the size of the first image).
     mutable IntSize m_sizeRespectingOrientation;
 
     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;
     int m_repetitionsComplete;  // How many repetitions we've finished.
     double m_desiredFrameStartTime;  // The system time at which we hope to see the next call to startAnimation().
 
     size_t m_frameCount;
 
     ImageAnimationPolicy m_animationPolicy; // Whether or not we can play animation.
 
     bool m_animationFinished : 1; // Whether or not we've completed the entire animation.
 
     bool m_allDataReceived : 1; // Whether or not we've received all our data.
     mutable bool m_haveSize : 1; // Whether or not our |m_size| member variable has the final overall image size yet.
     bool m_sizeAvailable : 1; // Whether or not we can obtain the size of the first image frame yet from ImageIO.
     mutable bool m_hasUniformFrameSize : 1;
     mutable bool m_haveFrameCount : 1;
 };
 
 DEFINE_IMAGE_TYPE_CASTS(BitmapImage);
 
 } // namespace blink
 
 #endif