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

third_party/WebKit/Source/core/frame/ImageBitmap.cpp

Index: third_party/WebKit/Source/core/frame/ImageBitmap.cpp
diff --git a/third_party/WebKit/Source/core/frame/ImageBitmap.cpp b/third_party/WebKit/Source/core/frame/ImageBitmap.cpp
index 279710f53f8f36905f798bef2fd58c65b554b5a7..93bd12baf8a8b0c184262671a5a71365091842a2 100644
--- a/third_party/WebKit/Source/core/frame/ImageBitmap.cpp
+++ b/third_party/WebKit/Source/core/frame/ImageBitmap.cpp
@@ -45,12 +45,6 @@ static inline IntRect normalizeRect(const IntRect& rect)
         std::max(rect.height(), -rect.height()));
 }
 
-static bool frameIsValid(const SkBitmap& frameBitmap)
-{
-    ASSERT(!frameBitmap.isNull() && !frameBitmap.empty() && frameBitmap.isImmutable());
-    return frameBitmap.colorType() == kN32_SkColorType;
-}
-
 ParsedOptions parseOptions(const ImageBitmapOptions& options, Optional<IntRect> cropRect, IntSize sourceSize)
 {
     ParsedOptions parsedOptions;
@@ -215,10 +209,8 @@ sk_sp<SkImage> ImageBitmap::getSkImageFromDecoder(std::unique_ptr<ImageDecoder>
     ImageFrame* frame = decoder->frameBufferAtIndex(0);
     if (!frame || frame->getStatus() != ImageFrame::FrameComplete)
         return nullptr;
-    SkBitmap bitmap = frame->bitmap();
-    if (!frameIsValid(bitmap))
-        return nullptr;
-    return SkImage::MakeFromBitmap(bitmap);
+    DCHECK(!frame->bitmap().isNull() && !frame->bitmap().empty());
+    return (frame->bitmap().colorType() == kN32_SkColorType) ? frame->finalizePixelsAndGetImage() : nullptr;

[dcheng, 2016/09/27 22:44:15]

Just curious: what does that mean if we get here and the color type doesn't
match what we expect? Does that mean an error of some sort occurred earlier in
the decoding pipeline?

[aleksandar.stojiljkovic, 2016/09/28 18:51:24]

kUnknown_SkColorType:
In initial state, SkBitmap has kUnknown_SkColorType. This happens on error but
also when file is partially received, the frame data is not yet received and the
frame not initialized.
In this case ASSERT/DCHECK(!isNull && !isEmpty) is covering the same as the
condition.

some other bitmap type:
Not an error but someone implements a decoder producing different bitmap type.
Don't think there is such case now - I'm working on a patch of such kind for
chromium [1]. Don't know if there are decoders in different chromium flavors
doing it (probably no need to care about it).
[1]
https://codereview.chromium.org/1460523002/


Otherwise, the change in this file is refactoring only. We're not using
isImmutable but it had the same meaning as getStatus() ==
ImageFrame::FrameComplete.

[dcheng, 2016/09/28 21:16:52]

I understand that this is a refactoring only change, but I don't really
understand it either. Is it possible to add a comment about why we'd only return
an SkImage if the color type matches exactly? How is it handled if it doesn't
match? Do we just not return an image at all? That doesn't seem completely right
either?

(I'm sure this makes more sense to someone who's already familiar with the
decoding pipeline, but I'm not, and so more comments seem like they'd be useful
here)

[aleksandar.stojiljkovic, 2016/09/28 22:10:22]

Yes, given the explanation I provided above, there is really no sense in doing
the color check. Removed it.
There are no decoders that produce other color types than N32.
If the frame's bitmap is empty (colortype kUnknown_SkColorType) but frame marked
as complete which cannot happen now, but let's open possibility for future, it
is still perfectly fine to call SkImage::MakeFromBitmap(m_bitmap) on empty
m_bitmap - it returns null.
So, no need to check color type - all cases are handled.

Related to your other concern, returning nullptr from this method is correct and
expected in case of error. All the clients of the call are handling it.

 }
 
 bool ImageBitmap::isResizeOptionValid(const ImageBitmapOptions& options, ExceptionState& exceptionState)