Separate user and raw stencil settings

Separate user and raw stencil settings

Adds a new GrUserStencilSettings class that describes in abstract terms
how a draw will use the stencil (e.g. kAlwaysIfInClip, kSetClipBit,
etc.). GrPipelineBuilder now only defines the GrUserStencilSettings.
When the GrPipeline is finalized, the user stencil settings are then
translated into concrete GrStencilSettings.

At this point, GrClipMaskManager only needs to tell the GrAppliedClip
whether or not there is a stencil clip. It does not need to modify
stencil settings and GrPipelineBuilder does not need
AutoRestoreStencil.

This is one step of the stencil overhaul. In the future it will also
allow us to clean up the special case handling for nvpr and the
stateful fClipMode member of GrClipMaskManager.

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1962243002

Committed: https://skia.googlesource.com/skia/+/12dbb3947e1aaf205b4fcf13b40e54e50650eb37

Committed: https://skia.googlesource.com/skia/+/93a379bd4d6b30d86c270b879cf172d80172a72b

[Chris Dalton, 2016-05-10 08:02:46]

Description was changed from

==========
Separate user and raw stencil settings

Adds a new GrUserStencilSettings class that describes in abstract terms
how a draw will use the stencil (e.g. kAlwaysIfInClip, kSetClipBit,
etc.). GrPipelineBuilder now only defines the GrUserStencilSettings.
When the GrPipeline is finalized, the user stencil settings are then
translated into concrete GrStencilSettings.

At this point, GrClipMaskManager only needs to tell the GrAppliedClip
whether or not there is a stencil clip. It does not need to modify
stencil settings and GrPipelineBuilder does not need
AutoRestoreStencil.

This is one step of the stencil overhaul. In the future it will also
allow us to clean up the special case handling for nvpr and the
stateful fClipMode member of GrClipMaskManager.

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

to

==========
Separate user and raw stencil settings

Adds a new GrUserStencilSettings class that describes in abstract terms
how a draw will use the stencil (e.g. kAlwaysIfInClip, kSetClipBit,
etc.). GrPipelineBuilder now only defines the GrUserStencilSettings.
When the GrPipeline is finalized, the user stencil settings are then
translated into concrete GrStencilSettings.

At this point, GrClipMaskManager only needs to tell the GrAppliedClip
whether or not there is a stencil clip. It does not need to modify
stencil settings and GrPipelineBuilder does not need
AutoRestoreStencil.

This is one step of the stencil overhaul. In the future it will also
allow us to clean up the special case handling for nvpr and the
stateful fClipMode member of GrClipMaskManager.

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[Chris Dalton, 2016-05-10 08:07:00]

cdalton@nvidia.com changed reviewers:
+ bsalomon@google.com, robertphillips@google.com

[bsalomon, 2016-05-10 14:06:21]

GrUserStencilSettings isn't pretty to read with all the template-based
constructors but I like the result of ensuring that all stencil settings are
compile time defined.

https://codereview.chromium.org/1962243002/diff/1/src/gpu/GrDrawTarget.cpp
File src/gpu/GrDrawTarget.cpp (right):

https://codereview.chromium.org/1962243002/diff/1/src/gpu/GrDrawTarget.cpp#ne...
src/gpu/GrDrawTarget.cpp:272: 
nit wrap. Should this be static?

https://codereview.chromium.org/1962243002/diff/1/src/gpu/GrPipelineBuilder.h
File src/gpu/GrPipelineBuilder.h (right):

https://codereview.chromium.org/1962243002/diff/1/src/gpu/GrPipelineBuilder.h...
src/gpu/GrPipelineBuilder.h:210: * The caller guarantees the pointer will remain
valid until it
It seems like you've designed GrUserStencilSettings such that this must be true,
or am I missing a possibility?

https://codereview.chromium.org/1962243002/diff/1/src/gpu/GrStencil.h
File src/gpu/GrStencil.h (right):

https://codereview.chromium.org/1962243002/diff/1/src/gpu/GrStencil.h#newcode62
src/gpu/GrStencil.h:62: // Tests that respect the clip bit (if in use).
Maybe say that these tests "always fail when the clip bit is not set"

https://codereview.chromium.org/1962243002/diff/1/src/gpu/GrStencil.h#newcode243
src/gpu/GrStencil.h:243: enum class GrStencilTest : uint16_t {
I'm wondering if this could be in another file that is only included in a
handful of places (pipeline, gpu)

[Chris Dalton, 2016-05-10 16:39:30]

https://codereview.chromium.org/1962243002/diff/1/src/gpu/GrDrawTarget.cpp
File src/gpu/GrDrawTarget.cpp (right):

https://codereview.chromium.org/1962243002/diff/1/src/gpu/GrDrawTarget.cpp#ne...
src/gpu/GrDrawTarget.cpp:272: 
On 2016/05/10 14:06:21, bsalomon wrote:
> nit wrap. Should this be static?

Done. (This function will disappear as soon as we fix the special case handling
for nvpr stencil settings.)

https://codereview.chromium.org/1962243002/diff/1/src/gpu/GrPipelineBuilder.h
File src/gpu/GrPipelineBuilder.h (right):

https://codereview.chromium.org/1962243002/diff/1/src/gpu/GrPipelineBuilder.h...
src/gpu/GrPipelineBuilder.h:210: * The caller guarantees the pointer will remain
valid until it
On 2016/05/10 14:06:21, bsalomon wrote:
> It seems like you've designed GrUserStencilSettings such that this must be
true,
> or am I missing a possibility?

Unfortunately it can still be declared in non-static scope. e.g:

 constexpr GrUserStencilSettings stackAllocated (...);
 pb.setUserStencil(&stackAllocated);

https://codereview.chromium.org/1962243002/diff/1/src/gpu/GrStencil.h
File src/gpu/GrStencil.h (right):

https://codereview.chromium.org/1962243002/diff/1/src/gpu/GrStencil.h#newcode62
src/gpu/GrStencil.h:62: // Tests that respect the clip bit (if in use).
On 2016/05/10 14:06:21, bsalomon wrote:
> Maybe say that these tests "always fail when the clip bit is not set"

How's the reworded comment?

https://codereview.chromium.org/1962243002/diff/1/src/gpu/GrStencil.h#newcode243
src/gpu/GrStencil.h:243: enum class GrStencilTest : uint16_t {
On 2016/05/10 14:06:21, bsalomon wrote:
> I'm wondering if this could be in another file that is only included in a
> handful of places (pipeline, gpu)

Done. Good idea.

https://codereview.chromium.org/1962243002/diff/20001/src/gpu/GrUserStencilSe...
File src/gpu/GrUserStencilSettings.h (right):

https://codereview.chromium.org/1962243002/diff/20001/src/gpu/GrUserStencilSe...
src/gpu/GrUserStencilSettings.h:56: enum class GrUserStencilTest : uint16_t {
I liked "StencilTest" better than "StencilFunc", but what do you think about
other possibilities:

StencilCompare
StencilCmp
StencilCompareOp
StencilComparison

[bsalomon, 2016-05-10 20:10:59]

lgtm

https://codereview.chromium.org/1962243002/diff/1/src/gpu/GrPipelineBuilder.h
File src/gpu/GrPipelineBuilder.h (right):

https://codereview.chromium.org/1962243002/diff/1/src/gpu/GrPipelineBuilder.h...
src/gpu/GrPipelineBuilder.h:210: * The caller guarantees the pointer will remain
valid until it
On 2016/05/10 16:39:30, Chris Dalton wrote:
> On 2016/05/10 14:06:21, bsalomon wrote:
> > It seems like you've designed GrUserStencilSettings such that this must be
> true,
> > or am I missing a possibility?
> 
> Unfortunately it can still be declared in non-static scope. e.g:
> 
>  constexpr GrUserStencilSettings stackAllocated (...);
>  pb.setUserStencil(&stackAllocated);

Right, of course.

https://codereview.chromium.org/1962243002/diff/1/src/gpu/GrStencil.h
File src/gpu/GrStencil.h (right):

https://codereview.chromium.org/1962243002/diff/1/src/gpu/GrStencil.h#newcode62
src/gpu/GrStencil.h:62: // Tests that respect the clip bit (if in use).
On 2016/05/10 16:39:30, Chris Dalton wrote:
> On 2016/05/10 14:06:21, bsalomon wrote:
> > Maybe say that these tests "always fail when the clip bit is not set"
> 
> How's the reworded comment?

better, thanks.

https://codereview.chromium.org/1962243002/diff/20001/src/gpu/GrUserStencilSe...
File src/gpu/GrUserStencilSettings.h (right):

https://codereview.chromium.org/1962243002/diff/20001/src/gpu/GrUserStencilSe...
src/gpu/GrUserStencilSettings.h:56: enum class GrUserStencilTest : uint16_t {
On 2016/05/10 16:39:30, Chris Dalton wrote:
> I liked "StencilTest" better than "StencilFunc", but what do you think about
> other possibilities:
> 
> StencilCompare
> StencilCmp
> StencilCompareOp
> StencilComparison

Test is ok with me.

[Chris Dalton, 2016-05-10 20:12:21]

The CQ bit was checked by cdalton@nvidia.com

[commit-bot: I haz the power, 2016-05-10 20:12:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1962243002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1962243002/100001

[commit-bot: I haz the power, 2016-05-10 20:13:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-10 20:13:42]

Try jobs failed on following builders:
  Test-Ubuntu-GCC-ShuttleA-GPU-GTX660-x86_64-Release-Trybot on client.skia
(JOB_FAILED,
http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-ShuttleA-GPU...)

[Chris Dalton, 2016-05-10 20:29:52]

The CQ bit was checked by cdalton@nvidia.com

[Chris Dalton, 2016-05-10 20:29:52]

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/1962243002/#ps120001
(title: "rebase")

[commit-bot: I haz the power, 2016-05-10 20:30:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1962243002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1962243002/120001

[commit-bot: I haz the power, 2016-05-10 20:32:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-10 20:32:57]

Try jobs failed on following builders:
  Build-Win-MSVC-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86_6...)

[Chris Dalton, 2016-05-10 21:02:35]

The CQ bit was checked by cdalton@nvidia.com

[Chris Dalton, 2016-05-10 21:02:35]

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/1962243002/#ps140001
(title: "msvc warning")

[commit-bot: I haz the power, 2016-05-10 21:02:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1962243002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1962243002/140001

[commit-bot: I haz the power, 2016-05-10 21:23:03]

Description was changed from

==========
Separate user and raw stencil settings

Adds a new GrUserStencilSettings class that describes in abstract terms
how a draw will use the stencil (e.g. kAlwaysIfInClip, kSetClipBit,
etc.). GrPipelineBuilder now only defines the GrUserStencilSettings.
When the GrPipeline is finalized, the user stencil settings are then
translated into concrete GrStencilSettings.

At this point, GrClipMaskManager only needs to tell the GrAppliedClip
whether or not there is a stencil clip. It does not need to modify
stencil settings and GrPipelineBuilder does not need
AutoRestoreStencil.

This is one step of the stencil overhaul. In the future it will also
allow us to clean up the special case handling for nvpr and the
stateful fClipMode member of GrClipMaskManager.

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
Separate user and raw stencil settings

Adds a new GrUserStencilSettings class that describes in abstract terms
how a draw will use the stencil (e.g. kAlwaysIfInClip, kSetClipBit,
etc.). GrPipelineBuilder now only defines the GrUserStencilSettings.
When the GrPipeline is finalized, the user stencil settings are then
translated into concrete GrStencilSettings.

At this point, GrClipMaskManager only needs to tell the GrAppliedClip
whether or not there is a stencil clip. It does not need to modify
stencil settings and GrPipelineBuilder does not need
AutoRestoreStencil.

This is one step of the stencil overhaul. In the future it will also
allow us to clean up the special case handling for nvpr and the
stateful fClipMode member of GrClipMaskManager.

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...

Committed:
https://skia.googlesource.com/skia/+/12dbb3947e1aaf205b4fcf13b40e54e50650eb37
==========

[commit-bot: I haz the power, 2016-05-10 21:23:05]

Committed patchset #8 (id:140001) as
https://skia.googlesource.com/skia/+/12dbb3947e1aaf205b4fcf13b40e54e50650eb37

[robertphillips, 2016-05-11 12:21:32]

A revert of this CL (patchset #8 id:140001) has been created in
https://codereview.chromium.org/1969693003/ by robertphillips@google.com.

The reason for reverting is: This seems to be breaking nanobench on the Windows
bots with:

Caught exception 3221225477 EXCEPTION_ACCESS_VIOLATION
GrDrawTarget::stencilPath +c7
GrStencilAndCoverPathRenderer::onDrawPath +fd
GrDrawContext::internalDrawPath +509
GrDrawContext::drawPath +223
GrBlurUtils::drawPathWithMaskFilter +250
SkGpuDevice::drawPath +2ea
SkCanvas::onDrawPath +2e3
SkRecordDraw +2e6
SkBigPicture::playback +e5
SkCanvas::onDrawPicture +12c
SkCanvas::drawPicture +145
SkRecordDraw +2e6
SkBigPicture::playback +e5
SkCanvas::onDrawPicture +12c
SkCanvas::drawPicture +145
SkRecordDraw +261
SkBigPicture::playback +e5
SkCanvas::onDrawPicture +12c
SkCanvas::drawPicture +145
SkMultiPictureDraw::draw +bf
SKPBench::drawMPDPicture +1e0
SKPBench::onDraw +34
Benchmark::draw +32
time +92
setup_gpu_bench +6e
nanobench_main +77b.

[Chris Dalton, 2016-05-11 16:14:01]

Description was changed from

==========
Separate user and raw stencil settings

Adds a new GrUserStencilSettings class that describes in abstract terms
how a draw will use the stencil (e.g. kAlwaysIfInClip, kSetClipBit,
etc.). GrPipelineBuilder now only defines the GrUserStencilSettings.
When the GrPipeline is finalized, the user stencil settings are then
translated into concrete GrStencilSettings.

At this point, GrClipMaskManager only needs to tell the GrAppliedClip
whether or not there is a stencil clip. It does not need to modify
stencil settings and GrPipelineBuilder does not need
AutoRestoreStencil.

This is one step of the stencil overhaul. In the future it will also
allow us to clean up the special case handling for nvpr and the
stateful fClipMode member of GrClipMaskManager.

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...

Committed:
https://skia.googlesource.com/skia/+/12dbb3947e1aaf205b4fcf13b40e54e50650eb37
==========

to

==========
Separate user and raw stencil settings

Adds a new GrUserStencilSettings class that describes in abstract terms
how a draw will use the stencil (e.g. kAlwaysIfInClip, kSetClipBit,
etc.). GrPipelineBuilder now only defines the GrUserStencilSettings.
When the GrPipeline is finalized, the user stencil settings are then
translated into concrete GrStencilSettings.

At this point, GrClipMaskManager only needs to tell the GrAppliedClip
whether or not there is a stencil clip. It does not need to modify
stencil settings and GrPipelineBuilder does not need
AutoRestoreStencil.

This is one step of the stencil overhaul. In the future it will also
allow us to clean up the special case handling for nvpr and the
stateful fClipMode member of GrClipMaskManager.

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...

Committed:
https://skia.googlesource.com/skia/+/12dbb3947e1aaf205b4fcf13b40e54e50650eb37
==========

[Chris Dalton, 2016-05-11 20:10:06]

Robert, does this fix look good to you?

[bsalomon, 2016-05-11 20:26:20]

lgtm

[Chris Dalton, 2016-05-11 20:28:27]

The CQ bit was checked by cdalton@nvidia.com

[commit-bot: I haz the power, 2016-05-11 20:28:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1962243002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1962243002/160001

[commit-bot: I haz the power, 2016-05-11 20:58:14]

Description was changed from

==========
Separate user and raw stencil settings

Adds a new GrUserStencilSettings class that describes in abstract terms
how a draw will use the stencil (e.g. kAlwaysIfInClip, kSetClipBit,
etc.). GrPipelineBuilder now only defines the GrUserStencilSettings.
When the GrPipeline is finalized, the user stencil settings are then
translated into concrete GrStencilSettings.

At this point, GrClipMaskManager only needs to tell the GrAppliedClip
whether or not there is a stencil clip. It does not need to modify
stencil settings and GrPipelineBuilder does not need
AutoRestoreStencil.

This is one step of the stencil overhaul. In the future it will also
allow us to clean up the special case handling for nvpr and the
stateful fClipMode member of GrClipMaskManager.

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...

Committed:
https://skia.googlesource.com/skia/+/12dbb3947e1aaf205b4fcf13b40e54e50650eb37
==========

to

==========
Separate user and raw stencil settings

Adds a new GrUserStencilSettings class that describes in abstract terms
how a draw will use the stencil (e.g. kAlwaysIfInClip, kSetClipBit,
etc.). GrPipelineBuilder now only defines the GrUserStencilSettings.
When the GrPipeline is finalized, the user stencil settings are then
translated into concrete GrStencilSettings.

At this point, GrClipMaskManager only needs to tell the GrAppliedClip
whether or not there is a stencil clip. It does not need to modify
stencil settings and GrPipelineBuilder does not need
AutoRestoreStencil.

This is one step of the stencil overhaul. In the future it will also
allow us to clean up the special case handling for nvpr and the
stateful fClipMode member of GrClipMaskManager.

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...

Committed:
https://skia.googlesource.com/skia/+/12dbb3947e1aaf205b4fcf13b40e54e50650eb37

Committed:
https://skia.googlesource.com/skia/+/93a379bd4d6b30d86c270b879cf172d80172a72b
==========

[commit-bot: I haz the power, 2016-05-11 20:58:15]

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

[scroggo, 2016-05-12 13:24:22]

scroggo@google.com changed reviewers:
+ scroggo@google.com

[scroggo, 2016-05-12 13:24:23]

This appears to be causing differences in Gold:

https://gold.skia.org/search2?blame=554784cd85029c05d9ed04b1aeb71520d196153a%...

Will one of you please triage?