Use asNewCustomStage instead of asABitmap in SkGpuDevice, also removed now-unecessary twoPointRadia…

Use asNewCustomStage instead of asABitmap in SkGpuDevice, also removed now-unecessary twoPointRadialParams parameter from asABitmap.

In SkGpuDevice we still fall back on using asABitmap for effects that don't have asNewCustomStage implemented, but it still simplifies things a fair amount.
Committed: https://code.google.com/p/skia/source/detail?r=4755

[rileya, 2012-07-20 21:21:46]

Actually hooked up asNewCustomStage to be used instead of the giant asABitmap
switch in skPaint2GrPaintShader().

Because it potentially slightly changes the caching behavior of the gradient
textures, I think it might be a good idea to watch for performance regressions
after this lands.

https://codereview.appspot.com/6430060/diff/1/src/device/xps/SkXPSDevice.cpp
File src/device/xps/SkXPSDevice.cpp (left):

https://codereview.appspot.com/6430060/diff/1/src/device/xps/SkXPSDevice.cpp#...
src/device/xps/SkXPSDevice.cpp:1017: SkScalar twoPointRadialParams[3];
This was the one place that twoPointRadialParams could potentially still be
used, if a two point radial implementation were to be added; however,
asAGradient can return the information necessary to compute those params, so I
think it's fine.

https://codereview.appspot.com/6430060/diff/1/src/gpu/SkGpuDevice.cpp
File src/gpu/SkGpuDevice.cpp (right):

https://codereview.appspot.com/6430060/diff/1/src/gpu/SkGpuDevice.cpp#newcode603
src/gpu/SkGpuDevice.cpp:603: grPaint->setTexture(kShaderTextureIdx, texture);
Tom: I saw you made a recent commit replacing a lot of setTexture() instances
with GrSingleTextureEffect, would this be a good candidate for another such a
change?

[TomH, 2012-07-20 21:27:00]

https://codereview.appspot.com/6430060/diff/1/src/effects/SkGradientShader.cpp
File src/effects/SkGradientShader.cpp (right):

https://codereview.appspot.com/6430060/diff/1/src/effects/SkGradientShader.cp...
src/effects/SkGradientShader.cpp:1883: virtual BitmapType asABitmap(SkBitmap*
bitmap,
If we're no longer passing in twoPointRadialParams to be filled out, can this
function even return meaningfully any more? Should it ever be called?

https://codereview.appspot.com/6430060/diff/1/src/gpu/SkGpuDevice.cpp
File src/gpu/SkGpuDevice.cpp (right):

https://codereview.appspot.com/6430060/diff/1/src/gpu/SkGpuDevice.cpp#newcode603
src/gpu/SkGpuDevice.cpp:603: grPaint->setTexture(kShaderTextureIdx, texture);
On 2012/07/20 21:21:46, rileya wrote:
> Tom: I saw you made a recent commit replacing a lot of setTexture() instances
> with GrSingleTextureEffect, would this be a good candidate for another such a
> change?

Yes, looks like an excellent candidate.

[reed1, 2012-07-21 16:52:01]

lgtm -- defers to other reviewers

[bsalomon, 2012-07-23 12:13:37]

https://codereview.appspot.com/6430060/diff/1/src/effects/SkGradientShader.cpp
File src/effects/SkGradientShader.cpp (right):

https://codereview.appspot.com/6430060/diff/1/src/effects/SkGradientShader.cp...
src/effects/SkGradientShader.cpp:1883: virtual BitmapType asABitmap(SkBitmap*
bitmap,
On 2012/07/20 21:27:01, TomH wrote:
> If we're no longer passing in twoPointRadialParams to be filled out, can this
> function even return meaningfully any more? Should it ever be called?

Right, should we get rid of its enum?

https://codereview.appspot.com/6430060/diff/1/src/gpu/SkGpuDevice.cpp
File src/gpu/SkGpuDevice.cpp (left):

https://codereview.appspot.com/6430060/diff/1/src/gpu/SkGpuDevice.cpp#oldcode625
src/gpu/SkGpuDevice.cpp:625: }
nice!

[rileya, 2012-07-23 13:13:26]

https://codereview.appspot.com/6430060/diff/1/src/effects/SkGradientShader.cpp
File src/effects/SkGradientShader.cpp (right):

https://codereview.appspot.com/6430060/diff/1/src/effects/SkGradientShader.cp...
src/effects/SkGradientShader.cpp:1883: virtual BitmapType asABitmap(SkBitmap*
bitmap,
On 2012/07/23 12:13:37, bsalomon wrote:
> On 2012/07/20 21:27:01, TomH wrote:
> > If we're no longer passing in twoPointRadialParams to be filled out, can
this
> > function even return meaningfully any more? Should it ever be called?
> 
> Right, should we get rid of its enum?
> 

Would making it return something like a "kGradient_BitmapType" indicating to use
asAGradient to determine the appropriate gradient type/parameters work? This
still seems a bit ugly, requiring calling both asABitmap and asAGradient, and
then the matrix returned by asABitmap would still be dependent on the gradient
type, making it sort of meaningless without checking the result of
asAGradient... but if the ultimate goal is to remove asABitmap entirely, it'd at
least be a step towards that.

[rileya, 2012-07-25 13:59:38]

asNewCustomStage no longer uses asABitmap. Moved Gradient_Shader into a private
header and renamed to SkGradientShaderBase, renamed its protected
"commonAsABitmap" to a public "getGradientTableBitmap". 

Next CL I'd like to move each of the gradient effect subclasses into their own
files to reduce the size of SkGradientShader.cpp, and move the GrGradientEffects
files to the same directory as their Sk counterparts.

[bsalomon, 2012-07-25 14:26:16]

On 2012/07/25 13:59:38, rileya wrote:
> asNewCustomStage no longer uses asABitmap. Moved Gradient_Shader into a
private
> header and renamed to SkGradientShaderBase, renamed its protected
> "commonAsABitmap" to a public "getGradientTableBitmap". 
> 
> Next CL I'd like to move each of the gradient effect subclasses into their own
> files to reduce the size of SkGradientShader.cpp, and move the
GrGradientEffects
> files to the same directory as their Sk counterparts.

Is it possible to submit the file re-org / base class rename as a separate CL?
It's a bit hard to see what's new and what's moved in what was
SkGradientShader.cpp.

[reed1, 2012-07-25 14:42:45]

On 2012/07/25 14:26:16, bsalomon wrote:
> On 2012/07/25 13:59:38, rileya wrote:
> > asNewCustomStage no longer uses asABitmap. Moved Gradient_Shader into a
> private
> > header and renamed to SkGradientShaderBase, renamed its protected
> > "commonAsABitmap" to a public "getGradientTableBitmap". 
> > 
> > Next CL I'd like to move each of the gradient effect subclasses into their
own
> > files to reduce the size of SkGradientShader.cpp, and move the
> GrGradientEffects
> > files to the same directory as their Sk counterparts.
> 
> Is it possible to submit the file re-org / base class rename as a separate CL?
> It's a bit hard to see what's new and what's moved in what was
> SkGradientShader.cpp.

+1

[rileya, 2012-07-25 14:56:20]

On 2012/07/25 14:42:45, reed1 wrote:
> On 2012/07/25 14:26:16, bsalomon wrote:
> > On 2012/07/25 13:59:38, rileya wrote:
> > > asNewCustomStage no longer uses asABitmap. Moved Gradient_Shader into a
> > private
> > > header and renamed to SkGradientShaderBase, renamed its protected
> > > "commonAsABitmap" to a public "getGradientTableBitmap". 
> > > 
> > > Next CL I'd like to move each of the gradient effect subclasses into their
> own
> > > files to reduce the size of SkGradientShader.cpp, and move the
> > GrGradientEffects
> > > files to the same directory as their Sk counterparts.
> > 
> > Is it possible to submit the file re-org / base class rename as a separate
CL?
> > It's a bit hard to see what's new and what's moved in what was
> > SkGradientShader.cpp.
> 
> +1

Done.

[bsalomon, 2012-07-25 15:21:15]

I was thinking of the other order: move code then change code. But whatever :)

http://codereview.appspot.com/6430060/diff/11001/src/effects/SkGradientShader...
File src/effects/SkGradientShaderPriv.h (right):

http://codereview.appspot.com/6430060/diff/11001/src/effects/SkGradientShader...
src/effects/SkGradientShaderPriv.h:21: int count) {
Are all these static functions used in more than one cpp now?

http://codereview.appspot.com/6430060/diff/10004/src/gpu/effects/GrGradientEf...
File src/gpu/effects/GrGradientEffects.cpp (right):

http://codereview.appspot.com/6430060/diff/10004/src/gpu/effects/GrGradientEf...
src/gpu/effects/GrGradientEffects.cpp:454: : INHERITED(ctx, shader, sampler)
nit: I don't know that we have a rule about how the initializer list gets
indented. However, the two things I see in the code are either 1) not indented
or 2) indented 4 spaces.

http://codereview.appspot.com/6430060/diff/10004/src/gpu/effects/GrGradientEf...
File src/gpu/effects/GrGradientEffects.h (right):

http://codereview.appspot.com/6430060/diff/10004/src/gpu/effects/GrGradientEf...
src/gpu/effects/GrGradientEffects.h:115: GrSamplerState* sampler, SkScalar
center,
It seems a bit odd to pass both the SkShader and these other params. Is it
possible to pass the gradient shader subclass and extract the params using
member functions?

[rileya, 2012-07-25 15:39:49]

On 2012/07/25 15:21:15, bsalomon wrote:
> I was thinking of the other order: move code then change code. But whatever :)

Oh, whoops, that probably would've made more sense... oh well.

http://codereview.appspot.com/6430060/diff/11001/src/effects/SkGradientShader...
File src/effects/SkGradientShaderPriv.h (right):

http://codereview.appspot.com/6430060/diff/11001/src/effects/SkGradientShader...
src/effects/SkGradientShaderPriv.h:21: int count) {
On 2012/07/25 15:21:15, bsalomon wrote:
> Are all these static functions used in more than one cpp now?

Not currently, but if we want to split up SkGradientShader into files for each
gradient type to make it more manageable, I believe that would be the case.

http://codereview.appspot.com/6430060/diff/10004/src/gpu/effects/GrGradientEf...
File src/gpu/effects/GrGradientEffects.h (right):

http://codereview.appspot.com/6430060/diff/10004/src/gpu/effects/GrGradientEf...
src/gpu/effects/GrGradientEffects.h:115: GrSamplerState* sampler, SkScalar
center,
On 2012/07/25 15:21:15, bsalomon wrote:
> It seems a bit odd to pass both the SkShader and these other params. Is it
> possible to pass the gradient shader subclass and extract the params using
> member functions?
This is what I was going to do, but since I reverted the move of
Gradient_Shader, that subclass isn't visible from here. This CL originally used
asAGradient to get these, but it resulted in duplicating a fair amount of math
(to get the 3 special parameters out of the basic gradient info).

After reorganizing and making the gradient subclasses visible from here, this
can take the appropriate subclass and get the parameters directly that way.

[bsalomon, 2012-07-25 15:56:06]

On 2012/07/25 15:39:49, rileya wrote:
> On 2012/07/25 15:21:15, bsalomon wrote:
> > I was thinking of the other order: move code then change code. But whatever
:)
> 
> Oh, whoops, that probably would've made more sense... oh well.
> 
>
http://codereview.appspot.com/6430060/diff/11001/src/effects/SkGradientShader...
> File src/effects/SkGradientShaderPriv.h (right):
> 
>
http://codereview.appspot.com/6430060/diff/11001/src/effects/SkGradientShader...
> src/effects/SkGradientShaderPriv.h:21: int count) {
> On 2012/07/25 15:21:15, bsalomon wrote:
> > Are all these static functions used in more than one cpp now?
> 
> Not currently, but if we want to split up SkGradientShader into files for each
> gradient type to make it more manageable, I believe that would be the case.
> 
>
http://codereview.appspot.com/6430060/diff/10004/src/gpu/effects/GrGradientEf...
> File src/gpu/effects/GrGradientEffects.h (right):
> 
>
http://codereview.appspot.com/6430060/diff/10004/src/gpu/effects/GrGradientEf...
> src/gpu/effects/GrGradientEffects.h:115: GrSamplerState* sampler, SkScalar
> center,
> On 2012/07/25 15:21:15, bsalomon wrote:
> > It seems a bit odd to pass both the SkShader and these other params. Is it
> > possible to pass the gradient shader subclass and extract the params using
> > member functions?
> This is what I was going to do, but since I reverted the move of
> Gradient_Shader, that subclass isn't visible from here. This CL originally
used
> asAGradient to get these, but it resulted in duplicating a fair amount of math
> (to get the 3 special parameters out of the basic gradient info).
> 
> After reorganizing and making the gradient subclasses visible from here, this
> can take the appropriate subclass and get the parameters directly that way.

Ok, LGTM.