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

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

Index: third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp
diff --git a/third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp b/third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp
index 6a4db8fa2d711b56af64eac34f940d4fd870781b..e179986e802dcc0903a7b339ad864b7fcb7e82cf 100644
--- a/third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp
+++ b/third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp
@@ -27,6 +27,7 @@
 #include "platform/image-decoders/ImageFrame.h"
 
 #include "platform/RuntimeEnabledFeatures.h"
+#include "platform/graphics/skia/SkiaUtils.h"
 #include "platform/image-decoders/ImageDecoder.h"
 
 namespace blink {
@@ -88,14 +89,25 @@ void ImageFrame::zeroFillPixelData()
 
 bool ImageFrame::copyBitmapData(const ImageFrame& other)
 {
-    if (this == &other)
-        return true;
-
+    DCHECK_NE(this, &other);
     m_hasAlpha = other.m_hasAlpha;
     m_bitmap.reset();
     return other.m_bitmap.copyTo(&m_bitmap, other.m_bitmap.colorType());
 }
 
+bool ImageFrame::takeBitmapDataIfWritable(ImageFrame* other)
+{
+    DCHECK(other);
+    DCHECK_NE(this, other);
+    if (other->m_bitmap.isImmutable())
+        return false;
+    m_hasAlpha = other->m_hasAlpha;
+    m_bitmap.reset();
+    m_bitmap.swap(other->m_bitmap);
+    other->m_status = FrameEmpty;
+    return true;
+}
+
 bool ImageFrame::setSizeAndColorProfile(int newWidth, int newHeight, const ICCProfile& newIccProfile)
 {
     // setSizeAndColorProfile() should only be called once, it leaks memory otherwise.
@@ -119,6 +131,18 @@ bool ImageFrame::hasAlpha() const
     return m_hasAlpha;
 }
 
+sk_sp<SkImage> ImageFrame::finalizePixelsAndGetImage()
+{
+    DCHECK(m_status == FrameComplete);

[Peter Kasting, 2016/09/22 21:44:08]

Nit: DCHECK_EQ(FrameComplete, m_status)

[aleksandar.stojiljkovic, 2016/09/27 18:08:28]

Done.

+
+    // We don't set the status to immutable until there is a request to get
+    // SkImage and decoding is done.  We set the bitmap to immutable here to
+    // avoid copying in SkImage::MakeFromBitmap.

[Peter Kasting, 2016/09/22 21:44:08]

Nit: Second sentence is already in the header and can be eliminated.  Not really
sure what the first sentence is saying.  Are you justifying why setStatus() does
not mark the bitmap as immutable?  If so, move this comment there and say
something like this instead:

An immutable bitmap would mean callers could construct an SkImage from this
without a copy.  However, setting the bitmap immutable here would defeat
takeBitmapDataIfWritable().  Instead we let the bitmap stay mutable until
someone calls finalizePixelsAndGetImage() to actually get the SkImage.

[aleksandar.stojiljkovic, 2016/09/27 18:08:28]

Done. Rephrased it, removed the note about copy as it is in the header and
joined it with the comment in setStatus.

+    if (!m_bitmap.isImmutable())

[Peter Kasting, 2016/09/22 21:44:08]

Nit: Conditional check unnecessary

[aleksandar.stojiljkovic, 2016/09/27 18:08:28]

Done.

+        m_bitmap.setImmutable();
+    return SkImage::MakeFromBitmap(m_bitmap);
+}
+
 void ImageFrame::setHasAlpha(bool alpha)
 {
     m_hasAlpha = alpha;
@@ -134,7 +158,6 @@ void ImageFrame::setStatus(Status status)
         // Send pending pixels changed notifications now, because we can't do this after
         // the bitmap has been marked immutable.
         notifyBitmapIfPixelsChanged();
-        m_bitmap.setImmutable(); // Tell the bitmap it's done.
     }
 }