Bug 58364 – [chromium] TilingData mishandles very small texture sizes

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Adrienne Walker]

[chromium] TilingData mishandles very small texture sizes

[Adrienne Walker]

Created attachment 89250 [details]
Patch

[Vincent Scheib]

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.

[Adrienne Walker]

Created attachment 89310 [details]
Patch

[Adrienne Walker]

(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.

[Vincent Scheib]

Comment on attachment 89310 [details]
Patch

LGTM

[James Robinson]

Comment on attachment 89310 [details]
Patch

Works for me too!

[Adrienne Walker]

Committed r83674: <http://trac.webkit.org/changeset/83674>

[Adrienne Walker]

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.

[Adrienne Walker]

Created attachment 89609 [details]
Patch

[Vincent Scheib]

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());

[Adrienne Walker]

Created attachment 89614 [details]
Patch

[Vincent Scheib]

Comment on attachment 89614 [details]
Patch

LGTM

[James Robinson]

Comment on attachment 89614 [details]
Patch

Works for me.

[Adrienne Walker]

Committed r83891: <http://trac.webkit.org/changeset/83891>