Check rect under-invalidations

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

Index: third_party/WebKit/Source/platform/graphics/GraphicsLayer.cpp
diff --git a/third_party/WebKit/Source/platform/graphics/GraphicsLayer.cpp b/third_party/WebKit/Source/platform/graphics/GraphicsLayer.cpp
index c1b7be81f7e2cecac5ff5f6147f069a674172a32..5069ff788eaf41086d045b70e78cae3a2319ccdb 100644
--- a/third_party/WebKit/Source/platform/graphics/GraphicsLayer.cpp
+++ b/third_party/WebKit/Source/platform/graphics/GraphicsLayer.cpp
@@ -34,6 +34,7 @@
 #include "platform/TraceEvent.h"
 #include "platform/geometry/FloatRect.h"
 #include "platform/geometry/LayoutRect.h"
+#include "platform/geometry/Region.h"
 #include "platform/graphics/BitmapImage.h"
 #include "platform/graphics/CompositorFactory.h"
 #include "platform/graphics/CompositorFilterOperations.h"
@@ -42,6 +43,7 @@
 #include "platform/graphics/Image.h"
 #include "platform/graphics/LinkHighlight.h"
 #include "platform/graphics/filters/SkiaImageFilterBuilder.h"
+#include "platform/graphics/paint/DisplayItemCacheSkipper.h"
 #include "platform/graphics/paint/DrawingRecorder.h"
 #include "platform/graphics/paint/PaintController.h"
 #include "platform/scroll/ScrollableArea.h"
@@ -76,11 +78,20 @@ struct PaintInvalidationInfo {
     // This is for comparison only. Don't dereference because the client may have died.
     const DisplayItemClient* client;
     String clientDebugName;
-    FloatRect rect;
+    IntRect rect;
     PaintInvalidationReason reason;
 };
 
-typedef HashMap<const GraphicsLayer*, Vector<PaintInvalidationInfo>> PaintInvalidationTrackingMap;
+struct PaintInvalidationTracking {
+    DISALLOW_NEW_EXCEPT_PLACEMENT_NEW();
+    Vector<PaintInvalidationInfo> trackedPaintInvalidations;
+#if DCHECK_IS_ON()
+    RefPtr<SkPicture> lastPaintedPicture;
+    Region paintInvalidationRegionSinceLastPaint;
+#endif
+};
+
+typedef HashMap<const GraphicsLayer*, PaintInvalidationTracking> PaintInvalidationTrackingMap;
 static PaintInvalidationTrackingMap& paintInvalidationTrackingMap()
 {
     DEFINE_STATIC_LOCAL(PaintInvalidationTrackingMap, map, ());
@@ -110,6 +121,7 @@ GraphicsLayer::GraphicsLayer(GraphicsLayerClient* client)
     , m_painted(false)
     , m_textPainted(false)
     , m_imagePainted(false)
+    , m_isTrackingPaintInvalidations(client && client->isTrackingPaintInvalidations())
     , m_paintingPhase(GraphicsLayerPaintAllWithOverflowClip)
     , m_parent(0)
     , m_maskLayer(0)
@@ -153,7 +165,7 @@ GraphicsLayer::~GraphicsLayer()
     removeAllChildren();
     removeFromParent();
 
-    resetTrackedPaintInvalidations();
+    paintInvalidationTrackingMap().remove(this);
     ASSERT(!m_parent);
 }
 
@@ -319,8 +331,18 @@ IntRect GraphicsLayer::interestRect()
 
 void GraphicsLayer::paint(const IntRect* interestRect, GraphicsContext::DisabledMode disabledMode)
 {
-    if (paintWithoutCommit(interestRect, disabledMode))
+    if (paintWithoutCommit(interestRect, disabledMode)) {
         getPaintController().commitNewDisplayItems(offsetFromLayoutObjectWithSubpixelAccumulation());
+#if DCHECK_IS_ON()
+        if (isTrackingOrCheckingPaintInvalidations()) {
+            RefPtr<SkPicture> newPicture = capturePicture();
+            checkUnderPaintInvalidations(*newPicture);
+            PaintInvalidationTracking& tracking = paintInvalidationTrackingMap().add(this, PaintInvalidationTracking()).storedValue->value;
+            tracking.lastPaintedPicture = newPicture;
+            tracking.paintInvalidationRegionSinceLastPaint = Region();
+        }
+#endif
+    }
 }
 
 bool GraphicsLayer::paintWithoutCommit(const IntRect* interestRect, GraphicsContext::DisabledMode disabledMode)
@@ -351,10 +373,9 @@ bool GraphicsLayer::paintWithoutCommit(const IntRect* interestRect, GraphicsCont
 #ifndef NDEBUG
     if (contentsOpaque() && s_drawDebugRedFill) {
         FloatRect rect(FloatPoint(), size());
-        if (!DrawingRecorder::useCachedDrawingIfPossible(context, *this, DisplayItem::DebugRedFill)) {
-            DrawingRecorder recorder(context, *this, DisplayItem::DebugRedFill, rect);
-            context.fillRect(rect, SK_ColorRED);
-        }
+        DisplayItemCacheSkipper cacheSkipper(context);

[pdr., 2016/06/09 19:21:44]

This change makes sense, but why was it needed for this patch?

[Xianzhu, 2016/06/09 20:47:44]

This is not needed for this patch. Reverted.

+        DrawingRecorder recorder(context, *this, DisplayItem::DebugRedFill, rect);
+        context.fillRect(rect, SK_ColorRED);
     }
 #endif
 
@@ -533,33 +554,55 @@ WebLayer* GraphicsLayer::contentsLayerIfRegistered()
     return m_contentsLayer;
 }
 
+void GraphicsLayer::setTracksPaintInvalidations(bool tracksPaintInvalidations)
+{
+    resetTrackedPaintInvalidations();
+    m_isTrackingPaintInvalidations = tracksPaintInvalidations;
+}
+
 void GraphicsLayer::resetTrackedPaintInvalidations()
 {
-    paintInvalidationTrackingMap().remove(this);
+    auto it = paintInvalidationTrackingMap().find(this);
+    if (it == paintInvalidationTrackingMap().end())
+        return;
+
+    if (!RuntimeEnabledFeatures::slimmingPaintUnderInvalidationCheckingEnabled())
+        paintInvalidationTrackingMap().remove(it);
+    else
+        it->value.trackedPaintInvalidations.clear();
 }
 
 bool GraphicsLayer::hasTrackedPaintInvalidations() const
 {
     PaintInvalidationTrackingMap::iterator it = paintInvalidationTrackingMap().find(this);
     if (it != paintInvalidationTrackingMap().end())
-        return !it->value.isEmpty();
+        return !it->value.trackedPaintInvalidations.isEmpty();
     return false;
 }
 
-void GraphicsLayer::trackPaintInvalidation(const DisplayItemClient& client, const FloatRect& rect, PaintInvalidationReason reason)
+void GraphicsLayer::trackPaintInvalidation(const DisplayItemClient& client, const IntRect& rect, PaintInvalidationReason reason)
 {
-    // The caller must check isTrackingPaintInvalidations() before calling this method
+    // The caller must check isTrackingOrCheckingPaintInvalidations() before calling this method
     // to avoid constructing the rect unnecessarily.
-    ASSERT(isTrackingPaintInvalidations());
+    DCHECK(isTrackingOrCheckingPaintInvalidations());
 
-    Vector<PaintInvalidationInfo>& infos = paintInvalidationTrackingMap().add(this, Vector<PaintInvalidationInfo>()).storedValue->value;
+    PaintInvalidationTracking& tracking = paintInvalidationTrackingMap().add(this, PaintInvalidationTracking()).storedValue->value;
     // Omit the entry for trackObjectPaintInvalidation() if the last entry is for the same client.
     // This is to avoid duplicated entries for setNeedsDisplayInRect() and trackObjectPaintInvalidation().
-    if (rect.isEmpty() && !infos.isEmpty() && infos.last().client == &client)
+    if (rect.isEmpty() && !tracking.trackedPaintInvalidations.isEmpty() && tracking.trackedPaintInvalidations.last().client == &client)
         return;
 
     PaintInvalidationInfo info = { &client, client.debugName(), rect, reason };
-    infos.append(info);
+    tracking.trackedPaintInvalidations.append(info);
+
+#if DCHECK_IS_ON()
+    if (!rect.isEmpty()) {
+        // TODO(crbug.com/496260): Some antialiasing effects overflows the paint invalidation rect.
+        IntRect r = rect;
+        r.inflate(1);
+        tracking.paintInvalidationRegionSinceLastPaint.unite(r);
+    }
+#endif
 }
 
 static bool comparePaintInvalidationInfo(const PaintInvalidationInfo& a, const PaintInvalidationInfo& b)
@@ -732,7 +775,7 @@ PassRefPtr<JSONObject> GraphicsLayer::layerTreeAsJSON(LayerTreeFlags flags, Rend
     PaintInvalidationTrackingMap::iterator it = paintInvalidationTrackingMap().find(this);
     if (it != paintInvalidationTrackingMap().end()) {
         if (flags & LayerTreeIncludesPaintInvalidations) {
-            Vector<PaintInvalidationInfo>& infos = it->value;
+            Vector<PaintInvalidationInfo>& infos = it->value.trackedPaintInvalidations;
             if (!infos.isEmpty()) {
                 std::sort(infos.begin(), infos.end(), &comparePaintInvalidationInfo);
                 RefPtr<JSONArray> paintInvalidationsJSON = JSONArray::create();
@@ -875,11 +918,6 @@ void GraphicsLayer::setSize(const FloatSize& size)
 
     m_layer->layer()->setBounds(flooredIntSize(m_size));
     // Note that we don't resize m_contentsLayer. It's up the caller to do that.
-
-#ifndef NDEBUG
-    // The red debug fill and needs to be invalidated if the layer resizes.
-    setDisplayItemsUncached();
-#endif
 }
 
 void GraphicsLayer::setTransform(const TransformationMatrix& transform)
@@ -1035,7 +1073,7 @@ void GraphicsLayer::setContentsNeedsDisplay()
 {
     if (WebLayer* contentsLayer = contentsLayerIfRegistered()) {
         contentsLayer->invalidate();
-        if (isTrackingPaintInvalidations())
+        if (isTrackingOrCheckingPaintInvalidations())
             trackPaintInvalidation(*this, m_contentsRect, PaintInvalidationFull);
     }
 }
@@ -1051,8 +1089,8 @@ void GraphicsLayer::setNeedsDisplay()
         m_linkHighlights[i]->invalidate();
     getPaintController().invalidateAll();
 
-    if (isTrackingPaintInvalidations())
-        trackPaintInvalidation(*this, FloatRect(FloatPoint(), m_size), PaintInvalidationFull);
+    if (isTrackingOrCheckingPaintInvalidations())
+        trackPaintInvalidation(*this, IntRect(IntPoint(), expandedIntSize(m_size)), PaintInvalidationFull);
 }
 
 void GraphicsLayer::setNeedsDisplayInRect(const IntRect& rect, PaintInvalidationReason invalidationReason, const DisplayItemClient& client)
@@ -1066,7 +1104,7 @@ void GraphicsLayer::setNeedsDisplayInRect(const IntRect& rect, PaintInvalidation
     for (size_t i = 0; i < m_linkHighlights.size(); ++i)
         m_linkHighlights[i]->invalidate();
 
-    if (isTrackingPaintInvalidations())
+    if (isTrackingOrCheckingPaintInvalidations())
         trackPaintInvalidation(client, rect, invalidationReason);
 }
 
@@ -1077,8 +1115,8 @@ void GraphicsLayer::displayItemClientWasInvalidated(const DisplayItemClient& dis
 
     getPaintController().displayItemClientWasInvalidated(displayItemClient);
 
-    if (isTrackingPaintInvalidations())
-        trackPaintInvalidation(displayItemClient, FloatRect(), invalidationReason);
+    if (isTrackingOrCheckingPaintInvalidations())
+        trackPaintInvalidation(displayItemClient, IntRect(), invalidationReason);
 }
 
 void GraphicsLayer::setContentsRect(const IntRect& rect)
@@ -1219,6 +1257,80 @@ void GraphicsLayer::setCompositorMutableProperties(uint32_t properties)
         layer->setCompositorMutableProperties(properties);
 }
 
+#if DCHECK_IS_ON()
+
+PassRefPtr<SkPicture> GraphicsLayer::capturePicture()
+{
+    if (!drawsContent())
+        return nullptr;
+
+    IntSize intSize = expandedIntSize(size());
+    GraphicsContext graphicsContext(getPaintController());
+    graphicsContext.beginRecording(IntRect(IntPoint(0, 0), intSize));
+    getPaintController().paintArtifact().replay(graphicsContext);

[pdr., 2016/06/09 19:21:44]

Why look at the pixels instead of just comparing the display item sizes and
types?

[Xianzhu, 2016/06/09 20:47:44]

Several reasons:

1. For incremental invalidation which just invalidate part of an display item,
we expect that only the incremental part of the display item changed. It seems
not feasible to check if a display item didn't change outside a certain rect
without rasterizing it.

2. We need to trust display item's visualRect to check if a changed display item
is covered by the paint invalidation rects. Because visual rects and paint
invalidation rects are actually from the same code path, we'll miss bugs about
paint invalidation rect calculation.

3. Sometimes we need rasterization invalidation when the display item itself
doesn't change but its location changes. We can find display items whose
location changed and check if they are covered by paint invalidation rects, but
this will have the same problem as 2.
 

+    return graphicsContext.endRecording();
+}
+
+void GraphicsLayer::checkUnderPaintInvalidations(const SkPicture& newPicture)
+{
+    if (!drawsContent())
+        return;
+
+    auto it = paintInvalidationTrackingMap().find(this);
+    if (it == paintInvalidationTrackingMap().end())
+        return;
+    const PaintInvalidationTracking& tracking = it->value;
+    if (!tracking.lastPaintedPicture)
+        return;
+
+    SkBitmap oldBitmap;
+    int width = static_cast<int>(ceilf(std::min(tracking.lastPaintedPicture->cullRect().width(), newPicture.cullRect().width())));
+    int height = static_cast<int>(ceilf(std::min(tracking.lastPaintedPicture->cullRect().height(), newPicture.cullRect().height())));
+    oldBitmap.allocPixels(SkImageInfo::MakeN32Premul(width, height));
+    SkCanvas(oldBitmap).drawPicture(tracking.lastPaintedPicture.get());
+
+    SkBitmap newBitmap;
+    newBitmap.allocPixels(SkImageInfo::MakeN32Premul(width, height));
+    SkCanvas(newBitmap).drawPicture(&newPicture);
+
+    oldBitmap.lockPixels();
+    newBitmap.lockPixels();
+    int mismatchingPixels = 0;
+    for (int y = 0; y < height; ++y) {
+        for (int x = 0; x < width; ++x) {
+            SkColor oldPixel = oldBitmap.getColor(x, y);
+            SkColor newPixel = newBitmap.getColor(x, y);
+            if (oldPixel != newPixel && !tracking.paintInvalidationRegionSinceLastPaint.contains(IntPoint(x, y))) {
+                if (mismatchingPixels < 50)
+                    LOG(ERROR) << debugName() << " Uninvalidated old/new pixels mismatch at " << x << "," << y << " old:" << std::hex << oldPixel << " new:" << newPixel;
+                else if (mismatchingPixels == 50)
+                    LOG(ERROR) << "and more...";
+                ++mismatchingPixels;
+                *newBitmap.getAddr32(x, y) = SK_ColorRED;
+            } else {
+                *newBitmap.getAddr32(x, y) = SK_ColorTRANSPARENT;
+            }
+        }
+    }
+
+    oldBitmap.unlockPixels();
+    newBitmap.unlockPixels();
+
+    // Crash in layout test paint invalidation tracking mode if slimmingPaintUnderInvalidationChecking is not enabled.
+    if (!RuntimeEnabledFeatures::slimmingPaintUnderInvalidationCheckingEnabled())
+        CHECK(!mismatchingPixels) << "Run chrome/content_shell with --enable-blink-features=slimmingPaintUnderInvalidationChecking to visualize under-invalidations";
+
+    // Visualize under-invalidations by overlaying the new bitmap (containing red pixels indicating under-invalidations,
+    // and transparent pixels otherwise) onto the painting.
+    SkPictureRecorder recorder;
+    recorder.beginRecording(width, height);
+    recorder.getRecordingCanvas()->drawBitmap(newBitmap, 0, 0);
+    RefPtr<SkPicture> picture = fromSkSp(recorder.finishRecordingAsPicture());
+    getPaintController().appendDebugDrawingAfterCommit(*this, picture, offsetFromLayoutObjectWithSubpixelAccumulation());
+}
+
+#endif // DCHECK_IS_ON()
+
 } // namespace blink
 
 #ifndef NDEBUG