Default geometry processor

Default geometry processor

BUG=skia:

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

Committed: https://skia.googlesource.com/skia/+/4973d9da4aeb7c4d8b8e67e167586c7cc9534eee

[joshualitt, 2014-10-26 19:42:59]

joshualitt@google.com changed reviewers:
+ bsalomon@google.com, joshualitt@google.com

[joshualitt, 2014-10-26 19:43:00]

Brian, no hurry on reviewing this, its the patch before the GeoShader patch. 
Most of the change is in program builder / vertex builder, and the
DefaultGPFactory which is all new.

[bsalomon, 2014-10-27 13:43:43]

I haven't made it all the way through yet. Some initial thoughts inline

https://codereview.chromium.org/678953002/diff/60001/include/gpu/GrGeometryPr...
File include/gpu/GrGeometryProcessor.h (right):

https://codereview.chromium.org/678953002/diff/60001/include/gpu/GrGeometryPr...
include/gpu/GrGeometryProcessor.h:43: const char* uViewM() const { return
"uViewM"; }
Does any code outside of the geo processor subclass itself need to know how the
view transformation occurs?  Why not just send the draw state's view matrix to
GrGLGP's setData function and let it handle it however it likes without exposing
any of the that through the base class?

https://codereview.chromium.org/678953002/diff/60001/src/gpu/GrAARectRenderer...
File src/gpu/GrAARectRenderer.cpp (right):

https://codereview.chromium.org/678953002/diff/60001/src/gpu/GrAARectRenderer...
src/gpu/GrAARectRenderer.cpp:21: class GrAlignedRectEffect : public
GrGeometryProcessor {
Why don't we just delete these unused effects (separately) rather than modifying
them. We aren't testing them, they are slower in too many cases, they don't
actually draw correctly, and if go back to this approach we base it off the
other analytic rect processors that are currently used for clipping.

https://codereview.chromium.org/678953002/diff/60001/src/gpu/GrDefaultGeoProc...
File src/gpu/GrDefaultGeoProcFactory.cpp (right):

https://codereview.chromium.org/678953002/diff/60001/src/gpu/GrDefaultGeoProc...
src/gpu/GrDefaultGeoProcFactory.cpp:17: * .  It also leaves coverage untouched. 
Behind the scenes, we may add per vertex color or local
odd wrapping here

https://codereview.chromium.org/678953002/diff/60001/src/gpu/GrDefaultGeoProc...
File src/gpu/GrDefaultGeoProcFactory.h (right):

https://codereview.chromium.org/678953002/diff/60001/src/gpu/GrDefaultGeoProc...
src/gpu/GrDefaultGeoProcFactory.h:73: kUV_GPType = 0x02,
Is this local coords? "UV" doesn't seem right to me.

https://codereview.chromium.org/678953002/diff/60001/src/gpu/gl/GrGLProgramDe...
File src/gpu/gl/GrGLProgramDesc.cpp (right):

https://codereview.chromium.org/678953002/diff/60001/src/gpu/gl/GrGLProgramDe...
src/gpu/gl/GrGLProgramDesc.cpp:263: // We still have a default geometry
processor installed on the opt draw state, but it is
I think we will have to figure out how to make the path draws work with no GP.
Maybe this comes about when we create this geometry bundle object.

[joshualitt, 2014-11-06 22:35:11]

Feedback incorporated

>
https://codereview.chromium.org/678953002/diff/60001/include/gpu/GrGeometryPr...
> include/gpu/GrGeometryProcessor.h:43: const char* uViewM() const { return
> "uViewM"; }
> Does any code outside of the geo processor subclass itself need to know how
the
> view transformation occurs?  Why not just send the draw state's view matrix to
> GrGLGP's setData function and let it handle it however it likes without
exposing
> any of the that through the base class?

Would it be alright to do this in the follow up CL?  I first need to force
renderers to set the default gp explicitly and then we can have them store the
viewM if they need it in their processor's constructor.

[bsalomon, 2014-11-07 14:44:03]

I'm ok with landing as is (lgtm). The bit about glPosition is something I think
should be done, but can be follow-up. If not done now I think the function
should be renamed to something like transformToNormalizedDeviceSpace for
clarity.

https://codereview.chromium.org/678953002/diff/100001/src/gpu/GrDefaultGeoPro...
File src/gpu/GrDefaultGeoProcFactory.cpp (right):

https://codereview.chromium.org/678953002/diff/100001/src/gpu/GrDefaultGeoPro...
src/gpu/GrDefaultGeoProcFactory.cpp:86: GrVertexAttrib kDefaultPositionGeoProc[]
= {
moving the vertex attribs themselves to GP will be a nice cleanup here. (just a
comment)

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

https://codereview.chromium.org/678953002/diff/100001/src/gpu/GrOptDrawState....
src/gpu/GrOptDrawState.cpp:69: // TODO(joshualitt) processors should do this
themselves
don't care if you change it, but just want to clarify that you mean rendering
code should do this right (as opposed to processors)?

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

https://codereview.chromium.org/678953002/diff/100001/src/gpu/GrOvalRenderer....
src/gpu/GrOvalRenderer.cpp:106:
vsBuilder->transformPositionToDeviceSpace("pos3");
This name is a bit confusing. I'd expect it to take a local coordinate to Skia's
non-normalized device space. But it actually converts from Skia device space to
GL normalized device space.

Can't we just make the builder auto convert gl_Position as the last line of the
shader without being asked to? For the most part processors only have to think
in terms of Skia's coordinate systems, it'd be nice to do that here too.

[joshualitt, 2014-11-07 19:26:05]

The CQ bit was checked by joshualitt@google.com

[commit-bot: I haz the power, 2014-11-07 19:26:30]

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

[commit-bot: I haz the power, 2014-11-07 19:29:46]

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

[commit-bot: I haz the power, 2014-11-07 19:29:47]

Try jobs failed on following builders:
  Build-Win-VS2013-x86-Debug-Trybot on client.skia.compile
(http://build.chromium.org/p/client.skia.compile/builders/Build-Win-VS2013-x86...)

[joshualitt, 2014-11-07 19:37:53]

The CQ bit was checked by joshualitt@google.com

[commit-bot: I haz the power, 2014-11-07 19:38:47]

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

[commit-bot: I haz the power, 2014-11-07 19:47:13]

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

[joshualitt, 2014-11-07 20:06:28]

A revert of this CL (patchset #9 id:160001) has been created in
https://codereview.chromium.org/691313003/ by joshualitt@google.com.

The reason for reverting is: breaks nexus 5.

[joshualitt, 2014-11-08 16:34:55]

The CQ bit was checked by joshualitt@google.com

[commit-bot: I haz the power, 2014-11-08 16:35:45]

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

[commit-bot: I haz the power, 2014-11-08 17:24:29]

Committed patchset #10 (id:180001) as
https://skia.googlesource.com/skia/+/4973d9da4aeb7c4d8b8e67e167586c7cc9534eee