Don't cache GIF frames if it would be too large.

third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp

Index: third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp
diff --git a/third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp b/third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp
index 68e9334457a0978512762e0a267028649ede44d6..cbcad72ee0e124750f26edc9a90cd3c82d72748c 100644
--- a/third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp
+++ b/third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp
@@ -34,6 +34,7 @@ namespace blink {
 
 GIFImageDecoder::GIFImageDecoder(AlphaOption alphaOption, GammaAndColorProfileOption colorOptions, size_t maxDecodedBytes)
     : ImageDecoder(alphaOption, colorOptions, maxDecodedBytes)
+    , m_purgeAggressively(false)
     , m_repetitionCount(cAnimationLoopOnce)
 {
 }
@@ -301,6 +302,38 @@ void GIFImageDecoder::decode(size_t index)
     if (failed())
         return;
 
+    if (!m_purgeAggressively) {
+        // We don't want to cache so much that we cause a memory issue.
+        // If we used a LRU cache we would fill it and then on next animation loop
+        // we would need to decode all the frames again -- the LRU would give no
+        // benefit and would consume more memory.
+        // So instead, simply purge unused frames if caching all of the frames of
+        // the image would use too much memory.

[scroggo_chromium, 2016/06/27 19:04:32]

Maybe specify what you mean by "too much memory"?

[cblume, 2016/06/27 23:11:17]

Done.

+        // As we decode we will learn the total number of frames, and thus total
+        // possible image memoroy used.

[scroggo_chromium, 2016/06/27 19:04:32]

memory*

[cblume, 2016/06/27 23:11:17]

Done.

+
+        uint64_t frameArea = this->decodedSize().area();

[scroggo_chromium, 2016/06/27 19:04:32]

nit: I don't think Blink uses this-> to call class methods.

nit: could be const.

[cblume, 2016/06/27 23:11:17]

Done.

+        // We are about to multiply by 4, which may require an extra bit of storage
+        bool wouldOverflow = frameMemoryUsage > (1 << 62);

[scroggo_chromium, 2016/06/27 19:04:32]

It looks like "frameMemoryUsage" has not yet been declared? Did you mean
frameArea?

[cblume, 2016/06/27 23:11:17]

Yeah, this was a mistake which I fixed in Patch Set 3.

+        if (!wouldOverflow) {

[scroggo_chromium, 2016/06/27 19:04:32]

Just to make sure I understand - it looks like if we would have overflowed, we
do *not* purge aggressively (here and below)? I know I said something like
"overflow is probably not the right limit", but it looks like we end up with
something like:

[0, LIMIT]: OK
[LIMIT, overflow): Too much memory
[overflow, infinity): OK

I was thinking more like one of the following:

A) LIMIT < overflow:
[0, LIMIT]: OK
[LIMIT, overflow]: Too much memory
[overflow, infinity]: Too much memory, but we have to calculate differently

B) LIMIT > overflow:
[0, overflow): OK
[overflow, LIMIT]: OK, but we have to calculate differently
(LIMIT, infinity): Too much memory

[cblume, 2016/06/27 23:11:17]

Oh whoops.
Yeah, I definitely only wanted to allow within-memory. 

+            uint64_t frameMemoryUsage = frameArea * 4; // 4 bytes per pixel
+            // We are about to multiply by a size_t, which does not have a fixed
+            // size.
+            // To simplify things, let's make sure our per-frame memory usage and
+            // index can be stored in 32 bits and store the multiplicand in a 64-bit
+            // number.
+            wouldOverflow = frameMemoryUsage > (1 << 31);
+            wouldOverflow |= index > (1 << 31);

[scroggo_chromium, 2016/06/27 19:04:32]

Why not combine these into one statement?

    wouldOverflow = (frameMemoryUsage > (1 << 32)) || (index > (1 << 31));

(Arguably, you could drop the variable wouldOverflow, which is typically used in
a conditional and then modified later before it's used again. The variable name
"wouldOverflow" might make it a little clearer what you're doing, though.)

[cblume, 2016/06/27 23:11:17]

I like combining.
I think I prefer keeping the variable name. I think it does make the code more
clear.

+            if (!wouldOverflow) {
+                uint64_t totalMemoryUsage = static_cast<uint32_t>(frameMemoryUsage)

[scroggo_chromium, 2016/06/27 19:04:32]

Why do you need to cast this?

[cblume, 2016/06/27 23:11:18]

Well, we could actually do uint128_t = uint64_t * uint64_t. This would be more
simple and would allow for crazy huge memory usage which might be possible.

But there is no uint128_t.

Given that, since we just confirmed above that these do not have values over
32-bits, we could just multiply them (uint64_t = uint64_t * uint64_t). The casts
aren't necessary. I did this to try to make it more clear that I'm enforcing
overflow protection. But maybe I'm too verbose. :) It is already pretty clear.
I'll take them out.

+                    * static_cast<uint32_t>(index);
+                if (wouldOverflow || totalMemoryUsage < m_maxDecodedBytes) {

[scroggo_chromium, 2016/06/27 19:04:32]

wouldOverflow can never be true here, right?

Also, do you really want 

    totalMemoryUsage > m_maxDecodedBytes

?

[cblume, 2016/06/27 23:11:17]

Oops, I definitely meant > (which means this should be enter-able.

+                    m_purgeAggressively = true;
+                }
+            }
+        }
+    }
+
     Vector<size_t> framesToDecode;
     size_t frameToDecode = index;
     do {
@@ -314,6 +347,9 @@ void GIFImageDecoder::decode(size_t index)
             return;
         }
 
+        if (m_purgeAggressively)
+            this->clearCacheExceptFrame(*i);

[scroggo_chromium, 2016/06/27 19:04:32]

nit: I think "this->" is unnecessary here?

[cblume, 2016/06/27 23:11:18]

Done.

+
         // We need more data to continue decoding.
         if (m_frameBufferCache[*i].getStatus() != ImageFrame::FrameComplete)
             break;