Initial change to create GeometryProcessor

Initial change to create GeometryProcessor

BUG=skia:

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

[joshua.litt, 2014-08-28 16:41:46]

joshualitt@chromium.org changed reviewers:
+ bsalomon@chromium.org

[joshua.litt, 2014-08-28 16:41:46]

This is a good first step towards making vertex effects their own thing.  It
looks like most of the bots are going to have a test or two fail from crc
miscompares, so I won't try and check this in until there are fewer ignored gms.

Still, this is more or less how the final change will look I think.

One thing, I will refactor all of this after we separate fragment effects from
geometry processors, so this CL doesn't have any of the cleanup which will
result from that.

[bsalomon, 2014-08-28 17:07:35]

bsalomon@google.com changed reviewers:
+ bsalomon@google.com, egdaniel@google.com, jvanverth@google.com,
robertphillips@google.com
- bsalomon@chromium.org

[bsalomon, 2014-08-28 17:24:00]

I will start taking a look. Adding everyone else from the gpu team since I think
we all may have an interest in this.

One thing I realized during my work on the resource cache is that what we've
been calling GrGeometryProcessor and GrFragmentEffect will need to have a common
base class for handling tracking of deferred reads and writes to GrGpuResources.
Rob and I just had a naming discussion. This is where we landed (of course open
to further suggestions):

GrProgramResource
    wrapper around a GrGpuResource that manages refs vs pending reads vs pending
writes.

GrProgramElement
    owns a bunch of GrProgramResources.
    can be owned by GrDrawTarget's client drawing code and/or have pending
executions in the GrIODB
    base class for the below classes.

GrColorProcessor
    rename of GrEffect.
    reads a color in / writes a color out

GrGeometryProcessor
    writes TS,VS,GS,FS code (restricted to coverage computation in FS)
    intimately tied to a particular client of GrDrawTarget (e.g. GrOvalRenderer)
and vertex array specification and data.
   

GrColorProcessor may be a little confusing since they're also used in the
coverage pipeline. But I think with inserting GrGP into the coverage pipeline,
the symmetry between color and coverage processing is starting to dissolve. We
also may get rid of uniform coverage (that is specifying a non-solid constant
coverage on GrDrawState). Moreover, we are currently unnecessarily making color
stages use 4 component inputs and outputs when the really could be scalar. So we
anticipate splitting out GrCoverageProcessor from GrColorProcessor.

This is maybe not the right venue for the above brain dump, but I wanted people
to have it in mind as they look at this change.

[robertphillips, 2014-08-29 15:04:53]

I defer to those that know this part of the code better but here are some nits.

https://codereview.chromium.org/509153002/diff/140001/include/gpu/GrEffectSta...
File include/gpu/GrEffectStage.h (right):

https://codereview.chromium.org/509153002/diff/140001/include/gpu/GrEffectSta...
include/gpu/GrEffectStage.h:31: 
We don't usually add the explicit empty INHERITED() calls.

https://codereview.chromium.org/509153002/diff/140001/src/gpu/GrContext.cpp
File src/gpu/GrContext.cpp (right):

https://codereview.chromium.org/509153002/diff/140001/src/gpu/GrContext.cpp#n...
src/gpu/GrContext.cpp:1741: SkASSERT(0 == fDrawState->numColorStages() && 0 ==
fDrawState->numCoverageStages() &&
Usually we do this as "NULL == fDrawState...". At least in Ganesh.

https://codereview.chromium.org/509153002/diff/140001/src/gpu/GrOvalRenderer.cpp
File src/gpu/GrOvalRenderer.cpp (right):

https://codereview.chromium.org/509153002/diff/140001/src/gpu/GrOvalRenderer....
src/gpu/GrOvalRenderer.cpp:817: drawState->setGeometryProcessor(effect,
kEllipseOuterOffsetAttrIndex,
Line this next line up ?

https://codereview.chromium.org/509153002/diff/140001/src/gpu/GrOvalRenderer....
src/gpu/GrOvalRenderer.cpp:1171: drawState->setGeometryProcessor(effect,
Line this up ?

https://codereview.chromium.org/509153002/diff/140001/src/gpu/gl/GrGLProgramD...
File src/gpu/gl/GrGLProgramDesc.h (right):

https://codereview.chromium.org/509153002/diff/140001/src/gpu/gl/GrGLProgramD...
src/gpu/gl/GrGLProgramDesc.h:75: 
Can this all fit on one line?

https://codereview.chromium.org/509153002/diff/140001/src/gpu/gl/builders/GrG...
File src/gpu/gl/builders/GrGLProgramBuilder.cpp (right):

https://codereview.chromium.org/509153002/diff/140001/src/gpu/gl/builders/GrG...
src/gpu/gl/builders/GrGLProgramBuilder.cpp:342: 
NULL != ?

https://codereview.chromium.org/509153002/diff/140001/src/gpu/gl/builders/GrG...
src/gpu/gl/builders/GrGLProgramBuilder.cpp:418: this->nameVariable(&inColorName,
'\0', "input");
I thought we were doing away with the manual '\t' and '\n' stuff ?

https://codereview.chromium.org/509153002/diff/140001/tests/GLProgramsTest.cpp
File tests/GLProgramsTest.cpp (right):

https://codereview.chromium.org/509153002/diff/140001/tests/GLProgramsTest.cp...
tests/GLProgramsTest.cpp:48: int offset = 0;
NULL != ?

[bsalomon, 2014-08-29 15:07:26]

https://codereview.chromium.org/509153002/diff/120001/src/gpu/GrDrawState.h
File src/gpu/GrDrawState.h (right):

https://codereview.chromium.org/509153002/diff/120001/src/gpu/GrDrawState.h#n...
src/gpu/GrDrawState.h:196: const GrEffect* addColorEffect(const GrEffect*
effect, int attr0 = -1, int attr1 = -1) {
Maybe a next small next change is to remove the attrs from addC*Effect().

https://codereview.chromium.org/509153002/diff/120001/src/gpu/GrRODrawState.cpp
File src/gpu/GrRODrawState.cpp (right):

https://codereview.chromium.org/509153002/diff/120001/src/gpu/GrRODrawState.c...
src/gpu/GrRODrawState.cpp:41: if (this->getGeometryProcessor() &&
that.getGeometryProcessor()) {
the empty if() block feels a little weird... but having trouble coming up with
anything significantly better...

if (this->getGP()) {
  if (!that.getGP()) {
     return kImcomp;
  } else if (!Compat(this->getGP(), that.getGP()) {
    return kImcomp;
  }
} else if (that.getGP()) {
   return kImcomp
}

https://codereview.chromium.org/509153002/diff/120001/src/gpu/GrRODrawState.c...
src/gpu/GrRODrawState.cpp:157: if (this->getGeometryProcessor()) {
Hopefully we can remove this feature from GP's once the class separation ocurrs.
I don't think they need it.

https://codereview.chromium.org/509153002/diff/120001/src/gpu/gl/GrGLProgramD...
File src/gpu/gl/GrGLProgramDesc.h (right):

https://codereview.chromium.org/509153002/diff/120001/src/gpu/gl/GrGLProgramD...
src/gpu/gl/GrGLProgramDesc.h:170: SkBool8                    
fSeparateCoverageFromColor;
eek! What's this for?

https://codereview.chromium.org/509153002/diff/120001/src/gpu/gl/GrGpuGL_prog...
File src/gpu/gl/GrGpuGL_program.cpp (right):

https://codereview.chromium.org/509153002/diff/120001/src/gpu/gl/GrGpuGL_prog...
src/gpu/gl/GrGpuGL_program.cpp:130: GrGLProgram* program =
GrGLProgram::Create(fGpu, desc, geometryProcessor, colorStages, coverageStages);
wrap line?

https://codereview.chromium.org/509153002/diff/140001/include/gpu/GrEffectSta...
File include/gpu/GrEffectStage.h (right):

https://codereview.chromium.org/509153002/diff/140001/include/gpu/GrEffectSta...
include/gpu/GrEffectStage.h:32: GrEffectStage(const GrEffectStage& other) :
INHERITED() {
We don't require explicit calls of default cons for members or base classes.

[joshua.litt, 2014-09-02 16:06:22]

I will push a new cl momentarily.

https://codereview.chromium.org/509153002/diff/120001/src/gpu/GrDrawState.h
File src/gpu/GrDrawState.h (right):

https://codereview.chromium.org/509153002/diff/120001/src/gpu/GrDrawState.h#n...
src/gpu/GrDrawState.h:196: const GrEffect* addColorEffect(const GrEffect*
effect, int attr0 = -1, int attr1 = -1) {
On 2014/08/29 15:07:25, bsalomon wrote:
> Maybe a next small next change is to remove the attrs from addC*Effect().

yep, I am going to do a refactoring pass after this cl lands

https://codereview.chromium.org/509153002/diff/120001/src/gpu/GrRODrawState.cpp
File src/gpu/GrRODrawState.cpp (right):

https://codereview.chromium.org/509153002/diff/120001/src/gpu/GrRODrawState.c...
src/gpu/GrRODrawState.cpp:41: if (this->getGeometryProcessor() &&
that.getGeometryProcessor()) {
On 2014/08/29 15:07:25, bsalomon wrote:
> the empty if() block feels a little weird... but having trouble coming up with
> anything significantly better...
> 
> if (this->getGP()) {
>   if (!that.getGP()) {
>      return kImcomp;
>   } else if (!Compat(this->getGP(), that.getGP()) {
>     return kImcomp;
>   }
> } else if (that.getGP()) {
>    return kImcomp
> }

Acknowledged.

https://codereview.chromium.org/509153002/diff/120001/src/gpu/GrRODrawState.c...
src/gpu/GrRODrawState.cpp:157: if (this->getGeometryProcessor()) {
On 2014/08/29 15:07:25, bsalomon wrote:
> Hopefully we can remove this feature from GP's once the class separation
ocurrs.
> I don't think they need it.

Acknowledged.

https://codereview.chromium.org/509153002/diff/120001/src/gpu/gl/GrGLProgramD...
File src/gpu/gl/GrGLProgramDesc.h (right):

https://codereview.chromium.org/509153002/diff/120001/src/gpu/gl/GrGLProgramD...
src/gpu/gl/GrGLProgramDesc.h:170: SkBool8                    
fSeparateCoverageFromColor;
On 2014/08/29 15:07:25, bsalomon wrote:
> eek! What's this for?
I might have done something dumb.  Basically, I use this in GrGLProgramBuilder
to determine if the GP takes as input either inputColor or inputCoverage.

https://codereview.chromium.org/509153002/diff/120001/src/gpu/gl/GrGpuGL_prog...
File src/gpu/gl/GrGpuGL_program.cpp (right):

https://codereview.chromium.org/509153002/diff/120001/src/gpu/gl/GrGpuGL_prog...
src/gpu/gl/GrGpuGL_program.cpp:130: GrGLProgram* program =
GrGLProgram::Create(fGpu, desc, geometryProcessor, colorStages, coverageStages);
On 2014/08/29 15:07:25, bsalomon wrote:
> wrap line?

Acknowledged.

https://codereview.chromium.org/509153002/diff/140001/include/gpu/GrEffectSta...
File include/gpu/GrEffectStage.h (right):

https://codereview.chromium.org/509153002/diff/140001/include/gpu/GrEffectSta...
include/gpu/GrEffectStage.h:31: 
On 2014/08/29 15:04:53, robertphillips wrote:
> We don't usually add the explicit empty INHERITED() calls.

Well, if I don't do this then I get a warning on mac:

[13:40:23.791929] ../../include/gpu/GrEffectStage.h: In copy constructor
‘GrEffectStage::GrEffectStage(const GrEffectStage&)’:
[13:40:23.791974] ../../include/gpu/GrEffectStage.h:32: warning: base class
‘class SkRefCnt’ should be explicitly initialized in the copy constructor

https://codereview.chromium.org/509153002/diff/140001/include/gpu/GrEffectSta...
include/gpu/GrEffectStage.h:32: GrEffectStage(const GrEffectStage& other) :
INHERITED() {
On 2014/08/29 15:07:25, bsalomon wrote:
> We don't require explicit calls of default cons for members or base classes.

see above

https://codereview.chromium.org/509153002/diff/140001/src/gpu/GrContext.cpp
File src/gpu/GrContext.cpp (right):

https://codereview.chromium.org/509153002/diff/140001/src/gpu/GrContext.cpp#n...
src/gpu/GrContext.cpp:1741: SkASSERT(0 == fDrawState->numColorStages() && 0 ==
fDrawState->numCoverageStages() &&
On 2014/08/29 15:04:53, robertphillips wrote:
> Usually we do this as "NULL == fDrawState...". At least in Ganesh.

Acknowledged.

https://codereview.chromium.org/509153002/diff/140001/src/gpu/GrOvalRenderer.cpp
File src/gpu/GrOvalRenderer.cpp (right):

https://codereview.chromium.org/509153002/diff/140001/src/gpu/GrOvalRenderer....
src/gpu/GrOvalRenderer.cpp:1171: drawState->setGeometryProcessor(effect,
On 2014/08/29 15:04:53, robertphillips wrote:
> Line this up ?

Acknowledged.

https://codereview.chromium.org/509153002/diff/140001/src/gpu/gl/GrGLProgramD...
File src/gpu/gl/GrGLProgramDesc.h (right):

https://codereview.chromium.org/509153002/diff/140001/src/gpu/gl/GrGLProgramD...
src/gpu/gl/GrGLProgramDesc.h:75: 
On 2014/08/29 15:04:53, robertphillips wrote:
> Can this all fit on one line?

Acknowledged.

https://codereview.chromium.org/509153002/diff/140001/src/gpu/gl/builders/GrG...
File src/gpu/gl/builders/GrGLProgramBuilder.cpp (right):

https://codereview.chromium.org/509153002/diff/140001/src/gpu/gl/builders/GrG...
src/gpu/gl/builders/GrGLProgramBuilder.cpp:342: 
On 2014/08/29 15:04:53, robertphillips wrote:
> NULL != ?

Acknowledged.

https://codereview.chromium.org/509153002/diff/140001/src/gpu/gl/builders/GrG...
src/gpu/gl/builders/GrGLProgramBuilder.cpp:418: this->nameVariable(&inColorName,
'\0', "input");
On 2014/08/29 15:04:53, robertphillips wrote:
> I thought we were doing away with the manual '\t' and '\n' stuff ?

Acknowledged.

https://codereview.chromium.org/509153002/diff/140001/tests/GLProgramsTest.cpp
File tests/GLProgramsTest.cpp (right):

https://codereview.chromium.org/509153002/diff/140001/tests/GLProgramsTest.cp...
tests/GLProgramsTest.cpp:48: int offset = 0;
On 2014/08/29 15:04:53, robertphillips wrote:
> NULL != ?

Acknowledged.

[joshua.litt, 2014-09-03 13:47:26]

all feedback should be incorporated

[egdaniel, 2014-09-03 14:21:25]

https://codereview.chromium.org/509153002/diff/240001/expectations/gm/ignored...
File expectations/gm/ignored-tests.txt (right):

https://codereview.chromium.org/509153002/diff/240001/expectations/gm/ignored...
expectations/gm/ignored-tests.txt:57: # joshualitt
Do you know what is causing these gm's to change? My initial feeling should be
that this change should have no effect on the actual final renderings

[bsalomon, 2014-09-03 21:07:00]

https://codereview.chromium.org/509153002/diff/240001/src/gpu/GrDrawState.cpp
File src/gpu/GrDrawState.cpp (right):

https://codereview.chromium.org/509153002/diff/240001/src/gpu/GrDrawState.cpp...
src/gpu/GrDrawState.cpp:86: fGeometryProcessor.reset(new
GrEffectStage(*that.fGeometryProcessor.get()));
SkNEW()

https://codereview.chromium.org/509153002/diff/240001/src/gpu/GrDrawState.cpp...
src/gpu/GrDrawState.cpp:319: fGeometryProcessor.reset(new
GrEffectStage(*ds->getGeometryProcessor()));
SkNEW()

https://codereview.chromium.org/509153002/diff/240001/src/gpu/gl/GrGLProgram.h
File src/gpu/gl/GrGLProgram.h (right):

https://codereview.chromium.org/509153002/diff/240001/src/gpu/gl/GrGLProgram....
src/gpu/gl/GrGLProgram.h:44: const GrEffectStage* geometryProcessor,
should/can this be const& for the gp param (here and below)?

https://codereview.chromium.org/509153002/diff/260001/src/gpu/GrDrawState.h
File src/gpu/GrDrawState.h (right):

https://codereview.chromium.org/509153002/diff/260001/src/gpu/GrDrawState.h#n...
src/gpu/GrDrawState.h:169: /// @name Effect Stages
Prolly should update this comment.

https://codereview.chromium.org/509153002/diff/260001/src/gpu/gl/GrGLProgramD...
File src/gpu/gl/GrGLProgramDesc.cpp (right):

https://codereview.chromium.org/509153002/diff/260001/src/gpu/gl/GrGLProgramD...
src/gpu/gl/GrGLProgramDesc.cpp:76: inputColorIsUsed =
effect->willUseInputColor();
I'm not sure that this is right since the GP is consuming the coverage attr/uni
not the color.

[joshua.litt, 2014-09-03 21:18:53]

https://codereview.chromium.org/509153002/diff/240001/src/gpu/gl/GrGLProgram.h
File src/gpu/gl/GrGLProgram.h (right):

https://codereview.chromium.org/509153002/diff/240001/src/gpu/gl/GrGLProgram....
src/gpu/gl/GrGLProgram.h:44: const GrEffectStage* geometryProcessor,
GrGpuGL_program uses a NULL pointer to indicate no GP.  Derefing it to make a
const ref would be a bit awkward, but I can do it for setdata. 
GrGLProgramBuilder uses the NULL pointer to indicate no GP so it would still
require a pointer.

On 2014/09/03 21:07:00, bsalomon wrote:
> should/can this be const& for the gp param (here and below)?

[bsalomon, 2014-09-04 13:17:40]

lgtm

https://codereview.chromium.org/509153002/diff/240001/src/gpu/gl/GrGLProgram.h
File src/gpu/gl/GrGLProgram.h (right):

https://codereview.chromium.org/509153002/diff/240001/src/gpu/gl/GrGLProgram....
src/gpu/gl/GrGLProgram.h:44: const GrEffectStage* geometryProcessor,
On 2014/09/03 21:18:53, joshua.litt wrote:
> GrGpuGL_program uses a NULL pointer to indicate no GP.  Derefing it to make a
> const ref would be a bit awkward, but I can do it for setdata. 
> GrGLProgramBuilder uses the NULL pointer to indicate no GP so it would still
> require a pointer.

Oh, of course. ptr is right.

> 
> On 2014/09/03 21:07:00, bsalomon wrote:
> > should/can this be const& for the gp param (here and below)?
>

https://codereview.chromium.org/509153002/diff/280001/src/gpu/gl/builders/GrG...
File src/gpu/gl/builders/GrGLProgramBuilder.h (right):

https://codereview.chromium.org/509153002/diff/280001/src/gpu/gl/builders/GrG...
src/gpu/gl/builders/GrGLProgramBuilder.h:361: virtual void
emitGeometryProcessor(const GrEffectStage* geometryProcessor,
assert that we don't call this?

[joshua.litt, 2014-09-04 15:15:36]

The CQ bit was checked by joshualitt@chromium.org

[commit-bot: I haz the power, 2014-09-04 15:16:28]

CQ is trying da patch. Follow status at

https://skia-tree-status.appspot.com/cq/joshualitt@chromium.org/509153002/300001

[commit-bot: I haz the power, 2014-09-04 15:16:36]

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

[commit-bot: I haz the power, 2014-09-04 15:16:37]

Failed to apply patch for expectations/gm/ignored-tests.txt:
While running git apply --index -p1;
  error: patch failed: expectations/gm/ignored-tests.txt:55
  error: expectations/gm/ignored-tests.txt: patch does not apply

Patch:       expectations/gm/ignored-tests.txt
Index: expectations/gm/ignored-tests.txt
diff --git a/expectations/gm/ignored-tests.txt
b/expectations/gm/ignored-tests.txt
index
52a1a79892cc58c79771e963079130f3836625de..8a90bf8dacf14d65880e5c3f5d62cf01cc271f42
100644
--- a/expectations/gm/ignored-tests.txt
+++ b/expectations/gm/ignored-tests.txt
@@ -55,3 +55,7 @@ downsamplebitmap_text_high_72.00pt
 downsamplebitmap_text_low_72.00pt
 downsamplebitmap_text_medium_72.00pt
 downsamplebitmap_text_none_72.00pt
+
+# joshualitt
+convex_poly_clip
+bezier_cubic_effects

[joshua.litt, 2014-09-04 15:23:58]

The CQ bit was checked by joshualitt@chromium.org

[commit-bot: I haz the power, 2014-09-04 15:24:39]

CQ is trying da patch. Follow status at

https://skia-tree-status.appspot.com/cq/joshualitt@chromium.org/509153002/320001

[commit-bot: I haz the power, 2014-09-04 15:33:21]

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

[commit-bot: I haz the power, 2014-09-04 15:33:22]

Try jobs failed on following builders:
  Build-Ubuntu13.10-GCC4.8-x86_64-Release-Trybot on tryserver.skia
(http://108.170.220.76:10117/builders/Build-Ubuntu13.10-GCC4.8-x86_64-Release-...)

[joshua.litt, 2014-09-04 15:41:34]

The CQ bit was checked by joshualitt@chromium.org

[commit-bot: I haz the power, 2014-09-04 15:41:43]

CQ is trying da patch. Follow status at

https://skia-tree-status.appspot.com/cq/joshualitt@chromium.org/509153002/360001

[commit-bot: I haz the power, 2014-09-04 15:56:50]

Committed patchset #19 (id:360001) as bd769d0f1c8cf6ccbb2738dfad1624a4c828e4eb