WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Remove numTiles declaration
(61.54 KB, patch)
2012-03-28 11:56 PDT
,
Adrienne Walker
no flags
Details
Formatted Diff
Diff
Revert prepainting change
(61.64 KB, patch)
2012-03-28 12:01 PDT
,
Adrienne Walker
no flags
Details
Formatted Diff
Diff
Now with fewer test crashes
(61.66 KB, patch)
2012-03-28 16:15 PDT
,
Adrienne Walker
no flags
Details
Formatted Diff
Diff
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
Adrienne Walker
Comment 1
2012-03-28 11:15:32 PDT
Created
attachment 134344
[details]
Patch
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
Committed
r112432
: <
http://trac.webkit.org/changeset/112432
>
Adrienne Walker
Comment 12
2012-03-28 15:09:34 PDT
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
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.
Top of Page
Format For Printing
XML
Clone This Bug