Apply coverage in XP base class when using dst reads

Apply coverage in XP base class when using dst reads

Moves the coverage logic into GrGLXferProcessor for XPs that perform
dst reads. XPs that don't use a dst read are still responsible to
handle coverage on their own.

BUG=skia:

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

[Chris Dalton, 2015-06-05 06:35:20]

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

[Chris Dalton, 2015-06-05 06:35:20]

I split the patch for mixed samples support into two logical changes.

[egdaniel, 2015-06-05 16:44:01]

Just the one minor naming question

https://codereview.chromium.org/1170553002/diff/1/src/gpu/gl/GrGLXferProcessor.h
File src/gpu/gl/GrGLXferProcessor.h (right):

https://codereview.chromium.org/1170553002/diff/1/src/gpu/gl/GrGLXferProcesso...
src/gpu/gl/GrGLXferProcessor.h:67: virtual void onEmitCode(const EmitArgs&) {
SkFAIL("onEmitCode not implemented."); }
So some pretty unhelpful feed back here...but something about the naming now of
onEmitCode and onEmitShaderBlend doesn't really shout out what these functions
do and when/what a subclass needs to implement for each. Maybe drop the on and
have them be emitShaderBlendCode and emitFixedFunctionBlendCode. Sorry not too
helpful on good names.

[Chris Dalton, 2015-06-05 17:41:37]

https://codereview.chromium.org/1170553002/diff/1/src/gpu/gl/GrGLXferProcessor.h
File src/gpu/gl/GrGLXferProcessor.h (right):

https://codereview.chromium.org/1170553002/diff/1/src/gpu/gl/GrGLXferProcesso...
src/gpu/gl/GrGLXferProcessor.h:67: virtual void onEmitCode(const EmitArgs&) {
SkFAIL("onEmitCode not implemented."); }
On 2015/06/05 16:44:01, egdaniel wrote:
> So some pretty unhelpful feed back here...but something about the naming now
of
> onEmitCode and onEmitShaderBlend doesn't really shout out what these functions
> do and when/what a subclass needs to implement for each. Maybe drop the on and
> have them be emitShaderBlendCode and emitFixedFunctionBlendCode. Sorry not too
> helpful on good names.

I've thought about it some and have come up with some more semi-good names :)

For the first I still wonder if "onEmitCode" might be ok? It would be consistent
with onGetBlendInfo and onHasSecondaryOutput, where it's the full implementation
of that function in the subclass, just only called when not doing a dst read.

For the second, I've so far come up with "emitBlendCodeForDstRead".

Thoughts?

[egdaniel, 2015-06-05 18:11:00]

On 2015/06/05 17:41:37, Chris Dalton wrote:
>
https://codereview.chromium.org/1170553002/diff/1/src/gpu/gl/GrGLXferProcessor.h
> File src/gpu/gl/GrGLXferProcessor.h (right):
> 
>
https://codereview.chromium.org/1170553002/diff/1/src/gpu/gl/GrGLXferProcesso...
> src/gpu/gl/GrGLXferProcessor.h:67: virtual void onEmitCode(const EmitArgs&) {
> SkFAIL("onEmitCode not implemented."); }
> On 2015/06/05 16:44:01, egdaniel wrote:
> > So some pretty unhelpful feed back here...but something about the naming now
> of
> > onEmitCode and onEmitShaderBlend doesn't really shout out what these
functions
> > do and when/what a subclass needs to implement for each. Maybe drop the on
and
> > have them be emitShaderBlendCode and emitFixedFunctionBlendCode. Sorry not
too
> > helpful on good names.
> 
> I've thought about it some and have come up with some more semi-good names :)
> 
> For the first I still wonder if "onEmitCode" might be ok? It would be
consistent
> with onGetBlendInfo and onHasSecondaryOutput, where it's the full
implementation
> of that function in the subclass, just only called when not doing a dst read.
> 
> For the second, I've so far come up with "emitBlendCodeForDstRead".
> 
> Thoughts?

I guess my feeling is I want it to seem like these are two mutual exclusives
paths the emitCode may take. Keeping onEmitCode makes it seem like that path
will always be called from emitCode and the DstRead version will be called in
addition if it is doing dst read. Just want it to be clear to a sub class that
for a given draw one or the other will be called and not both.

[Chris Dalton, 2015-06-05 18:58:58]

On 2015/06/05 18:11:00, egdaniel wrote:
> On 2015/06/05 17:41:37, Chris Dalton wrote:
> >
>
https://codereview.chromium.org/1170553002/diff/1/src/gpu/gl/GrGLXferProcessor.h
> > File src/gpu/gl/GrGLXferProcessor.h (right):
> > 
> >
>
https://codereview.chromium.org/1170553002/diff/1/src/gpu/gl/GrGLXferProcesso...
> > src/gpu/gl/GrGLXferProcessor.h:67: virtual void onEmitCode(const EmitArgs&)
{
> > SkFAIL("onEmitCode not implemented."); }
> > On 2015/06/05 16:44:01, egdaniel wrote:
> > > So some pretty unhelpful feed back here...but something about the naming
now
> > of
> > > onEmitCode and onEmitShaderBlend doesn't really shout out what these
> functions
> > > do and when/what a subclass needs to implement for each. Maybe drop the on
> and
> > > have them be emitShaderBlendCode and emitFixedFunctionBlendCode. Sorry not
> too
> > > helpful on good names.
> > 
> > I've thought about it some and have come up with some more semi-good names
:)
> > 
> > For the first I still wonder if "onEmitCode" might be ok? It would be
> consistent
> > with onGetBlendInfo and onHasSecondaryOutput, where it's the full
> implementation
> > of that function in the subclass, just only called when not doing a dst
read.
> > 
> > For the second, I've so far come up with "emitBlendCodeForDstRead".
> > 
> > Thoughts?
> 
> I guess my feeling is I want it to seem like these are two mutual exclusives
> paths the emitCode may take. Keeping onEmitCode makes it seem like that path
> will always be called from emitCode and the DstRead version will be called in
> addition if it is doing dst read. Just want it to be clear to a sub class that
> for a given draw one or the other will be called and not both.

Ah, that makes sense. So now I'm thinking:

emitOutputsForBlendState
emitBlendCodeForDstRead

Does this sound ok?

[egdaniel, 2015-06-05 19:31:24]

On 2015/06/05 18:58:58, Chris Dalton wrote:
> On 2015/06/05 18:11:00, egdaniel wrote:
> > On 2015/06/05 17:41:37, Chris Dalton wrote:
> > >
> >
>
https://codereview.chromium.org/1170553002/diff/1/src/gpu/gl/GrGLXferProcessor.h
> > > File src/gpu/gl/GrGLXferProcessor.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1170553002/diff/1/src/gpu/gl/GrGLXferProcesso...
> > > src/gpu/gl/GrGLXferProcessor.h:67: virtual void onEmitCode(const
EmitArgs&)
> {
> > > SkFAIL("onEmitCode not implemented."); }
> > > On 2015/06/05 16:44:01, egdaniel wrote:
> > > > So some pretty unhelpful feed back here...but something about the naming
> now
> > > of
> > > > onEmitCode and onEmitShaderBlend doesn't really shout out what these
> > functions
> > > > do and when/what a subclass needs to implement for each. Maybe drop the
on
> > and
> > > > have them be emitShaderBlendCode and emitFixedFunctionBlendCode. Sorry
not
> > too
> > > > helpful on good names.
> > > 
> > > I've thought about it some and have come up with some more semi-good names
> :)
> > > 
> > > For the first I still wonder if "onEmitCode" might be ok? It would be
> > consistent
> > > with onGetBlendInfo and onHasSecondaryOutput, where it's the full
> > implementation
> > > of that function in the subclass, just only called when not doing a dst
> read.
> > > 
> > > For the second, I've so far come up with "emitBlendCodeForDstRead".
> > > 
> > > Thoughts?
> > 
> > I guess my feeling is I want it to seem like these are two mutual exclusives
> > paths the emitCode may take. Keeping onEmitCode makes it seem like that path
> > will always be called from emitCode and the DstRead version will be called
in
> > addition if it is doing dst read. Just want it to be clear to a sub class
that
> > for a given draw one or the other will be called and not both.
> 
> Ah, that makes sense. So now I'm thinking:
> 
> emitOutputsForBlendState
> emitBlendCodeForDstRead
> 
> Does this sound ok?

yup

[egdaniel, 2015-06-05 23:48:37]

lgtm

[Chris Dalton, 2015-06-06 01:24:23]

The CQ bit was checked by cdalton@nvidia.com

[commit-bot: I haz the power, 2015-06-06 01:24:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1170553002/20001

[commit-bot: I haz the power, 2015-06-06 01:25:52]

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

[commit-bot: I haz the power, 2015-06-06 01:25:53]

Try jobs failed on following builders:
  skia_presubmit-Trybot on client.skia.fyi (JOB_FAILED,
http://build.chromium.org/p/client.skia.fyi/builders/skia_presubmit-Trybot/bu...)

[reed1, 2015-06-08 17:34:18]

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

[reed1, 2015-06-08 17:34:19]

api lgtm

[Chris Dalton, 2015-06-08 18:29:38]

The CQ bit was checked by cdalton@nvidia.com

[commit-bot: I haz the power, 2015-06-08 18:30:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1170553002/20001

[commit-bot: I haz the power, 2015-06-08 19:14:50]

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