Save a bitmap copy when advancing to dependent GIF and WebP animation frames

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 338 matching lines @@
 {
     // Initialize the frame rect in our buffer.
     ImageFrame* const buffer = &m_frameBufferCache[frameIndex];
 
     size_t requiredPreviousFrameIndex = buffer->requiredPreviousFrameIndex();
     if (requiredPreviousFrameIndex == kNotFound) {
         // This frame doesn't rely on any previous data.
         if (!buffer->setSize(size().width(), size().height()))
             return setFailed();
     } else {
-        const ImageFrame* prevBuffer = &m_frameBufferCache[requiredPreviousFrameIndex];
+        ImageFrame* prevBuffer = &m_frameBufferCache[requiredPreviousFrameIndex];
         ASSERT(prevBuffer->getStatus() == ImageFrame::FrameComplete);
 
-        // Preserve the last frame as the starting state for this frame.
-        if (!buffer->copyBitmapData(*prevBuffer))
-            return setFailed();
+        if (buffer->getDisposalMethod() == ImageFrame::DisposeOverwritePrevious) {
+            // The next frame will also use |prevBuffer| as its starting state,
+            // so we can't take over its image data as we do below.  Copy the
+            // data instead.
+            if (!buffer->copyBitmapData(*prevBuffer))
+                return setFailed();
+        } else {
+            // This is the only frame to use |prevBuffer| as its starting state,
+            // and we'll always clear the old frame data after initializing this
+            // frame anyway, so we can save time by just moving its data over.

[Peter Kasting, 2016/07/20 01:00:28]

Nit: I still think you should explicitly say what ensures we always clear the
old frame data, so if that changes in the future it will be more clear.

It doesn't seem like this is true because of m_purgeAggressively, since that
itself is not always set.  So it seems like this must be due to
ImageFrameGenerator::decode() calling clearCacheExceptFrame(), or else your
statement that this always happens is in fact incorrect.

+            buffer->takeBitmapData(prevBuffer);
+        }
 
         if (prevBuffer->getDisposalMethod() == ImageFrame::DisposeOverwriteBgcolor) {
             // We want to clear the previous frame to transparent, without
             // affecting pixels in the image outside of the frame.
             const IntRect& prevRect = prevBuffer->originalFrameRect();
             ASSERT(!prevRect.contains(IntRect(IntPoint(), size())));
             buffer->zeroFillFrameRect(prevRect);
         }
     }
 
@@ skipping 42 matching lines @@
         m_purgeAggressively = true;
         return;
     }
 
     const uint64_t totalMemoryUsage = frameMemoryUsage * index;
     if (totalMemoryUsage > m_maxDecodedBytes) {
         m_purgeAggressively = true;
     }
 }
 } // namespace blink