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) {
+            // Preserve the last frame as the starting state for this frame as
+            // the next frame would also use it.

[Peter Kasting, 2016/07/19 03:04:17]

Nit: How about:

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.

[aleksandar.stojiljkovic, 2016/07/19 11:10:40]

Done.

+            if (!buffer->copyBitmapData(*prevBuffer))
+                return setFailed();
+        } else {
+            // Use the required frame as the starting state for this frame.

[Peter Kasting, 2016/07/19 03:04:17]

Nit: How about:

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.

Also, if the above is true, why is it true (that we will always clear)? Is it
because of ImageFrameGenerator::decode() calling clearCacheExceptFrame()?  If so
why do we also need |m_purgeAggressively|? I'm confused about all this.

[aleksandar.stojiljkovic, 2016/07/19 11:10:40]

Done.

+            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