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..35e7e9fc4d39a71493b7f5fef679dbc855dd796f 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);

[urvang, 2016/10/03 21:07:23]

You only need to call this method when number of frames (seen so far) changes,
right?
Then, you can do this only at the end of UpdateDemuxer() at line#240.

[cblume, 2016/10/10 21:18:38]

Sounds good.

It looks like updateDemuxer() is called from frameCount(), which is called from
a handful of locations. And updateDemuxer() exits early after it is called for
each setData(). So presumably then updateDemuxer() is run once per incoming data
set (as data arrives). Is my understanding correct?

[urvang, 2016/10/10 21:30:50]

Yes, updateDemuxer() updates the demuxer and related metadata (like frame count)
whenever we have received some new data (since the last update).

+
     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);

[urvang, 2016/09/26 18:37:05]

I think you need to purge after line#444.
Otherwise, the decoding of 'i'th frame may NOT have completed yet, which means
that the frame at (i-1) may still be needed (to decode rest of the pixels of ith
frame).

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

Done.
I should do this on the GIF decoder as well.
I updated the GIFImageDecoder version over at https://crrev.com/2382143004

[urvang, 2016/10/03 19:12:36]

I don't see this change yet. Forgot to upload?

[cblume, 2016/10/10 21:18:38]

Oh, yeah. Sorry.

+
         // We need more data to continue decoding.
         if (m_frameBufferCache[*i].getStatus() != ImageFrame::FrameComplete)
             break;
@@ -505,4 +511,33 @@ bool WEBPImageDecoder::decodeSingleFrame(const uint8_t* dataBytes, size_t dataSi
     }
 }
 
+void WEBPImageDecoder::updateAggressivePurging(size_t index)

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

This looks to be the exact same method as in GIFImageDecoder (after you update
it in crrev.com/2371563002). Once APNG is in, we may want the same thing there.
I think this method belongs in ImageDecoder.

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

Good call.
I've moved this into ImageDecoder in https://crrev.com/2376033005

+{
+    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();
+    const uint64_t frameMemoryUsage = frameArea * 4; // 4 bytes per pixel
+    if (frameMemoryUsage / 4 != frameArea) { // overflow occurred
+        m_purgeAggressively = true;
+        return;
+    }
+
+    const uint64_t totalMemoryUsage = frameMemoryUsage * index;

[skal, 2016/09/26 12:22:51]

this multiply is still even more likely to overflow that then one at 532.

You should also check that the presentation if the result is exact
(before comparing to m_maxDecodedBytes), with something like:
if (totalMemoryUsage / frameMemoryUsage != index) { // overflow occurred
...
}


(sorry i misplaced the line of my previous similar comment)

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

Ah, got ya. Fixed.
I've moved this into ImageDecoder in https://crrev.com/2376033005

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