[chromium] TilingData mishandles very small texture sizes
Created attachment 89250 [details] Patch
Comment on attachment 89250 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=89250&action=review I don't think this is the right way to fix the problem -- see notes and suggested test cases. > Source/WebCore/platform/graphics/gpu/TilingData.cpp:69 > + m_maxTextureSize = max(3, maxTextureSize); I think we must respect max texture size. If a max is passed in that's too restrictive to function, tiling data should hand out 0s for the computation accessors such as numTiles(). If you did perform logic here, the case of no border texels should allow small maximums. > Source/WebKit/chromium/tests/TilingDataTest.cpp:56 > + EXPECT_EQ(0, TilingData(4, 0, 4, false).numTiles()); EXPECT_EQ(0, TilingData(-10, 1, 1, false).numTiles()); EXPECT_EQ(0, TilingData(-1, 1, 1, false).numTiles()); EXPECT_EQ(0, TilingData(0, 1, 1, false).numTiles()); > Source/WebKit/chromium/tests/TilingDataTest.cpp:58 > + EXPECT_EQ(1, TilingData(1, 1, 1, false).numTiles()); EXPECT_EQ(2, TilingData(1, 1, 2, false).numTiles()); > Source/WebKit/chromium/tests/TilingDataTest.cpp:99 > + EXPECT_EQ(0, TilingData(4, 0, 4, true).numTiles()); EXPECT_EQ(0, TilingData(-10, 1, 1, true).numTiles()); EXPECT_EQ(0, TilingData(-1, 1, 1, true).numTiles()); EXPECT_EQ(0, TilingData(0, 1, 1, true).numTiles()); > Source/WebKit/chromium/tests/TilingDataTest.cpp:101 > + EXPECT_EQ(1, TilingData(1, 1, 1, true).numTiles()); If the max texture size can't accommodate borders, then it's a no go: EXPECT_EQ(0, TilingData(1, 1, 1, true).numTiles()); //fix for previous line EXPECT_EQ(0, TilingData(1, 1, 2, true).numTiles()); > Source/WebKit/chromium/tests/TilingDataTest.cpp:-378 > - EXPECT_EQ(16, data.numTilesY()); This test shouldn't need to be modified? If no border texels, a max size of 2 should be valid. This test block should be duplicated for the case with border texels.
Created attachment 89310 [details] Patch
(In reply to comment #2) > (From update of attachment 89250 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=89250&action=review > > I don't think this is the right way to fix the problem -- see notes and suggested test cases. Good suggestion--that keeps TilingData more clean. I can put the min texture size in LayerTilerChromium instead. > [everything else] Can't argue with more tests.
Comment on attachment 89310 [details] Patch LGTM
Comment on attachment 89310 [details] Patch Works for me too!
Committed r83674: <http://trac.webkit.org/changeset/83674>
Reopening because I think the implications of the previous patch were misunderstood. After some discussion with Vince, it seems like the better approach would be to return 1 tile in case tile size == texture size. This would also prevent the need for a "minimum texture size" in bug 57113.
Created attachment 89609 [details] Patch
Comment on attachment 89609 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=89609&action=review > Source/WebCore/platform/graphics/gpu/TilingData.cpp:48 > + return totalSize > 0 && maxTextureSize == totalSize ? 1 : 0; >= totalSize > Source/WebKit/chromium/tests/TilingDataTest.cpp:110 > + EXPECT_EQ(1, TilingData(2, 2, 2, true).numTiles()); EXPECT_EQ(1, TilingData(2, 1, 1, true).numTiles());
Created attachment 89614 [details] Patch
Comment on attachment 89614 [details] Patch LGTM
Comment on attachment 89614 [details] Patch Works for me.
Committed r83891: <http://trac.webkit.org/changeset/83891>