GPU Gradients

Two and three color GPU gradients without textures.

R=bsalomon@google.com

Committed: https://code.google.com/p/skia/source/detail?r=11158

[dierk, 2013-08-13 00:29:20]

Here's the latest update on GPU gradients. Intended to be tested with
GM:gradients_gpu, and currently runs and draws blank (white) gradients for 2
color gradients (and 3 color not far off).

Stuff to go in next: (which will hopefully fix the non-drawing issue..)
 - Add support for 3 color gradients (just need to change usage of mix so that i
uses all three color uniforms)
 - Using the colors provided from the gradient instead of silly hardcoded values
 - Modulating the input color instead of just setting the output color to
something

More coming soon...

[reed1, 2013-08-13 12:33:41]

just nits -- leaving the meat of the review to brian

https://codereview.chromium.org/22854005/diff/11001/samplecode/SampleApp.cpp
File samplecode/SampleApp.cpp (right):

https://codereview.chromium.org/22854005/diff/11001/samplecode/SampleApp.cpp#...
samplecode/SampleApp.cpp:87: #define DEFAULT_TO_GPU 1
// can we have a comment describing this flag

https://codereview.chromium.org/22854005/diff/11001/src/effects/gradients/SkG...
File src/effects/gradients/SkGradientShaderPriv.h (right):

https://codereview.chromium.org/22854005/diff/11001/src/effects/gradients/SkG...
src/effects/gradients/SkGradientShaderPriv.h:252: int numColors;
nit:
- does this guy really need to be public?
- why not name it fNumColors?

[bsalomon, 2013-08-13 14:28:36]

Idea: The YCoord and NumColors are never both valid, they're mutually exclusive.
Would it be clearer or uglier to have a single field where >= 0 is a texture row
and < 0 is a special case color count? It might make the isEqual a bit simpler.

https://codereview.chromium.org/22854005/diff/11001/gm/gradients_gpu.cpp
File gm/gradients_gpu.cpp (right):

https://codereview.chromium.org/22854005/diff/11001/gm/gradients_gpu.cpp#newc...
gm/gradients_gpu.cpp:110: for (size_t j = 0; j < 1; ++j){
//SK_ARRAY_COUNT(gGradMakers); ++j) {
Is this intended?

https://codereview.chromium.org/22854005/diff/11001/samplecode/SampleApp.cpp
File samplecode/SampleApp.cpp (right):

https://codereview.chromium.org/22854005/diff/11001/samplecode/SampleApp.cpp#...
samplecode/SampleApp.cpp:87: #define DEFAULT_TO_GPU 1
On 2013/08/13 12:33:41, reed1 wrote:
> // can we have a comment describing this flag

I think this probably should not be checked in.

https://codereview.chromium.org/22854005/diff/11001/src/effects/gradients/SkG...
File src/effects/gradients/SkGradientShader.cpp (right):

https://codereview.chromium.org/22854005/diff/11001/src/effects/gradients/SkG...
src/effects/gradients/SkGradientShader.cpp:883: GrGLEffect::EffectKey
GrGLGradientEffect::GenMatrixKey(const GrDrawEffect& drawEffect) {
Since this is no longer just doing the matrix it should probably be renamed to
something more accurate. GenMatrixAndColorTypeKey(), GenBaseGradientKey(), ...

https://codereview.chromium.org/22854005/diff/11001/src/effects/gradients/SkG...
src/effects/gradients/SkGradientShader.cpp:891: EffectKey key =
GrGLEffectMatrix::GenKey(e.getMatrix(), drawEffect, kCoordsType, texture);
Should probably only call GrGLEffectMatrix::GenKey inside the above if.

https://codereview.chromium.org/22854005/diff/11001/src/effects/gradients/SkG...
src/effects/gradients/SkGradientShader.cpp:969: if (info.fColorCount == 2) { //
if 2 color
Style nit: 2 == info.fColorCount

https://codereview.chromium.org/22854005/diff/11001/src/effects/gradients/SkG...
src/effects/gradients/SkGradientShader.cpp:973: } else if (info.fColorCount == 3
&& (abs(pos[1] - 0.5) < 0.01)) { // if 3 color symmetric
I think there is an abs for SkScalar.

https://codereview.chromium.org/22854005/diff/11001/src/effects/gradients/SkG...
src/effects/gradients/SkGradientShader.cpp:978: numColors = 4;
? This at least deserves a comment since the number of colors is not really
necessarily 4.

https://codereview.chromium.org/22854005/diff/11001/src/effects/gradients/SkG...
src/effects/gradients/SkGradientShader.cpp:1024: this->useAtlas() ==
s.useAtlas() &&
Is useAtlas() equivalent to NULL == fTextureAccess.getTexture()? If so, we
already checked that.

https://codereview.chromium.org/22854005/diff/11001/src/effects/gradients/SkG...
src/effects/gradients/SkGradientShader.cpp:1027: && this->numColors ==
s.numColors; // do I need this check?
Yes, you probably need to compare the colors too for the 2/3 cases. This is done
implicitly for the texture case by checking that they refer to the same row of
the same texture.

https://codereview.chromium.org/22854005/diff/11001/src/effects/gradients/SkG...
File src/effects/gradients/SkGradientShaderPriv.h (right):

https://codereview.chromium.org/22854005/diff/11001/src/effects/gradients/SkG...
src/effects/gradients/SkGradientShaderPriv.h:306: static inline bool
is2Color(const GrGradientEffect& e){
How about an enum named ColorType (or something like that) and values like
kTwoColor, kThreeColor, kTexture and have two functions (one for
GrGradientEffect and one for EffectKey) that return the type.

style-nit: static members have initial caps

Can you make the key values (0x4000 and 0x6000) enum values alongside
kMatrixKeyBitCnt above?  It might also be good to define them in terms of
kMatrixKeyBitCnt to ensure that they don't collide should
GrGLEffectMatrix::kKeyBits ever change.

https://codereview.chromium.org/22854005/diff/11001/src/effects/gradients/SkG...
src/effects/gradients/SkGradientShaderPriv.h:346: // emit code that gets a
fragment's color from an expression for t; Only used if 4 or more
Do you mean the sampler array is only used for 4+? It seems like this function
is still called for all cases by the subclasses.

[bsalomon, 2013-08-13 15:23:31]

One other thought... we already have a lot of gradient GMs. Can we adapt one of
them to use 2 and 3 color gradients rather than adding a new test? It's not a
huge deal but each additional test adds to bot cycle times a little bit and is
another thing to rebaseline.

[dierk, 2013-08-13 19:50:17]

https://codereview.chromium.org/22854005/diff/11001/gm/gradients_gpu.cpp
File gm/gradients_gpu.cpp (right):

https://codereview.chromium.org/22854005/diff/11001/gm/gradients_gpu.cpp#newc...
gm/gradients_gpu.cpp:110: for (size_t j = 0; j < 1; ++j){
//SK_ARRAY_COUNT(gGradMakers); ++j) {
Was done for test purposes when modifications were made to the LinearGradient
inherited class but not others. Since changes were pushed into base class I've
switched it back.

On 2013/08/13 14:28:37, bsalomon wrote:
> Is this intended?

https://codereview.chromium.org/22854005/diff/11001/samplecode/SampleApp.cpp
File samplecode/SampleApp.cpp (right):

https://codereview.chromium.org/22854005/diff/11001/samplecode/SampleApp.cpp#...
samplecode/SampleApp.cpp:87: #define DEFAULT_TO_GPU 1
On 2013/08/13 12:33:41, reed1 wrote:
> // can we have a comment describing this flag

Done.

https://codereview.chromium.org/22854005/diff/11001/src/effects/gradients/SkG...
File src/effects/gradients/SkGradientShader.cpp (right):

https://codereview.chromium.org/22854005/diff/11001/src/effects/gradients/SkG...
src/effects/gradients/SkGradientShader.cpp:883: GrGLEffect::EffectKey
GrGLGradientEffect::GenMatrixKey(const GrDrawEffect& drawEffect) {
On 2013/08/13 14:28:37, bsalomon wrote:
> Since this is no longer just doing the matrix it should probably be renamed to
> something more accurate. GenMatrixAndColorTypeKey(), GenBaseGradientKey(), ...

Done.

https://codereview.chromium.org/22854005/diff/11001/src/effects/gradients/SkG...
src/effects/gradients/SkGradientShader.cpp:891: EffectKey key =
GrGLEffectMatrix::GenKey(e.getMatrix(), drawEffect, kCoordsType, texture);
Why is that? I thought I needed the matrix key to be generated in the 2 and 3
color cases as well (hence why I was only seeing the end color of the mix
before)

On 2013/08/13 14:28:37, bsalomon wrote:
> Should probably only call GrGLEffectMatrix::GenKey inside the above if.

https://codereview.chromium.org/22854005/diff/11001/src/effects/gradients/SkG...
src/effects/gradients/SkGradientShader.cpp:969: if (info.fColorCount == 2) { //
if 2 color
On 2013/08/13 14:28:37, bsalomon wrote:
> Style nit: 2 == info.fColorCount

Done.

https://codereview.chromium.org/22854005/diff/11001/src/effects/gradients/SkG...
src/effects/gradients/SkGradientShader.cpp:973: } else if (info.fColorCount == 3
&& (abs(pos[1] - 0.5) < 0.01)) { // if 3 color symmetric
On 2013/08/13 14:28:37, bsalomon wrote:
> I think there is an abs for SkScalar.

Done.

https://codereview.chromium.org/22854005/diff/11001/src/effects/gradients/SkG...
src/effects/gradients/SkGradientShader.cpp:978: numColors = 4;
On 2013/08/13 14:28:37, bsalomon wrote:
> ? This at least deserves a comment since the number of colors is not really
> necessarily 4.

Done.

https://codereview.chromium.org/22854005/diff/11001/src/effects/gradients/SkG...
src/effects/gradients/SkGradientShader.cpp:1024: this->useAtlas() ==
s.useAtlas() &&
I'm not sure. It was already there.

The useAtlas() function returns true if fRow == -1, is that the same?

On 2013/08/13 14:28:37, bsalomon wrote:
> Is useAtlas() equivalent to NULL == fTextureAccess.getTexture()? If so, we
> already checked that.

https://codereview.chromium.org/22854005/diff/11001/src/effects/gradients/SkG...
src/effects/gradients/SkGradientShader.cpp:1027: && this->numColors ==
s.numColors; // do I need this check?
Will add once colors are piped in.

On 2013/08/13 14:28:37, bsalomon wrote:
> Yes, you probably need to compare the colors too for the 2/3 cases. This is
done
> implicitly for the texture case by checking that they refer to the same row of
> the same texture.

https://codereview.chromium.org/22854005/diff/11001/src/effects/gradients/SkG...
File src/effects/gradients/SkGradientShaderPriv.h (right):

https://codereview.chromium.org/22854005/diff/11001/src/effects/gradients/SkG...
src/effects/gradients/SkGradientShaderPriv.h:252: int numColors;
On 2013/08/13 12:33:41, reed1 wrote:
> nit:
> - does this guy really need to be public?
> - why not name it fNumColors?

Done.

https://codereview.chromium.org/22854005/diff/11001/src/effects/gradients/SkG...
src/effects/gradients/SkGradientShaderPriv.h:306: static inline bool
is2Color(const GrGradientEffect& e){
On 2013/08/13 14:28:37, bsalomon wrote:
> How about an enum named ColorType (or something like that) and values like
> kTwoColor, kThreeColor, kTexture and have two functions (one for
> GrGradientEffect and one for EffectKey) that return the type.
> 
> style-nit: static members have initial caps
> 
> Can you make the key values (0x4000 and 0x6000) enum values alongside
> kMatrixKeyBitCnt above?  It might also be good to define them in terms of
> kMatrixKeyBitCnt to ensure that they don't collide should
> GrGLEffectMatrix::kKeyBits ever change.

Done.

https://codereview.chromium.org/22854005/diff/11001/src/effects/gradients/SkG...
src/effects/gradients/SkGradientShaderPriv.h:346: // emit code that gets a
fragment's color from an expression for t; Only used if 4 or more
On 2013/08/13 14:28:37, bsalomon wrote:
> Do you mean the sampler array is only used for 4+? It seems like this function
> is still called for all cases by the subclasses.

Done.

https://codereview.chromium.org/22854005/diff/21001/src/effects/gradients/SkG...
File src/effects/gradients/SkGradientShader.cpp (right):

https://codereview.chromium.org/22854005/diff/21001/src/effects/gradients/SkG...
src/effects/gradients/SkGradientShader.cpp:928: //same as 2 color for now...
causes bug from unused UNI
How to mix for 3 colors on the GPU? Is there a way to avoid doing a branch on
the GPU based on the t-value (as I imagine a branch would be much slower)?

[bsalomon, 2013-08-13 20:40:03]

https://codereview.chromium.org/22854005/diff/21001/src/effects/gradients/SkG...
File src/effects/gradients/SkGradientShader.cpp (right):

https://codereview.chromium.org/22854005/diff/21001/src/effects/gradients/SkG...
src/effects/gradients/SkGradientShader.cpp:928: //same as 2 color for now...
causes bug from unused UNI
On 2013/08/13 19:50:18, dierk wrote:
> How to mix for 3 colors on the GPU? Is there a way to avoid doing a branch on
> the GPU based on the t-value (as I imagine a branch would be much slower)?

I think this works, but not sure if it's faster than a branch:

oneMinus2T = 1 - 2t
color = clamp(oneMinus2T, 0, 1) * low + (1 - max(abs(oneMinus2T),1)) * mid +
clamp(-oneMinus2T, 0, 1) * high

clamp min and max may map to instructions.

[bsalomon, 2013-08-13 21:07:52]

https://codereview.chromium.org/22854005/diff/11001/src/effects/gradients/SkG...
File src/effects/gradients/SkGradientShader.cpp (right):

https://codereview.chromium.org/22854005/diff/11001/src/effects/gradients/SkG...
src/effects/gradients/SkGradientShader.cpp:1024: this->useAtlas() ==
s.useAtlas() &&
On 2013/08/13 19:50:17, dierk wrote:
> I'm not sure. It was already there.
> 
> The useAtlas() function returns true if fRow == -1, is that the same?

It should be. When you update the color comparison here we should probably add
some asserts that all of our assumptions are true around the consistency of
fRow, getTexture(), and the number of colors.

> 
> On 2013/08/13 14:28:37, bsalomon wrote:
> > Is useAtlas() equivalent to NULL == fTextureAccess.getTexture()? If so, we
> > already checked that.
>

https://codereview.chromium.org/22854005/diff/21001/src/effects/gradients/SkG...
File src/effects/gradients/SkGradientShaderPriv.h (right):

https://codereview.chromium.org/22854005/diff/21001/src/effects/gradients/SkG...
src/effects/gradients/SkGradientShaderPriv.h:260: inline int getNumColors()
const { return fNumColors; };
Do we need fNumColors or can we just store ColorType directly? It seems like the
enum is a bit less misleading... it accurately describes the way color lookup is
implemented while fNumColors is only related to the number of colors (and the t
values...)

https://codereview.chromium.org/22854005/diff/21001/src/effects/gradients/SkG...
src/effects/gradients/SkGradientShaderPriv.h:319: kColorKeyMask = kThreeColorKey
maybe kTwoColorKey | kThreeColorKey... I realize that's identical currently, but
it seems like better documentation of  the intent of kColorKeyMask.

https://codereview.chromium.org/22854005/diff/21001/src/effects/gradients/SkG...
src/effects/gradients/SkGradientShaderPriv.h:322: inline
GrGradientEffect::ColorType getColorType(EffectKey key){
static?

[dierk, 2013-08-14 21:33:15]

https://codereview.chromium.org/22854005/diff/21001/src/effects/gradients/SkG...
File src/effects/gradients/SkGradientShaderPriv.h (right):

https://codereview.chromium.org/22854005/diff/21001/src/effects/gradients/SkG...
src/effects/gradients/SkGradientShaderPriv.h:319: kColorKeyMask = kThreeColorKey
On 2013/08/13 21:07:52, bsalomon wrote:
> maybe kTwoColorKey | kThreeColorKey... I realize that's identical currently,
but
> it seems like better documentation of  the intent of kColorKeyMask.

Done.

https://codereview.chromium.org/22854005/diff/21001/src/effects/gradients/SkG...
src/effects/gradients/SkGradientShaderPriv.h:322: inline
GrGradientEffect::ColorType getColorType(EffectKey key){
On 2013/08/13 21:07:52, bsalomon wrote:
> static?

Done.

[bsalomon, 2013-08-15 13:16:42]

This is coming along well. Other than the imcomplete isEqual() part it looks
right to me. You probably want to sync up to TOT, there was a change that landed
yesterday that refactored uniform handles a bit. I'm not sure if you'll have any
conflicts, probably not.

https://codereview.chromium.org/22854005/diff/40001/src/effects/gradients/SkG...
File src/effects/gradients/SkGradientShader.cpp (right):

https://codereview.chromium.org/22854005/diff/40001/src/effects/gradients/SkG...
src/effects/gradients/SkGradientShader.cpp:956: builder->fsCodeAppendf("\tfloat
oneMinus2t = 1 - (2*%s);\n",
Some GLSL compilers will give you heartache about integer literals. You have to
make these 1.0 and 2.0. :(

https://codereview.chromium.org/22854005/diff/40001/src/effects/gradients/SkG...
src/effects/gradients/SkGradientShader.cpp:1009: } else if (3 ==
info.fColorCount && (SkScalarAbs(pos[1] - 0.5) < 0.01)) { // if 3 color
symmetric
nit: line wrap

https://codereview.chromium.org/22854005/diff/40001/src/effects/gradients/SkG...
src/effects/gradients/SkGradientShader.cpp:1024: // We always filter the
gradient table. Each table is one row of a texture, so always y-clamp.
nit: line wrap

https://codereview.chromium.org/22854005/diff/40001/src/effects/gradients/SkG...
File src/effects/gradients/SkGradientShaderPriv.h (right):

https://codereview.chromium.org/22854005/diff/40001/src/effects/gradients/SkG...
src/effects/gradients/SkGradientShaderPriv.h:254: enum ColorType{
nit missing space before {

[dierk, 2013-08-15 18:18:47]

Fixed color bug for 3 color gradients, but it's still drawing wrong for Sweep
gradients. Is that the case which you said won't work for this optimization? If
so I'll add a check so that Sweeps will use the texture for 3 color gradients.

[dierk, 2013-08-15 18:50:44]

Reverted 3 color sweeps back to texture-lookup method (so I can land this)

[bsalomon, 2013-08-15 19:21:18]

I'm a little nervous about just disabling sweep in case this is masking some
other issue (e.g. we're using the two color shader with a 3 color gradient
because key isn't computed correctly)

https://codereview.chromium.org/22854005/diff/51001/src/effects/gradients/SkG...
File src/effects/gradients/SkGradientShader.cpp (right):

https://codereview.chromium.org/22854005/diff/51001/src/effects/gradients/SkG...
src/effects/gradients/SkGradientShader.cpp:957: builder->fsCodeAppendf("\tvec4
colorTemp = clamp(oneMinus2t, 0, 1) * %s +(1 - min(abs(oneMinus2t), 1)) * %s +
clamp(-oneMinus2t, 0, 1) * %s;\n",
more 1s and 0s that need .0

[bsalomon, 2013-08-15 19:25:29]

https://codereview.chromium.org/22854005/diff/51001/src/effects/gradients/SkG...
File src/effects/gradients/SkGradientShader.cpp (right):

https://codereview.chromium.org/22854005/diff/51001/src/effects/gradients/SkG...
src/effects/gradients/SkGradientShader.cpp:1058: if (2 == this->fColorType) {
Is this the bug? Shouldn't we be checking enum values here?

[dierk, 2013-08-15 19:56:51]

https://codereview.chromium.org/22854005/diff/51001/src/effects/gradients/SkG...
File src/effects/gradients/SkGradientShader.cpp (right):

https://codereview.chromium.org/22854005/diff/51001/src/effects/gradients/SkG...
src/effects/gradients/SkGradientShader.cpp:957: builder->fsCodeAppendf("\tvec4
colorTemp = clamp(oneMinus2t, 0, 1) * %s +(1 - min(abs(oneMinus2t), 1)) * %s +
clamp(-oneMinus2t, 0, 1) * %s;\n",
On 2013/08/15 19:21:18, bsalomon wrote:
> more 1s and 0s that need .0

Done.

https://codereview.chromium.org/22854005/diff/51001/src/effects/gradients/SkG...
src/effects/gradients/SkGradientShader.cpp:1058: if (2 == this->fColorType) {
Good catch, changed it the enum but I'm still seeing the sweep bug.

On 2013/08/15 19:25:29, bsalomon wrote:
> Is this the bug? Shouldn't we be checking enum values here?

[reed1, 2013-08-15 21:32:15]

https://codereview.chromium.org/22854005/diff/72001/src/effects/gradients/SkG...
File src/effects/gradients/SkGradientShader.cpp (right):

https://codereview.chromium.org/22854005/diff/72001/src/effects/gradients/SkG...
src/effects/gradients/SkGradientShader.cpp:1056: if (this->fColorType ==
s.getColorType()){
nit: Skia doesn't use this-> for fields, but it does use it for methods

e.g.

fWidth += 3;
this->computeHeight();

https://codereview.chromium.org/22854005/diff/72001/src/effects/gradients/SkG...
File src/effects/gradients/SkGradientShaderPriv.h (right):

https://codereview.chromium.org/22854005/diff/72001/src/effects/gradients/SkG...
src/effects/gradients/SkGradientShaderPriv.h:260: inline ColorType
getColorType() const {
nit: not need for 'inline' in a class header when the impl is right here.

https://codereview.chromium.org/22854005/diff/72001/src/effects/gradients/SkG...
src/effects/gradients/SkGradientShaderPriv.h:263: const inline SkColor*
getColors() const {
1. same nit about not needing 'inline' here
2. very odd to see inline between const and a type

https://codereview.chromium.org/22854005/diff/72001/src/effects/gradients/SkG...
src/effects/gradients/SkGradientShaderPriv.h:263: const inline SkColor*
getColors() const {
API question: given that getColors is only valid for colorCount == 2 or 3 (is
that true?), perhaps this could be named/framed differently...

e.g.

getColor(int index) -- this guy could assert on index being in range

Just thinking out loud

https://codereview.chromium.org/22854005/diff/72001/src/effects/gradients/SkG...
src/effects/gradients/SkGradientShaderPriv.h:294: SkColor fColors[3];
// Short comment on why [3] here

or maybe an enum...

enum {
    kMaxAnalyticColors = 3 // anything above this we resort to the texture
};
SkColor fColors[kMaxAnalyticColors];

https://codereview.chromium.org/22854005/diff/72001/src/effects/gradients/SkG...
src/effects/gradients/SkGradientShaderPriv.h:324: static inline
GrGradientEffect::ColorType getColorType(EffectKey key){
again with the 'inline' :)

[bsalomon, 2013-08-16 13:06:04]

A couple minor comments, otherwise looks good.

https://codereview.chromium.org/22854005/diff/77001/src/effects/gradients/SkG...
File src/effects/gradients/SkGradientShaderPriv.h (right):

https://codereview.chromium.org/22854005/diff/77001/src/effects/gradients/SkG...
src/effects/gradients/SkGradientShaderPriv.h:261: kMaxAnalyticColors = 3 // if
more use texture
Can this be private, it seems only to be used for the size of fColors

https://codereview.chromium.org/22854005/diff/77001/src/effects/gradients/SkG...
src/effects/gradients/SkGradientShaderPriv.h:298: int fNumColors;
Is this used anywhere?

[bsalomon, 2013-08-16 14:39:00]

One other thing: Let's not use "_gpu" in the name of the GM since the generated
PNGs for GMs are suffixed with _<config> and "gpu" is a config. It might confuse
people and their scripts.

[dierk, 2013-08-16 16:59:56]

https://codereview.chromium.org/22854005/diff/72001/src/effects/gradients/SkG...
File src/effects/gradients/SkGradientShader.cpp (right):

https://codereview.chromium.org/22854005/diff/72001/src/effects/gradients/SkG...
src/effects/gradients/SkGradientShader.cpp:1056: if (this->fColorType ==
s.getColorType()){
On 2013/08/15 21:32:15, reed1 wrote:
> nit: Skia doesn't use this-> for fields, but it does use it for methods
> 
> e.g.
> 
> fWidth += 3;
> this->computeHeight();

Done.

https://codereview.chromium.org/22854005/diff/72001/src/effects/gradients/SkG...
File src/effects/gradients/SkGradientShaderPriv.h (right):

https://codereview.chromium.org/22854005/diff/72001/src/effects/gradients/SkG...
src/effects/gradients/SkGradientShaderPriv.h:260: inline ColorType
getColorType() const {
On 2013/08/15 21:32:15, reed1 wrote:
> nit: not need for 'inline' in a class header when the impl is right here.

Done.

https://codereview.chromium.org/22854005/diff/72001/src/effects/gradients/SkG...
src/effects/gradients/SkGradientShaderPriv.h:263: const inline SkColor*
getColors() const {
On 2013/08/15 21:32:15, reed1 wrote:
> 1. same nit about not needing 'inline' here
> 2. very odd to see inline between const and a type

Done.

https://codereview.chromium.org/22854005/diff/72001/src/effects/gradients/SkG...
src/effects/gradients/SkGradientShaderPriv.h:294: SkColor fColors[3];
On 2013/08/15 21:32:15, reed1 wrote:
> // Short comment on why [3] here
> 
> or maybe an enum...
> 
> enum {
>     kMaxAnalyticColors = 3 // anything above this we resort to the texture
> };
> SkColor fColors[kMaxAnalyticColors];

Done.

https://codereview.chromium.org/22854005/diff/72001/src/effects/gradients/SkG...
src/effects/gradients/SkGradientShaderPriv.h:324: static inline
GrGradientEffect::ColorType getColorType(EffectKey key){
On 2013/08/15 21:32:15, reed1 wrote:
> again with the 'inline' :)

Done.

[bsalomon, 2013-08-16 17:32:59]

lgtm

[commit-bot: I haz the power, 2013-08-16 17:53:04]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/dierk@google.com/22854005/10001

[commit-bot: I haz the power, 2013-08-16 18:07:18]

Retried try job too often on Build-Mac10.7-Clang-x86-Release-Trybot for step(s)
BuildGm
http://108.170.217.252:10117/buildstatus?builder=Build-Mac10.7-Clang-x86-Rele...

[commit-bot: I haz the power, 2013-08-16 18:48:37]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/dierk@google.com/22854005/10001

[commit-bot: I haz the power, 2013-08-16 18:58:15]

Retried try job too often on Build-Mac10.7-Clang-x86-Release-Trybot for step(s)
BuildGm
http://108.170.217.252:10117/buildstatus?builder=Build-Mac10.7-Clang-x86-Rele...

[commit-bot: I haz the power, 2013-08-16 20:16:54]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/dierk@google.com/22854005/100001

[commit-bot: I haz the power, 2013-08-16 20:26:30]

Retried try job too often on Build-Ubuntu12-GCC-x86_64-Release-Trybot for
step(s) BuildGm
http://108.170.217.252:10117/buildstatus?builder=Build-Ubuntu12-GCC-x86_64-Re...

[commit-bot: I haz the power, 2013-08-19 19:36:34]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/dierk@google.com/22854005/104001

[commit-bot: I haz the power, 2013-08-19 22:08:10]

Retried try job too often on Build-Ubuntu12-GCC-x86_64-Release-Trybot for
step(s) BuildGm
http://173.255.115.253:10117/buildstatus?builder=Build-Ubuntu12-GCC-x86_64-Re...

[scroggo, 2013-08-19 22:11:44]

On 2013/08/19 22:08:10, I haz the power (commit-bot) wrote:
> Retried try job too often on Build-Ubuntu12-GCC-x86_64-Release-Trybot for
> step(s) BuildGm
>
http://173.255.115.253:10117/buildstatus?builder=Build-Ubuntu12-GCC-x86_64-Re...

FYI: It turns out this is a problem with the commit queue with respect to git. A
git cl dcommit will work, and rmistry@ is working on fixing the commit queue
issue.

[bsalomon, 2013-09-09 15:36:39]

Committed patchset #22 manually as r11158 (presubmit successful).