Made gradient effects use GrTextureStripAtlas.

Made gradient effects use GrTextureStripAtlas.
Committed: https://code.google.com/p/skia/source/detail?r=5101

[rileya, 2012-08-14 17:57:18]

Actually hooked up GrTextureStripAtlas to be used by GPU gradients (we still
fall back on the original method if for some reason the Atlas fails to return a
row).

[bsalomon, 2012-08-14 19:08:26]

http://codereview.appspot.com/6450131/diff/6001/src/effects/gradients/SkGradi...
File src/effects/gradients/SkGradientShader.cpp (right):

http://codereview.appspot.com/6450131/diff/6001/src/effects/gradients/SkGradi...
src/effects/gradients/SkGradientShader.cpp:743: fUseAtlas = false;
is fUseAtlas an alias for (-1 == fRow)? Maybe we should remove it (and make
usesAtlas() do the -1 check)?

http://codereview.appspot.com/6450131/diff/6001/src/effects/gradients/SkGradi...
File src/effects/gradients/SkGradientShaderPriv.h (right):

http://codereview.appspot.com/6450131/diff/6001/src/effects/gradients/SkGradi...
src/effects/gradients/SkGradientShaderPriv.h:238: bool useAtlas() const { return
fUseAtlas; }
Do these two functions need to be public?

[TomH, 2012-08-14 19:35:00]

Modulo Brian's questions, LGTM.

http://codereview.appspot.com/6450131/diff/6001/src/effects/gradients/SkGradi...
File src/effects/gradients/SkGradientShader.cpp (right):

http://codereview.appspot.com/6450131/diff/6001/src/effects/gradients/SkGradi...
src/effects/gradients/SkGradientShader.cpp:695: if (yCoord != fCachedYCoord) {
Future work: Should we think about having uman do this caching, instead of
requiring each stage to reimplement?

http://codereview.appspot.com/6450131/diff/6001/src/effects/gradients/SkGradi...
src/effects/gradients/SkGradientShader.cpp:705: // We look up into an atlas row,
so specify the appropriate Y offset
This comment is unclear. Expand or contract.

[rileya, 2012-08-14 19:42:40]

http://codereview.appspot.com/6450131/diff/6001/src/effects/gradients/SkGradi...
File src/effects/gradients/SkGradientShaderPriv.h (right):

http://codereview.appspot.com/6450131/diff/6001/src/effects/gradients/SkGradi...
src/effects/gradients/SkGradientShaderPriv.h:238: bool useAtlas() const { return
fUseAtlas; }
On 2012/08/14 19:08:26, bsalomon wrote:
> Do these two functions need to be public?

Unless we make GrGLGradientStage a friend, I think so, since in various
functions it gets a GrCustomStage reference that it casts to GrGradientEffect
and uses to call these.

[bsalomon, 2012-08-14 19:46:39]

On 2012/08/14 19:42:40, rileya wrote:
>
http://codereview.appspot.com/6450131/diff/6001/src/effects/gradients/SkGradi...
> File src/effects/gradients/SkGradientShaderPriv.h (right):
> 
>
http://codereview.appspot.com/6450131/diff/6001/src/effects/gradients/SkGradi...
> src/effects/gradients/SkGradientShaderPriv.h:238: bool useAtlas() const {
return
> fUseAtlas; }
> On 2012/08/14 19:08:26, bsalomon wrote:
> > Do these two functions need to be public?
> 
> Unless we make GrGLGradientStage a friend, I think so, since in various
> functions it gets a GrCustomStage reference that it casts to GrGradientEffect
> and uses to call these.

Oh, right.

[rileya, 2012-08-14 20:06:15]

http://codereview.appspot.com/6450131/diff/6001/src/effects/gradients/SkGradi...
File src/effects/gradients/SkGradientShader.cpp (right):

http://codereview.appspot.com/6450131/diff/6001/src/effects/gradients/SkGradi...
src/effects/gradients/SkGradientShader.cpp:695: if (yCoord != fCachedYCoord) {
On 2012/08/14 19:35:00, TomH wrote:
> Future work: Should we think about having uman do this caching, instead of
> requiring each stage to reimplement?

I don't know much about uman's implementation, but yeah I think that would be an
improvement; I can't think of any reason you'd ever not want to cache the value.

http://codereview.appspot.com/6450131/diff/6001/src/effects/gradients/SkGradi...
src/effects/gradients/SkGradientShader.cpp:705: // We look up into an atlas row,
so specify the appropriate Y offset
On 2012/08/14 19:35:00, TomH wrote:
> This comment is unclear. Expand or contract.

Got rid of it, the code is clear enough on its own.

http://codereview.appspot.com/6450131/diff/6001/src/effects/gradients/SkGradi...
src/effects/gradients/SkGradientShader.cpp:743: fUseAtlas = false;
On 2012/08/14 19:08:26, bsalomon wrote:
> is fUseAtlas an alias for (-1 == fRow)? Maybe we should remove it (and make
> usesAtlas() do the -1 check)?

Done.

[bsalomon, 2012-08-14 20:17:17]

LGTM modulo nits.

http://codereview.appspot.com/6450131/diff/6001/src/effects/gradients/SkGradi...
File src/effects/gradients/SkGradientShader.cpp (right):

http://codereview.appspot.com/6450131/diff/6001/src/effects/gradients/SkGradi...
src/effects/gradients/SkGradientShader.cpp:695: if (yCoord != fCachedYCoord) {
On 2012/08/14 20:06:15, rileya wrote:
> On 2012/08/14 19:35:00, TomH wrote:
> > Future work: Should we think about having uman do this caching, instead of
> > requiring each stage to reimplement?
> 
> I don't know much about uman's implementation, but yeah I think that would be
an
> improvement; I can't think of any reason you'd ever not want to cache the
value.

I've thought about making it an optional feature of uman. There are instances
where an effect has to do a transformation of the data before setting the
uniform and in those cases it may be faster for the effect itself to compare the
untransformed old vs new value rather than always transforming.

http://codereview.appspot.com/6450131/diff/9002/src/effects/gradients/SkGradi...
File src/effects/gradients/SkGradientShader.cpp (right):

http://codereview.appspot.com/6450131/diff/9002/src/effects/gradients/SkGradi...
src/effects/gradients/SkGradientShader.cpp:755: if (-1 != fRow) {
maybe this->useAtlas() is slightly clearer?

http://codereview.appspot.com/6450131/diff/9002/src/effects/gradients/SkGradi...
File src/effects/gradients/SkGradientShaderPriv.h (right):

http://codereview.appspot.com/6450131/diff/9002/src/effects/gradients/SkGradi...
src/effects/gradients/SkGradientShaderPriv.h:243: return
INHERITED::isEqual(stage) && useAtlas() == s.useAtlas() && fYCoord ==
s.getYCoord();
style nit: this->useAtlas().

[rileya, 2012-08-14 20:29:29]

http://codereview.appspot.com/6450131/diff/9002/src/effects/gradients/SkGradi...
File src/effects/gradients/SkGradientShader.cpp (right):

http://codereview.appspot.com/6450131/diff/9002/src/effects/gradients/SkGradi...
src/effects/gradients/SkGradientShader.cpp:755: if (-1 != fRow) {
On 2012/08/14 20:17:17, bsalomon wrote:
> maybe this->useAtlas() is slightly clearer?

Done.

http://codereview.appspot.com/6450131/diff/9002/src/effects/gradients/SkGradi...
File src/effects/gradients/SkGradientShaderPriv.h (right):

http://codereview.appspot.com/6450131/diff/9002/src/effects/gradients/SkGradi...
src/effects/gradients/SkGradientShaderPriv.h:243: return
INHERITED::isEqual(stage) && useAtlas() == s.useAtlas() && fYCoord ==
s.getYCoord();
On 2012/08/14 20:17:17, bsalomon wrote:
> style nit: this->useAtlas().

And done.

[bsalomon, 2012-08-14 20:42:21]

Do it!


On Tue, Aug 14, 2012 at 4:29 PM, <rileya@google.com> wrote:

>
> http://codereview.appspot.com/**6450131/diff/9002/src/effects/**
>
gradients/SkGradientShader.cpp<http://codereview.appspot.com/6450131/diff/9002/src/effects/gradients/SkGradientShader.cpp>
> File src/effects/gradients/**SkGradientShader.cpp (right):
>
> http://codereview.appspot.com/**6450131/diff/9002/src/effects/**
>
gradients/SkGradientShader.**cpp#newcode755<http://codereview.appspot.com/6450131/diff/9002/src/effects/gradients/SkGradientShader.cpp#newcode755>
> src/effects/gradients/**SkGradientShader.cpp:755: if (-1 != fRow) {
> On 2012/08/14 20:17:17, bsalomon wrote:
>
>> maybe this->useAtlas() is slightly clearer?
>>
>
> Done.
>
>
> http://codereview.appspot.com/**6450131/diff/9002/src/effects/**gradients/
>
**SkGradientShaderPriv.h<http://codereview.appspot.com/6450131/diff/9002/src/effects/gradients/SkGradientShaderPriv.h>
> File src/effects/gradients/**SkGradientShaderPriv.h (right):
>
> http://codereview.appspot.com/**6450131/diff/9002/src/effects/**gradients/
>
**SkGradientShaderPriv.h#**newcode243<http://codereview.appspot.com/6450131/diff/9002/src/effects/gradients/SkGradientShaderPriv.h#newcode243>
> src/effects/gradients/**SkGradientShaderPriv.h:243: return
> INHERITED::isEqual(stage) && useAtlas() == s.useAtlas() && fYCoord ==
> s.getYCoord();
> On 2012/08/14 20:17:17, bsalomon wrote:
>
>> style nit: this->useAtlas().
>>
>
> And done.
>
>
http://codereview.appspot.com/**6450131/<http://codereview.appspot.com/6450131/>
>