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

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

Index: third_party/WebKit/Source/platform/image-decoders/ImageFrame.h
diff --git a/third_party/WebKit/Source/platform/image-decoders/ImageFrame.h b/third_party/WebKit/Source/platform/image-decoders/ImageFrame.h
index 3541f7dcc1eef4f979dc05d574220a55a52d69d7..247cd8eb2365aa8755ba8eb992c90ee341832c50 100644
--- a/third_party/WebKit/Source/platform/image-decoders/ImageFrame.h
+++ b/third_party/WebKit/Source/platform/image-decoders/ImageFrame.h
@@ -97,6 +97,11 @@ public:
     // 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.
@@ -125,7 +130,15 @@ public:
     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; }