Bug 58364 - [chromium] TilingData mishandles very small texture sizes
Summary: [chromium] TilingData mishandles very small texture sizes
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Adrienne Walker
URL:
Keywords:
Depends on:
Blocks: 57113
  Show dependency treegraph
 
Reported: 2011-04-12 12:45 PDT by Adrienne Walker
Modified: 2011-04-14 13:50 PDT (History)
4 users (show)

See Also:


Attachments
Patch (5.05 KB, patch)
2011-04-12 12:49 PDT, Adrienne Walker
no flags Details | Formatted Diff | Diff
Patch (5.33 KB, patch)
2011-04-12 17:09 PDT, Adrienne Walker
no flags Details | Formatted Diff | Diff
Patch (3.00 KB, patch)
2011-04-14 11:09 PDT, Adrienne Walker
no flags Details | Formatted Diff | Diff
Patch (3.76 KB, patch)
2011-04-14 11:43 PDT, Adrienne Walker
jamesr: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adrienne Walker 2011-04-12 12:45:44 PDT
[chromium] TilingData mishandles very small texture sizes
Comment 1 Adrienne Walker 2011-04-12 12:49:37 PDT
Created attachment 89250 [details]
Patch
Comment 2 Vincent Scheib 2011-04-12 14:01:17 PDT
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.
Comment 3 Adrienne Walker 2011-04-12 17:09:07 PDT
Created attachment 89310 [details]
Patch
Comment 4 Adrienne Walker 2011-04-12 17:10:57 PDT
(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 5 Vincent Scheib 2011-04-12 17:34:45 PDT
Comment on attachment 89310 [details]
Patch

LGTM
Comment 6 James Robinson 2011-04-12 17:35:56 PDT
Comment on attachment 89310 [details]
Patch

Works for me too!
Comment 7 Adrienne Walker 2011-04-12 17:50:53 PDT
Committed r83674: <http://trac.webkit.org/changeset/83674>
Comment 8 Adrienne Walker 2011-04-14 11:07:59 PDT
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.
Comment 9 Adrienne Walker 2011-04-14 11:09:27 PDT
Created attachment 89609 [details]
Patch
Comment 10 Vincent Scheib 2011-04-14 11:30:32 PDT
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());
Comment 11 Adrienne Walker 2011-04-14 11:43:19 PDT
Created attachment 89614 [details]
Patch
Comment 12 Vincent Scheib 2011-04-14 13:25:31 PDT
Comment on attachment 89614 [details]
Patch

LGTM
Comment 13 James Robinson 2011-04-14 13:26:52 PDT
Comment on attachment 89614 [details]
Patch

Works for me.
Comment 14 Adrienne Walker 2011-04-14 13:50:15 PDT
Committed r83891: <http://trac.webkit.org/changeset/83891>