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

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

 /*
  * Copyright (C) 2006 Apple Computer, 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
  *    notice, this list of conditions and the following disclaimer in the
@@ skipping 16 matching lines @@
 
 #include "platform/image-decoders/gif/GIFImageReader.h"
 #include "wtf/NotFound.h"
 #include "wtf/PtrUtil.h"
 #include <limits>
 
 namespace blink {
 
 GIFImageDecoder::GIFImageDecoder(AlphaOption alphaOption, GammaAndColorProfileOption colorOptions, size_t maxDecodedBytes)
     : ImageDecoder(alphaOption, colorOptions, maxDecodedBytes)
+    , m_purgeAggressively(false)
     , m_repetitionCount(cAnimationLoopOnce)
 {
 }
 
 GIFImageDecoder::~GIFImageDecoder()
 {
 }
 
 void GIFImageDecoder::onSetData(SegmentReader* data)
 {
@@ skipping 247 matching lines @@
     buffer->setRequiredPreviousFrameIndex(findRequiredPreviousFrame(index, false));
 }
 
 void GIFImageDecoder::decode(size_t index)
 {
     parse(GIFFrameCountQuery);
 
     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 {
         framesToDecode.append(frameToDecode);
         frameToDecode = m_frameBufferCache[frameToDecode].requiredPreviousFrameIndex();
     } while (frameToDecode != kNotFound && m_frameBufferCache[frameToDecode].getStatus() != ImageFrame::FrameComplete);
 
     for (auto i = framesToDecode.rbegin(); i != framesToDecode.rend(); ++i) {
         if (!m_reader->decode(*i)) {
             setFailed();
             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;
     }
 
     // It is also a fatal error if all data is received and we have decoded all
     // frames available but the file is truncated.
     if (index >= m_frameBufferCache.size() - 1 && isAllDataReceived() && m_reader && !m_reader->parseCompleted())
         setFailed();
 }
@@ skipping 41 matching lines @@
 
     // Update our status to be partially complete.
     buffer->setStatus(ImageFrame::FramePartial);
 
     // Reset the alpha pixel tracker for this frame.
     m_currentBufferSawAlpha = false;
     return true;
 }
 
 } // namespace blink