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..25347976347c0d67d60d64a16e0b2a2d5eb0c7b2 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 {
@@ -96,6 +97,18 @@ bool ImageFrame::copyBitmapData(const ImageFrame& other)
     return other.m_bitmap.copyTo(&m_bitmap, other.m_bitmap.colorType());
 }
 
+bool ImageFrame::takeBitmapDataIfWritable(ImageFrame* other)
+{
+    DCHECK(other);
+    if (other->m_bitmap.isImmutable() || this == other)

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

Tiny efficiency nit: Reverse order of checks

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

It isn't obvious when/if this could happen (this == other) so while debugging
most common use cases I found that the current order is leaving the check
earlier.

"this == other" I copied from copyBitmapData.

[Peter Kasting, 2016/09/21 21:54:49]

I suppose you could make this (and maybe that function also?) DCHECK that this
!= other, if it never makes sense for that to happen (which it probably
doesn't).

[aleksandar.stojiljkovic, 2016/09/22 09:41:19]

Done.

+        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 +132,26 @@ bool ImageFrame::hasAlpha() const
     return m_hasAlpha;
 }
 
+const SkBitmap& ImageFrame::bitmap() const
+{
+    // We don't want to set the status to immutable here as we do in
+    // finalizePixelsAndGetImage() because mutable bitmap is reusable as the
+    // next animation frame by calling takeBitmapDataIfWritable.

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

Nit: The header comments should already justify any functional difference
between these two, so I'd remove this comment and just inline the method into
the header.

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

Done.

+    return m_bitmap;
+}
+
+PassRefPtr<SkImage> ImageFrame::finalizePixelsAndGetImage()
+{
+    DCHECK(m_status != FrameComplete || (!m_bitmap.isNull() && !m_bitmap.empty()));
+
+    // 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.
+    if (!m_bitmap.isImmutable() && m_status == FrameComplete)
+        m_bitmap.setImmutable();
+    return fromSkSp(SkImage::MakeFromBitmap(m_bitmap));
+}
+
 void ImageFrame::setHasAlpha(bool alpha)
 {
     m_hasAlpha = alpha;
@@ -134,7 +167,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.
     }
 }