Don't invalidate paint on background image change if it's obscured

third_party/WebKit/Source/platform/graphics/BitmapImage.cpp

Index: third_party/WebKit/Source/platform/graphics/BitmapImage.cpp
diff --git a/third_party/WebKit/Source/platform/graphics/BitmapImage.cpp b/third_party/WebKit/Source/platform/graphics/BitmapImage.cpp
index 19ed900844e6134debd272d79dc5a90b4a48d498..6bd22c923ea3ab9b77ed1e4398427c6af54cfd5c 100644
--- a/third_party/WebKit/Source/platform/graphics/BitmapImage.cpp
+++ b/third_party/WebKit/Source/platform/graphics/BitmapImage.cpp
@@ -384,10 +384,17 @@ bool BitmapImage::frameHasAlphaAtIndex(size_t index)
     if (m_frames.size() <= index)
         return true;
 
+    if (m_frames[index].m_haveMetadata && !m_frames[index].m_hasAlpha)
+        return false;
+
+    // m_hasAlpha may change after m_haveMetadata is set to true, so always ask
+    // ImageSource for the value if the cached value is the default value.
+    bool hasAlpha = m_source.frameHasAlphaAtIndex(index);

[Stephen Chennney, 2016/08/09 14:22:31]

Based on trying a similar change I believe this has a very large impact on
performance. Bug crbug.com/593430 should be fixed instead, or run cluster
telemetry to verify performance.

[Xianzhu, 2016/08/09 18:51:48]

Which benchmark do you think the best to reflect the performance of this?

I thought of letting DeferredImageDecoder notify ImageSource when it finishes
decoding a frame, but not sure the proper way to do this. Besides the hasAlpha
flag that is always true for deferred decoded images, the isComplete flag is
also always false because of lacking of the notification.

(Static analysis of performance:

1. The above line is basically equivalent to the following which should be fast
(except for the lock that I'm not sure about its impact):

  // In ImageSource::frameHasAlphaAtIndex():
  if (!m_decoder)
    return true;
  // In DeferredImageDecoder::frameHasAlphaAtIndex():
  if (m_actualDecoder) // false
    ...
  if (!m_frameGenerator->isMultiFrame()) {
    // In ImageFrameGenerator
    MutexLocker lock(m_alphaMutex);
    if (index < m_hasAlpha.size())
      return m_hasAlpha[index];
  }
  return true;

2. In case hasAlpha == false, it will be cached below, and will be used from
cache in the future.)

[Stephen Chennney, 2016/08/09 19:06:04]

I would try a cluster telemetry run. Here's the one I did:
https://00e9e64bac8558f036879b6a7b17bf91e4e821e538348abc6c-apidata.googleuser...

I think you need an @google account to see it.

Your change is better than mine in that you will stop checking once you discover
that it is opaque, so I would be willing for sure to take the perf hit given the
expected gain from reduced invalidation.
 
Right. The issue is that decode now happens on a raster thread, so getting a
message back to the renderer is challenging. It was on my agenda to look into it
but I never got there.

+
     if (m_frames[index].m_haveMetadata)
-        return m_frames[index].m_hasAlpha;
+        m_frames[index].m_hasAlpha = hasAlpha;
 
-    return m_source.frameHasAlphaAtIndex(index);
+    return hasAlpha;
 }
 
 bool BitmapImage::currentFrameKnownToBeOpaque(MetadataMode metadataMode)