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

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

 /*
  * Copyright (C) 2006 Samuel Weinig (sam.weinig@gmail.com)
  * Copyright (C) 2004, 2005, 2006, 2008 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  * 1. Redistributions of source code must retain the above copyright
  *    notice, this list of conditions and the following disclaimer.
  * 2. Redistributions in binary form must reproduce the above copyright
@@ skipping 366 matching lines @@
     }
 
     return Image::imageForDefaultFrame();
 }
 
 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)
 {
     if (metadataMode == PreCacheMetadata) {
         // frameHasAlphaAtIndex() conservatively returns false for uncached frames. To increase the
         // chance of an accurate answer, pre-cache the current frame metadata.
         frameAtIndex(currentFrame());
     }
     return !frameHasAlphaAtIndex(currentFrame());
@@ skipping 209 matching lines @@
 
     // We need to draw this frame if we advanced to it while not skipping, or if
     // while trying to skip frames we hit the last frame and thus had to stop.
     if (skippingFrames != advancedAnimation)
         getImageObserver()->animationAdvanced(this);
 
     return advancedAnimation;
 }
 
 } // namespace blink