Fixing SkTileGrid to clamp rather than clip content and querries that are outside the bounds of the…

Fixing SkTileGrid to clamp rather than clip content and querries that are outside the bounds of the grid

This fix prevents border padding and offsets on the tile grid structure from resulting in bad clipping. The job of clipping contents is left to the playback canvas.

BUG=https://code.google.com/p/skia/issues/detail?id=1209
TEST=TileGrid unit test, 
Committed: https://code.google.com/p/skia/source/detail?r=8576

[Justin Novosad, 2013-04-08 19:51:57]

PTAL

[Tom Hudson, 2013-04-09 10:10:50]

https://codereview.chromium.org/13493016/diff/2002/include/core/SkMath.h
File include/core/SkMath.h (right):

https://codereview.chromium.org/13493016/diff/2002/include/core/SkMath.h#newc...
include/core/SkMath.h:60: *  @param value    The value we want returned pinned
between [min...max]
Nit: Skia doesn't usually use @param when it's completely redundant, as here.

https://codereview.chromium.org/13493016/diff/2002/include/core/SkMath.h#newc...
include/core/SkMath.h:65: static inline int SkClampMinMax(int value, int min,
int max) {
Given that SkClampMax doesn't use the trinary operator, you could follow this up
by checking generated assembly, or just comparing performance? Maybe it doesn't
make a difference...

https://codereview.chromium.org/13493016/diff/2002/src/core/SkTileGrid.cpp
File src/core/SkTileGrid.cpp (left):

https://codereview.chromium.org/13493016/diff/2002/src/core/SkTileGrid.cpp#ol...
src/core/SkTileGrid.cpp:42: if (!SkIRect::Intersects(dilatedBounds,
fGridBounds)) {
Either this code was erroneous, or the new version could end up not adding data
to any tile, right? Something smells wrong here.

[Justin Novosad, 2013-04-09 14:47:32]

On 2013/04/09 10:10:50, Tom Hudson wrote:
> https://codereview.chromium.org/13493016/diff/2002/include/core/SkMath.h
> File include/core/SkMath.h (right):
> 
>
https://codereview.chromium.org/13493016/diff/2002/include/core/SkMath.h#newc...
> include/core/SkMath.h:60: *  @param value    The value we want returned pinned
> between [min...max]
> Nit: Skia doesn't usually use @param when it's completely redundant, as here.
> 
>
https://codereview.chromium.org/13493016/diff/2002/include/core/SkMath.h#newc...
> include/core/SkMath.h:65: static inline int SkClampMinMax(int value, int min,
> int max) {
> Given that SkClampMax doesn't use the trinary operator, you could follow this
up
> by checking generated assembly, or just comparing performance? Maybe it
doesn't
> make a difference...
> 
> https://codereview.chromium.org/13493016/diff/2002/src/core/SkTileGrid.cpp
> File src/core/SkTileGrid.cpp (left):
> 
>
https://codereview.chromium.org/13493016/diff/2002/src/core/SkTileGrid.cpp#ol...
> src/core/SkTileGrid.cpp:42: if (!SkIRect::Intersects(dilatedBounds,
> fGridBounds)) {
> Either this code was erroneous, or the new version could end up not adding
data
> to any tile, right? Something smells wrong here.

Actually what this does it extends the coverage of border tiles to infinity so
that nothing ever gets dropped from the tile grid anymore.

[Justin Novosad, 2013-04-09 14:47:50]

https://codereview.chromium.org/13493016/diff/2002/include/core/SkMath.h
File include/core/SkMath.h (right):

https://codereview.chromium.org/13493016/diff/2002/include/core/SkMath.h#newc...
include/core/SkMath.h:65: static inline int SkClampMinMax(int value, int min,
int max) {
On 2013/04/09 10:10:50, Tom Hudson wrote:
> Given that SkClampMax doesn't use the trinary operator, you could follow this
up
> by checking generated assembly, or just comparing performance? Maybe it
doesn't
> make a difference...
Ternary operators are actually more likely to trigger optimizations (e.g. the MS
compiler avoids branching by using conditional assignment x86 instruction) 

Anyways, I agree. This should be micro-benched and tuned. skbug.com/1215

[Tom Hudson, 2013-04-09 15:14:01]

On 2013/04/09 14:47:32, junov wrote:
> Actually what this does it extends the coverage of border tiles to infinity so
> that nothing ever gets dropped from the tile grid anymore.

Ah, OK. LGTM.

[reed1, 2013-04-09 15:23:42]

https://codereview.chromium.org/13493016/diff/14001/include/core/SkMath.h
File include/core/SkMath.h (right):

https://codereview.chromium.org/13493016/diff/14001/include/core/SkMath.h#new...
include/core/SkMath.h:62: static inline int SkClampMinMax(int value, int min,
int max) {
How is this different from SkPin32() ?

[Justin Novosad, 2013-04-09 15:32:55]

The difference is the people find it when they search the code for "Clamp" and
when they look in SkMath.h, where there are other clamp function.  :-)

I'll change to use SkPin32. I see that there are already performance
considerations for conditional assignment instructions.  We should just
eliminate the clamp methods in SkMath and use these everywhere.

On 2013/04/09 15:23:42, reed1 wrote:
> https://codereview.chromium.org/13493016/diff/14001/include/core/SkMath.h
> File include/core/SkMath.h (right):
> 
>
https://codereview.chromium.org/13493016/diff/14001/include/core/SkMath.h#new...
> include/core/SkMath.h:62: static inline int SkClampMinMax(int value, int min,
> int max) {
> How is this different from SkPin32() ?

[reed1, 2013-04-09 15:34:45]

is there a perf difference, now that we are performing more culling at playback
instead of at record?

[Justin Novosad, 2013-04-09 15:45:32]

On 2013/04/09 15:34:45, reed1 wrote:
> is there a perf difference, now that we are performing more culling at
playback
> instead of at record?

That is a good point. With our skps, it is not an issue because there is very
little out of bounds content. Nonetheless, there are situations where this is
going to be sub-optimal. Since it is the search method that was causing the bug,
perhaps I can put back the culling that was in insert.

[Justin Novosad, 2013-04-09 15:50:49]

Patch.

[reed1, 2013-04-09 15:56:16]

https://codereview.chromium.org/13493016/diff/19002/src/core/SkTileGrid.cpp
File src/core/SkTileGrid.cpp (right):

https://codereview.chromium.org/13493016/diff/19002/src/core/SkTileGrid.cpp#n...
src/core/SkTileGrid.cpp:69: adjustedQuery.sort(); // in case the inset inverted
the rectangle
If we might invert, then (I presume) we are making the query rect smaller (by
fMargin). Why is that? Naively I would have guessed we would, if anything,
enlarge the query rect, to be conservative in which items we return...

[Justin Novosad, 2013-04-09 16:04:33]

For performance, we expand bounding boxes by fMargin upon insertion, and we
contract the query rect by fMargin upon look-up. This helps achieve one-to-one
correspondence between compositor tiles and tile grid tiles, in order to avoid
the tile merging slow-path whenever possible.

[reed1, 2013-04-09 16:09:07]

Happy to defer (my) understanding of the inset/outset part of this code (since
it is unchanged, other than the additional .sort() call) by this CL.

lgtm