Add coverage/color distinction to GrGLProgram

[bsalomon, 2011-05-16 13:37:22]

This change divides stages between color and coverage computing. In the skia
pipeline there are a couple things that should occur on the final computed color
before its partial coverage of a pixel should be considered. The first use of
this distinction is to apply the color filter correctly on the color output
before it is modulated by a mask filter or AA value.

This change doesn't actually use the new functionality. I broke up the change to
make life easier on reviewers. The full thing is here:
http://codereview.appspot.com/4535062/

[Scroggo, 2011-05-16 14:36:17]

http://codereview.appspot.com/4538064/diff/1/gpu/src/GrGLProgram.cpp
File gpu/src/GrGLProgram.cpp (right):

http://codereview.appspot.com/4538064/diff/1/gpu/src/GrGLProgram.cpp#newcode295
gpu/src/GrGLProgram.cpp:295: str->printf("(%s * %s)", src, value);
I think this should be:

str->printf("(%s * %s)", dst, value);

http://codereview.appspot.com/4538064/diff/1/gpu/src/GrGLProgram.cpp#newcode342
gpu/src/GrGLProgram.cpp:342: programData->fUniLocations.reset();
So if there is an unimplemented transfer mode we do not reset the uniform
locations?  Why?

http://codereview.appspot.com/4538064/diff/1/gpu/src/GrGLProgram.cpp#newcode374
gpu/src/GrGLProgram.cpp:374: switch (fProgramDesc.fColorType) {
Style guide says to include default always.

Also, kNone_ColorType is not included; it seems like one or the other should be
here.

http://codereview.appspot.com/4538064/diff/1/gpu/src/GrGLProgram.cpp#newcode449
gpu/src/GrGLProgram.cpp:449: if (!inColor.size() && (SkXfermode::kIDC_Coeff ==
uniformCoeff ||
Worth it to skip this part if needColorFilterUniform is already false? (I'm
assuming it will never switch from false to true here...)

http://codereview.appspot.com/4538064/diff/1/gpu/src/GrGLProgram.cpp#newcode451
gpu/src/GrGLProgram.cpp:451: needColorFilterUniform = false;
This will get set by needBlendInputs, below.

[bsalomon, 2011-05-16 17:14:11]

I took a look at the comments on the combined CL and am addressing them here as
well as scroggo's. Some apply to the second part and will address them before
upload it separately.

scroggo's comments
------------------
All comments accounted for.

junov's comments
----------------
>http://codereview.appspot.com/4535062/diff/1011/gpu/include/GrDrawTarget.h#ne...
>gpu/include/GrDrawTarget.h:54: kNumStages = 3,
>How does the fixed pipeline rendering path take this? If we don't care, maybe
>that path should be officially retired.
This are related to Pt 2 (coming soon!). The short answer: it still works the
same as before the CL, it had support for multiple stages. Longer answer: There
are lots of things that don't work on the fixed path (non-linear gradients,
edgeAA, etc, etc). I'd be all for getting rid of it except that we know people
are using it. I think our m.o. should be don't break what works but don't add
new functionality. When we get gm-gpu on the bots I'd like to have the fixed
pipe run so we can ensure the "don't break" part.


>http://codereview.appspot.com/4535062/diff/1011/gpu/src/GrGLProgram.cpp#newco...
>gpu/src/GrGLProgram.cpp:199: code->appendf("\t%s = %s;\n", outputVar, var1);
>This case (and the case below) adds unnecessary code to the shader. Even though
>the assignments are likely to get optimized-out, they give unnecessary work to
>the shader compiler. Suggestion : this function could return char* that points
>to the var name that contains the assignment result.  In the no-op cases,
return
>var0 or var1, otherwise return outputVar.  Similarly, the declaration of
>outputVar should be conditional to outputVar being used, so this function
should
>be responsible for declaring it if necessary.
>Maybe my suggestion is oversimplifying the issue though.  I just realized
>outputVar can be gl_FragColor.  It is still possible to eliminate the no-op
>assigment. But it is a little more complicated.

>http://codereview.appspot.com/4535062/diff/1011/gpu/src/GrGLProgram.cpp#newco...
>gpu/src/GrGLProgram.cpp:225: code->appendf("\t%s = %s;\n", outputVar, var1);
>Same comment as above.  Also, these two functions are very similar, perhaps a
>template could be used to avoid code duplication.

Yeah, the output can be a shader ouput, and the inputs can be expressions. I've
tried to avoid going down paths that calls these in many of the case when it'd
be optimized out. I think we're at the point where it makes sense to rethink how
the generator flows. Instead of just emitting code immediately we should build
up a tree that has tentative code injections but can be culled when we later
realize it would dead code. Something more like a real compiler parse tree.

I just tried rewriting these as templates, it wasn't any prettier :) The
different default values and getting strings into the templated version made it
messy.

>http://codereview.appspot.com/4535062/diff/1011/gpu/src/GrGLProgram.cpp#newco...
>gpu/src/GrGLProgram.cpp:292: str->printf("((vec4(1,1,1,1) - %s) * %s)", dst,
>value);
>the literal (1,1,1,1) has come up multiple time in this code change. It may be
>worth declaring a const in the shader code.

Done.

Reed's comments:
----------------
>http://codereview.appspot.com/4535062/diff/1011/gpu/src/GrGpuGLShaders.cpp#ne...
>gpu/src/GrGpuGLShaders.cpp:173: GrPrintf("\nTest Program %d\n-------------\n",
>t);
>wanna keep this in the CL?

I think its handy to have this block when I make a change to the generator and a
shader fails to compile. It makes it easy to step through the code generator on
that iteration.

>http://codereview.appspot.com/4535062/diff/1011/gpu/src/GrGpuGLShaders.cpp#ne...
>gpu/src/GrGpuGLShaders.cpp:191: int idx = (int)(random.nextF() *
>(SkXfermode::kMultiply_Mode+1));
>Why kMultiply here? Are you trying to get the number-of-xfermodes?

Not total number, but number of xfer modes expressible as SkXferMode::Coeffs. I
added a new count for this.

Other comments apply to Pt 2.

[reed1, 2011-05-16 17:36:00]

LGTM, but what do I know.

[bsalomon, 2011-05-16 19:45:50]

Justin and Leon, Any objections?

On 2011/05/16 17:36:00, reed1 wrote:
> LGTM, but what do I know.

[Leon, 2011-05-16 19:51:05]

No.  All my objections were addressed.

On Mon, May 16, 2011 at 3:45 PM, <bsalomon@google.com> wrote:

> Justin and Leon, Any objections?
>
>
> On 2011/05/16 17:36:00, reed1 wrote:
>
>> LGTM, but what do I know.
>>
>
>
>
> http://codereview.appspot.com/4538064/
>



-- 
Leon Scroggins III
scroggo@google.com

[junov, 2011-05-16 19:58:18]

http://codereview.appspot.com/4538064/diff/12001/gpu/src/GrGLProgram.cpp
File gpu/src/GrGLProgram.cpp (right):

http://codereview.appspot.com/4538064/diff/12001/gpu/src/GrGLProgram.cpp#newc...
gpu/src/GrGLProgram.cpp:211: code->appendf("\t%s = %s;\n", outputVar,
all_ones_vec(4));
In my previous review, I meant to suggest making this a GLSL const.  But I don't
feel that strongly about it.  This is fine.

[bsalomon, 2011-05-16 20:01:21]

On Mon, May 16, 2011 at 3:58 PM, <junov@google.com> wrote:

>
> http://codereview.appspot.com/4538064/diff/12001/gpu/src/GrGLProgram.cpp
>
> File gpu/src/GrGLProgram.cpp (right):
>
>
>
http://codereview.appspot.com/4538064/diff/12001/gpu/src/GrGLProgram.cpp#newc...
> gpu/src/GrGLProgram.cpp:211: code->appendf("\t%s = %s;\n", outputVar,
> all_ones_vec(4));
> In my previous review, I meant to suggest making this a GLSL const.  But
> I don't feel that strongly about it.  This is fine.
>
>
Oh, gotcha, not a bad idea. Will consider doing this in a future change.

[junov, 2011-05-16 20:11:39]

LGTM

On 2011/05/16 20:01:21, bsalomon wrote:
> On Mon, May 16, 2011 at 3:58 PM, <mailto:junov@google.com> wrote:
> 
> >
> > http://codereview.appspot.com/4538064/diff/12001/gpu/src/GrGLProgram.cpp
> >
> > File gpu/src/GrGLProgram.cpp (right):
> >
> >
> >
>
http://codereview.appspot.com/4538064/diff/12001/gpu/src/GrGLProgram.cpp#newc...
> > gpu/src/GrGLProgram.cpp:211: code->appendf("\t%s = %s;\n", outputVar,
> > all_ones_vec(4));
> > In my previous review, I meant to suggest making this a GLSL const.  But
> > I don't feel that strongly about it.  This is fine.
> >
> >
> Oh, gotcha, not a bad idea. Will consider doing this in a future change.

[bsalomon, 2011-05-16 20:56:26]

Closed with r1339