Remove the AA requirement for selecting GrEffect-based clipping.

Remove the AA requirement for selecting GrEffect-based clipping.

Also, optionally use the scissor for a bw clip rect element instead of an effect.

Committed: https://skia.googlesource.com/skia/+/a73218bbbdcbe458651d10815e8d3b73d71b8e11

Committed: https://skia.googlesource.com/skia/+/e9a729cb4d3f05b9c750dc1f63a9cc65b5659f04

[robertphillips, 2014-07-02 14:44:35]

lgtm w/ suggestions & questions

https://codereview.chromium.org/367013003/diff/20001/src/gpu/GrClipMaskManage...
File src/gpu/GrClipMaskManager.cpp (right):

https://codereview.chromium.org/367013003/diff/20001/src/gpu/GrClipMaskManage...
src/gpu/GrClipMaskManager.cpp:115: 
SkASSERT(NULL != scissorRect && scissorRect->isEmpty()); ?

https://codereview.chromium.org/367013003/diff/20001/src/gpu/GrClipMaskManage...
src/gpu/GrClipMaskManager.cpp:172: }
// We don't want to exit if we convert a BW rect clip to a scissor  ?

https://codereview.chromium.org/367013003/diff/20001/src/gpu/GrClipMaskManage...
src/gpu/GrClipMaskManager.cpp:188: rect.offset(clipToRTOffset.fX,
clipToRTOffset.fY);
Kind of hand wavy here but what about something like:

if (kFillBW_GrEffectEdgeType == edgeType) {
   if (scissorRect->isEmpty()) {
       rect.roundOut(scissorRect);
       failIfNoEffect = false;
   } else if (scissorRect->contains(rect)) {  // new magical IRect contains Rect
method
       rect.roundOut(scissorRect);
       failIfNoEffect = false;
   } else if (rect.contains(scissorRect)) {  // actually want rounded out 'rect'
       failIfNoEffect = false;
   } else {
       effect.reset(GrConvexPolyEffect::Create(edgeType, rect));
   }
} else {
 ...

Also do we have to worry about expanding clips in all of this?

https://codereview.chromium.org/367013003/diff/20001/src/gpu/GrClipMaskManage...
src/gpu/GrClipMaskManager.cpp:287: if (NULL == devBounds ||
Add magical SkIRect contains Rect method ?

https://codereview.chromium.org/367013003/diff/20001/src/gpu/GrClipMaskManage...
src/gpu/GrClipMaskManager.cpp:290: } else {
Why not use devBounds intersected with scissorRect in this case?

https://codereview.chromium.org/367013003/diff/20001/src/gpu/GrClipMaskManage...
src/gpu/GrClipMaskManager.cpp:293: } else {
Add SkIRect::intersectWH or, failing that, use SkIRect::intersect(0, 0, width,
height) ?

[bsalomon, 2014-07-02 16:36:47]

https://codereview.chromium.org/367013003/diff/20001/src/gpu/GrClipMaskManage...
File src/gpu/GrClipMaskManager.cpp (right):

https://codereview.chromium.org/367013003/diff/20001/src/gpu/GrClipMaskManage...
src/gpu/GrClipMaskManager.cpp:115: 
On 2014/07/02 14:44:34, robertphillips wrote:
> SkASSERT(NULL != scissorRect && scissorRect->isEmpty()); ?

Added the NULL assertion, I'm not sure this function should care how the caller
init'ed it.

https://codereview.chromium.org/367013003/diff/20001/src/gpu/GrClipMaskManage...
src/gpu/GrClipMaskManager.cpp:172: }
On 2014/07/02 14:44:34, robertphillips wrote:
> // We don't want to exit if we convert a BW rect clip to a scissor  ?

Done.

https://codereview.chromium.org/367013003/diff/20001/src/gpu/GrClipMaskManage...
src/gpu/GrClipMaskManager.cpp:188: rect.offset(clipToRTOffset.fX,
clipToRTOffset.fY);
On 2014/07/02 14:44:34, robertphillips wrote:
> Kind of hand wavy here but what about something like:
> 
> if (kFillBW_GrEffectEdgeType == edgeType) {
>    if (scissorRect->isEmpty()) {
>        rect.roundOut(scissorRect);
>        failIfNoEffect = false;
>    } else if (scissorRect->contains(rect)) {  // new magical IRect contains
Rect
> method
>        rect.roundOut(scissorRect);
>        failIfNoEffect = false;
>    } else if (rect.contains(scissorRect)) {  // actually want rounded out
'rect'
>        failIfNoEffect = false;
>    } else {
>        effect.reset(GrConvexPolyEffect::Create(edgeType, rect));
>    }
> } else {
>  ...
> 
> Also do we have to worry about expanding clips in all of this?

Expanding how? The round out? I think that's what the raster code does. It's
also what we do for a single BW rect clip.

I was assuming that the the clip stack reducer had caught most of the
containment optimizations for us already.

https://codereview.chromium.org/367013003/diff/20001/src/gpu/GrClipMaskManage...
src/gpu/GrClipMaskManager.cpp:287: if (NULL == devBounds ||
On 2014/07/02 14:44:34, robertphillips wrote:
> Add magical SkIRect contains Rect method ?

This current code is actually overkill because contains does empty checks. Wrote
a static inline helper.

https://codereview.chromium.org/367013003/diff/20001/src/gpu/GrClipMaskManage...
src/gpu/GrClipMaskManager.cpp:290: } else {
On 2014/07/02 14:44:34, robertphillips wrote:
> Why not use devBounds intersected with scissorRect in this case?

Scissor changes prevent batching. If we don't see an obvious win for using the
scissor, might as well disable it.

https://codereview.chromium.org/367013003/diff/20001/src/gpu/GrClipMaskManage...
src/gpu/GrClipMaskManager.cpp:293: } else {
On 2014/07/02 14:44:34, robertphillips wrote:
> Add SkIRect::intersectWH or, failing that, use SkIRect::intersect(0, 0, width,
> height) ?

The intersect() methods on SkIRect all do empty checks which we don't need here.
There are intersectNoEmptyCheck() methods but none that match the requirements
here. I'm not sure adding such a specialized case (e.g. intersectWHNoEmptyCheck)
makes sense.

[robertphillips, 2014-07-02 16:48:28]

https://codereview.chromium.org/367013003/diff/20001/src/gpu/GrClipMaskManage...
File src/gpu/GrClipMaskManager.cpp (right):

https://codereview.chromium.org/367013003/diff/20001/src/gpu/GrClipMaskManage...
src/gpu/GrClipMaskManager.cpp:115: 
Fair enough.

https://codereview.chromium.org/367013003/diff/20001/src/gpu/GrClipMaskManage...
src/gpu/GrClipMaskManager.cpp:290: } else {
That would make a good comment.

[bsalomon, 2014-07-02 17:28:46]

https://codereview.chromium.org/367013003/diff/20001/src/gpu/GrClipMaskManage...
File src/gpu/GrClipMaskManager.cpp (right):

https://codereview.chromium.org/367013003/diff/20001/src/gpu/GrClipMaskManage...
src/gpu/GrClipMaskManager.cpp:290: } else {
On 2014/07/02 16:48:28, robertphillips wrote:
> That would make a good comment.

Done, and added a comment about effects always shrinking the clip.

[bsalomon, 2014-07-02 17:28:50]

The CQ bit was checked by bsalomon@google.com

[commit-bot: I haz the power, 2014-07-02 17:29:09]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/bsalomon@google.com/367013003/80001

[commit-bot: I haz the power, 2014-07-02 17:40:58]

Change committed as a73218bbbdcbe458651d10815e8d3b73d71b8e11

[bsalomon, 2014-07-02 18:10:49]

A revert of this CL has been created in
https://codereview.chromium.org/369613002/ by bsalomon@google.com.

The reason for reverting is: Assertion failing..

[bsalomon, 2014-07-02 18:30:07]

The CQ bit was checked by bsalomon@google.com

[commit-bot: I haz the power, 2014-07-02 18:30:34]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/bsalomon@google.com/367013003/100001

[commit-bot: I haz the power, 2014-07-02 18:42:33]

Change committed as e9a729cb4d3f05b9c750dc1f63a9cc65b5659f04

[mtklein, 2014-07-02 19:54:28]

A revert of this CL has been created in
https://codereview.chromium.org/361313002/ by mtklein@google.com.

The reason for reverting is: We think this is breaking the roll..