Changeset 107130 in webkit

Timestamp:

Feb 8, 2012 1:41:47 PM (12 years ago)

Author:

shawnsingh@chromium.org

Message:

[chromium] Remove incorrect early exit in CCDamageTracker
​https://bugs.webkit.org/show_bug.cgi?id=76924

Reviewed by James Robinson.

Source/WebCore:

New unit test added to CCDamageTrackerTest.cpp

This patch does three things: (1) adds unit test that demonstrates
that early exiting in CCDamageTracker is wrong, (2) removes the
early exit and cleans up the surrounding code, and (3) re-names
several functions in CCDamageTracker so that state updating is
implied by the name, and not just a bad side-effect of the functions.

• platform/graphics/chromium/cc/CCDamageTracker.cpp:

(WebCore::CCDamageTracker::updateDamageTrackingState):
(WebCore::CCDamageTracker::trackDamageFromActiveLayers):
(WebCore::CCDamageTracker::trackDamageFromSurfaceMask):
(WebCore::CCDamageTracker::trackDamageFromLeftoverRects):

• platform/graphics/chromium/cc/CCDamageTracker.h:

(CCDamageTracker):

• platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:

(WebCore::CCLayerTreeHostImpl::trackDamageForAllSurfaces):

Source/WebKit/chromium:

• tests/CCDamageTrackerTest.cpp:

(WebKitTests::emulateDrawingOneFrame):
(WebKitTests::TEST_F):
(WebKitTests):

Location:

trunk/Source

Files:

• WebCore/ChangeLog (modified) (1 diff)
• WebCore/platform/graphics/chromium/cc/CCDamageTracker.cpp (modified) (5 diffs)
• WebCore/platform/graphics/chromium/cc/CCDamageTracker.h (modified) (2 diffs)
• WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp (modified) (1 diff)
• WebKit/chromium/ChangeLog (modified) (1 diff)
• WebKit/chromium/tests/CCDamageTrackerTest.cpp (modified) (1 diff)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2012-02-08  Shawn Singh  <shawnsingh@chromium.org>
+
+        [chromium] Remove incorrect early exit in CCDamageTracker
+        https://bugs.webkit.org/show_bug.cgi?id=76924
+
+        Reviewed by James Robinson.
+
+        New unit test added to CCDamageTrackerTest.cpp
+
+        This patch does three things: (1) adds unit test that demonstrates
+        that early exiting in CCDamageTracker is wrong, (2) removes the
+        early exit and cleans up the surrounding code, and (3) re-names
+        several functions in CCDamageTracker so that state updating is
+        implied by the name, and not just a bad side-effect of the functions.
+
+        * platform/graphics/chromium/cc/CCDamageTracker.cpp:
+        (WebCore::CCDamageTracker::updateDamageTrackingState):
+        (WebCore::CCDamageTracker::trackDamageFromActiveLayers):
+        (WebCore::CCDamageTracker::trackDamageFromSurfaceMask):
+        (WebCore::CCDamageTracker::trackDamageFromLeftoverRects):
+        * platform/graphics/chromium/cc/CCDamageTracker.h:
+        (CCDamageTracker):
+        * platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:
+        (WebCore::CCLayerTreeHostImpl::trackDamageForAllSurfaces):
+
 2012-02-08  James Robinson  <jamesr@chromium.org>
 

trunk/Source/WebCore/platform/graphics/chromium/cc/CCDamageTracker.cpp

@@ -57 +57 @@
 }
 
-void CCDamageTracker::updateDamageRectForNextFrame(const Vector<RefPtr<CCLayerImpl> >& layerList, int targetSurfaceLayerID, CCLayerImpl* targetSurfaceMaskLayer)
-{
-    //
-    // This function computes the "damage rect" of a target surface. The damage
-    // rect is the region of the surface that may have changed and needs to be redrawn.
-    // This can be used to scissor what is actually drawn, to save GPU computation and
-    // bandwidth.
+void CCDamageTracker::updateDamageTrackingState(const Vector<RefPtr<CCLayerImpl> >& layerList, int targetSurfaceLayerID, CCLayerImpl* targetSurfaceMaskLayer)
+{
+    //
+    // This function computes the "damage rect" of a target surface, and updates the state
+    // that is used to correctly track damage across frames. The damage rect is the region
+    // of the surface that may have changed and needs to be redrawn. This can be used to
+    // scissor what is actually drawn, to save GPU computation and bandwidth.
     //
     // The surface's damage rect is computed as the union of all possible changes that
@@ -122 +122 @@
     //
 
+    // These functions cannot be bypassed with early-exits, even if we know what the
+    // damage will be for this frame, because we need to update the damage tracker state
+    // to correctly track the next frame.
+    FloatRect damageFromActiveLayers = trackDamageFromActiveLayers(layerList, targetSurfaceLayerID);
+    FloatRect damageFromSurfaceMask = trackDamageFromSurfaceMask(targetSurfaceMaskLayer);
+    FloatRect damageFromLeftoverRects = trackDamageFromLeftoverRects();
+
     // If the target surface already knows its entire region is damaged, we can return early.
     // FIXME: this should go away, or will be cleaner, after refactoring into RenderPass/RenderSchedule.
     CCLayerImpl* layer = layerList[0].get();
-    if (layer->targetRenderSurface()->surfacePropertyChangedOnlyFromDescendant()) {
-        m_currentDamageRect = FloatRect(layer->targetRenderSurface()->contentRect());
-        // FIXME: this early exit is incorrect: https://bugs.webkit.org/show_bug.cgi?id=76924
-        return;
-    }
-
-    FloatRect damageFromActiveLayers = computeDamageFromActiveLayers(layerList, targetSurfaceLayerID);
-    FloatRect damageFromSurfaceMask = computeDamageFromSurfaceMask(targetSurfaceMaskLayer);
-    FloatRect damageFromLeftoverRects = computeDamageFromLeftoverRects();
-
-    if (m_forceFullDamageNextUpdate) {
-        m_currentDamageRect = FloatRect(layer->targetRenderSurface()->contentRect());
+    CCRenderSurface* targetSurface = layer->targetRenderSurface();
+
+    if (m_forceFullDamageNextUpdate || targetSurface->surfacePropertyChangedOnlyFromDescendant()) {
+        m_currentDamageRect = FloatRect(targetSurface->contentRect());
         m_forceFullDamageNextUpdate = false;
     } else {
+        // FIXME: can we need to clamp this damage to the surface's content rect? (affects performance, but not correctness)
         m_currentDamageRect = damageFromActiveLayers;
         m_currentDamageRect.uniteIfNonZero(damageFromSurfaceMask);
@@ -162 +162 @@
 }
 
-FloatRect CCDamageTracker::computeDamageFromActiveLayers(const Vector<RefPtr<CCLayerImpl> >& layerList, int targetSurfaceLayerID)
+FloatRect CCDamageTracker::trackDamageFromActiveLayers(const Vector<RefPtr<CCLayerImpl> >& layerList, int targetSurfaceLayerID)
 {
     FloatRect damageRect = FloatRect();
@@ -178 +178 @@
 }
 
-FloatRect CCDamageTracker::computeDamageFromSurfaceMask(CCLayerImpl* targetSurfaceMaskLayer)
+FloatRect CCDamageTracker::trackDamageFromSurfaceMask(CCLayerImpl* targetSurfaceMaskLayer)
 {
     FloatRect damageRect = FloatRect();
@@ -194 +194 @@
 }
 
-FloatRect CCDamageTracker::computeDamageFromLeftoverRects()
+FloatRect CCDamageTracker::trackDamageFromLeftoverRects()
 {
     // After computing damage for all active layers, any leftover items in the current

trunk/Source/WebCore/platform/graphics/chromium/cc/CCDamageTracker.h

@@ -45 +45 @@
 
     void forceFullDamageNextUpdate() { m_forceFullDamageNextUpdate = true; }
-    void updateDamageRectForNextFrame(const Vector<RefPtr<CCLayerImpl> >& layerList, int targetSurfaceLayerID, CCLayerImpl* targetSurfaceMaskLayer);
+    void updateDamageTrackingState(const Vector<RefPtr<CCLayerImpl> >& layerList, int targetSurfaceLayerID, CCLayerImpl* targetSurfaceMaskLayer);
     const FloatRect& currentDamageRect() { return m_currentDamageRect; }
 
@@ -51 +51 @@
     CCDamageTracker();
 
-    FloatRect computeDamageFromActiveLayers(const Vector<RefPtr<CCLayerImpl> >& layerList, int targetSurfaceLayerID);
-    FloatRect computeDamageFromSurfaceMask(CCLayerImpl* targetSurfaceMaskLayer);
-    FloatRect computeDamageFromLeftoverRects();
+    FloatRect trackDamageFromActiveLayers(const Vector<RefPtr<CCLayerImpl> >& layerList, int targetSurfaceLayerID);
+    FloatRect trackDamageFromSurfaceMask(CCLayerImpl* targetSurfaceMaskLayer);
+    FloatRect trackDamageFromLeftoverRects();
 
     FloatRect removeRectFromCurrentFrame(int layerID);
     void saveRectForNextFrame(int layerID, const FloatRect& targetSpaceRect);
 
-    // These helper functions are used only in computeDamageFromActiveLayers().
+    // These helper functions are used only in trackDamageFromActiveLayers().
     void extendDamageForLayer(CCLayerImpl*, FloatRect& targetDamageRect);
     void extendDamageForRenderSurface(CCLayerImpl*, FloatRect& targetDamageRect);

trunk/Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp

@@ -149 +149 @@
         CCRenderSurface* renderSurface = renderSurfaceLayer->renderSurface();
         ASSERT(renderSurface);
-        renderSurface->damageTracker()->updateDamageRectForNextFrame(renderSurface->layerList(), renderSurfaceLayer->id(), renderSurfaceLayer->maskLayer());
+        renderSurface->damageTracker()->updateDamageTrackingState(renderSurface->layerList(), renderSurfaceLayer->id(), renderSurfaceLayer->maskLayer());
     }
 }

trunk/Source/WebKit/chromium/ChangeLog

@@ +1 @@
+2012-02-08  Shawn Singh  <shawnsingh@chromium.org>
+
+        [chromium] Remove incorrect early exit in CCDamageTracker
+        https://bugs.webkit.org/show_bug.cgi?id=76924
+
+        Reviewed by James Robinson.
+
+        * tests/CCDamageTrackerTest.cpp:
+        (WebKitTests::emulateDrawingOneFrame):
+        (WebKitTests::TEST_F):
+        (WebKitTests):
+
 2012-02-08  Sadrul Habib Chowdhury  <sadrul@chromium.org>
 

trunk/Source/WebKit/chromium/tests/CCDamageTrackerTest.cpp

@@ -71 +71 @@
     for (int i = renderSurfaceLayerList.size() - 1; i >= 0; --i) {
         CCRenderSurface* targetSurface = renderSurfaceLayerList[i]->renderSurface();
-        targetSurface->damageTracker()->updateDamageRectForNextFrame(targetSurface->layerList(), targetSurface->owningLayerId(), renderSurfaceLayerList[i]->maskLayer());
+        targetSurface->damageTracker()->updateDamageTrackingState(targetSurface->layerList(), targetSurface->owningLayerId(), renderSurfaceLayerList[i]->maskLayer());
     }