Add GrEllipseEdgeEffect

Add GrEllipseEdgeEffect.

Adds the effect that replaces the old oval rendering code. Also hooks in code to set attribute names and indices for effects.

Committed: http://code.google.com/p/skia/source/detail?r=8092

[jvanverth1, 2013-03-05 20:02:03]

(Brian, I couldn't get the static Effect to work -- it kept crashing somewhere
while running gms.)

[robertphillips, 2013-03-05 20:30:13]

nits + some comment suggestions

https://codereview.chromium.org/12462008/diff/1/gyp/gpu.gypi
File gyp/gpu.gypi (right):

https://codereview.chromium.org/12462008/diff/1/gyp/gpu.gypi#newcode124
gyp/gpu.gypi:124: '<(skia_src_path)/gpu/effects/Gr1DKernelEffect.h',
What is texture strip atlas doing up here?

https://codereview.chromium.org/12462008/diff/1/include/gpu/GrEffect.h
File include/gpu/GrEffect.h (right):

https://codereview.chromium.org/12462008/diff/1/include/gpu/GrEffect.h#newcod...
include/gpu/GrEffect.h:182: 
two over length lines here

https://codereview.chromium.org/12462008/diff/1/include/gpu/GrEffect.h#newcod...
include/gpu/GrEffect.h:185: * The effect subclass manages the lifetime of the
name (this function only stores a pointer). This
remove "supposed to be"?

https://codereview.chromium.org/12462008/diff/1/include/gpu/GrEffectStage.h
File include/gpu/GrEffectStage.h (right):

https://codereview.chromium.org/12462008/diff/1/include/gpu/GrEffectStage.h#n...
include/gpu/GrEffectStage.h:187: void setVertexAttribIndices(const int
indices[], int count) {
Is there a reserve we can use here?

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

https://codereview.chromium.org/12462008/diff/1/src/gpu/GrContext.cpp#newcode...
src/gpu/GrContext.cpp:1108: if (isCircle) {
kCircleAttrBindings? As is I think just passing kEdge_... would be clearer.

https://codereview.chromium.org/12462008/diff/1/src/gpu/GrContext.cpp#newcode...
src/gpu/GrContext.cpp:1145: 
Why not "static const" style? Also, some words here about contention for the
stage?

https://codereview.chromium.org/12462008/diff/1/src/gpu/GrSWMaskHelper.cpp
File src/gpu/GrSWMaskHelper.cpp (right):

https://codereview.chromium.org/12462008/diff/1/src/gpu/GrSWMaskHelper.cpp#ne...
src/gpu/GrSWMaskHelper.cpp:196: // rendering (kGlyphMaskStage in GrTextContext)
shouldn't this be ellipse rendering?

https://codereview.chromium.org/12462008/diff/1/src/gpu/effects/GrEllipseEdge...
File src/gpu/effects/GrEllipseEdgeEffect.cpp (right):

https://codereview.chromium.org/12462008/diff/1/src/gpu/effects/GrEllipseEdge...
src/gpu/effects/GrEllipseEdgeEffect.cpp:1: /*
2013

https://codereview.chromium.org/12462008/diff/1/src/gpu/effects/GrEllipseEdge...
src/gpu/effects/GrEllipseEdgeEffect.cpp:36: 
Reprise the layout of the vec4? e.g., w == xRad/yRad?

https://codereview.chromium.org/12462008/diff/1/src/gpu/effects/GrEllipseEdge...
src/gpu/effects/GrEllipseEdgeEffect.cpp:48: static inline EffectKey GenKey(const
GrEffectStage& stage, const GrGLCaps&) {
// Why 0x1?

https://codereview.chromium.org/12462008/diff/1/src/gpu/effects/GrEllipseEdge...
src/gpu/effects/GrEllipseEdgeEffect.cpp:55: private:
unused?

https://codereview.chromium.org/12462008/diff/1/src/gpu/effects/GrEllipseEdge...
src/gpu/effects/GrEllipseEdgeEffect.cpp:66: void
GrEllipseEdgeEffect::getConstantColorComponents(GrColor* color, uint32_t*
validFlags) const {
Remove dead code?

https://codereview.chromium.org/12462008/diff/1/src/gpu/effects/GrEllipseEdge...
File src/gpu/effects/GrEllipseEdgeEffect.h (right):

https://codereview.chromium.org/12462008/diff/1/src/gpu/effects/GrEllipseEdge...
src/gpu/effects/GrEllipseEdgeEffect.h:16: * The output of this effect is a
modulation of the input color and coverage for an axis-aligned
overlength line

[robertphillips, 2013-03-05 20:30:26]

Otherwise lgtm

[bsalomon, 2013-03-05 20:42:45]

Three high level comment/questions:

1) We want to avoid adding code where effects directly manipulate fields on
GrGLShaderBuilder (fVSAttrs). I think we need an interface on GrGLShaderBuilder
to allow effects to add attributes.

2) What happens if effects are sharing an attribute? Can you bind two attribute
variables to the same attribute index in GL? If not, we may need the shader
builder to canonically manage the names of the attributes and give two effects
that request the same attribute index the same variable.

3) I'm wondering if you considered setting the attrib mapping and effect at
once. I wonder if it would give us a bit more safety/validation that they're
consistent. It could be that it's not practical but I'd guess that we always
know the mapping at the time we install the effect.

[jvanverth1, 2013-03-06 13:28:52]

On 2013/03/05 20:42:45, bsalomon wrote:
> Three high level comment/questions:
> 
> 1) We want to avoid adding code where effects directly manipulate fields on
> GrGLShaderBuilder (fVSAttrs). I think we need an interface on
GrGLShaderBuilder
> to allow effects to add attributes.

Okay, I can add that.

> 2) What happens if effects are sharing an attribute? Can you bind two
attribute
> variables to the same attribute index in GL? If not, we may need the shader
> builder to canonically manage the names of the attributes and give two effects
> that request the same attribute index the same variable.

Yeah, the interface I have here doesn't work well for sharing attributes (two
different effects could use different identifiers for the same attribute, which
is a no-no). I'll see if I can come up with something better.

> 3) I'm wondering if you considered setting the attrib mapping and effect at
> once. I wonder if it would give us a bit more safety/validation that they're
> consistent. It could be that it's not practical but I'd guess that we always
> know the mapping at the time we install the effect.

So something like setEffect(effect, attribs, attribcount)?

[bsalomon, 2013-03-06 14:07:05]

On 2013/03/06 13:28:52, JimVV wrote:
> On 2013/03/05 20:42:45, bsalomon wrote:
> > Three high level comment/questions:
> > 
> > 1) We want to avoid adding code where effects directly manipulate fields on
> > GrGLShaderBuilder (fVSAttrs). I think we need an interface on
> GrGLShaderBuilder
> > to allow effects to add attributes.
> 
> Okay, I can add that.
> 
> > 2) What happens if effects are sharing an attribute? Can you bind two
> attribute
> > variables to the same attribute index in GL? If not, we may need the shader
> > builder to canonically manage the names of the attributes and give two
effects
> > that request the same attribute index the same variable.
> 
> Yeah, the interface I have here doesn't work well for sharing attributes (two
> different effects could use different identifiers for the same attribute,
which
> is a no-no). I'll see if I can come up with something better.

It may mean that the we lose pretty names (e.g. "aAttrib0"). Oh well.
> 
> > 3) I'm wondering if you considered setting the attrib mapping and effect at
> > once. I wonder if it would give us a bit more safety/validation that they're
> > consistent. It could be that it's not practical but I'd guess that we always
> > know the mapping at the time we install the effect.
> 
> So something like setEffect(effect, attribs, attribcount)?

Something like that. Maybe the effect knows the attrib count?

[jvanverth1, 2013-03-06 18:35:42]

On 2013/03/06 14:07:05, bsalomon wrote:
> On 2013/03/06 13:28:52, JimVV wrote:
> > On 2013/03/05 20:42:45, bsalomon wrote:
> > > Three high level comment/questions:
> > > 
> > > 1) We want to avoid adding code where effects directly manipulate fields
on
> > > GrGLShaderBuilder (fVSAttrs). I think we need an interface on
> > GrGLShaderBuilder
> > > to allow effects to add attributes.
> > 
> > Okay, I can add that.
> > 
> > > 2) What happens if effects are sharing an attribute? Can you bind two
> > attribute
> > > variables to the same attribute index in GL? If not, we may need the
shader
> > > builder to canonically manage the names of the attributes and give two
> effects
> > > that request the same attribute index the same variable.
> > 
> > Yeah, the interface I have here doesn't work well for sharing attributes
(two
> > different effects could use different identifiers for the same attribute,
> which
> > is a no-no). I'll see if I can come up with something better.
> 
> It may mean that the we lose pretty names (e.g. "aAttrib0"). Oh well.

Thinking about it more, it might be okay. Since setting the attribute indices
has to go through GrDrawState, it could keep track and make sure that there's a
1:1 mapping between indices and names. Or maybe the client submits a list of
attribute name/index pairs. But that both assumes that the client knows the code
structure of the effect, and prevents the single call to setEffect().

As far as ensuring that two Effects use the same name for a shared attribute, we
could have a header file with the shared names in it.

> > > 3) I'm wondering if you considered setting the attrib mapping and effect
at
> > > once. I wonder if it would give us a bit more safety/validation that
they're
> > > consistent. It could be that it's not practical but I'd guess that we
always
> > > know the mapping at the time we install the effect.
> > 
> > So something like setEffect(effect, attribs, attribcount)?
> 
> Something like that. Maybe the effect knows the attrib count?

It does already, since it has an SkSTArray of attribute names.

[bsalomon, 2013-03-06 19:00:44]

On 2013/03/06 18:35:42, JimVV wrote:
> 
> Thinking about it more, it might be okay. Since setting the attribute indices
> has to go through GrDrawState, it could keep track and make sure that there's
a
> 1:1 mapping between indices and names. Or maybe the client submits a list of
> attribute name/index pairs. But that both assumes that the client knows the
code
> structure of the effect, and prevents the single call to setEffect().
> 
> As far as ensuring that two Effects use the same name for a shared attribute,
we
> could have a header file with the shared names in it.

I don't know that having informative names is the most important thing in this
context. I'm not sure if I totally get what you're saying about the header. So
maybe what I'm about to type is irrelevant. I imagine that the person who codes
the draw that points two effects at the same attrib maybe isn't the person who
wrote both the effects. Third party code may want to use one of our effects in
conjunction with their own custom effect.

Here's an idea (just throwing it out there, not committed to it at all):

The GrGLShaderBuilder interface used by an effect to add the attrib variable
takes a suggested name string and returns the actual name. The first effect to
reference an attrib index wins and its name is used for the GLSL var. A comment
could be added for each attrib declaration indicating which effects reference it
and what they would have called it.

> > > So something like setEffect(effect, attribs, attribcount)?
> > 
> > Something like that. Maybe the effect knows the attrib count?
> 
> It does already, since it has an SkSTArray of attribute names.

So maybe we shouldn't pass it in again to setEffect? It just seems like an
opportunity to get it wrong.

[jvanverth1, 2013-03-06 20:24:43]

On 2013/03/06 19:00:44, bsalomon wrote:
> I don't know that having informative names is the most important thing in this
> context. I'm not sure if I totally get what you're saying about the header. So
> maybe what I'm about to type is irrelevant. I imagine that the person who
codes
> the draw that points two effects at the same attrib maybe isn't the person who
> wrote both the effects. Third party code may want to use one of our effects in
> conjunction with their own custom effect.
> 
> Here's an idea (just throwing it out there, not committed to it at all):
> 
> The GrGLShaderBuilder interface used by an effect to add the attrib variable
> takes a suggested name string and returns the actual name. The first effect to
> reference an attrib index wins and its name is used for the GLSL var. A
comment
> could be added for each attrib declaration indicating which effects reference
it
> and what they would have called it.

My idea for the shared header is: say we have Effect A that wants to use, say, a
color attribute, and Effect B that wants to use the same color attribute. To
ensure they use the same one, we need some sort of key. We can't use the
attribute index because that's set by the client, not the Effects. So instead,
use the name "aColor" as the key. And to ensure that they use the same string,
have them include "GrEffectSharedAttribs.h" or some such thing and reference a
#define COL_ATTR_NAME. Any other shared attrib names could go in that same file.

However, I see that this doesn't work if you want to add a new third-party
Effect that uses one of our existing Effect's attributes.

> > > > So something like setEffect(effect, attribs, attribcount)?
> > > 
> > > Something like that. Maybe the effect knows the attrib count?
> > 
> > It does already, since it has an SkSTArray of attribute names.
> 
> So maybe we shouldn't pass it in again to setEffect? It just seems like an
> opportunity to get it wrong.

Yes, that makes sense.

[jvanverth1, 2013-03-07 15:21:54]

I also addressed two of the issues Brian raised (partial encapsulation of
GrGLShaderBuilder, and passing the attribute array into
GrDrawState::setEffect(). I still don't have a good answer for shared
attributes.

https://codereview.chromium.org/12462008/diff/1/gyp/gpu.gypi
File gyp/gpu.gypi (right):

https://codereview.chromium.org/12462008/diff/1/gyp/gpu.gypi#newcode124
gyp/gpu.gypi:124: '<(skia_src_path)/gpu/effects/Gr1DKernelEffect.h',
On 2013/03/05 20:30:13, robertphillips wrote:
> What is texture strip atlas doing up here?

Done. Also removed GrCircleEdgeEffect, which I didn't mean to include in this
change.

https://codereview.chromium.org/12462008/diff/1/include/gpu/GrEffect.h
File include/gpu/GrEffect.h (right):

https://codereview.chromium.org/12462008/diff/1/include/gpu/GrEffect.h#newcod...
include/gpu/GrEffect.h:182: 
On 2013/03/05 20:30:13, robertphillips wrote:
> two over length lines here

Done.

https://codereview.chromium.org/12462008/diff/1/include/gpu/GrEffect.h#newcod...
include/gpu/GrEffect.h:185: * The effect subclass manages the lifetime of the
name (this function only stores a pointer). This
On 2013/03/05 20:30:13, robertphillips wrote:
> remove "supposed to be"?

Done.

https://codereview.chromium.org/12462008/diff/1/include/gpu/GrEffectStage.h
File include/gpu/GrEffectStage.h (right):

https://codereview.chromium.org/12462008/diff/1/include/gpu/GrEffectStage.h#n...
include/gpu/GrEffectStage.h:187: void setVertexAttribIndices(const int
indices[], int count) {
On 2013/03/05 20:30:13, robertphillips wrote:
> Is there a reserve we can use here?

Done.

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

https://codereview.chromium.org/12462008/diff/1/src/gpu/GrContext.cpp#newcode...
src/gpu/GrContext.cpp:1108: if (isCircle) {
On 2013/03/05 20:30:13, robertphillips wrote:
> kCircleAttrBindings? As is I think just passing kEdge_... would be clearer.

Done.

https://codereview.chromium.org/12462008/diff/1/src/gpu/GrContext.cpp#newcode...
src/gpu/GrContext.cpp:1145: 
On 2013/03/05 20:30:13, robertphillips wrote:
> Why not "static const" style? Also, some words here about contention for the
> stage?

All the other stage constants use enums. Should I change them to static const?
And comment added.

https://codereview.chromium.org/12462008/diff/1/src/gpu/GrSWMaskHelper.cpp
File src/gpu/GrSWMaskHelper.cpp (right):

https://codereview.chromium.org/12462008/diff/1/src/gpu/GrSWMaskHelper.cpp#ne...
src/gpu/GrSWMaskHelper.cpp:196: // rendering (kGlyphMaskStage in GrTextContext)
On 2013/03/05 20:30:13, robertphillips wrote:
> shouldn't this be ellipse rendering?

Done.

https://codereview.chromium.org/12462008/diff/1/src/gpu/effects/GrEllipseEdge...
File src/gpu/effects/GrEllipseEdgeEffect.cpp (right):

https://codereview.chromium.org/12462008/diff/1/src/gpu/effects/GrEllipseEdge...
src/gpu/effects/GrEllipseEdgeEffect.cpp:1: /*
On 2013/03/05 20:30:13, robertphillips wrote:
> 2013

Done.

https://codereview.chromium.org/12462008/diff/1/src/gpu/effects/GrEllipseEdge...
src/gpu/effects/GrEllipseEdgeEffect.cpp:36: 
On 2013/03/05 20:30:13, robertphillips wrote:
> Reprise the layout of the vec4? e.g., w == xRad/yRad?

Done.

https://codereview.chromium.org/12462008/diff/1/src/gpu/effects/GrEllipseEdge...
src/gpu/effects/GrEllipseEdgeEffect.cpp:48: static inline EffectKey GenKey(const
GrEffectStage& stage, const GrGLCaps&) {
On 2013/03/05 20:30:13, robertphillips wrote:
> // Why 0x1?

Done.

https://codereview.chromium.org/12462008/diff/1/src/gpu/effects/GrEllipseEdge...
src/gpu/effects/GrEllipseEdgeEffect.cpp:55: private:
On 2013/03/05 20:30:13, robertphillips wrote:
> unused?

Done.

https://codereview.chromium.org/12462008/diff/1/src/gpu/effects/GrEllipseEdge...
src/gpu/effects/GrEllipseEdgeEffect.cpp:66: void
GrEllipseEdgeEffect::getConstantColorComponents(GrColor* color, uint32_t*
validFlags) const {
On 2013/03/05 20:30:13, robertphillips wrote:
> Remove dead code?

Done.

https://codereview.chromium.org/12462008/diff/1/src/gpu/effects/GrEllipseEdge...
File src/gpu/effects/GrEllipseEdgeEffect.h (right):

https://codereview.chromium.org/12462008/diff/1/src/gpu/effects/GrEllipseEdge...
src/gpu/effects/GrEllipseEdgeEffect.h:16: * The output of this effect is a
modulation of the input color and coverage for an axis-aligned
On 2013/03/05 20:30:13, robertphillips wrote:
> overlength line

Done.

[bsalomon, 2013-03-07 15:59:13]

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

https://codereview.chromium.org/12462008/diff/1012/src/gpu/GrContext.cpp#newc...
src/gpu/GrContext.cpp:1152: int edgeAttrIndex = 1;
static const?

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

https://codereview.chromium.org/12462008/diff/1012/src/gpu/GrDrawState.h#newc...
src/gpu/GrDrawState.h:492: fStages[stageIdx].setEffect(effect);
Any reason not to move this code to StageEffect::setEffect?

[jvanverth1, 2013-03-07 22:00:44]

I've set it up to automatically create the vertex attribute names for Effects,
as Brian suggested. Now each GrEffect sets a list of attribute types (using
GrSLType), and there's a corresponding list of indices set in the GrEffectStage.
The GrGLShaderBuilder takes these, auto-creates attribute names, and adds them
to the program if they aren't already there. The GrEffect can query the name to
use in its code.

Plus these changes:

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

https://codereview.chromium.org/12462008/diff/1012/src/gpu/GrContext.cpp#newc...
src/gpu/GrContext.cpp:1152: int edgeAttrIndex = 1;
On 2013/03/07 15:59:13, bsalomon wrote:
> static const?

Done.

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

https://codereview.chromium.org/12462008/diff/1012/src/gpu/GrDrawState.h#newc...
src/gpu/GrDrawState.h:492: fStages[stageIdx].setEffect(effect);
On 2013/03/07 15:59:13, bsalomon wrote:
> Any reason not to move this code to StageEffect::setEffect?

Done.

[bsalomon, 2013-03-08 14:31:58]

Most of my inline comments are minor stuff that you can take or leave.

I do think there should be some attribute validation call on GrDrawState that we
check at a high level (e.g. GrDrawTarget::checkDraw()) and bail if there are
inconsistencies.

The builder auto-mangles the uniform names to prefix with "u". Maybe we should
do the same with attribs ("a")?

The only part that leaves me a bit uneasy is that the type of the attribute is
determined by the first effect that references an index. If we had a validate
call maybe we could check that they're all consistent for any index (and assert
in the builder that this is true).

https://codereview.chromium.org/12462008/diff/27001/include/gpu/GrEffect.h
File include/gpu/GrEffect.h (right):

https://codereview.chromium.org/12462008/diff/27001/include/gpu/GrEffect.h#ne...
include/gpu/GrEffect.h:185: * Subclasses call this from their constructor to
register vertex attributes. This must only be
Should we document the current limitation here? (at most 2)

https://codereview.chromium.org/12462008/diff/27001/include/gpu/GrEffectStage.h
File include/gpu/GrEffectStage.h (right):

https://codereview.chromium.org/12462008/diff/27001/include/gpu/GrEffectStage...
include/gpu/GrEffectStage.h:144: int vertexAttribCount =
fVertexAttribIndices.count();
maybe we should add op== to SkTArray?

https://codereview.chromium.org/12462008/diff/27001/include/gpu/GrEffectStage...
include/gpu/GrEffectStage.h:178: const GrEffectRef* setEffect(const GrEffectRef*
EffectRef, const int* indices = NULL) {
attributeIndices or attrIndices?

https://codereview.chromium.org/12462008/diff/27001/src/gpu/GrDrawState.h
File src/gpu/GrDrawState.h (left):

https://codereview.chromium.org/12462008/diff/27001/src/gpu/GrDrawState.h#old...
src/gpu/GrDrawState.h:1026: kEllipse_EdgeType,
nice!

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

https://codereview.chromium.org/12462008/diff/27001/src/gpu/GrDrawState.h#new...
src/gpu/GrDrawState.h:486: const GrEffectRef* setEffect(int stageIdx, const
GrEffectRef* effect, const int* indices = NULL) {
line wrap

https://codereview.chromium.org/12462008/diff/27001/src/gpu/gl/GrGLProgram.cpp
File src/gpu/gl/GrGLProgram.cpp (right):

https://codereview.chromium.org/12462008/diff/27001/src/gpu/gl/GrGLProgram.cp...
src/gpu/gl/GrGLProgram.cpp:908: builder.fsCodeAppendf("\tif (all(lessThan(%s,
vec4(0.0)))) {\n\t\tdiscard;\n\t}\n", outCoverage.c_str());
shouldn't this be lessThanEqual?

[jvanverth1, 2013-03-08 17:39:55]

I've added the validate method, though I think it's too complex to run in
Release. It will get better once we cut down on the built-in attributes. I also
changed the attribute name prefix to "aAttr". 

There is an assert in GrGLShaderBuilder::addAttribute that ensures that checks
the type when we're adding an attribute that's already there.

https://codereview.chromium.org/12462008/diff/27001/include/gpu/GrEffect.h
File include/gpu/GrEffect.h (right):

https://codereview.chromium.org/12462008/diff/27001/include/gpu/GrEffect.h#ne...
include/gpu/GrEffect.h:185: * Subclasses call this from their constructor to
register vertex attributes. This must only be
On 2013/03/08 14:31:58, bsalomon wrote:
> Should we document the current limitation here? (at most 2)

Done. Also created a constant GrEffect::kMaxVertexAttribs.

https://codereview.chromium.org/12462008/diff/27001/include/gpu/GrEffectStage.h
File include/gpu/GrEffectStage.h (right):

https://codereview.chromium.org/12462008/diff/27001/include/gpu/GrEffectStage...
include/gpu/GrEffectStage.h:144: int vertexAttribCount =
fVertexAttribIndices.count();
On 2013/03/08 14:31:58, bsalomon wrote:
> maybe we should add op== to SkTArray?

Done.

https://codereview.chromium.org/12462008/diff/27001/include/gpu/GrEffectStage...
include/gpu/GrEffectStage.h:178: const GrEffectRef* setEffect(const GrEffectRef*
EffectRef, const int* indices = NULL) {
On 2013/03/08 14:31:58, bsalomon wrote:
> attributeIndices or attrIndices?

Done.

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

https://codereview.chromium.org/12462008/diff/27001/src/gpu/GrDrawState.h#new...
src/gpu/GrDrawState.h:486: const GrEffectRef* setEffect(int stageIdx, const
GrEffectRef* effect, const int* indices = NULL) {
On 2013/03/08 14:31:58, bsalomon wrote:
> line wrap

Done.

https://codereview.chromium.org/12462008/diff/27001/src/gpu/gl/GrGLProgram.cpp
File src/gpu/gl/GrGLProgram.cpp (right):

https://codereview.chromium.org/12462008/diff/27001/src/gpu/gl/GrGLProgram.cp...
src/gpu/gl/GrGLProgram.cpp:908: builder.fsCodeAppendf("\tif (all(lessThan(%s,
vec4(0.0)))) {\n\t\tdiscard;\n\t}\n", outCoverage.c_str());
On 2013/03/08 14:31:58, bsalomon wrote:
> shouldn't this be lessThanEqual?

Done.

[bsalomon, 2013-03-08 18:27:20]

lgtm (w/ some minor comments inline)

https://codereview.chromium.org/12462008/diff/52001/include/core/SkTArray.h
File include/core/SkTArray.h (right):

https://codereview.chromium.org/12462008/diff/52001/include/core/SkTArray.h#n...
include/core/SkTArray.h:284: int leftCount = count();
this->count();

https://codereview.chromium.org/12462008/diff/52001/include/gpu/GrEffect.h
File include/gpu/GrEffect.h (right):

https://codereview.chromium.org/12462008/diff/52001/include/gpu/GrEffect.h#ne...
include/gpu/GrEffect.h:187: * Subclasses call this from their constructor to
register vertex attributes (at most 2). This
(at most kMaxVertexAttribs) ?

https://codereview.chromium.org/12462008/diff/52001/src/gpu/gl/GrGLShaderBuil...
File src/gpu/gl/GrGLShaderBuilder.cpp (right):

https://codereview.chromium.org/12462008/diff/52001/src/gpu/gl/GrGLShaderBuil...
src/gpu/gl/GrGLShaderBuilder.cpp:512: SkString attributeName("aAttr");
I was thinking the prepend should be done in addAttribute so that the prefix is
also auto applied to Color, Coverage, Edge, ...

[commit-bot: I haz the power, 2013-03-12 12:25:47]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/jvanverth@google.com/12462008/71001

[commit-bot: I haz the power, 2013-03-12 12:26:12]

Change committed as 8092