[chromium] When prepainting fails, tiles dirty rects may be cleared
authorcommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 24 Mar 2012 05:54:38 +0000 (05:54 +0000)
committercommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 24 Mar 2012 05:54:38 +0000 (05:54 +0000)
https://bugs.webkit.org/show_bug.cgi?id=82107

Patch by Dana Jansens <danakj@chromium.org> on 2012-03-23
Reviewed by Adrienne Walker.

Source/WebCore:

When prepainting, if a tile is unable to be reserved due to memory
limits, we bail out of prepareToUpdateTiles. But we would have
cleared the dirty rect of any previous tiles. This leaves them
in a bad state where their textures are reserved, but their textureIds
are set to 0, and they are not marked dirty. This means that they will
not be updated and displayed if they become visible, since it is
assumed that valid textures with zero textureId must have a dirty
region.

We fix this by not clearing the dirty rects until we know we are
going to update the layer.

Unit test: TiledLayerChromiumTest.pushTilesAfterIdlePaintFailed

* platform/graphics/chromium/TiledLayerChromium.cpp:
(WebCore::TiledLayerChromium::prepareToUpdateTiles):
* platform/graphics/chromium/cc/CCTiledLayerImpl.cpp:
(WebCore::CCTiledLayerImpl::hasTextureIdForTileAt):
(WebCore):
* platform/graphics/chromium/cc/CCTiledLayerImpl.h:
(CCTiledLayerImpl):

Source/WebKit/chromium:

* tests/TiledLayerChromiumTest.cpp:
(WTF::FakeTextureAllocator::createTexture):
(WTF::FakeLayerTextureUpdater::Texture::updateRect):
(FakeCCTiledLayerImpl):
(WTF::FakeCCTiledLayerImpl::hasTextureIdForTileAt):
(WTF::TEST):
(WTF):

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

Source/WebCore/ChangeLog
Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp
Source/WebCore/platform/graphics/chromium/cc/CCTiledLayerImpl.cpp
Source/WebCore/platform/graphics/chromium/cc/CCTiledLayerImpl.h
Source/WebKit/chromium/ChangeLog
Source/WebKit/chromium/tests/TiledLayerChromiumTest.cpp

index 71fddd5..138d641 100644 (file)
@@ -1,3 +1,32 @@
+2012-03-23  Dana Jansens  <danakj@chromium.org>
+
+        [chromium] When prepainting fails, tiles dirty rects may be cleared
+        https://bugs.webkit.org/show_bug.cgi?id=82107
+
+        Reviewed by Adrienne Walker.
+
+        When prepainting, if a tile is unable to be reserved due to memory
+        limits, we bail out of prepareToUpdateTiles. But we would have
+        cleared the dirty rect of any previous tiles. This leaves them
+        in a bad state where their textures are reserved, but their textureIds
+        are set to 0, and they are not marked dirty. This means that they will
+        not be updated and displayed if they become visible, since it is
+        assumed that valid textures with zero textureId must have a dirty
+        region.
+
+        We fix this by not clearing the dirty rects until we know we are
+        going to update the layer.
+
+        Unit test: TiledLayerChromiumTest.pushTilesAfterIdlePaintFailed
+
+        * platform/graphics/chromium/TiledLayerChromium.cpp:
+        (WebCore::TiledLayerChromium::prepareToUpdateTiles):
+        * platform/graphics/chromium/cc/CCTiledLayerImpl.cpp:
+        (WebCore::CCTiledLayerImpl::hasTextureIdForTileAt):
+        (WebCore):
+        * platform/graphics/chromium/cc/CCTiledLayerImpl.h:
+        (CCTiledLayerImpl):
+
 2012-03-23  Stephanie Lewis  <slewis@apple.com>
 
         https://bugs.webkit.org/show_bug.cgi?id=81963 WebProcess can get stuck in GC during many low memory signals.
index 581022e..af594d4 100644 (file)
@@ -467,7 +467,16 @@ void TiledLayerChromium::prepareToUpdateTiles(bool idle, int left, int top, int
             }
 
             dirtyLayerRect.unite(tile->m_dirtyRect);
-            tile->copyAndClearDirty();
+        }
+    }
+
+    // For tiles that were not culled, we are going to update the area currently marked as dirty. So
+    // clear that dirty area and mark it for update instead.
+    for (int j = top; j <= bottom; ++j) {
+        for (int i = left; i <= right; ++i) {
+            UpdatableTile* tile = tileAt(i, j);
+            if (tile->m_updated)
+                tile->copyAndClearDirty();
         }
     }
 
index 560e33a..0517ef0 100644 (file)
@@ -101,6 +101,11 @@ bool CCTiledLayerImpl::hasTileAt(int i, int j) const
     return m_tiler->tileAt(i, j);
 }
 
+bool CCTiledLayerImpl::hasTextureIdForTileAt(int i, int j) const
+{
+    return hasTileAt(i, j) && tileAt(i, j)->textureId();
+}
+
 DrawableTile* CCTiledLayerImpl::tileAt(int i, int j) const
 {
     return static_cast<DrawableTile*>(m_tiler->tileAt(i, j));
index 30a4aa6..68a1db1 100644 (file)
@@ -75,6 +75,7 @@ protected:
     explicit CCTiledLayerImpl(int id);
     // Exposed for testing.
     bool hasTileAt(int, int) const;
+    bool hasTextureIdForTileAt(int, int) const;
 
     virtual TransformationMatrix quadTransform() const;
 
index ac32cc1..840c6ac 100644 (file)
@@ -1,3 +1,18 @@
+2012-03-23  Dana Jansens  <danakj@chromium.org>
+
+        [chromium] When prepainting fails, tiles dirty rects may be cleared
+        https://bugs.webkit.org/show_bug.cgi?id=82107
+
+        Reviewed by Adrienne Walker.
+
+        * tests/TiledLayerChromiumTest.cpp:
+        (WTF::FakeTextureAllocator::createTexture):
+        (WTF::FakeLayerTextureUpdater::Texture::updateRect):
+        (FakeCCTiledLayerImpl):
+        (WTF::FakeCCTiledLayerImpl::hasTextureIdForTileAt):
+        (WTF::TEST):
+        (WTF):
+
 2012-03-23  W. James MacLean  <wjmaclean@chromium.org>
 
         [chromium] CCLayerTreeHostImpl::scrollBegin() should return ScrollFailed for CCInputHandlerClient::Gesture type when wheel handlers found.
index 25e761b..06a9a97 100644 (file)
@@ -69,7 +69,7 @@ private:
 
 class FakeTextureAllocator : public TextureAllocator {
 public:
-    virtual unsigned createTexture(const IntSize&, GC3Denum) { return 0; }
+    virtual unsigned createTexture(const IntSize&, GC3Denum) { return 1; }
     virtual void deleteTexture(unsigned, const IntSize&, GC3Denum) { }
 };
 
@@ -86,7 +86,12 @@ public:
         }
         virtual ~Texture() { }
 
-        virtual void updateRect(GraphicsContext3D*, TextureAllocator*, const IntRect&, const IntRect&) { m_layer->updateRect(); }
+        virtual void updateRect(GraphicsContext3D*, TextureAllocator* allocator, const IntRect&, const IntRect&)
+        {
+            if (allocator)
+                texture()->allocate(allocator);
+            m_layer->updateRect();
+        }
         virtual void prepareRect(const IntRect&) { m_layer->prepareRect(); }
 
     private:
@@ -144,10 +149,8 @@ public:
         : CCTiledLayerImpl(id) { }
     virtual ~FakeCCTiledLayerImpl() { }
 
-    bool hasTileAt(int i, int j)
-    {
-        return CCTiledLayerImpl::hasTileAt(i, j);
-    }
+    using CCTiledLayerImpl::hasTileAt;
+    using CCTiledLayerImpl::hasTextureIdForTileAt;
 };
 
 class FakeTiledLayerChromium : public TiledLayerChromium {
@@ -434,6 +437,88 @@ TEST(TiledLayerChromiumTest, pushIdlePaintTiles)
     }
 }
 
+TEST(TiledLayerChromiumTest, pushTilesAfterIdlePaintFailed)
+{
+    OwnPtr<TextureManager> textureManager = TextureManager::create(1024*1024, 1024*1024, 1024);
+    DebugScopedSetImplThread implThread;
+    RefPtr<FakeTiledLayerChromium> layer1 = adoptRef(new FakeTiledLayerChromium(textureManager.get()));
+    OwnPtr<FakeCCTiledLayerImpl> layerImpl1(adoptPtr(new FakeCCTiledLayerImpl(0)));
+    RefPtr<FakeTiledLayerChromium> layer2 = adoptRef(new FakeTiledLayerChromium(textureManager.get()));
+    OwnPtr<FakeCCTiledLayerImpl> layerImpl2(adoptPtr(new FakeCCTiledLayerImpl(0)));
+
+    FakeTextureAllocator textureAllocator;
+    CCTextureUpdater updater(&textureAllocator);
+
+    // For this test we have two layers. layer1 exhausts most texture memory, leaving room for 2 more tiles from
+    // layer2, but not all three tiles. First we paint layer1, and one tile from layer2. Then when we idle paint
+    // layer2, we will fail on the third tile of layer2, and this should not leave the second tile in a bad state.
+
+    // This requires 4*30000 bytes of memory.
+    IntRect layer2Rect(0, 0, 100, 300);
+    layer2->setBounds(layer2Rect.size());
+    layer2->setVisibleLayerRect(layer2Rect);
+    layer2->invalidateRect(layer2Rect);
+
+    // This uses 960000 bytes, leaving 88576 bytes of memory left, which is enough for 2 tiles only in the other layer.
+    IntRect layerRect(IntPoint::zero(), IntSize(100, 2400));
+    layer1->setBounds(layerRect.size());
+    layer1->setVisibleLayerRect(layerRect);
+    layer1->invalidateRect(layerRect);
+    layer1->prepareToUpdate(layerRect, 0);
+
+    // Paint a single tile in layer2 so that it will idle paint.
+    layer2->prepareToUpdate(IntRect(0, 0, 100, 100), 0);
+
+    // We should need idle-painting for both remaining tiles in layer2.
+    EXPECT_TRUE(layer2->needsIdlePaint(layer2Rect));
+
+    // Commit the frame over to impl.
+    layer1->updateCompositorResources(0, updater);
+    layer2->updateCompositorResources(0, updater);
+    updater.update(0, 5000);
+    layer1->pushPropertiesTo(layerImpl1.get());
+    layer2->pushPropertiesTo(layerImpl2.get());
+
+    // Now idle paint layer2. We are going to run out of memory though!
+    layer2->prepareToUpdate(IntRect(0, 0, 100, 100), 0);
+    layer2->prepareToUpdateIdle(layer2Rect, 0);
+
+    // Oh well, commit the frame and push.
+    layer1->updateCompositorResources(0, updater);
+    layer2->updateCompositorResources(0, updater);
+    updater.update(0, 5000);
+    layer1->pushPropertiesTo(layerImpl1.get());
+    layer2->pushPropertiesTo(layerImpl2.get());
+
+    // Sanity check, we should have textures for the big layer.
+    EXPECT_TRUE(layerImpl1->hasTextureIdForTileAt(0, 0));
+
+    // We should only have the first tile from layer2 since it failed to idle update.
+    EXPECT_TRUE(layerImpl2->hasTileAt(0, 0));
+    EXPECT_TRUE(layerImpl2->hasTextureIdForTileAt(0, 0));
+    EXPECT_FALSE(layerImpl2->hasTileAt(0, 1));
+    EXPECT_FALSE(layerImpl2->hasTileAt(0, 2));
+
+    // Now if layer2 becomes fully visible, we should be able to paint it and push valid textures.
+    textureManager->unprotectAllTextures();
+
+    layer2->prepareToUpdate(layer2Rect, 0);
+    layer1->prepareToUpdate(IntRect(), 0);
+
+    layer1->updateCompositorResources(0, updater);
+    layer2->updateCompositorResources(0, updater);
+    updater.update(0, 5000);
+    layer1->pushPropertiesTo(layerImpl1.get());
+    layer2->pushPropertiesTo(layerImpl2.get());
+
+    EXPECT_TRUE(layerImpl2->hasTileAt(0, 0));
+    EXPECT_TRUE(layerImpl2->hasTileAt(0, 1));
+    EXPECT_TRUE(layerImpl2->hasTileAt(0, 2));
+    EXPECT_TRUE(layerImpl2->hasTextureIdForTileAt(0, 0));
+    EXPECT_TRUE(layerImpl2->hasTextureIdForTileAt(0, 1));
+    EXPECT_TRUE(layerImpl2->hasTextureIdForTileAt(0, 2));
+}
+
 TEST(TiledLayerChromiumTest, pushIdlePaintedOccludedTiles)
 {
     OwnPtr<TextureManager> textureManager = TextureManager::create(4*1024*1024, 2*1024*1024, 1024);