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

third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp

Index: third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp
diff --git a/third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp b/third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp
index a769b6d09bb28180c851a3f3e7128ba6077dbcf1..ba4cca2bb13c8da2bcc7cbc66cdad6fd869999b3 100644
--- a/third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp
+++ b/third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp
@@ -356,11 +356,14 @@ bool GIFImageDecoder::initFrameBuffer(size_t frameIndex)
         if (!buffer->setSizeAndColorProfile(size().width(), size().height(), ImageFrame::ICCProfile()))
             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))
+        // We try to reuse |prevBuffer| as starting state to avoid copying.
+        // For DisposeOverwritePrevious, the next frame will also use
+        // |prevBuffer| as its starting state, so we can't take over its image
+        // data using takeBitmapDataIfWritable.  Copy the data instead.
+        if ((buffer->getDisposalMethod() == ImageFrame::DisposeOverwritePrevious || !buffer->takeBitmapDataIfWritable(prevBuffer)) && !buffer->copyBitmapData(*prevBuffer))

[Ken Russell (switch to Gerrit), 2016/09/29 00:40:04]

This is a lot of logic for a single if-statement. It's also not clear to me that
if buffer->takeBitmapDataIfWritable() succeeds, that we should run the logic
below, since presumably prevBuffer is already emptied out. Same comment in the
WEBP image decoder.

[aleksandar.stojiljkovic, 2016/09/29 08:03:53]

It is a nit suggested by pkasting@ and it takes less space this way than with
original:

https://codereview.chromium.org/2155973002/diff2/20001:200001/third_party/Web...
Regardless of the way the new frame got the data from previous frame (via take
or via copy), if disposal method is DisposeOverwriteBgcolor, new frame |buffer|
decoding starts from zeroing (making it fully transparent) previous frame
subframe. It keeps the pixels outside subframe it got from previous frame (which
again previous frame potentially gets from a long dependency chain) but it needs
to erase only the subframe before decoding and writing new values to |buffer|.

             return setFailed();
 
         if (prevBuffer->getDisposalMethod() == ImageFrame::DisposeOverwriteBgcolor) {