gl programs rewrite

gl programs rewrite

BUG=skia:

Committed: https://skia.googlesource.com/skia/+/07a255310aca9f3e83bf741dc663a58818ad681c

Committed: https://skia.googlesource.com/skia/+/651713408c5a5d9565665967ad09981250c7a8c9

[joshua.litt, 2014-10-05 16:04:29]

joshualitt@chromium.org changed reviewers:
+ bsalomon@google.com, egdaniel@google.com

[joshua.litt, 2014-10-05 16:04:30]

Let me know if I missed some area of randomness.

[egdaniel, 2014-10-06 14:19:02]

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

https://codereview.chromium.org/628633003/diff/120001/src/gpu/gl/GrGpuGL_prog...
src/gpu/gl/GrGpuGL_program.cpp:230: const GrGeometryStage*                    
geometryProcessor = NULL;
extra spaces?

https://codereview.chromium.org/628633003/diff/120001/tests/GLProgramsTest.cpp
File tests/GLProgramsTest.cpp (right):

https://codereview.chromium.org/628633003/diff/120001/tests/GLProgramsTest.cp...
tests/GLProgramsTest.cpp:29: const GrCacheID& cacheId,
move param up and onto same line

https://codereview.chromium.org/628633003/diff/120001/tests/GLProgramsTest.cp...
tests/GLProgramsTest.cpp:55: // than two attributes.  In addition, we 'pad' the
below array with GPs up to 6 entries.
add into comment that the 6 comes from 4 ff va's and the 2 general va's

https://codereview.chromium.org/628633003/diff/120001/tests/GLProgramsTest.cp...
tests/GLProgramsTest.cpp:56: GrVertexAttrib genericVertexAttribs[] = {
kGenericVertexAttribs

https://codereview.chromium.org/628633003/diff/120001/tests/GLProgramsTest.cp...
tests/GLProgramsTest.cpp:83: // TODO end test hack
What is this TODO for?

https://codereview.chromium.org/628633003/diff/120001/tests/GLProgramsTest.cp...
tests/GLProgramsTest.cpp:139: void set_random_color_coverage_stages(GrGpuGL*
gpu,
put some of these params on same line

[bsalomon, 2014-10-06 14:29:04]

I'm not sure that GrGpuGL is still the right place for this to live, but let's
punt on that until we have an alternate subclass.

https://codereview.chromium.org/628633003/diff/120001/include/gpu/GrTypes.h
File include/gpu/GrTypes.h (right):

https://codereview.chromium.org/628633003/diff/120001/include/gpu/GrTypes.h#n...
include/gpu/GrTypes.h:227: kPublicGrBlendCoeffCount
Do we still need this? or can we at least define it below as a static int?

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

https://codereview.chromium.org/628633003/diff/120001/src/gpu/GrOptDrawState....
src/gpu/GrOptDrawState.cpp:172: if (this->isColorWriteDisabled()) {
Greg is this the right place for this? Anywhere else we should be looking at
this value? It seems like we can make the blend flags be 0,1 in this case in the
blend opts stuff.

https://codereview.chromium.org/628633003/diff/120001/tests/GLProgramsTest.cpp
File tests/GLProgramsTest.cpp (right):

https://codereview.chromium.org/628633003/diff/120001/tests/GLProgramsTest.cp...
tests/GLProgramsTest.cpp:45: SkDEBUGFAIL("Could not allocate render target");
Maybe just SkDebugf? Hopefully this ripples up and causes the test to fail.

https://codereview.chromium.org/628633003/diff/120001/tests/GLProgramsTest.cp...
tests/GLProgramsTest.cpp:139: void set_random_color_coverage_stages(GrGpuGL*
gpu,
static?

https://codereview.chromium.org/628633003/diff/120001/tests/GLProgramsTest.cp...
tests/GLProgramsTest.cpp:139: void set_random_color_coverage_stages(GrGpuGL*
gpu,
On 2014/10/06 14:19:02, egdaniel wrote:
> put some of these params on same line

we definitely have mixed styles. In a lot of places once we wrap, we put each
param on its own line.

https://codereview.chromium.org/628633003/diff/120001/tests/GLProgramsTest.cp...
tests/GLProgramsTest.cpp:251:
gpu->drawState()->setHint(GrDrawState::kVertexColorsAreOpaque_Hint,
random->nextBool());
Maybe just go ahead with 

if (int i = 1, i <= kLast_Hint; i << 1) {
 gpu->drawState()->setHint(i, random->nextBool());
}

?

https://codereview.chromium.org/628633003/diff/120001/tests/GLProgramsTest.cp...
tests/GLProgramsTest.cpp:259: GR_STATIC_ASSERT(GrDrawState::kLastPublicStateBit
== GrDrawState::kCoverageDrawing_StateBit);
loop?

[egdaniel, 2014-10-07 13:46:13]

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

https://codereview.chromium.org/628633003/diff/120001/src/gpu/GrOptDrawState....
src/gpu/GrOptDrawState.cpp:172: if (this->isColorWriteDisabled()) {
It is already checked in the normal flow of creating an optimized state and
simply sets the blend coeffs to 0,1 and we skip draw. Just landed a change where
create will return a NULL ODS if we would end up drawing nothing. So don't need
to do this check here

On 2014/10/06 14:29:04, bsalomon wrote:
> Greg is this the right place for this? Anywhere else we should be looking at
> this value? It seems like we can make the blend flags be 0,1 in this case in
the
> blend opts stuff.

[joshua.litt, 2014-10-07 13:52:42]

feedback incorporated

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

https://codereview.chromium.org/628633003/diff/140001/tests/GLProgramsTest.cp...
tests/GLProgramsTest.cpp:406: *ods,
whoops I'll fix this before committing

[bsalomon, 2014-10-07 14:02:09]

lgtm

[joshua.litt, 2014-10-07 14:25:24]

The CQ bit was checked by joshualitt@chromium.org

[commit-bot: I haz the power, 2014-10-07 14:26:24]

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

[joshua.litt, 2014-10-07 14:28:31]

The CQ bit was unchecked by joshualitt@chromium.org

[joshua.litt, 2014-10-07 14:29:41]

The CQ bit was checked by joshualitt@chromium.org

[commit-bot: I haz the power, 2014-10-07 14:30:24]

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

[commit-bot: I haz the power, 2014-10-07 14:44:14]

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

[commit-bot: I haz the power, 2014-10-07 14:44:15]

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-10-07 14:55:57]

The CQ bit was checked by joshualitt@chromium.org

[commit-bot: I haz the power, 2014-10-07 14:56:55]

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

[commit-bot: I haz the power, 2014-10-07 15:05:25]

Committed patchset #10 (id:180001) as 07a255310aca9f3e83bf741dc663a58818ad681c

[joshua.litt, 2014-10-07 15:36:24]

A revert of this CL (patchset #10 id:180001) has been created in
https://codereview.chromium.org/631183003/ by joshualitt@chromium.org.

The reason for reverting is: breaks angle bot.

[joshua.litt, 2014-10-08 17:47:14]

Brian, you mind giving this a quick once over to make sure I captured
everything?

[bsalomon, 2014-10-08 17:54:05]

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

https://codereview.chromium.org/628633003/diff/250001/src/gpu/gl/GrGLProgramD...
src/gpu/gl/GrGLProgramDesc.cpp:308: // --------DO NOT MOVE HEADER ABOVE THIS
LINE--------------------------------------------------
Would this work?

{
    KeyHeader header = ...
    void fKey; // can't push back more data while header ptr is live

} // done with header.

[joshua.litt, 2014-10-08 18:55:13]

The CQ bit was checked by joshualitt@chromium.org

[commit-bot: I haz the power, 2014-10-08 18:56:26]

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

[commit-bot: I haz the power, 2014-10-09 01:18:46]

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

[commit-bot: I haz the power, 2014-10-09 01:18:49]

Failed to apply the patch.

[joshua.litt, 2014-10-09 14:24:23]

The CQ bit was checked by joshualitt@chromium.org

[commit-bot: I haz the power, 2014-10-09 14:25:23]

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

[commit-bot: I haz the power, 2014-10-09 14:25:40]

Committed patchset #15 (id:270001) as 651713408c5a5d9565665967ad09981250c7a8c9