Check fIgnoresCoverage in GrPipeline::AreEqual

Check fIgnoresCoverage in GrPipeline::AreEqual

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

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

[csmartdalton, 2016-07-03 17:06:25]

Description was changed from

==========
Check fIgnoresCoverage in GrPipeline::AreEqual

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

to

==========
Check fIgnoresCoverage in GrPipeline::AreEqual

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

[csmartdalton, 2016-07-03 17:09:48]

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

[bsalomon, 2016-07-06 13:20:15]

Description was changed from

==========
Check fIgnoresCoverage in GrPipeline::AreEqual

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

to

==========
Check fIgnoresCoverage in GrPipeline::AreEqual

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

[bsalomon, 2016-07-06 13:20:16]

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

[bsalomon, 2016-07-06 13:21:15]

My intuition is that this is derived state and should be covered by the
GrProcessor comparisons and fixed function comparisons but I'd like Greg to
weigh in.

[csmartdalton, 2016-07-06 15:22:24]

On 2016/07/06 13:21:15, bsalomon wrote:
> My intuition is that this is derived state and should be covered by the
> GrProcessor comparisons and fixed function comparisons but I'd like Greg to
> weigh in.

The GP only implements computePipelineOptimizations. And then the pipeline uses
those hints to decide under the hood, based on its XP and other things, whether
it will ignore coverage. The GP really can't know if the pipeline will be
ignoring coverage without asking. IMO this is an important part of deciding
whether two pipelines are equal since it affects how the pipeline itself
behaves. in both cases the pipeline declares a coverage variable, but when
ignoring coverage the pipeline just ignores that variable at the end of the
shader.

The problem I ran into was that a batch with meaningful coverage got combined
with a batch that had “don't care” coverage (in other words would just write out
1.0 to coverage when asked). This is a perfectly valid combination as long as
the two pipelines really are identical. However, if the pipeline that gets kept
is secretly ignoring coverage, then the batch with meaningful coverage will
start drawing incorrectly.

Is there a usecase where we don't want AreEqual to think about whether it
ignores coverage? If so then maybe we could put the check in CanCombine instead
(and consider renaming AreEqual).

[bsalomon, 2016-07-06 15:47:13]

On 2016/07/06 15:22:24, csmartdalton wrote:
> On 2016/07/06 13:21:15, bsalomon wrote:
> > My intuition is that this is derived state and should be covered by the
> > GrProcessor comparisons and fixed function comparisons but I'd like Greg to
> > weigh in.
> 
> The GP only implements computePipelineOptimizations. And then the pipeline
uses
> those hints to decide under the hood, based on its XP and other things,
whether
> it will ignore coverage. The GP really can't know if the pipeline will be
> ignoring coverage without asking. IMO this is an important part of deciding
> whether two pipelines are equal since it affects how the pipeline itself
> behaves. in both cases the pipeline declares a coverage variable, but when
> ignoring coverage the pipeline just ignores that variable at the end of the
> shader.
> 
> The problem I ran into was that a batch with meaningful coverage got combined
> with a batch that had “don't care” coverage (in other words would just write
out
> 1.0 to coverage when asked). This is a perfectly valid combination as long as
> the two pipelines really are identical. However, if the pipeline that gets
kept
> is secretly ignoring coverage, then the batch with meaningful coverage will
> start drawing incorrectly.
> 
> Is there a usecase where we don't want AreEqual to think about whether it
> ignores coverage? If so then maybe we could put the check in CanCombine
instead
> (and consider renaming AreEqual).

Ok, I get it now. In this case would it be preferable outcome to choose the
pipeline that had ignores=false rather than block batching? I'm not proposing we
actually do that, but just trying to understand.


In the not too distant future we won't be making pipelines in batches until
flush time so this won't be an issue. At that time I think we will just include
the GP in the pipeline object rather than pass it separately. And that will all
occur after all batching has been performed so you would know the coverage state
of all the batched draws.

[csmartdalton, 2016-07-06 16:00:14]

On 2016/07/06 15:47:13, bsalomon wrote:
> On 2016/07/06 15:22:24, csmartdalton wrote:
> > On 2016/07/06 13:21:15, bsalomon wrote:
> > > My intuition is that this is derived state and should be covered by the
> > > GrProcessor comparisons and fixed function comparisons but I'd like Greg
to
> > > weigh in.
> > 
> > The GP only implements computePipelineOptimizations. And then the pipeline
> uses
> > those hints to decide under the hood, based on its XP and other things,
> whether
> > it will ignore coverage. The GP really can't know if the pipeline will be
> > ignoring coverage without asking. IMO this is an important part of deciding
> > whether two pipelines are equal since it affects how the pipeline itself
> > behaves. in both cases the pipeline declares a coverage variable, but when
> > ignoring coverage the pipeline just ignores that variable at the end of the
> > shader.
> > 
> > The problem I ran into was that a batch with meaningful coverage got
combined
> > with a batch that had “don't care” coverage (in other words would just write
> out
> > 1.0 to coverage when asked). This is a perfectly valid combination as long
as
> > the two pipelines really are identical. However, if the pipeline that gets
> kept
> > is secretly ignoring coverage, then the batch with meaningful coverage will
> > start drawing incorrectly.
> > 
> > Is there a usecase where we don't want AreEqual to think about whether it
> > ignores coverage? If so then maybe we could put the check in CanCombine
> instead
> > (and consider renaming AreEqual).
> 
> Ok, I get it now. In this case would it be preferable outcome to choose the
> pipeline that had ignores=false rather than block batching? I'm not proposing
we
> actually do that, but just trying to understand.
> 
> 
> In the not too distant future we won't be making pipelines in batches until
> flush time so this won't be an issue. At that time I think we will just
include
> the GP in the pipeline object rather than pass it separately. And that will
all
> occur after all batching has been performed so you would know the coverage
state
> of all the batched draws.

Yes that's right, if we're at the point that we've decided we want to combine
the two batches then we'd rather not have pipe line issues prevent it from
happening. If there was already an easy way to pick which pipeline you wanted to
keep then your suggestion would be ideal.

(e.g. instead of combining a new batch into itself, onCombine could return a
pointer to the combined batch, potentially allowed to be "this", "that", or a
brand new batch...)

[bsalomon, 2016-07-06 16:09:05]

On 2016/07/06 16:00:14, csmartdalton wrote:
> On 2016/07/06 15:47:13, bsalomon wrote:
> > On 2016/07/06 15:22:24, csmartdalton wrote:
> > > On 2016/07/06 13:21:15, bsalomon wrote:
> > > > My intuition is that this is derived state and should be covered by the
> > > > GrProcessor comparisons and fixed function comparisons but I'd like Greg
> to
> > > > weigh in.
> > > 
> > > The GP only implements computePipelineOptimizations. And then the pipeline
> > uses
> > > those hints to decide under the hood, based on its XP and other things,
> > whether
> > > it will ignore coverage. The GP really can't know if the pipeline will be
> > > ignoring coverage without asking. IMO this is an important part of
deciding
> > > whether two pipelines are equal since it affects how the pipeline itself
> > > behaves. in both cases the pipeline declares a coverage variable, but when
> > > ignoring coverage the pipeline just ignores that variable at the end of
the
> > > shader.
> > > 
> > > The problem I ran into was that a batch with meaningful coverage got
> combined
> > > with a batch that had “don't care” coverage (in other words would just
write
> > out
> > > 1.0 to coverage when asked). This is a perfectly valid combination as long
> as
> > > the two pipelines really are identical. However, if the pipeline that gets
> > kept
> > > is secretly ignoring coverage, then the batch with meaningful coverage
will
> > > start drawing incorrectly.
> > > 
> > > Is there a usecase where we don't want AreEqual to think about whether it
> > > ignores coverage? If so then maybe we could put the check in CanCombine
> > instead
> > > (and consider renaming AreEqual).
> > 
> > Ok, I get it now. In this case would it be preferable outcome to choose the
> > pipeline that had ignores=false rather than block batching? I'm not
proposing
> we
> > actually do that, but just trying to understand.
> > 
> > 
> > In the not too distant future we won't be making pipelines in batches until
> > flush time so this won't be an issue. At that time I think we will just
> include
> > the GP in the pipeline object rather than pass it separately. And that will
> all
> > occur after all batching has been performed so you would know the coverage
> state
> > of all the batched draws.
> 
> Yes that's right, if we're at the point that we've decided we want to combine
> the two batches then we'd rather not have pipe line issues prevent it from
> happening. If there was already an easy way to pick which pipeline you wanted
to
> keep then your suggestion would be ideal.
> 
> (e.g. instead of combining a new batch into itself, onCombine could return a
> pointer to the combined batch, potentially allowed to be "this", "that", or a
> brand new batch...)

Ok, given our future direction I think I'd prefer to keep things simple and land
this rather than complicate batch/pipeline selection in the short term. lgtm

[csmartdalton, 2016-07-06 16:10:05]

On 2016/07/06 16:09:05, bsalomon wrote:
> On 2016/07/06 16:00:14, csmartdalton wrote:
> > On 2016/07/06 15:47:13, bsalomon wrote:
> > > On 2016/07/06 15:22:24, csmartdalton wrote:
> > > > On 2016/07/06 13:21:15, bsalomon wrote:
> > > > > My intuition is that this is derived state and should be covered by
the
> > > > > GrProcessor comparisons and fixed function comparisons but I'd like
Greg
> > to
> > > > > weigh in.
> > > > 
> > > > The GP only implements computePipelineOptimizations. And then the
pipeline
> > > uses
> > > > those hints to decide under the hood, based on its XP and other things,
> > > whether
> > > > it will ignore coverage. The GP really can't know if the pipeline will
be
> > > > ignoring coverage without asking. IMO this is an important part of
> deciding
> > > > whether two pipelines are equal since it affects how the pipeline itself
> > > > behaves. in both cases the pipeline declares a coverage variable, but
when
> > > > ignoring coverage the pipeline just ignores that variable at the end of
> the
> > > > shader.
> > > > 
> > > > The problem I ran into was that a batch with meaningful coverage got
> > combined
> > > > with a batch that had “don't care” coverage (in other words would just
> write
> > > out
> > > > 1.0 to coverage when asked). This is a perfectly valid combination as
long
> > as
> > > > the two pipelines really are identical. However, if the pipeline that
gets
> > > kept
> > > > is secretly ignoring coverage, then the batch with meaningful coverage
> will
> > > > start drawing incorrectly.
> > > > 
> > > > Is there a usecase where we don't want AreEqual to think about whether
it
> > > > ignores coverage? If so then maybe we could put the check in CanCombine
> > > instead
> > > > (and consider renaming AreEqual).
> > > 
> > > Ok, I get it now. In this case would it be preferable outcome to choose
the
> > > pipeline that had ignores=false rather than block batching? I'm not
> proposing
> > we
> > > actually do that, but just trying to understand.
> > > 
> > > 
> > > In the not too distant future we won't be making pipelines in batches
until
> > > flush time so this won't be an issue. At that time I think we will just
> > include
> > > the GP in the pipeline object rather than pass it separately. And that
will
> > all
> > > occur after all batching has been performed so you would know the coverage
> > state
> > > of all the batched draws.
> > 
> > Yes that's right, if we're at the point that we've decided we want to
combine
> > the two batches then we'd rather not have pipe line issues prevent it from
> > happening. If there was already an easy way to pick which pipeline you
wanted
> to
> > keep then your suggestion would be ideal.
> > 
> > (e.g. instead of combining a new batch into itself, onCombine could return a
> > pointer to the combined batch, potentially allowed to be "this", "that", or
a
> > brand new batch...)
> 
> Ok, given our future direction I think I'd prefer to keep things simple and
land
> this rather than complicate batch/pipeline selection in the short term. lgtm

Agreed. Thanks.

[csmartdalton, 2016-07-06 16:10:15]

The CQ bit was checked by csmartdalton@google.com

[commit-bot: I haz the power, 2016-07-06 16:10:25]

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-07-06 16:47:40]

Description was changed from

==========
Check fIgnoresCoverage in GrPipeline::AreEqual

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

to

==========
Check fIgnoresCoverage in GrPipeline::AreEqual

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

Committed:
https://skia.googlesource.com/skia/+/afd6340d560846a8fa88d888cf460b6866237035
==========

[commit-bot: I haz the power, 2016-07-06 16:47:41]

Committed patchset #2 (id:20001) as
https://skia.googlesource.com/skia/+/afd6340d560846a8fa88d888cf460b6866237035