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..ce175145fce8ff747ec0f05affc813bb7873b702 100644
--- a/third_party/WebKit/Source/platform/image-decoders/ImageFrame.h
+++ b/third_party/WebKit/Source/platform/image-decoders/ImageFrame.h
@@ -32,6 +32,7 @@
 #include "public/platform/WebVector.h"
 #include "third_party/skia/include/core/SkBitmap.h"
 #include "third_party/skia/include/core/SkColorPriv.h"
+#include "third_party/skia/include/core/SkImage.h"
 #include "wtf/Allocator.h"
 #include "wtf/Assertions.h"
 #include "wtf/PassRefPtr.h"
@@ -97,6 +98,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.

[Peter Kasting, 2016/08/30 06:59:40]

Nit: Maybe better second sentence: "This fails if the provided frame's bitmap is
marked immutable, since moving its data out from under it destructively modifies
it."

[aleksandar.stojiljkovic, 2016/09/21 20:56:57]

The issue here is not in destructive modification (we don't need that frame
data) but if it is done we cannot write to it when decoding the next frame.

+    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 +131,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.
+    const SkBitmap& bitmap() const;
+
+    // Create SkImage from bitmap() and return it.  If frame is complete, the
+    // bitmap is set immutable before creating SkImage to avoid copying bitmap
+    // in SkImage::MakeFromBitmap(m_bitmap).
+    PassRefPtr<SkImage> finalizePixelsAndGetImage();

[Peter Kasting, 2016/08/30 06:59:40]

Given that this doesn't always finalize the pixels (if decoding isn't complete),
maybe combining these two pieces of functionality isn't a good idea.  It's
convenient, but it's less clear than I thought it would be since I had thought
we were only calling this when the frame was complete, and would thus finalize
unconditionally.

It looks like all but one caller knows the frame is complete already.  So we
could make these do something like:

    frame->finalizePixels();  // or ->markBitmapImmutable(), if you want to be
more literal
    frame->image();

And that one put the finalizePixels() call under a conditional.  Then it's clear
when callers are finalizing vs. not.

finalizePixels() could DCHECK that the frame is complete, so people don't
accidentally call it for incomplete frames, which would be bad.  It would be
useful for the places that are currently doing this manually (by getting the
bitmap and calling setImmutable() on it) as well, assuming we want to keep
those, because it's a bit briefer and clearer than that code (and it has the
DCHECK() guard).  Finally we could put a comment on the declaration of
finalizePixels() that this results in saving a copy when image() is used, but
disables passing this frame to takeBitmapDataIfWriteable(), so callers need to
decide carefully what will result in best performance.  (It would be nice to
know, in practice, what sorts of sequences of calls can result in these
optimization cases overlapping.)

Then we could put a comment on image() that calling it when things are not
finalized incurs a copy.  If you wanted, this could even be split into two
functions, e.g. finalizedImage() and nonFinalizedImage() (or image() and
copyImage(), or something), each of which would simply DCHECK whether or not the
bitmap was immutable.  This would force callers to be explicit about which one
they thought they were doing, so as to avoid unintentional copies.

[aleksandar.stojiljkovic, 2016/09/21 20:56:57]

I like finalizePixelsAndGetImage as it explains what it is doing (and as you
said, the only problem is the condition inside). Handled that one caller
separately, updated the comment here and added DCHECK as you suggested.

+
     // 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; }