Cap memory usage in webp catch up

third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp

Index: third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp
diff --git a/third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp b/third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp
index 1e55c716838dfe0a1dfd4ad611bfa2dad053086c..a4a2fe3616a33224ef34e59bcb1067bdd7d26252 100644
--- a/third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp
+++ b/third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp
@@ -131,6 +131,7 @@ WEBPImageDecoder::WEBPImageDecoder(AlphaOption alphaOption, GammaAndColorProfile
     , m_haveAlreadyParsedThisData(false)
     , m_repetitionCount(cAnimationLoopOnce)
     , m_decodedHeight(0)
+    , m_purgeAggressively(false)
 {
     m_blendFunction = (alphaOption == AlphaPremultiplied) ? alphaBlendPremultiplied : alphaBlendNonPremultiplied;
 }
@@ -412,6 +413,8 @@ void WEBPImageDecoder::decode(size_t index)
     if (failed())
         return;
 
+    updateAggressivePurging(index);
+
     Vector<size_t> framesToDecode;
     size_t frameToDecode = index;
     do {
@@ -433,6 +436,9 @@ void WEBPImageDecoder::decode(size_t index)
         if (failed())
             return;
 
+        if (m_purgeAggressively)
+            clearCacheExceptFrame(*i);

[skal, 2016/09/24 17:15:53]

don't you want to reset m_purgeAggressively to false here, after clearing the
cache?

[cblume, 2016/09/25 05:30:05]

No, we want m_purgeAggressively to remain.

The idea is supposed to be:
1.) Assume there is enough space to cache everything.
2.) As we decode, we may learn caching all the image frames requires more space
than m_maxDecodedBytes.
3.) At that point, we *could* keep only some frames cached. But ideally, the
choice of which frames to cache would be done by the caller.
4.) So rather than arbitrarily cache some frames, only hold onto keyframes we
will need soon and purge everything else.

This sort of permanently puts it into a "fast-for-forward-animation" caching
mode while trimming down memory. It is perhaps the best we can do at this
granularity.

[scroggo_chromium, 2016/09/26 12:04:48]

This is probably worth a comment. I had the same question on the GIF version.

[cblume, 2016/10/01 11:06:23]

Good idea.
I added a comment to the header above m_purgeAggressively's definition.
I also added this to the base ImageDecoder as per other code review comments.
You can see it over at: https://codereview.chromium.org/2376033005/

+
         // We need more data to continue decoding.
         if (m_frameBufferCache[*i].getStatus() != ImageFrame::FrameComplete)
             break;
@@ -505,4 +511,47 @@ bool WEBPImageDecoder::decodeSingleFrame(const uint8_t* dataBytes, size_t dataSi
     }
 }
 
+void WEBPImageDecoder::updateAggressivePurging(size_t index)
+{
+    if (m_purgeAggressively)
+        return;
+
+    // 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) or would overflow 32 bits..
+    //
+    // 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) {
+        m_purgeAggressively = true;
+            return;

[skal, 2016/09/24 17:14:27]

indent is off

[cblume, 2016/09/25 05:30:05]

Done.

+    }
+
+    const uint64_t frameMemoryUsage = frameArea * 4; // 4 bytes per pixel

[skal, 2016/09/24 17:14:27]

you could simplify the test to just check that the result is representable:

if (frameMemoryUsage / 4 != frameArea) {   // overflow occurred
       m_purgeAggressively = true;
       return;
}

[cblume, 2016/09/25 05:30:05]

Oh, I see what you mean.
First initialize it: frameMemoryUsage = frameArea * 4;
Then check that it didn't overflow with: if (frameMemoryUsage / 4 != frameArea)
{

Yeah, that is a much better idea. I like that a lot. Thanks!

I'll go add this to the GIF decoder as well.

+    // 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;
+        return;
+    }
+
+    const uint64_t totalMemoryUsage = frameMemoryUsage * index;

[skal, 2016/09/24 17:14:27]

and here too:

if (totalMemoryUsage / index != frameMemoryUsage) {
  m_purgeAggressively = true;
  return;
}

[cblume, 2016/09/25 05:30:05]

This one is different.
This one is checking against m_maxDecodedBytes, which is the memory limit the
decoder shouldn't have been surpassing.
This is where we find if caching all the frames would be too large.

+    if (totalMemoryUsage > m_maxDecodedBytes) {
+        m_purgeAggressively = true;
+    }
+}
 } // namespace blink