Define clear regions in terms of GrFixedClip

Define clear regions in terms of GrFixedClip                           
                                                                       
Updates clear APIs to take GrFixedClip instead of a rectangle. This    
will allow us to use window rectangles with clears. Removes stencil    
knobs from GrFixedClip.

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2262473003

Committed: https://skia.googlesource.com/skia/+/29df76096fd30941086324902a82656df2d8becd

[csmartdalton, 2016-08-19 19:17:44]

Description was changed from

==========
Define clear regions in terms of GrFixedClip

Updates clear APIs to take GrFixedClip clip instead of a rectangle.
This will allow us to use window rectangles with clears. Removes
stencil knobs from GrFixedClip.

BUG=skia:
==========

to

==========
Define clear regions in terms of GrFixedClip

Updates clear APIs to take GrFixedClip clip instead of a rectangle.
This will allow us to use window rectangles with clears. Removes
stencil knobs from GrFixedClip.

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2262473003
==========

[csmartdalton, 2016-08-19 19:56:02]

csmartdalton@google.com changed reviewers:
+ bsalomon@google.com, egdaniel@google.com

[csmartdalton, 2016-08-19 19:57:02]

Description was changed from

==========
Define clear regions in terms of GrFixedClip

Updates clear APIs to take GrFixedClip clip instead of a rectangle.
This will allow us to use window rectangles with clears. Removes
stencil knobs from GrFixedClip.

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2262473003
==========

to

==========
Define clear regions in terms of GrFixedClip                           
                                                                       
Updates clear APIs to take GrFixedClip instead of a rectangle. This    
will allow us to use window rectangles with clears. Removes stencil    
knobs from GrFixedClip.

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2262473003
==========

[egdaniel, 2016-08-19 20:09:37]

besides one little nitty thing, quick look at vulkan stuff looks fine

https://codereview.chromium.org/2262473003/diff/20001/src/gpu/vk/GrVkGpuComma...
File src/gpu/vk/GrVkGpuCommandBuffer.cpp (right):

https://codereview.chromium.org/2262473003/diff/20001/src/gpu/vk/GrVkGpuComma...
src/gpu/vk/GrVkGpuCommandBuffer.cpp:200: : SkIRect::MakeWH(vkRT->width(),
vkRT->height());
For some minimal optimization, we don't need to do the BottomLeft check and flip
if !clip.scissorEnabled(). Same in onClear.

[csmartdalton, 2016-08-19 20:46:49]

https://codereview.chromium.org/2262473003/diff/20001/src/gpu/vk/GrVkGpuComma...
File src/gpu/vk/GrVkGpuCommandBuffer.cpp (right):

https://codereview.chromium.org/2262473003/diff/20001/src/gpu/vk/GrVkGpuComma...
src/gpu/vk/GrVkGpuCommandBuffer.cpp:200: : SkIRect::MakeWH(vkRT->width(),
vkRT->height());
On 2016/08/19 20:09:37, egdaniel wrote:
> For some minimal optimization, we don't need to do the BottomLeft check and
flip
> if !clip.scissorEnabled(). Same in onClear.

Done. Looks better now.

[csmartdalton, 2016-08-23 02:10:45]

This is good to go.

[bsalomon, 2016-08-23 15:12:49]

https://codereview.chromium.org/2262473003/diff/80001/src/gpu/GrFixedClip.h
File src/gpu/GrFixedClip.h (right):

https://codereview.chromium.org/2262473003/diff/80001/src/gpu/GrFixedClip.h#n...
src/gpu/GrFixedClip.h:32: static const GrFixedClip& disabled();
static method should be capitalized, "Disabled"

https://codereview.chromium.org/2262473003/diff/80001/src/gpu/batches/GrClear...
File src/gpu/batches/GrClearBatch.h (right):

https://codereview.chromium.org/2262473003/diff/80001/src/gpu/batches/GrClear...
src/gpu/batches/GrClearBatch.h:24: if (!batch->renderTarget()) {
Can this happen or will something upstack have caught it?

[csmartdalton, 2016-08-23 15:23:57]

https://codereview.chromium.org/2262473003/diff/80001/src/gpu/batches/GrClear...
File src/gpu/batches/GrClearBatch.h (right):

https://codereview.chromium.org/2262473003/diff/80001/src/gpu/batches/GrClear...
src/gpu/batches/GrClearBatch.h:24: if (!batch->renderTarget()) {
On 2016/08/23 15:12:49, bsalomon wrote:
> Can this happen or will something upstack have caught it?

This is the constructor's way of telling us it failed. If that's confusing we
can pass in a "success" bool and/or update the comment.

https://codereview.chromium.org/2262473003/diff/80001/src/gpu/batches/GrClear...
src/gpu/batches/GrClearBatch.h:57: return;
Here the constructor aborts before setting fRenderTarget.

[csmartdalton, 2016-08-23 15:34:28]

https://codereview.chromium.org/2262473003/diff/80001/src/gpu/GrFixedClip.h
File src/gpu/GrFixedClip.h (right):

https://codereview.chromium.org/2262473003/diff/80001/src/gpu/GrFixedClip.h#n...
src/gpu/GrFixedClip.h:32: static const GrFixedClip& disabled();
On 2016/08/23 15:12:49, bsalomon wrote:
> static method should be capitalized, "Disabled"

Done.

[bsalomon, 2016-08-23 16:04:49]

https://codereview.chromium.org/2262473003/diff/80001/src/gpu/batches/GrClear...
File src/gpu/batches/GrClearBatch.h (right):

https://codereview.chromium.org/2262473003/diff/80001/src/gpu/batches/GrClear...
src/gpu/batches/GrClearBatch.h:24: if (!batch->renderTarget()) {
On 2016/08/23 15:23:57, csmartdalton wrote:
> On 2016/08/23 15:12:49, bsalomon wrote:
> > Can this happen or will something upstack have caught it?
> 
> This is the constructor's way of telling us it failed. If that's confusing we
> can pass in a "success" bool and/or update the comment.

I get that, but I'm just wondering whether we would have (or could have) caught
the non-intersection case up in GrDC.

[csmartdalton, 2016-08-23 16:31:23]

https://codereview.chromium.org/2262473003/diff/80001/src/gpu/batches/GrClear...
File src/gpu/batches/GrClearBatch.h (right):

https://codereview.chromium.org/2262473003/diff/80001/src/gpu/batches/GrClear...
src/gpu/batches/GrClearBatch.h:24: if (!batch->renderTarget()) {
On 2016/08/23 16:04:49, bsalomon wrote:
> On 2016/08/23 15:23:57, csmartdalton wrote:
> > On 2016/08/23 15:12:49, bsalomon wrote:
> > > Can this happen or will something upstack have caught it?
> > 
> > This is the constructor's way of telling us it failed. If that's confusing
we
> > can pass in a "success" bool and/or update the comment.
> 
> I get that, but I'm just wondering whether we would have (or could have)
caught
> the non-intersection case up in GrDC.

Ah, right. Added comments to that file to discuss this.

https://codereview.chromium.org/2262473003/diff/100001/src/gpu/GrDrawContext.cpp
File src/gpu/GrDrawContext.cpp (left):

https://codereview.chromium.org/2262473003/diff/100001/src/gpu/GrDrawContext....
src/gpu/GrDrawContext.cpp:210: return;
This is where it used to bail early on a non-intersection case.

https://codereview.chromium.org/2262473003/diff/100001/src/gpu/GrDrawContext.cpp
File src/gpu/GrDrawContext.cpp (right):

https://codereview.chromium.org/2262473003/diff/100001/src/gpu/GrDrawContext....
src/gpu/GrDrawContext.cpp:228: return;
Still bails in the "draw instead of clear" case.

https://codereview.chromium.org/2262473003/diff/100001/src/gpu/GrDrawContext....
src/gpu/GrDrawContext.cpp:241: return;
But now it relies on GrClearBatch::Make to detect non-intersection in the
"clipped clear" case.

The reason for this that this method now takes a const GrFixedClip&. So it made
more sense to just pass ours along rather than making a temp GrFixedClip on the
stack just so we can fix up the scissor rect. Especially since GrClearBatch will
be copying the GrFixedClip anyway, it may as well do the intersection (and
non-intersection check) itself and save a copy of GrFixedClip.

[bsalomon, 2016-08-23 16:57:29]

https://codereview.chromium.org/2262473003/diff/100001/src/gpu/GrDrawContext.cpp
File src/gpu/GrDrawContext.cpp (right):

https://codereview.chromium.org/2262473003/diff/100001/src/gpu/GrDrawContext....
src/gpu/GrDrawContext.cpp:241: return;
On 2016/08/23 16:31:23, csmartdalton wrote:
> But now it relies on GrClearBatch::Make to detect non-intersection in the
> "clipped clear" case.
> 
> The reason for this that this method now takes a const GrFixedClip&. So it
made
> more sense to just pass ours along rather than making a temp GrFixedClip on
the
> stack just so we can fix up the scissor rect. Especially since GrClearBatch
will
> be copying the GrFixedClip anyway, it may as well do the intersection (and
> non-intersection check) itself and save a copy of GrFixedClip.

Sounds reasonable. lgtm,

[csmartdalton, 2016-08-23 19:51:29]

Mark Kilgard suggests this might not be helpful with clears. I'll run some
numbers before committing to it.

https://codereview.chromium.org/2262473003/diff/120001/src/gpu/GrClipStackCli...
File src/gpu/GrClipStackClip.cpp (right):

https://codereview.chromium.org/2262473003/diff/120001/src/gpu/GrClipStackCli...
src/gpu/GrClipStackClip.cpp:566: GrFixedClip fFixedClip;
This is much better. GrFixedClip REALLY needs to be final if certain things
(clears) are going to be making assumptions about it based on the fact that it's
a fixed clip.

[Mark Kilgard, 2016-08-23 22:13:33]

On 2016/08/23 19:51:29, csmartdalton wrote:
> Mark Kilgard suggests this might not be helpful with clears. I'll run some
> numbers before committing to it.

Actually on further consultation on how fast clears work (or really don't work
in the stencil-only case), I *do* think window rectangle testing during stencil
clears should help, consuming less memory bandwidth.

Doesn't change the fact that performance analysis is a good idea.  :-)

To observe a benefit, try at 16:1 where there are a lot of stencil samples per
pixel so a stencil clear is maximally expensive and make sure there really are
large stencil clears.

[csmartdalton, 2016-08-31 03:45:25]

On 2016/08/23 22:13:33, Mark Kilgard wrote:
> On 2016/08/23 19:51:29, csmartdalton wrote:
> > Mark Kilgard suggests this might not be helpful with clears. I'll run some
> > numbers before committing to it.
> 
> Actually on further consultation on how fast clears work (or really don't work
> in the stencil-only case), I *do* think window rectangle testing during
stencil
> clears should help, consuming less memory bandwidth.
> 
> Doesn't change the fact that performance analysis is a good idea.  :-)
> 
> To observe a benefit, try at 16:1 where there are a lot of stencil samples per
> pixel so a stencil clear is maximally expensive and make sure there really are
> large stencil clears.

I was unable to observe a clear, real-world benefit on my PC from using window
rectangles during 16x stencil clears, but I'm still inclined to keep this CL
having confirmation that it will save memory bandwidth at least when clearing
the stencil buffer.

[csmartdalton, 2016-08-31 15:35:52]

On 2016/08/31 03:45:25, csmartdalton wrote:
> On 2016/08/23 22:13:33, Mark Kilgard wrote:
> > On 2016/08/23 19:51:29, csmartdalton wrote:
> > > Mark Kilgard suggests this might not be helpful with clears. I'll run some
> > > numbers before committing to it.
> > 
> > Actually on further consultation on how fast clears work (or really don't
work
> > in the stencil-only case), I *do* think window rectangle testing during
> stencil
> > clears should help, consuming less memory bandwidth.
> > 
> > Doesn't change the fact that performance analysis is a good idea.  :-)
> > 
> > To observe a benefit, try at 16:1 where there are a lot of stencil samples
per
> > pixel so a stencil clear is maximally expensive and make sure there really
are
> > large stencil clears.
> 
> I was unable to observe a clear, real-world benefit on my PC from using window
> rectangles during 16x stencil clears, but I'm still inclined to keep this CL
> having confirmation that it will save memory bandwidth at least when clearing
> the stencil buffer.

To be clear, I didn't see a definite benefit on the Blink skps. I didn't write a
specific test to constantly clear the stencil buffer with big window rectangles
in place.

[bsalomon, 2016-08-31 17:46:54]

On 2016/08/31 15:35:52, csmartdalton wrote:
> On 2016/08/31 03:45:25, csmartdalton wrote:
> > On 2016/08/23 22:13:33, Mark Kilgard wrote:
> > > On 2016/08/23 19:51:29, csmartdalton wrote:
> > > > Mark Kilgard suggests this might not be helpful with clears. I'll run
some
> > > > numbers before committing to it.
> > > 
> > > Actually on further consultation on how fast clears work (or really don't
> work
> > > in the stencil-only case), I *do* think window rectangle testing during
> > stencil
> > > clears should help, consuming less memory bandwidth.
> > > 
> > > Doesn't change the fact that performance analysis is a good idea.  :-)
> > > 
> > > To observe a benefit, try at 16:1 where there are a lot of stencil samples
> per
> > > pixel so a stencil clear is maximally expensive and make sure there really
> are
> > > large stencil clears.
> > 
> > I was unable to observe a clear, real-world benefit on my PC from using
window
> > rectangles during 16x stencil clears, but I'm still inclined to keep this CL
> > having confirmation that it will save memory bandwidth at least when
clearing
> > the stencil buffer.
> 
> To be clear, I didn't see a definite benefit on the Blink skps. I didn't write
a
> specific test to constantly clear the stencil buffer with big window
rectangles
> in place.

That seems reasonable to me. Feel free to land it.

[csmartdalton, 2016-08-31 18:27:05]

The CQ bit was checked by csmartdalton@google.com

[csmartdalton, 2016-08-31 18:27:05]

The patchset sent to the CQ was uploaded after l-g-t-m from bsalomon@google.com
Link to the patchset: https://codereview.chromium.org/2262473003/#ps160001
(title: "rebase")

[commit-bot: I haz the power, 2016-08-31 18:27:14]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-31 18:55:55]

Description was changed from

==========
Define clear regions in terms of GrFixedClip                           
                                                                       
Updates clear APIs to take GrFixedClip instead of a rectangle. This    
will allow us to use window rectangles with clears. Removes stencil    
knobs from GrFixedClip.

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2262473003
==========

to

==========
Define clear regions in terms of GrFixedClip                           
                                                                       
Updates clear APIs to take GrFixedClip instead of a rectangle. This    
will allow us to use window rectangles with clears. Removes stencil    
knobs from GrFixedClip.

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2262473003

Committed:
https://skia.googlesource.com/skia/+/29df76096fd30941086324902a82656df2d8becd
==========

[commit-bot: I haz the power, 2016-08-31 18:55:56]

Committed patchset #9 (id:160001) as
https://skia.googlesource.com/skia/+/29df76096fd30941086324902a82656df2d8becd