RESOLVED FIXED 82486
[chromium] Fix tiled layer assert for huge layers
https://bugs.webkit.org/show_bug.cgi?id=82486
Summary [chromium] Fix tiled layer assert for huge layers
Adrienne Walker
Reported 2012-03-28 11:06:50 PDT
[chromium] Fix tiled layer assert for huge layers
Attachments
Patch (60.79 KB, patch)
2012-03-28 11:15 PDT, Adrienne Walker
no flags
Remove numTiles declaration (61.54 KB, patch)
2012-03-28 11:56 PDT, Adrienne Walker
no flags
Revert prepainting change (61.64 KB, patch)
2012-03-28 12:01 PDT, Adrienne Walker
no flags
Now with fewer test crashes (61.66 KB, patch)
2012-03-28 16:15 PDT, Adrienne Walker
no flags
Adrienne Walker
Comment 1 2012-03-28 11:15:32 PDT
Adrienne Walker
Comment 2 2012-03-28 11:19:13 PDT
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 3 2012-03-28 11:38:36 PDT
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
Comment 4 2012-03-28 11:52:51 PDT
(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 5 2012-03-28 11:56:28 PDT
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
Comment 6 2012-03-28 11:56:47 PDT
Created attachment 134357 [details] Remove numTiles declaration
Adrienne Walker
Comment 7 2012-03-28 12:01:18 PDT
Created attachment 134360 [details] Revert prepainting change
Dana Jansens
Comment 8 2012-03-28 12:13:34 PDT
Comment on attachment 134360 [details] Revert prepainting change Thanks :) lgtm
James Robinson
Comment 9 2012-03-28 13:05:11 PDT
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
Comment 10 2012-03-28 13:10:10 PDT
(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
Comment 11 2012-03-28 13:20:44 PDT
Adrienne Walker
Comment 13 2012-03-28 16:15:35 PDT
Created attachment 134436 [details] Now with fewer test crashes
Adrienne Walker
Comment 14 2012-03-28 16:17:13 PDT
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 15 2012-03-28 17:56:33 PDT
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
Comment 16 2012-03-28 17:56:38 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.