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

third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "modules/notifications/NotificationImageLoader.h"
 
 #include "core/dom/ExecutionContext.h"
 #include "core/fetch/ResourceLoaderOptions.h"
 #include "platform/Histogram.h"
 #include "platform/image-decoders/ImageDecoder.h"
@@ skipping 77 matching lines @@
 
     if (m_data) {
         DEFINE_THREAD_SAFE_STATIC_LOCAL(CustomCountHistogram, fileSizeHistogram, new CustomCountHistogram("Notifications.Icon.FileSize", 1, 10000000 /* ~10mb max */, 50 /* buckets */));
         fileSizeHistogram.count(m_data->size());
 
         std::unique_ptr<ImageDecoder> decoder = ImageDecoder::create(ImageDecoder::determineImageType(*m_data.get()), ImageDecoder::AlphaPremultiplied, ImageDecoder::GammaAndColorProfileApplied);
         if (decoder) {
             decoder->setData(m_data.get(), true /* allDataReceived */);
             // The |ImageFrame*| is owned by the decoder.
             ImageFrame* imageFrame = decoder->frameBufferAtIndex(0);
-            if (imageFrame) {
-                (*m_imageCallback)(imageFrame->bitmap());
+            DCHECK(decoder->failed() || imageFrame->getStatus() == ImageFrame::FrameComplete);

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

This check isn't safe if somehow failed() is false but |imageFrame| is null.

If that should never happen, I'd change this to:

    if (imageFrame && imageFrame->getStatus() == ImageFrame::FrameComplete) {
        ...
    } else {
        DCHECK(decoder->failed());
    }

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

Done. Reverted. As allDataReceived is set to true it is expected that
either it is complete or failed. Didn't see in code how it could be null.

+            if (imageFrame && imageFrame->getStatus() == ImageFrame::FrameComplete) {
+                SkBitmap bitmap = imageFrame->bitmap();
+                bitmap.setImmutable();

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

Why is setting the bitmap as immutable here important?  At the very least this
needs a comment.

This question basically applies to everywhere in this CL that does this.  I was
under the impression only people getting an SkImage needed to do this (to avoid
a copy), but we seem to be doing it many other places.

If this is because we're passing the bitmap to places that will turn it into an
image, can we change to passing an SkImage now?

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

That was my plan too, with all the cases where SkBitmap was ported to SkImage.
After this patch, only remaining case where we do this is with
WebImage(SkBitmap) construction:
https://cs.chromium.org/chromium/src/third_party/WebKit/public/platform/WebIm...
and there is WebImage::getSkBitmap API that would need to be replaced with
SkImage variant.
The reason I left the SkBitmap/setImmutable used here is that it is WebKit
public/platform API and don't know the deprecation policy here / if anybody else
outside chromium codebase is using it.

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

Reverted this to original implementation - in ImageSkiaRep::ImageSkiaRep(const
SkBitmap& src, float scale) where this code leads to, bitmap is set to
immutable.
[1]
https://cs.chromium.org/chromium/src/ui/gfx/image/image_skia_rep.cc?rcl=14744...

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

Blink is Chrome's fork, so we assume that we only want things Chrome is using.

So you should always go ahead and rip out things that you see Chromium code no
longer needing.

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

Checked in details - a lot of layout tests are using WebImage and the code needs
SkBitmap for direct access to pixels - so I left SkBitmap but there in the
WebImage::getSkBitmap API, and seems to be no need to call setImmutable - on
rare places where it is converted to SkImage it is already happening  on client
side of the call through [1]
https://cs.chromium.org/chromium/src/ui/gfx/image/image_skia_rep.cc?rcl=14744...

+                (*m_imageCallback)(bitmap);
                 return;
             }
         }
     }
     runCallbackWithEmptyBitmap();
 }
 
 void NotificationImageLoader::didFail(const ResourceError& error)
 {
     DEFINE_THREAD_SAFE_STATIC_LOCAL(CustomCountHistogram, failedTimeHistogram, new CustomCountHistogram("Notifications.Icon.LoadFailTime", 1, 1000 * 60 * 60 /* 1 hour max */, 50 /* buckets */));
@@ skipping 11 matching lines @@
 {
     // If this has been stopped it is not desirable to trigger further work,
     // there is a shutdown of some sort in progress.
     if (m_stopped)
         return;
 
     (*m_imageCallback)(SkBitmap());
 }
 
 } // namespace blink