Dual Source Blending

oops, meant to add people to reviewers not cc.

[bsalomon, 2011-05-19 19:14:00]

Fixes incorrect blending when xfer modeshas dst coeff of kZero, kSA, or kSC and
there is some form of coverage (mask-filter, glyph mask, edge AA).

[Stephen White, 2011-05-19 19:44:24]

LGTM.  My comments are really just structural -- I don't know much about this GL
extension to be honest.

http://codereview.appspot.com/4535088/diff/9021/gpu/src/GrDrawTarget.cpp
File gpu/src/GrDrawTarget.cpp (right):

http://codereview.appspot.com/4535088/diff/9021/gpu/src/GrDrawTarget.cpp#newc...
gpu/src/GrDrawTarget.cpp:547: if (s >= fCurrDrawState.fFirstCoverageStage) {
Maybe I'm being stupid, but will this ever be true?  It seems like the loop will
stop before this happens.  (Perhaps this case is now handled at the top of this
function).

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

http://codereview.appspot.com/4535088/diff/9021/gpu/src/GrGLProgram.cpp#newco...
gpu/src/GrGLProgram.cpp:754: GR_GL(AttachShader(programData->fProgramID,
programData->fFShaderID));
It seems like both CompileFSAndVS and bindOutputsAttribsAndLinkProgram() are
calling glAttachShader() now.  Is that intentional?

http://codereview.appspot.com/4535088/diff/9021/gpu/src/GrGpuGLShaders.cpp
File gpu/src/GrGpuGLShaders.cpp (right):

http://codereview.appspot.com/4535088/diff/9021/gpu/src/GrGpuGLShaders.cpp#ne...
gpu/src/GrGpuGLShaders.cpp:815: desc.fFirstCoverageStage =
fCurrDrawState.fFirstCoverageStage;
This is the same as line 811.  Intentional?

http://codereview.appspot.com/4535088/diff/9021/gpu/src/GrGpuGLShaders.cpp#ne...
gpu/src/GrGpuGLShaders.cpp:822: desc.fFirstCoverageStage =
fCurrDrawState.fFirstCoverageStage;
Same as line 817.

http://codereview.appspot.com/4535088/diff/9021/gpu/src/GrGpuGLShaders.cpp#ne...
gpu/src/GrGpuGLShaders.cpp:829: desc.fFirstCoverageStage =
fCurrDrawState.fFirstCoverageStage;
Same as line 824.

http://codereview.appspot.com/4535088/diff/9021/samplecode/SampleXfermodesBlu...
File samplecode/SampleXfermodesBlur.cpp (right):

http://codereview.appspot.com/4535088/diff/9021/samplecode/SampleXfermodesBlu...
samplecode/SampleXfermodesBlur.cpp:57: r.set(ww/3, hh/3, ww*19/20, hh*19/20);
These constants are all rather mysterious, but I suppose they probably came from
another, equally mysterious sample.  :)

[bsalomon, 2011-05-19 20:21:40]

I got to the point where I could no longer read what I'd written! :) All the
comments were correct and I made changes for them.

http://codereview.appspot.com/4535088/diff/9021/gpu/src/GrDrawTarget.cpp
File gpu/src/GrDrawTarget.cpp (right):

http://codereview.appspot.com/4535088/diff/9021/gpu/src/GrDrawTarget.cpp#newc...
gpu/src/GrDrawTarget.cpp:547: if (s >= fCurrDrawState.fFirstCoverageStage) {
On 2011/05/19 19:44:24, Stephen White wrote:
> Maybe I'm being stupid, but will this ever be true?  It seems like the loop
will
> stop before this happens.  (Perhaps this case is now handled at the top of
this
> function).

Agh, it's a stale if from before I added the loop at the top and changed the
start value for this loop. Good catch!

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

http://codereview.appspot.com/4535088/diff/9021/gpu/src/GrGLProgram.cpp#newco...
gpu/src/GrGLProgram.cpp:754: GR_GL(AttachShader(programData->fProgramID,
programData->fFShaderID));
On 2011/05/19 19:44:24, Stephen White wrote:
> It seems like both CompileFSAndVS and bindOutputsAttribsAndLinkProgram() are
> calling glAttachShader() now.  Is that intentional?

Nope, stale code from an intermediate state of this change.

http://codereview.appspot.com/4535088/diff/9021/gpu/src/GrGpuGLShaders.cpp
File gpu/src/GrGpuGLShaders.cpp (right):

http://codereview.appspot.com/4535088/diff/9021/gpu/src/GrGpuGLShaders.cpp#ne...
gpu/src/GrGpuGLShaders.cpp:815: desc.fFirstCoverageStage =
fCurrDrawState.fFirstCoverageStage;
On 2011/05/19 19:44:24, Stephen White wrote:
> This is the same as line 811.  Intentional?

I just really wanted to be sure it was set :)

http://codereview.appspot.com/4535088/diff/9021/samplecode/SampleXfermodesBlu...
File samplecode/SampleXfermodesBlur.cpp (right):

http://codereview.appspot.com/4535088/diff/9021/samplecode/SampleXfermodesBlu...
samplecode/SampleXfermodesBlur.cpp:57: r.set(ww/3, hh/3, ww*19/20, hh*19/20);
On 2011/05/19 19:44:24, Stephen White wrote:
> These constants are all rather mysterious, but I suppose they probably came
from
> another, equally mysterious sample.  :)

Ha, yeah copied from SampleXfermodes. I put a comment about what it's drawing.

[Stephen White, 2011-05-19 20:32:23]

LGTM

On 2011/05/19 20:21:40, bsalomon wrote:
> I got to the point where I could no longer read what I'd written! :) All the
> comments were correct and I made changes for them.
> 
> http://codereview.appspot.com/4535088/diff/9021/gpu/src/GrDrawTarget.cpp
> File gpu/src/GrDrawTarget.cpp (right):
> 
>
http://codereview.appspot.com/4535088/diff/9021/gpu/src/GrDrawTarget.cpp#newc...
> gpu/src/GrDrawTarget.cpp:547: if (s >= fCurrDrawState.fFirstCoverageStage) {
> On 2011/05/19 19:44:24, Stephen White wrote:
> > Maybe I'm being stupid, but will this ever be true?  It seems like the loop
> will
> > stop before this happens.  (Perhaps this case is now handled at the top of
> this
> > function).
> 
> Agh, it's a stale if from before I added the loop at the top and changed the
> start value for this loop. Good catch!
> 
> http://codereview.appspot.com/4535088/diff/9021/gpu/src/GrGLProgram.cpp
> File gpu/src/GrGLProgram.cpp (right):
> 
>
http://codereview.appspot.com/4535088/diff/9021/gpu/src/GrGLProgram.cpp#newco...
> gpu/src/GrGLProgram.cpp:754: GR_GL(AttachShader(programData->fProgramID,
> programData->fFShaderID));
> On 2011/05/19 19:44:24, Stephen White wrote:
> > It seems like both CompileFSAndVS and bindOutputsAttribsAndLinkProgram() are
> > calling glAttachShader() now.  Is that intentional?
> 
> Nope, stale code from an intermediate state of this change.
> 
> http://codereview.appspot.com/4535088/diff/9021/gpu/src/GrGpuGLShaders.cpp
> File gpu/src/GrGpuGLShaders.cpp (right):
> 
>
http://codereview.appspot.com/4535088/diff/9021/gpu/src/GrGpuGLShaders.cpp#ne...
> gpu/src/GrGpuGLShaders.cpp:815: desc.fFirstCoverageStage =
> fCurrDrawState.fFirstCoverageStage;
> On 2011/05/19 19:44:24, Stephen White wrote:
> > This is the same as line 811.  Intentional?
> 
> I just really wanted to be sure it was set :)
> 
>
http://codereview.appspot.com/4535088/diff/9021/samplecode/SampleXfermodesBlu...
> File samplecode/SampleXfermodesBlur.cpp (right):
> 
>
http://codereview.appspot.com/4535088/diff/9021/samplecode/SampleXfermodesBlu...
> samplecode/SampleXfermodesBlur.cpp:57: r.set(ww/3, hh/3, ww*19/20, hh*19/20);
> On 2011/05/19 19:44:24, Stephen White wrote:
> > These constants are all rather mysterious, but I suppose they probably came
> from
> > another, equally mysterious sample.  :)
> 
> Ha, yeah copied from SampleXfermodes. I put a comment about what it's drawing.

[junov, 2011-05-20 03:42:14]

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

http://codereview.appspot.com/4535088/diff/4003/gpu/src/GrGLProgram.cpp#newco...
gpu/src/GrGLProgram.cpp:191: if (var1IsScalar) {
I think this logic is unnecessary. The vec4 constructor is overloaded for
receiving scalars. The following should do the trick for both scalar and vector
literals:
code->appendf("\t%s = vec4(%s);\n", outputVar, var1);

[bsalomon, 2011-05-20 14:14:34]

junov is correct. I made that change and checked in at r1390. Closed.