Added bound computation to SkClipStack

[robertphillips, 2012-07-19 19:47:29]

From a conversation with Brian, I am going to try to hide the "isLargest" calls
w/in SkClipStack and make the SkClipStack code rely on the "setLargest" calls
less (i.e., add an "isInfinite" flag).

[reed1, 2012-07-20 15:29:18]

isLargest / setLargest are icky in general :)

1. unittests?
2. benchmarks? (i.e. are we slowing down "normal" ops?

[robertphillips, 2012-07-20 18:30:22]

PTAL

1) I will supply a unit test in the next patch
2) As far as I can tell on Windows w/ a repeat of 200 there are no performance
regressions

[bsalomon, 2012-07-20 19:03:26]

http://codereview.appspot.com/6419048/diff/3002/include/core/SkClipStack.h
File include/core/SkClipStack.h (right):

http://codereview.appspot.com/6419048/diff/3002/include/core/SkClipStack.h#ne...
include/core/SkClipStack.h:33: SkIRect getIntBounds(const SkIPoint& offset, int
width, int height) const;
The offset/width/height params feel a bit odd (and should at least be well
documented). It seems a bit strange that the scalar version also takes an
ipoint,iwidth,iheight. We normally return things that don't fit in a register
using pointer params.

Should the signature be
bool get(Int)Bounds(Sk(I)Rect* bounds);?

returns false if unbounded, caller has to do offset and catch the unbounded
case.

http://codereview.appspot.com/6419048/diff/3002/src/core/SkClipStack.cpp
File src/core/SkClipStack.cpp (right):

http://codereview.appspot.com/6419048/diff/3002/src/core/SkClipStack.cpp#newc...
src/core/SkClipStack.cpp:38: bool            fInverted;
fBoundsInverted? fBoundsAreInverted? fFiniteBoundInverted?

[robertphillips, 2012-07-20 20:17:29]

http://codereview.appspot.com/6419048/diff/3002/include/core/SkClipStack.h
File include/core/SkClipStack.h (right):

http://codereview.appspot.com/6419048/diff/3002/include/core/SkClipStack.h#ne...
include/core/SkClipStack.h:33: SkIRect getIntBounds(const SkIPoint& offset, int
width, int height) const;
I have added documentation (so you could see what that would look like) and yes
the parameters are a bit odd. I was trying to hide all the infinite plane issues
inside the SkClipStack class. If you think having the caller deal with that
isn't a problem then I will switch it around.

http://codereview.appspot.com/6419048/diff/3002/src/core/SkClipStack.cpp
File src/core/SkClipStack.cpp (right):

http://codereview.appspot.com/6419048/diff/3002/src/core/SkClipStack.cpp#newc...
src/core/SkClipStack.cpp:38: bool            fInverted;
On 2012/07/20 19:03:26, bsalomon wrote:
> fBoundsInverted? fBoundsAreInverted? fFiniteBoundInverted?

Done.

[robertphillips, 2012-07-22 14:27:32]

From a discussion w/ Brian, this patch moves the complexity of dealing w/ the
infinite bounds out of SkClipStack::get*Bounds and into SkClipStack's
GetConservativeBounds helper function and SkGpuDevice's check_bounds helper
function.

[bsalomon, 2012-07-23 12:24:17]

Mostly LGTM, small nits. reed@ should look since this class is his baby.

http://codereview.appspot.com/6419048/diff/5009/include/core/SkClipStack.h
File include/core/SkClipStack.h (right):

http://codereview.appspot.com/6419048/diff/5009/include/core/SkClipStack.h#ne...
include/core/SkClipStack.h:40: bool getBounds(SkRect* bound) const;
The return type is slightly unintuitive, simply because we often use a similar
bool foo(T* ret) pattern where the return is success/fail and failure means ret
is unmodified. For exampler. SkMatrix::invert. Should  we use an enum and/or
make the second return value a second param. Is this a candidate for that "warn
if return value unchecked" thingy?

http://codereview.appspot.com/6419048/diff/5009/include/core/SkClipStack.h#ne...
include/core/SkClipStack.h:155: * get_conservative_bounds returns a conservative
bound of the current clip.
Comment doesn't match function name.

http://codereview.appspot.com/6419048/diff/5009/include/core/SkClipStack.h#ne...
include/core/SkClipStack.h:160: * drawing areas (i.e., those resulting from a
saveLayer).
Maybe the comment should be really clear that the bounds is first translated by
-origin and then intersected with [0,maxWidth)x[0,maxHeight).

[robertphillips, 2012-07-23 13:19:48]

http://codereview.appspot.com/6419048/diff/5009/include/core/SkClipStack.h
File include/core/SkClipStack.h (right):

http://codereview.appspot.com/6419048/diff/5009/include/core/SkClipStack.h#ne...
include/core/SkClipStack.h:40: bool getBounds(SkRect* bound) const;
I have opted to return the inversion state in a parameter. The next patch to
this class will add an "intersectionOfRects" parameter (to capture GrClip's
isectRectValid functionality) so I thought this solution would fit better with
where the class is heading.

http://codereview.appspot.com/6419048/diff/5009/include/core/SkClipStack.h#ne...
include/core/SkClipStack.h:155: * get_conservative_bounds returns a conservative
bound of the current clip.
On 2012/07/23 12:24:17, bsalomon wrote:
> Comment doesn't match function name.

Done.

http://codereview.appspot.com/6419048/diff/5009/include/core/SkClipStack.h#ne...
include/core/SkClipStack.h:160: * drawing areas (i.e., those resulting from a
saveLayer).
On 2012/07/23 12:24:17, bsalomon wrote:
> Maybe the comment should be really clear that the bounds is first translated
by
> -origin and then intersected with [0,maxWidth)x[0,maxHeight).

Done.

[bsalomon, 2012-07-23 14:49:26]

On 2012/07/23 13:19:48, robertphillips wrote:
> http://codereview.appspot.com/6419048/diff/5009/include/core/SkClipStack.h
> File include/core/SkClipStack.h (right):
> 
>
http://codereview.appspot.com/6419048/diff/5009/include/core/SkClipStack.h#ne...
> include/core/SkClipStack.h:40: bool getBounds(SkRect* bound) const;
> I have opted to return the inversion state in a parameter. The next patch to
> this class will add an "intersectionOfRects" parameter (to capture GrClip's
> isectRectValid functionality) so I thought this solution would fit better with
> where the class is heading.
> 
>
http://codereview.appspot.com/6419048/diff/5009/include/core/SkClipStack.h#ne...
> include/core/SkClipStack.h:155: * get_conservative_bounds returns a
conservative
> bound of the current clip.
> On 2012/07/23 12:24:17, bsalomon wrote:
> > Comment doesn't match function name.
> 
> Done.
> 
>
http://codereview.appspot.com/6419048/diff/5009/include/core/SkClipStack.h#ne...
> include/core/SkClipStack.h:160: * drawing areas (i.e., those resulting from a
> saveLayer).
> On 2012/07/23 12:24:17, bsalomon wrote:
> > Maybe the comment should be really clear that the bounds is first translated
> by
> > -origin and then intersected with [0,maxWidth)x[0,maxHeight).
> 
> Done.

LGTM

[robertphillips, 2012-07-23 17:20:59]

This patch:

replaces the fInverted boolean with an enum
moves GetConservativeBounds into SkClipStack
splits the origin parameter to GetConservativeBounds into x & y components

[reed1, 2012-07-23 20:40:47]

http://codereview.appspot.com/6419048/diff/11002/include/core/SkClipStack.h
File include/core/SkClipStack.h (left):

http://codereview.appspot.com/6419048/diff/11002/include/core/SkClipStack.h#o...
include/core/SkClipStack.h:32: 
BoundsType seems more natural and BoundType, unless we're talking about knots

http://codereview.appspot.com/6419048/diff/11002/include/core/SkClipStack.h#o...
include/core/SkClipStack.h:137: 
I think if this is part of the public header, it need not be static, but just be
a member function...

http://codereview.appspot.com/6419048/diff/11002/include/core/SkClipStack.h
File include/core/SkClipStack.h (right):

http://codereview.appspot.com/6419048/diff/11002/include/core/SkClipStack.h#n...
include/core/SkClipStack.h:169: static void GetConservativeBounds(const
SkClipStack& clipStack,
can these 4 be expressed as SkIntRect?

http://codereview.appspot.com/6419048/diff/11002/src/core/SkClipStack.cpp
File src/core/SkClipStack.cpp (right):

http://codereview.appspot.com/6419048/diff/11002/src/core/SkClipStack.cpp#new...
src/core/SkClipStack.cpp:623: 
Why are we passing in ints, but promoting to scalars? Should they be scalars in
the first place?

[robertphillips, 2012-07-23 21:21:37]

Diffs may be a bit odd for this patch since path 7 was uploaded from the wrong
view.

http://codereview.appspot.com/6419048/diff/11002/include/core/SkClipStack.h
File include/core/SkClipStack.h (left):

http://codereview.appspot.com/6419048/diff/11002/include/core/SkClipStack.h#o...
include/core/SkClipStack.h:32: 
On 2012/07/23 20:40:47, reed1 wrote:
> BoundsType seems more natural and BoundType, unless we're talking about knots

Done.

http://codereview.appspot.com/6419048/diff/11002/include/core/SkClipStack.h#o...
include/core/SkClipStack.h:137: 
On 2012/07/23 20:40:47, reed1 wrote:
> I think if this is part of the public header, it need not be static, but just
be
> a member function...

Done.

http://codereview.appspot.com/6419048/diff/11002/include/core/SkClipStack.h#o...
include/core/SkClipStack.h:137: 
On 2012/07/23 20:40:47, reed1 wrote:
> I think if this is part of the public header, it need not be static, but just
be
> a member function...

Done.

http://codereview.appspot.com/6419048/diff/11002/include/core/SkClipStack.h
File include/core/SkClipStack.h (right):

http://codereview.appspot.com/6419048/diff/11002/include/core/SkClipStack.h#n...
include/core/SkClipStack.h:169: static void GetConservativeBounds(const
SkClipStack& clipStack,
I think that would be a bit odd in that we would have to translate by the LT and
then intersect the result with 0,0,R,B.

http://codereview.appspot.com/6419048/diff/11002/src/core/SkClipStack.cpp
File src/core/SkClipStack.cpp (right):

http://codereview.appspot.com/6419048/diff/11002/src/core/SkClipStack.cpp#new...
src/core/SkClipStack.cpp:623: 
I'm game but SkCanvas::saveLayer roundsOut the scalar bounds passed to it and
then the integer origin is what is passed around. It seemed odd to convert them
to scalars to pass them into this method.

[reed1, 2012-07-23 21:36:00]

lgtm

http://codereview.appspot.com/6419048/diff/7010/include/core/SkClipStack.h
File include/core/SkClipStack.h (right):

http://codereview.appspot.com/6419048/diff/7010/include/core/SkClipStack.h#ne...
include/core/SkClipStack.h:168: */
s/Get/get

[robertphillips, 2012-07-24 13:26:48]

http://codereview.appspot.com/6419048/diff/7010/include/core/SkClipStack.h
File include/core/SkClipStack.h (right):

http://codereview.appspot.com/6419048/diff/7010/include/core/SkClipStack.h#ne...
include/core/SkClipStack.h:168: */
On 2012/07/23 21:36:00, reed1 wrote:
> s/Get/get

Done.

[robertphillips, 2012-07-24 13:54:26]

committed as r4730