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->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) {
             // 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);

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

I don't understand why this preexisting code is calling this against buffer
instead of prevBuffer.

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

Partly covered in the comment above. At this point decoding advances from
prevBuffer to buffer, prevBuffer's content is copyed/moved to |buffer| and prev
buffer disposal method specifies what to do in |buffer|.
The comment states "clear the previous frame to transparent" and "frame" has a
meaning of subrectangle/subframe of the full buffer. It can be rephrased as
"clear the previous buffer's frame in the new buffer". Don't know if I managed
to clarify it.

         }
     }
 
     // Update our status to be partially complete.
     buffer->setStatus(ImageFrame::FramePartial);
 
     // Reset the alpha pixel tracker for this frame.
     m_currentBufferSawAlpha = false;
     return true;
 }
@@ skipping 35 matching lines @@
         m_purgeAggressively = true;
         return;
     }
 
     const uint64_t totalMemoryUsage = frameMemoryUsage * index;
     if (totalMemoryUsage > m_maxDecodedBytes) {
         m_purgeAggressively = true;
     }
 }
 } // namespace blink