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

third_party/WebKit/Source/platform/image-decoders/ImageFrame.h

 /*
  * Copyright (C) 2006 Apple Computer, Inc.  All rights reserved.
  * Copyright (C) Research In Motion Limited 2009-2010. 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
@@ skipping 79 matching lines @@
     // These do not touch other metadata, only the raw pixel data.
     void clearPixelData();
     void zeroFillPixelData();
     void zeroFillFrameRect(const IntRect&);
 
     // Makes this frame have an independent copy of the provided image's
     // pixel data, so that modifications in one frame are not reflected in
     // the other.  Returns whether the copy succeeded.
     bool copyBitmapData(const ImageFrame&);
 
+    // Moves the bitmap data from the provided frame to this one, leaving the
+    // provided frame empty.  Operation is successful only if bitmap data is not
+    // marked as done (immutable).  Returns whether the move succeeded.
+    bool takeBitmapDataIfWritable(ImageFrame*);
+
     // Copies the pixel data at [(startX, startY), (endX, startY)) to the
     // same X-coordinates on each subsequent row up to but not including
     // endY.
     void copyRowNTimes(int startX, int endX, int startY, int endY)
     {
         ASSERT(startX < width());
         ASSERT(endX <= width());
         ASSERT(startY < height());
         ASSERT(endY <= height());
         const int rowBytes = (endX - startX) * sizeof(PixelData);
         const PixelData* const startAddr = getAddr(startX, startY);
         for (int destY = startY + 1; destY < endY; ++destY)
             memcpy(getAddr(startX, destY), startAddr, rowBytes);
     }
 
     // Allocates space for the pixel data.  Must be called before any pixels
     // are written.  Must only be called once.  Returns whether allocation
     // succeeded.
     bool setSizeAndColorProfile(int newWidth, int newHeight, const ICCProfile& newIccProfile);
 
     bool hasAlpha() const;
     const IntRect& originalFrameRect() const { return m_originalFrameRect; }
     Status getStatus() const { return m_status; }
     unsigned duration() const { return m_duration; }
     DisposalMethod getDisposalMethod() const { return m_disposalMethod; }
     AlphaBlendSource getAlphaBlendSource() const { return m_alphaBlendSource; }
     bool premultiplyAlpha() const { return m_premultiplyAlpha; }
     SkBitmap::Allocator* allocator() const { return m_allocator; }
-    const SkBitmap& bitmap() const { return m_bitmap; }
+
+    // Returns the bitmap that is the output of decoding.
+    // If the decoding is complete, sets the bitmap status to immutable if
+    // |setImmutableIfDone| is true.
+    // When |setImmutableIfDone| is false, bitmap is not sealed before returned
+    // and that enables optimization - saving a bitmap copy per frame when
+    // decoding animations. See also takeBitmapDataIfWritable.

[Peter Kasting, 2016/08/26 19:04:04]

As a reader, I'm wondering why we would ever want to mark the bitmap immutable. 
Why not just always leave it mutable?  Perhaps this comment should motivate why
we want to make it immutable when possible.

[aleksandar.stojiljkovic, 2016/08/26 21:53:51]

It is convenient that bitmap carry information about "is decoding done" through
the layers of API, to help on optimizing caching and render quality.
Originally added as replacement (patch [1]) of "is Complete" API now we are
taking it a step back for one of the clients using isComplete through
ImageFrame::getStatus().
Related patch (from 2012)
[1]
https://chromium.googlesource.com/chromium/src/+/5943ea57ef97970aa46651e033c9...

Also, as SkImage is used around the blink, and SkImage is immutable snapshot,
creating SkImage from SkBitmap when SkBitmap isn't immutable creates a copy:

https://cs.chromium.org/chromium/src/third_party/skia/src/image/SkImage_Raste...

[Peter Kasting, 2016/08/27 04:19:58]

What does "taking it a step back for one of the clients using isComplete through
ImageFrame::getStatus()" mean?  I don't understand that sentence.

[aleksandar.stojiljkovic, 2016/08/28 08:58:31]

That sentence could be ignored - it just means that both getStatus() ==
FrameComplete and isImmutable are used for the same thing and we can use only
getStatus() == FrameComplete.

+    const SkBitmap& bitmap(bool setImmutableIfDone = true);

[Peter Kasting, 2016/08/26 19:04:04]

Nit: This is named like a simple accessor, but it can also have a side effect. 
This seems confusing.

I feel like it would be better to separate these into an accessor and a function
that makes the bitmap immutable, and have the latter called from the relevant
places we want that to happen.  Then it's clear who really needs this and why.

[aleksandar.stojiljkovic, 2016/08/26 21:53:51]

It would be clear if we have additional accessor that returns SkImage. It gets
called in places where immutable SkBitmap is expected and conversion to SkImage
follows. SkImage is expected to be immutable snapshot so we choose to set
ImageFrame::skbitmap to const internally before creating SkImage to avoid copy
within SkImage::MakeFromBitmap.

So, something like:
const SkBitmap& bitmap() const // doesnt set immutable 
const SkImage& image() // sets to immutable before creating SkImage from
SkBitmap.

I'll add it in next patch.

[Peter Kasting, 2016/08/27 04:19:58]

That second function still looks like a cheap accessor but actually isn't.

At the very least, I'd name the second function something like
"finalizePixelsAndGetImage()" and check in its implementation that the frame
status is already complete.

But I still think it might be clearer to just do something like:

  const SkBitmap& bitmap() const;
  void SetImmutable();

...and have callers that want both things to happen call both.

[aleksandar.stojiljkovic, 2016/08/28 08:58:31]

Done.
bitmap() is back to original "cheap accessor" constness.
Added finalizePixelsAndGetImage.
Didn't add setImmutable since it can be done simply by calling:
SkBitmap bitmap = frame->getBitmap();
bitmap.setImmutable();

+
     // Returns true if the pixels changed, but the bitmap has not yet been notified.
     bool pixelsChanged() const { return m_pixelsChanged; }
     size_t requiredPreviousFrameIndex() const { return m_requiredPreviousFrameIndex; }
     void setHasAlpha(bool alpha);
     void setOriginalFrameRect(const IntRect& r) { m_originalFrameRect = r; }
     void setStatus(Status);
     void setDuration(unsigned duration) { m_duration = duration; }
     void setDisposalMethod(DisposalMethod disposalMethod) { m_disposalMethod = disposalMethod; }
     void setAlphaBlendSource(AlphaBlendSource alphaBlendSource) { m_alphaBlendSource = alphaBlendSource; }
     void setPremultiplyAlpha(bool premultiplyAlpha) { m_premultiplyAlpha = premultiplyAlpha; }
@@ skipping 78 matching lines @@
     // The frame that must be decoded before this frame can be decoded.
     // WTF::kNotFound if this frame doesn't require any previous frame.
     // This is used by ImageDecoder::clearCacheExceptFrame(), and will never
     // be read for image formats that do not have multiple frames.
     size_t m_requiredPreviousFrameIndex;
 };
 
 } // namespace blink
 
 #endif