[chromium] Fix tiled layer assert for huge layers
Created attachment 134344 [details] Patch
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.
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?
(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
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.
Created attachment 134357 [details] Remove numTiles declaration
Created attachment 134360 [details] Revert prepainting change
Comment on attachment 134360 [details] Revert prepainting change Thanks :) lgtm
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
(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.
Committed r112432: <http://trac.webkit.org/changeset/112432>
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
Created attachment 134436 [details] Now with fewer test crashes
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; }
Comment on attachment 134436 [details] Now with fewer test crashes Clearing flags on attachment: 134436 Committed r112481: <http://trac.webkit.org/changeset/112481>
All reviewed patches have been landed. Closing bug.