Change SkTileGride geometry calculations to match the Chromium compositor.

Change SkTileGride geometry calculations to match the Chromium compositor.

This patch changes the semantics of tileWidth/Height to include the border region, and
uses an offset to take into account the fact that there is no outer border for outer
tiles. This patch also fixes a previous bug where the right column and bottom row were
considered to be included in bounds that are expressed as an SkIRect.

Companion Chromium CL required for roll: https://codereview.chromium.org/12221077/

TEST=TileGrid unit test
Committed: https://code.google.com/p/skia/source/detail?r=7885

[TomH, 2013-02-19 17:06:03]

Are these semantics more natural? Or are we just pulling their idiosyncracies
into our codebase? Is there some adapter logic that could sit between them and
src/core (even if it has to go into src/util, rather than into cc/) to map
Chrome concepts into something more neutral?

[junov1, 2013-02-19 18:09:35]

That is a very good point. I think the new border semantics are more natural,
except for the detail about the first row having no top border and the first
column having no left border. I will correct this by making fOffset explicitly
settable rather than inferring it form the overlapPixels value. That way we'll
avoid introducing Chromisms.  Patch on the way

[junov1, 2013-02-19 19:44:53]

Patch Set 2:
- Made the offset user-configurable.
- Added more unit testing to emulate the chromium compositor's access pattern
and verify that it yields fast grid-aligned queries when expected.
- Fixed off-by-one bugs revealed by new unit tests.

[reed1, 2013-02-19 20:08:24]

I am struggling to completely understand tile-size and overlap-size, and what
might be the clearest way to document/describe that.

I *think* the following is the clearest to me:

tile-size is the spacing interval between rectangles (tiles), and the last
parameter is the amount of overlap between tiles.

Either way, I wonder if the overal constructer might be clearer if we used a
custom struct to hold all of the *new* parameters (other than picture
width/height).

e.g.

struct TileGridInfo {
    SkISize  fLatice;
    SkISize  fMargin;
    // or whatever we choose to call these
};

Now the constructor can be

SkTileGridPicture(int width, int height, const TileGridInfo&);

Would we still need setOffset() if we could put lots of fields in that struct?

https://codereview.appspot.com/7350050/diff/5001/include/core/SkTileGridPictu...
File include/core/SkTileGridPicture.h (right):

https://codereview.appspot.com/7350050/diff/5001/include/core/SkTileGridPictu...
include/core/SkTileGridPicture.h:48: */
Skia tries hard to avoid passing points as parameters, when two (X and Y) will
do. Just a (slightly) arbitrary cutoff, as we do like to pass Rects when 4
parameters are needed. Saves the client from having to cons-up a point object
when they may naturally have x and y separate (or stored in a single value
already).

Notice that you passed width/height above, instead of SkISize. I think that is
correct.

[junov1, 2013-02-19 22:30:54]

Patch
Incorporated Mike's feedback.  I feel a bit unsure about location of the
definition TileGridInfo.  I put it in SkTileGridPicture.h because it needs to be
in a public header, but now SkTileGrid.h has a dependency on SkTileGridPicture.h
which doesn't quite feel right...

I am keeping the old SkTileGridPicture constructor around temporarily so that
the skia roll can go smoothly.

[reed1, 2013-02-19 22:50:45]

I see exactly 1 call-site for the current constructor. I would vote, in general,
to try to payoff any interproject debt as we go. In this case, I think it'll be
trivial to update the call-site as part of the DEPS roll, rather than leaving
the older API around (assuming we want to change it).

On a separate note, perhaps we can migrate this header into include/utils or
include/some-new-name-for-picture-utils? Just an effort to keep our /core
minimal. Not a real request for this CL.

[junov1, 2013-02-20 15:27:13]

On 2013/02/19 22:50:45, reed1 wrote:

> On a separate note, perhaps we can migrate this header into include/utils or
> include/some-new-name-for-picture-utils? Just an effort to keep our /core
> minimal. Not a real request for this CL.

I explored that idea a few weeks ago, and I backed-off because the picture
family of classes has several direct dependencies on the bounding box hierarchy
stuff.  I agree we should do this but it is several days of work to re-engineer
that part of the code.

[junov1, 2013-02-20 15:28:59]

Patch.

[reed1, 2013-02-20 15:56:06]

I like the new constructor organization. Just some questions about dox. lgtm
otherwise.

https://codereview.appspot.com/7350050/diff/14001/include/core/SkTileGridPict...
File include/core/SkTileGridPicture.h (right):

https://codereview.appspot.com/7350050/diff/14001/include/core/SkTileGridPict...
include/core/SkTileGridPicture.h:28: SkIPoint fOffset; //!< Offset to convert
from device to tile grid space
So fOffset is a phase correction? In what direction is it applied? i.e. if I say
1,1 for offset, am I shifting the recorded objects down and to the right, or up
and to the left?

https://codereview.appspot.com/7350050/diff/14001/src/core/SkTileGridPicture.cpp
File src/core/SkTileGridPicture.cpp (right):

https://codereview.appspot.com/7350050/diff/14001/src/core/SkTileGridPicture....
src/core/SkTileGridPicture.cpp:16: fInfo = info;
Why the -1 when computing tilecount?

[reed1, 2013-02-20 15:58:12]

In the new spirit of zero-technical-debt, lets please commit to a follow-on CL
that addresses the desire to move non-core subclasses out of core.

[junov1, 2013-02-20 16:34:38]

Created a skia issue to refactor rtree/tilegrid out of core:
https://code.google.com/p/skia/issues/detail?id=1129

https://codereview.appspot.com/7350050/diff/14001/src/core/SkTileGridPicture.cpp
File src/core/SkTileGridPicture.cpp (right):

https://codereview.appspot.com/7350050/diff/14001/src/core/SkTileGridPicture....
src/core/SkTileGridPicture.cpp:16: fInfo = info;
On 2013/02/20 15:56:06, reed1 wrote:
> Why the -1 when computing tilecount?

SkIRects are non-inclusive of the right() column and bottom() row, so the old
code was overshooting the bounds by one pixel all the time (conservative).  For
example, an SkIRect at 0,0 with a size of (1,1) will only have content at pixel
(0,0) and will report left=0 and right=1

[reed1, 2013-02-20 16:47:40]

On 2013/02/20 16:34:38, junov1 wrote:
> Created a skia issue to refactor rtree/tilegrid out of core:
> https://code.google.com/p/skia/issues/detail?id=1129
> 
>
https://codereview.appspot.com/7350050/diff/14001/src/core/SkTileGridPicture.cpp
> File src/core/SkTileGridPicture.cpp (right):
> 
>
https://codereview.appspot.com/7350050/diff/14001/src/core/SkTileGridPicture....
> src/core/SkTileGridPicture.cpp:16: fInfo = info;
> On 2013/02/20 15:56:06, reed1 wrote:
> > Why the -1 when computing tilecount?
> 
> SkIRects are non-inclusive of the right() column and bottom() row, so the old
> code was overshooting the bounds by one pixel all the time (conservative). 
For
> example, an SkIRect at 0,0 with a size of (1,1) will only have content at
pixel
> (0,0) and will report left=0 and right=1

Please paste that excellent answer in the code :)

[junov1, 2013-02-20 18:37:35]

Patch.

[reed1, 2013-02-20 21:37:13]

Some header dox questions, but lgtm

https://codereview.appspot.com/7350050/diff/26001/include/core/SkTileGridPict...
File include/core/SkTileGridPicture.h (right):

https://codereview.appspot.com/7350050/diff/26001/include/core/SkTileGridPict...
include/core/SkTileGridPicture.h:26: /** Tile placement interval */
// Can these values be <= 0 ?

https://codereview.appspot.com/7350050/diff/26001/include/core/SkTileGridPict...
include/core/SkTileGridPicture.h:29: /** Pixel coverage overlap between adjacent
tiles */
// Can these values be negative?

[junov1, 2013-02-20 21:45:47]

No and No. I will clarify in the dox.
Currently working on a patch to roll this into chromium. To avoid holding the
roll, I will not commit this until the roller patch is LGTMed by CC reviewers.

Companion CC patch is being reviewed here:
https://codereview.chromium.org/12221077/