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) {

[scroggo_chromium, 2016/06/28 15:55:28]

How do you feel about making this a private method? e.g.

void determineWhetherToPurgeAggressively() {
    if (m_purgeAggressively)
        return;

    const uint64_t frameArea = ...
    bool wouldOverflow = ...
    if (wouldOverflow) {
        m_purgeAggressively = true;
        return;
    }

    const uint64_t frameMemoryUsage = ...
    wouldOverflow = ...
    if (wouldOverflow) {
        m_purgeAggressively = true;
        return;
    }

    const uint64_t totalMemoryUsage = ...
    if (totalMemoryUsage > m_maxDecodedBytes)
        m_purgeAggressively = true;
}

That isolates the code around deciding whether to purge aggressively. It also
reduces your nested if/elses, and I think it makes the code a little easier to
follow.

(And maybe there's a shorter name that is clear about what it does...)

[cblume, 2016/06/28 16:53:33]

I like it. Done.

+        // 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 more memory than the image decoder is allowed
+        // (m_maxDecodedBytes).

[scroggo_chromium, 2016/06/28 15:55:28]

or overflow.

[cblume, 2016/06/28 16:53:33]

Done.

+        //
+        // As we decode we will learn the total number of frames, and thus total
+        // possible image memory used.
+
+        const uint64_t frameArea = decodedSize().area();
+        // We are about to multiply by 4, which may require an extra bit of storage
+        bool wouldOverflow = frameArea > (UINT64_C(1) << 62);
+        if (!wouldOverflow) {

[scroggo_chromium, 2016/06/28 15:55:28]

I think you want

    if (wouldOverflow)

? (No "!")

[cblume, 2016/06/28 16:53:33]

Done.

+            m_purgeAggressively = true;
+        } else {
+            const 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 > (UINT32_C(1) << 31))
+                || (index > (UINT32_C(1) << 31));
+            if (wouldOverflow) {
+                m_purgeAggressively = true;
+            } else {
+                const uint64_t totalMemoryUsage = frameMemoryUsage * index;
+                if (wouldOverflow || totalMemoryUsage > m_maxDecodedBytes) {

[scroggo_chromium, 2016/06/28 15:55:28]

No need to check wouldOverflow here. It is guaranteed to be false, since this is
the else statement to if (wouldOverflow)

[cblume, 2016/06/28 16:53:33]

Done.

+                    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)
+            clearCacheExceptFrame(*i);
+
         // 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