[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):

git-svn-id: http://svn.webkit.org/repository/webkit/trunk@107130 268f45cc-cd09-0410-ab3c-d52691b4dbfc

diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
index c81ae19..1232dae 100644 (file)
--- a/Source/WebCore/ChangeLog
+++ b/Source/WebCore/ChangeLog
@@ -1,3 +1,28 @@
+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>
 
         [chromium] Check that we can make the SharedGraphicsContext3D current before returning

diff --git a/Source/WebCore/platform/graphics/chromium/cc/CCDamageTracker.cpp b/Source/WebCore/platform/graphics/chromium/cc/CCDamageTracker.cpp
index 6e562e1..2d6f43f 100644 (file)
--- a/Source/WebCore/platform/graphics/chromium/cc/CCDamageTracker.cpp
+++ b/Source/WebCore/platform/graphics/chromium/cc/CCDamageTracker.cpp
@@ -56,13 +56,13 @@ CCDamageTracker::~CCDamageTracker()
 {
 }
 
-void CCDamageTracker::updateDamageRectForNextFrame(const Vector<RefPtr<CCLayerImpl> >& layerList, int targetSurfaceLayerID, CCLayerImpl* targetSurfaceMaskLayer)
+void CCDamageTracker::updateDamageTrackingState(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.
+    // 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
     // have happened to the surface since the last frame was drawn. This includes:
@@ -121,23 +121,23 @@ void CCDamageTracker::updateDamageRectForNextFrame(const Vector<RefPtr<CCLayerIm
     //         damage tracker is ready for the next frame.
     //
 
+    // 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();
+    CCRenderSurface* targetSurface = layer->targetRenderSurface();
 
-    if (m_forceFullDamageNextUpdate) {
-        m_currentDamageRect = FloatRect(layer->targetRenderSurface()->contentRect());
+    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);
         m_currentDamageRect.uniteIfNonZero(damageFromLeftoverRects);
@@ -161,7 +161,7 @@ void CCDamageTracker::saveRectForNextFrame(int layerID, const FloatRect& targetS
     m_nextRectHistory->set(layerID, targetSpaceRect);
 }
 
-FloatRect CCDamageTracker::computeDamageFromActiveLayers(const Vector<RefPtr<CCLayerImpl> >& layerList, int targetSurfaceLayerID)
+FloatRect CCDamageTracker::trackDamageFromActiveLayers(const Vector<RefPtr<CCLayerImpl> >& layerList, int targetSurfaceLayerID)
 {
     FloatRect damageRect = FloatRect();
 
@@ -177,7 +177,7 @@ FloatRect CCDamageTracker::computeDamageFromActiveLayers(const Vector<RefPtr<CCL
     return damageRect;
 }
 
-FloatRect CCDamageTracker::computeDamageFromSurfaceMask(CCLayerImpl* targetSurfaceMaskLayer)
+FloatRect CCDamageTracker::trackDamageFromSurfaceMask(CCLayerImpl* targetSurfaceMaskLayer)
 {
     FloatRect damageRect = FloatRect();
 
@@ -193,7 +193,7 @@ FloatRect CCDamageTracker::computeDamageFromSurfaceMask(CCLayerImpl* targetSurfa
     return damageRect;
 }
 
-FloatRect CCDamageTracker::computeDamageFromLeftoverRects()
+FloatRect CCDamageTracker::trackDamageFromLeftoverRects()
 {
     // After computing damage for all active layers, any leftover items in the current
     // rect history correspond to layers/surfaces that no longer exist. So, these regions

diff --git a/Source/WebCore/platform/graphics/chromium/cc/CCDamageTracker.h b/Source/WebCore/platform/graphics/chromium/cc/CCDamageTracker.h
index ac1a0ca..d428f26 100644 (file)
--- a/Source/WebCore/platform/graphics/chromium/cc/CCDamageTracker.h
+++ b/Source/WebCore/platform/graphics/chromium/cc/CCDamageTracker.h
@@ -44,20 +44,20 @@ public:
     ~CCDamageTracker();
 
     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; }
 
 private:
     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);
 

diff --git a/Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp b/Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp
index e930bc2..088377c 100644 (file)
--- a/Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp
+++ b/Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp
@@ -148,7 +148,7 @@ void CCLayerTreeHostImpl::trackDamageForAllSurfaces(CCLayerImpl* rootDrawLayer,
         CCLayerImpl* renderSurfaceLayer = renderSurfaceLayerList[surfaceIndex].get();
         CCRenderSurface* renderSurface = renderSurfaceLayer->renderSurface();
         ASSERT(renderSurface);
-        renderSurface->damageTracker()->updateDamageRectForNextFrame(renderSurface->layerList(), renderSurfaceLayer->id(), renderSurfaceLayer->maskLayer());
+        renderSurface->damageTracker()->updateDamageTrackingState(renderSurface->layerList(), renderSurfaceLayer->id(), renderSurfaceLayer->maskLayer());
     }
 }
 

diff --git a/Source/WebKit/chromium/ChangeLog b/Source/WebKit/chromium/ChangeLog
index bf2acbc..acedbad 100644 (file)
--- a/Source/WebKit/chromium/ChangeLog
+++ b/Source/WebKit/chromium/ChangeLog
@@ -1,3 +1,15 @@
+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>
 
         Add support for pinch gesture processing in the MT compositor.

diff --git a/Source/WebKit/chromium/tests/CCDamageTrackerTest.cpp b/Source/WebKit/chromium/tests/CCDamageTrackerTest.cpp
index 24fcfba..a9f7b1c 100644 (file)
--- a/Source/WebKit/chromium/tests/CCDamageTrackerTest.cpp
+++ b/Source/WebKit/chromium/tests/CCDamageTrackerTest.cpp
@@ -70,7 +70,7 @@ void emulateDrawingOneFrame(CCLayerImpl* root)
     // Iterate back-to-front, so that damage correctly propagates from descendant surfaces to ancestors.
     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());
     }
 
     root->resetAllChangeTrackingForSubtree();