[Chromium] Calls to paintContentsIfDirty() and updateCompositorResources() should...

https://bugs.webkit.org/show_bug.cgi?id=72630

Patch by David Reveman <reveman@chromium.org> on 2011-11-17
Reviewed by James Robinson.

Source/WebCore:

Layer property changes during paintContent() can leave the layer
in an invalid state as paintContentsIfDirty() has been called
without a matching updateCompositorResources() call. Removing
conditionals around these calls ensure they are balanced.

This patch is tested by the following unit test:
- CCLayerTreeHostTestOpacityChange.runMultiThread

* platform/graphics/chromium/ContentLayerChromium.cpp:
(WebCore::ContentLayerChromium::paintContentsIfDirty):
* platform/graphics/chromium/VideoLayerChromium.cpp:
(WebCore::VideoLayerChromium::updateCompositorResources):
* platform/graphics/chromium/cc/CCLayerTreeHost.cpp:
(WebCore::CCLayerTreeHost::paintMaskAndReplicaForRenderSurface):
(WebCore::CCLayerTreeHost::paintLayerContents):
(WebCore::CCLayerTreeHost::updateCompositorResources):
* platform/graphics/chromium/cc/CCLayerTreeHost.h:

Source/WebKit/chromium:

Add test that check if calls to paintContentsIfDirty() and
updateCompositorResources() are balanced when layer opacity
changes during paintContent().

* tests/CCLayerTreeHostTest.cpp:
(WTF::CCLayerTreeHostTest::layerTreeHost):
(WTF::TestOpacityChangeLayerDelegate::TestOpacityChangeLayerDelegate):
(WTF::TestOpacityChangeLayerDelegate::paintContents):
(WTF::TestOpacityChangeLayerDelegate::drawsContent):
(WTF::TestOpacityChangeLayerDelegate::preserves3D):
(WTF::TestOpacityChangeLayerDelegate::notifySyncRequired):
(WTF::ContentLayerChromiumWithUpdateTracking::create):
(WTF::ContentLayerChromiumWithUpdateTracking::paintContentsCount):
(WTF::ContentLayerChromiumWithUpdateTracking::resetPaintContentsCount):
(WTF::ContentLayerChromiumWithUpdateTracking::updateCount):
(WTF::ContentLayerChromiumWithUpdateTracking::resetUpdateCount):
(WTF::ContentLayerChromiumWithUpdateTracking::paintContentsIfDirty):
(WTF::ContentLayerChromiumWithUpdateTracking::updateCompositorResources):
(WTF::ContentLayerChromiumWithUpdateTracking::ContentLayerChromiumWithUpdateTracking):
(WTF::CCLayerTreeHostTestOpacityChange::CCLayerTreeHostTestOpacityChange):
(WTF::CCLayerTreeHostTestOpacityChange::beginTest):
(WTF::CCLayerTreeHostTestOpacityChange::commitCompleteOnCCThread):
(WTF::CCLayerTreeHostTestOpacityChange::afterTest):
(WTF::TEST_F):

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

diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
index e6616d5a3160a2adffd5b96188ecc8d9c442163e..fc4271eb887bea8d01c29f5810fc20221b771c29 100644 (file)
--- a/Source/WebCore/ChangeLog
+++ b/Source/WebCore/ChangeLog
@@ -1,3 +1,28 @@
+2011-11-17  David Reveman  <reveman@chromium.org>
+
+        [Chromium] Calls to paintContentsIfDirty() and updateCompositorResources() should be balanced.
+        https://bugs.webkit.org/show_bug.cgi?id=72630
+
+        Reviewed by James Robinson.
+
+        Layer property changes during paintContent() can leave the layer
+        in an invalid state as paintContentsIfDirty() has been called
+        without a matching updateCompositorResources() call. Removing
+        conditionals around these calls ensure they are balanced.
+
+        This patch is tested by the following unit test:
+        - CCLayerTreeHostTestOpacityChange.runMultiThread
+
+        * platform/graphics/chromium/ContentLayerChromium.cpp:
+        (WebCore::ContentLayerChromium::paintContentsIfDirty):
+        * platform/graphics/chromium/VideoLayerChromium.cpp:
+        (WebCore::VideoLayerChromium::updateCompositorResources):
+        * platform/graphics/chromium/cc/CCLayerTreeHost.cpp:
+        (WebCore::CCLayerTreeHost::paintMaskAndReplicaForRenderSurface):
+        (WebCore::CCLayerTreeHost::paintLayerContents):
+        (WebCore::CCLayerTreeHost::updateCompositorResources):
+        * platform/graphics/chromium/cc/CCLayerTreeHost.h:
+
 2011-11-17  Adam Barth  <abarth@webkit.org>
 
         Remove cargo-cult copy/pasting of ScriptExecutionContext namespace

diff --git a/Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp b/Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp
index ca8b3bd761f0815e47b6cfb0491a1d972879f04c..9eb1d799f6d91ecec6798106ec6027bb41e52520 100644 (file)
--- a/Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp
+++ b/Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp
@@ -94,21 +94,19 @@ void ContentLayerChromium::cleanupResources()
 
 void ContentLayerChromium::paintContentsIfDirty()
 {
-    ASSERT(drawsContent());
-
     updateTileSizeAndTilingOption();
 
-    const IntRect& layerRect = visibleLayerRect();
-    if (layerRect.isEmpty())
-        return;
+    IntRect layerRect;
+
+    // Always call prepareToUpdate() but with an empty layer rectangle when
+    // layer doesn't draw contents.
+    if (drawsContent())
+        layerRect = visibleLayerRect();
 
     IntRect dirty = enclosingIntRect(m_dirtyRect);
     dirty.intersect(IntRect(IntPoint(), contentBounds()));
     invalidateRect(dirty);
 
-    if (!drawsContent())
-        return;
-
     prepareToUpdate(layerRect);
     resetNeedsDisplay();
 }

diff --git a/Source/WebCore/platform/graphics/chromium/VideoLayerChromium.cpp b/Source/WebCore/platform/graphics/chromium/VideoLayerChromium.cpp
index 2394568f2d8278f5979a7fe0c5615d9f72098b67..e6bc77553b1c4f0db20a0798e70414f8a5beea1f 100644 (file)
--- a/Source/WebCore/platform/graphics/chromium/VideoLayerChromium.cpp
+++ b/Source/WebCore/platform/graphics/chromium/VideoLayerChromium.cpp
@@ -79,7 +79,7 @@ void VideoLayerChromium::cleanupResources()
 
 void VideoLayerChromium::updateCompositorResources(GraphicsContext3D* context, CCTextureUpdater& updater)
 {
-    if (!m_delegate || !m_provider)
+    if (!m_delegate || !m_provider || !drawsContent())
         return;
 
     if (m_dirtyRect.isEmpty() && texturesValid()) {
@@ -93,8 +93,6 @@ void VideoLayerChromium::updateCompositorResources(GraphicsContext3D* context, C
         return;
     }
 
-    ASSERT(drawsContent());
-
     m_planes = 0;
     m_skipsDraw = false;
 

diff --git a/Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp b/Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp
index 498e917dd0b3c6774b1230e1314d8f551b5e1444..bae6fb3ce934fdb4c3f7c5979a0a0bfdb46905b5 100644 (file)
--- a/Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp
+++ b/Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp
@@ -333,12 +333,6 @@ void CCLayerTreeHost::updateLayers(LayerChromium* rootLayer)
     paintLayerContents(m_updateList);
 }
 
-static void paintContentsIfDirty(LayerChromium* layer)
-{
-    if (layer->drawsContent())
-        layer->paintContentsIfDirty();
-}
-
 void CCLayerTreeHost::paintMaskAndReplicaForRenderSurface(LayerChromium* renderSurfaceLayer)
 {
     // Note: Masks and replicas only exist for layers that own render surfaces. If we reach this point
@@ -348,19 +342,19 @@ void CCLayerTreeHost::paintMaskAndReplicaForRenderSurface(LayerChromium* renderS
     if (renderSurfaceLayer->maskLayer()) {
         renderSurfaceLayer->maskLayer()->setLayerTreeHost(this);
         renderSurfaceLayer->maskLayer()->setVisibleLayerRect(IntRect(IntPoint(), renderSurfaceLayer->contentBounds()));
-        paintContentsIfDirty(renderSurfaceLayer->maskLayer());
+        renderSurfaceLayer->maskLayer()->paintContentsIfDirty();
     }
 
     LayerChromium* replicaLayer = renderSurfaceLayer->replicaLayer();
     if (replicaLayer) {
 
         replicaLayer->setLayerTreeHost(this);
-        paintContentsIfDirty(replicaLayer);
+        replicaLayer->paintContentsIfDirty();
 
         if (replicaLayer->maskLayer()) {
             replicaLayer->maskLayer()->setLayerTreeHost(this);
             replicaLayer->maskLayer()->setVisibleLayerRect(IntRect(IntPoint(), replicaLayer->maskLayer()->contentBounds()));
-            paintContentsIfDirty(replicaLayer->maskLayer());
+            replicaLayer->maskLayer()->paintContentsIfDirty();
         }
     }
 }
@@ -392,7 +386,7 @@ void CCLayerTreeHost::paintLayerContents(const LayerList& renderSurfaceLayerList
             ASSERT(layer->opacity());
             ASSERT(!layer->bounds().isEmpty());
 
-            paintContentsIfDirty(layer);
+            layer->paintContentsIfDirty();
         }
     }
 }
@@ -407,13 +401,13 @@ void CCLayerTreeHost::updateCompositorResources(GraphicsContext3D* context, CCTe
         ASSERT(renderSurface->drawOpacity());
 
         if (renderSurfaceLayer->maskLayer())
-            updateCompositorResources(renderSurfaceLayer->maskLayer(), context, updater);
+            renderSurfaceLayer->maskLayer()->updateCompositorResources(context, updater);
 
         if (renderSurfaceLayer->replicaLayer()) {
-            updateCompositorResources(renderSurfaceLayer->replicaLayer(), context, updater);
+            renderSurfaceLayer->replicaLayer()->updateCompositorResources(context, updater);
             
             if (renderSurfaceLayer->replicaLayer()->maskLayer())
-                updateCompositorResources(renderSurfaceLayer->replicaLayer()->maskLayer(), context, updater);
+                renderSurfaceLayer->replicaLayer()->maskLayer()->updateCompositorResources(context, updater);
         }
         
         const LayerList& layerList = renderSurface->layerList();
@@ -423,21 +417,11 @@ void CCLayerTreeHost::updateCompositorResources(GraphicsContext3D* context, CCTe
             if (layer->renderSurface() && layer->renderSurface() != renderSurface)
                 continue;
 
-            updateCompositorResources(layer, context, updater);
+            layer->updateCompositorResources(context, updater);
         }
     }
 }
 
-void CCLayerTreeHost::updateCompositorResources(LayerChromium* layer, GraphicsContext3D* context, CCTextureUpdater& updater)
-{
-    // For normal layers, these conditions should have already been checked while creating the render surface layer lists.
-    // For masks and replicas however, we may still need to check them here.
-    if (layer->bounds().isEmpty() || !layer->opacity() || !layer->drawsContent())
-        return;
-
-    layer->updateCompositorResources(context, updater);
-}
-
 void CCLayerTreeHost::clearPendingUpdate()
 {
     for (size_t surfaceIndex = 0; surfaceIndex < m_updateList.size(); ++surfaceIndex) {

diff --git a/Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.h b/Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.h
index d814149c54b053ecea6fcae05624448dd311d911..93dd98c41d78eb304ca3031b2f5519d8e6c62c35 100644 (file)
--- a/Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.h
+++ b/Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.h
@@ -193,7 +193,6 @@ private:
     void paintMaskAndReplicaForRenderSurface(LayerChromium*);
 
     void updateLayers(LayerChromium*);
-    void updateCompositorResources(LayerChromium*, GraphicsContext3D*, CCTextureUpdater&);
     void clearPendingUpdate();
 
     int m_compositorIdentifier;

diff --git a/Source/WebKit/chromium/ChangeLog b/Source/WebKit/chromium/ChangeLog
index 912884a3be7fe8705269bcb16d89df9786a63427..93007e7889e4aef83523cd37a603d0bf5416c1de 100644 (file)
--- a/Source/WebKit/chromium/ChangeLog
+++ b/Source/WebKit/chromium/ChangeLog
@@ -1,3 +1,35 @@
+2011-11-17  David Reveman  <reveman@chromium.org>
+
+        [Chromium] Calls to paintContentsIfDirty() and updateCompositorResources() should be balanced.
+        https://bugs.webkit.org/show_bug.cgi?id=72630
+
+        Reviewed by James Robinson.
+
+        Add test that check if calls to paintContentsIfDirty() and
+        updateCompositorResources() are balanced when layer opacity
+        changes during paintContent().
+
+        * tests/CCLayerTreeHostTest.cpp:
+        (WTF::CCLayerTreeHostTest::layerTreeHost):
+        (WTF::TestOpacityChangeLayerDelegate::TestOpacityChangeLayerDelegate):
+        (WTF::TestOpacityChangeLayerDelegate::paintContents):
+        (WTF::TestOpacityChangeLayerDelegate::drawsContent):
+        (WTF::TestOpacityChangeLayerDelegate::preserves3D):
+        (WTF::TestOpacityChangeLayerDelegate::notifySyncRequired):
+        (WTF::ContentLayerChromiumWithUpdateTracking::create):
+        (WTF::ContentLayerChromiumWithUpdateTracking::paintContentsCount):
+        (WTF::ContentLayerChromiumWithUpdateTracking::resetPaintContentsCount):
+        (WTF::ContentLayerChromiumWithUpdateTracking::updateCount):
+        (WTF::ContentLayerChromiumWithUpdateTracking::resetUpdateCount):
+        (WTF::ContentLayerChromiumWithUpdateTracking::paintContentsIfDirty):
+        (WTF::ContentLayerChromiumWithUpdateTracking::updateCompositorResources):
+        (WTF::ContentLayerChromiumWithUpdateTracking::ContentLayerChromiumWithUpdateTracking):
+        (WTF::CCLayerTreeHostTestOpacityChange::CCLayerTreeHostTestOpacityChange):
+        (WTF::CCLayerTreeHostTestOpacityChange::beginTest):
+        (WTF::CCLayerTreeHostTestOpacityChange::commitCompleteOnCCThread):
+        (WTF::CCLayerTreeHostTestOpacityChange::afterTest):
+        (WTF::TEST_F):
+
 2011-11-17  Peter Kasting  <pkasting@google.com>
 
         Unreviewed, rolling out r100676.

diff --git a/Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp b/Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp
index d1dcf5d866f2f15c10e25af2faee215be93b2779..21315a0e5071a36ddc002072f9cc2752b9996f6b 100644 (file)
--- a/Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp
+++ b/Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp
@@ -26,9 +26,11 @@
 
 #include "cc/CCLayerTreeHost.h"
 
+#include "ContentLayerChromium.h"
 #include "cc/CCLayerImpl.h"
 #include "cc/CCLayerTreeHostImpl.h"
 #include "cc/CCScopedThreadProxy.h"
+#include "cc/CCTextureUpdater.h"
 #include "cc/CCThreadTask.h"
 #include "GraphicsContext3DPrivate.h"
 #include <gtest/gtest.h>
@@ -227,6 +229,8 @@ public:
         m_timeoutTask = 0;
     }
 
+    CCLayerTreeHost* layerTreeHost() { return m_layerTreeHost.get(); }
+
 
 protected:
     CCLayerTreeHostTest()
@@ -826,4 +830,103 @@ TEST_F(CCLayerTreeHostTestSetVisible, runMultiThread)
     runTest(true);
 }
 
+class TestOpacityChangeLayerDelegate : public CCLayerDelegate {
+public:
+    TestOpacityChangeLayerDelegate(CCLayerTreeHostTest* test)
+        : m_test(test)
+    {
+    }
+
+    virtual void paintContents(GraphicsContext&, const IntRect&)
+    {
+        // Set layer opacity to 0.
+        m_test->layerTreeHost()->rootLayer()->setOpacity(0);
+    }
+
+    virtual bool drawsContent() const { return true; }
+    virtual bool preserves3D() { return false; }
+    virtual void notifySyncRequired() { }
+
+private:
+    CCLayerTreeHostTest* m_test;
+};
+
+class ContentLayerChromiumWithUpdateTracking : public ContentLayerChromium {
+public:
+    static PassRefPtr<ContentLayerChromiumWithUpdateTracking> create(CCLayerDelegate *delegate) { return adoptRef(new ContentLayerChromiumWithUpdateTracking(delegate)); }
+
+    int paintContentsCount() { return m_paintContentsCount; }
+    void resetPaintContentsCount() { m_paintContentsCount = 0; }
+
+    int updateCount() { return m_updateCount; }
+    void resetUpdateCount() { m_updateCount = 0; }
+
+    virtual void paintContentsIfDirty()
+    {
+        ContentLayerChromium::paintContentsIfDirty();
+        m_paintContentsCount++;
+    }
+
+    virtual void updateCompositorResources(GraphicsContext3D* context, CCTextureUpdater& updater)
+    {
+        ContentLayerChromium::updateCompositorResources(context, updater);
+        m_updateCount++;
+    }
+
+private:
+    explicit ContentLayerChromiumWithUpdateTracking(CCLayerDelegate *delegate)
+        : ContentLayerChromium(delegate)
+        , m_paintContentsCount(0)
+        , m_updateCount(0)
+    {
+        setBounds(IntSize(10, 10));
+    }
+
+    int m_paintContentsCount;
+    int m_updateCount;
+};
+
+// Layer opacity change during paint should not prevent compositor resources from being updated during commit.
+class CCLayerTreeHostTestOpacityChange : public CCLayerTreeHostTest {
+public:
+    CCLayerTreeHostTestOpacityChange()
+        : m_testOpacityChangeDelegate(this)
+        , m_updateCheckLayer(ContentLayerChromiumWithUpdateTracking::create(&m_testOpacityChangeDelegate))
+    {
+    }
+
+    virtual void beginTest()
+    {
+        m_layerTreeHost->setRootLayer(m_updateCheckLayer);
+        m_layerTreeHost->setViewport(IntSize(10, 10));
+
+        postSetNeedsCommitToMainThread();
+    }
+
+    virtual void commitCompleteOnCCThread(CCLayerTreeHostImpl*)
+    {
+        endTest();
+    }
+
+    virtual void afterTest()
+    {
+        // paintContentsIfDirty() should have been called once.
+        EXPECT_EQ(1, m_updateCheckLayer->paintContentsCount());
+
+        // updateCompositorResources() should have been called the same
+        // amout of times as paintContentsIfDirty().
+        EXPECT_EQ(m_updateCheckLayer->paintContentsCount(),
+                  m_updateCheckLayer->updateCount());
+    }
+
+private:
+    TestOpacityChangeLayerDelegate m_testOpacityChangeDelegate;
+    RefPtr<ContentLayerChromiumWithUpdateTracking> m_updateCheckLayer;
+};
+
+TEST_F(CCLayerTreeHostTestOpacityChange, runMultiThread)
+{
+    runTest(true);
+}
+
 } // namespace