Some improvements to reduce the number of pixels touched in generating alpha clip masks

Some improvements to reduce the number of pixels touched in generating alpha clip masks
Committed: https://code.google.com/p/skia/source/detail?r=6329

[TomH, 2012-11-07 13:57:07]

I think your title should be "Towards some improvements..."
And the abstract needs more work.

[robertphillips, 2012-11-07 14:23:55]

LGTM + 3 documentation suggestions & 1 question

https://codereview.appspot.com/6828043/diff/4001/include/core/SkMatrix.h
File include/core/SkMatrix.h (right):

https://codereview.appspot.com/6828043/diff/4001/include/core/SkMatrix.h#newc...
include/core/SkMatrix.h:175: void setTranslate(SkScalar dx, SkScalar dy);
const SkVector& ??

https://codereview.appspot.com/6828043/diff/4001/src/gpu/GrClipMaskManager.cpp
File src/gpu/GrClipMaskManager.cpp (right):

https://codereview.appspot.com/6828043/diff/4001/src/gpu/GrClipMaskManager.cp...
src/gpu/GrClipMaskManager.cpp:637: 
// The scratch texture we're drawing into can be substantially larger than the
mask. Only clear the part we care about.

https://codereview.appspot.com/6828043/diff/4001/src/gpu/GrClipMaskManager.cp...
src/gpu/GrClipMaskManager.cpp:677: // this is the bounds of the clip element in
the space of the alpha-mask.
// The temporary mask buffer can be substantially larger than the actually clip
stack element. We touch the minimum number of pixels necessary (and use decal
mode to combine it with the accumulator

https://codereview.appspot.com/6828043/diff/4001/src/gpu/effects/GrTextureDom...
File src/gpu/effects/GrTextureDomainEffect.cpp (right):

https://codereview.appspot.com/6828043/diff/4001/src/gpu/effects/GrTextureDom...
src/gpu/effects/GrTextureDomainEffect.cpp:56:
fEffectMatrix.emitCodeMakeFSCoords2D(builder, key, vertexCoords, &coords);
dom -> domain?

[reed1, 2012-11-07 14:45:48]

https://codereview.appspot.com/6828043/diff/4001/include/core/SkClipStack.h
File include/core/SkClipStack.h (right):

https://codereview.appspot.com/6828043/diff/4001/include/core/SkClipStack.h#n...
include/core/SkClipStack.h:140: const SkRect& getBounds() const;
Perhaps a LF between the methods and the fields, just for readability.

https://codereview.appspot.com/6828043/diff/4001/include/core/SkMatrix.h
File include/core/SkMatrix.h (right):

https://codereview.appspot.com/6828043/diff/4001/include/core/SkMatrix.h#newc...
include/core/SkMatrix.h:175: void setTranslate(SkScalar dx, SkScalar dy);
On 2012/11/07 14:23:55, robertphillips wrote:
> const SkVector& ??

+1, just to maintain our style, not that the compiler can't figure it out I
presume.

https://codereview.appspot.com/6828043/diff/4001/include/core/SkRandom.h
File include/core/SkRandom.h (right):

https://codereview.appspot.com/6828043/diff/4001/include/core/SkRandom.h#newc...
include/core/SkRandom.h:85: */
Yikes. Seems like we need(ed) a unittest to ensure the results is always
[min..max]

https://codereview.appspot.com/6828043/diff/4001/include/core/SkRect.h
File include/core/SkRect.h (right):

https://codereview.appspot.com/6828043/diff/4001/include/core/SkRect.h#newcod...
include/core/SkRect.h:358: 
I know it follows the existing pattern of MakeSize, but this name is
confusing... as it we are "making an irect". How about we rename this and
MakeSize to just Make()

[bsalomon, 2012-11-07 14:49:12]

https://codereview.appspot.com/6828043/diff/4001/include/core/SkMatrix.h
File include/core/SkMatrix.h (right):

https://codereview.appspot.com/6828043/diff/4001/include/core/SkMatrix.h#newc...
include/core/SkMatrix.h:175: void setTranslate(SkScalar dx, SkScalar dy);
On 2012/11/07 14:23:55, robertphillips wrote:
> const SkVector& ??

duh, done.

https://codereview.appspot.com/6828043/diff/4001/src/gpu/GrClipMaskManager.cpp
File src/gpu/GrClipMaskManager.cpp (right):

https://codereview.appspot.com/6828043/diff/4001/src/gpu/GrClipMaskManager.cp...
src/gpu/GrClipMaskManager.cpp:637: 
On 2012/11/07 14:23:55, robertphillips wrote:
> // The scratch texture we're drawing into can be substantially larger than the
> mask. Only clear the part we care about.

Done.

https://codereview.appspot.com/6828043/diff/4001/src/gpu/GrClipMaskManager.cp...
src/gpu/GrClipMaskManager.cpp:677: // this is the bounds of the clip element in
the space of the alpha-mask.
On 2012/11/07 14:23:55, robertphillips wrote:
> // The temporary mask buffer can be substantially larger than the actually
clip
> stack element. We touch the minimum number of pixels necessary (and use decal
> mode to combine it with the accumulator

Done.

https://codereview.appspot.com/6828043/diff/4001/src/gpu/effects/GrTextureDom...
File src/gpu/effects/GrTextureDomainEffect.cpp (right):

https://codereview.appspot.com/6828043/diff/4001/src/gpu/effects/GrTextureDom...
src/gpu/effects/GrTextureDomainEffect.cpp:56:
fEffectMatrix.emitCodeMakeFSCoords2D(builder, key, vertexCoords, &coords);
On 2012/11/07 14:23:55, robertphillips wrote:
> dom -> domain?

Done.

[bsalomon, 2012-11-07 14:55:46]

https://codereview.appspot.com/6828043/diff/4001/include/core/SkClipStack.h
File include/core/SkClipStack.h (right):

https://codereview.appspot.com/6828043/diff/4001/include/core/SkClipStack.h#n...
include/core/SkClipStack.h:140: const SkRect& getBounds() const;
On 2012/11/07 14:45:48, reed1 wrote:
> Perhaps a LF between the methods and the fields, just for readability.

Done.

https://codereview.appspot.com/6828043/diff/4001/include/core/SkRandom.h
File include/core/SkRandom.h (right):

https://codereview.appspot.com/6828043/diff/4001/include/core/SkRandom.h#newc...
include/core/SkRandom.h:85: */
On 2012/11/07 14:45:48, reed1 wrote:
> Yikes. Seems like we need(ed) a unittest to ensure the results is always
> [min..max]
Filed a bug to write an SkRandom unit test:
http://code.google.com/p/skia/issues/detail?id=960

https://codereview.appspot.com/6828043/diff/4001/include/core/SkRect.h
File include/core/SkRect.h (right):

https://codereview.appspot.com/6828043/diff/4001/include/core/SkRect.h#newcod...
include/core/SkRect.h:358: 
On 2012/11/07 14:45:48, reed1 wrote:
> I know it follows the existing pattern of MakeSize, but this name is
> confusing... as it we are "making an irect". How about we rename this and
> MakeSize to just Make()

I kind of like "MakeSize". how about MakeFromIRect? (Maybe these should all be
MakeFrom...)

[reed1, 2012-11-07 16:12:51]

I like MakeFromFoo or Make, more than MakeFoo. Perhaps both as aliases. e.g.

SkIRect MakeFromIRect(rect) {...}
SkIRect Make(rect) { return MakeFromIRect(rect); }

I can do that renaming in a separate CL. Perhaps for your, we go with
MakeFromIRect().

[reed1, 2012-11-07 16:13:30]

Of course, we can go down the operator-overloading path, and just make it all
magical... but I'm not sure I'm ready for that much excitement.

[bsalomon, 2012-11-07 16:14:43]

On 2012/11/07 16:13:30, reed1 wrote:
> Of course, we can go down the operator-overloading path, and just make it all
> magical... but I'm not sure I'm ready for that much excitement.

Yeah, My hesitation about Make() is that it may be harder to read in some
contexts where it isn't obvious what the type of the passed value is.

I'll update this CL to add MakeFromIRect() rather than MakeIRect().