Bug 82486 – [chromium] Fix tiled layer assert for huge layers

• Component: New Bugs
• Status: RESOLVED FIXED

[Adrienne Walker]

[chromium] Fix tiled layer assert for huge layers

[Adrienne Walker]

Created attachment 134344 [details]
Patch

[Adrienne Walker]

In running DRT with --enable-threaded-compositing, TilingData::assertTile was being hit sporadically from compositing/tiling/crash-huge-layer.html being loaded, but then trying to paint while some other test loaded.

In single-treaded compositing DRT, we never tried to update this layer, so it wouldn't crash.  Sad trombone noise.

[Dana Jansens]

Comment on attachment 134344 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=134344&action=review

woo

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:735
> +        if ((drawTransformIsAnimating() || screenSpaceTransformIsAnimating()) && m_tiler->numTilesX() <= 4 && m_tiler->numTilesY() <= 4)

s/4/3/ to not cause this to prepaint more tiles than before. Though it would instead paint less..

bool hasFewTiles = m_tiler->numTilesX() <= 9 && m_tiler->numTilesY() <= 9 && m_tiler->numTilesX() * m_tiler->numTilesY() <= 9; ?

> Source/WebCore/platform/graphics/gpu/TilingData.h:54
> +    int numTiles() const;

meant to remove this line?

[Adrienne Walker]

(In reply to comment #3)
> (From update of attachment 134344 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=134344&action=review
> 
> woo
> 
> > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:735
> > +        if ((drawTransformIsAnimating() || screenSpaceTransformIsAnimating()) && m_tiler->numTilesX() <= 4 && m_tiler->numTilesY() <= 4)
> 
> s/4/3/ to not cause this to prepaint more tiles than before. Though it would instead paint less..
> 
> bool hasFewTiles = m_tiler->numTilesX() <= 9 && m_tiler->numTilesY() <= 9 && m_tiler->numTilesX() * m_tiler->numTilesY() <= 9; ?

In some cases it would paint more (4x3 layer would get painted, but wouldn't before) but also paint less (9x1 layer wouldn't get painted, but would before).  I think this is a more reasonable bound on the amount of prepainting per frame, but don't feel too strongly about it.

> > Source/WebCore/platform/graphics/gpu/TilingData.h:54
> > +    int numTiles() const;
> 
> meant to remove this line?

OOPS

[Dana Jansens]

Comment on attachment 134344 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=134344&action=review

>>> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:735
>>> +        if ((drawTransformIsAnimating() || screenSpaceTransformIsAnimating()) && m_tiler->numTilesX() <= 4 && m_tiler->numTilesY() <= 4)
>> 
>> s/4/3/ to not cause this to prepaint more tiles than before. Though it would instead paint less..
>> 
>> bool hasFewTiles = m_tiler->numTilesX() <= 9 && m_tiler->numTilesY() <= 9 && m_tiler->numTilesX() * m_tiler->numTilesY() <= 9; ?
> 
> In some cases it would paint more (4x3 layer would get painted, but wouldn't before) but also paint less (9x1 layer wouldn't get painted, but would before).  I think this is a more reasonable bound on the amount of prepainting per frame, but don't feel too strongly about it.

I know Eric was concerned about limiting it to not negatively affect android, and we came up with ~9 tiles as a compromise. Here <= 4 && <= 4 allows 16 tiles which is almost double the amount. I'd rather decrease it than increase it right now.

[Adrienne Walker]

Created attachment 134357 [details]
Remove numTiles declaration

[Adrienne Walker]

Created attachment 134360 [details]
Revert prepainting change

[Dana Jansens]

Comment on attachment 134360 [details]
Revert prepainting change

Thanks :) lgtm

[James Robinson]

Comment on attachment 134360 [details]
Revert prepainting change

View in context: https://bugs.webkit.org/attachment.cgi?id=134360&action=review

OK

> Source/WebKit/chromium/tests/TilingDataTest.cpp:55
> +        ASSERT(numTiles >= 0);

in a unit test we should probably use ASSERT_TRUE() from gtest and not ASSERT() from wtf/ so it's still compiled in for release builds and produces gtest-style failures

[Adrienne Walker]

(In reply to comment #9)
> (From update of attachment 134360 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=134360&action=review
> 
> OK
> 
> > Source/WebKit/chromium/tests/TilingDataTest.cpp:55
> > +        ASSERT(numTiles >= 0);
> 
> in a unit test we should probably use ASSERT_TRUE() from gtest and not ASSERT() from wtf/ so it's still compiled in for release builds and produces gtest-style failures

You can't call gtest asserts from within a non-void function.  I'll make them EXPECT instead.

[Adrienne Walker]

Committed r112432: <http://trac.webkit.org/changeset/112432>

[Adrienne Walker]

Rolled out in http://trac.webkit.org/changeset/112449.

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpectations=true&tests=css3%2Ffilters%2Fcustom%2Fcustom-filter-shader-cache.html%2Ccss3%2Ffilters%2Fcustom%2Feffect-custom-combined-missing.html%2Ccss3%2Ffilters%2Fcustom%2Feffect-custom-parameters.html%2Ccss3%2Ffilters%2Fcustom%2Feffect-custom.html

[Adrienne Walker]

Created attachment 134436 [details]
Now with fewer test crashes

[Adrienne Walker]

diff --git a/Source/WebCore/platform/graphics/gpu/Texture.cpp b/Source/WebCore/platform/graphics/gpu/Texture.cpp
index 3cad13a..7c2b86e 100644
--- a/Source/WebCore/platform/graphics/gpu/Texture.cpp
+++ b/Source/WebCore/platform/graphics/gpu/Texture.cpp
@@ -92,8 +92,8 @@ PassRefPtr<Texture> Texture::create(GraphicsContext3D* context, Format format, i
     TilingData tiling(maxTextureSize, width, height, true);
 
     // Check for overflow.
-    int numTiles = width * height;
-    if (numTiles / width != height) {
+    int numTiles = tiling.numTilesX() * tiling.numTilesY();
+    if (numTiles / tiling.numTilesX() != tiling.numTilesY()) {
         tiling.setTotalSize(0, 0);
         numTiles = 0;
     }
@@ -110,8 +110,8 @@ PassRefPtr<Texture> Texture::create(GraphicsContext3D* context, Format format, i
         }
         textureIds->at(i) = textureId;
 
-        int xIndex = i % width;
-        int yIndex = i / width;
+        int xIndex = i % tiling.numTilesX();
+        int yIndex = i / tiling.numTilesX();
         IntRect tileBoundsWithBorder = tiling.tileBoundsWithBorder(xIndex, yIndex);
 
         unsigned int glFormat = 0;
@@ -177,8 +177,8 @@ void Texture::updateSubRect(void* pixels, const IntRect& updateRect)
     OwnArrayPtr<uint32_t> tempBuff = adoptArrayPtr(new uint32_t[tempBuffSize]);
 
     for (int tile = 0; tile < m_tiles.numTilesX() * m_tiles.numTilesY(); tile++) {
-        int xIndex = tile % m_tiles.totalSizeX();
-        int yIndex = tile / m_tiles.totalSizeX();
+        int xIndex = tile % m_tiles.numTilesX();
+        int yIndex = tile / m_tiles.numTilesX();
 
         // Intersect with tile
         IntRect tileBoundsWithBorder = m_tiles.tileBoundsWithBorder(xIndex, yIndex);
diff --git a/Source/WebKit/chromium/tests/TilingDataTest.cpp b/Source/WebKit/chromium/tests/TilingDataTest.cpp
index f01e25c..47c0361 100755
--- a/Source/WebKit/chromium/tests/TilingDataTest.cpp
+++ b/Source/WebKit/chromium/tests/TilingDataTest.cpp
@@ -52,9 +52,9 @@ public:
         int numTiles = numTilesX() * numTilesY();
 
         // Assert no overflow.
-        ASSERT(numTiles >= 0);
+        EXPECT_GE(numTiles, 0);
         if (numTiles > 0)
-            ASSERT(numTiles / numTilesX() == numTilesY());
+            EXPECT_EQ(numTiles / numTilesX(), numTilesY());
 
         return numTiles;
     }

[WebKit Review Bot]

Comment on attachment 134436 [details]
Now with fewer test crashes

Clearing flags on attachment: 134436

Committed r112481: <http://trac.webkit.org/changeset/112481>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.