Addition of functions to manage texture fetch shader code generation.

Introduction of set of functions to manage generation of texture fetch shader code.  

A new set of routines have been added to GrGLShaderBuilder to emit texture fetches, taking into consideration the format of the texture to be accessed, and the channel swizzle.  

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

[twiz1, 2012-07-31 21:09:50]

PTAL.

This is the first revision of how I think the texture fetch glsl shader code
generation, and shader key management will work.

High-level summary: 
 - GrGLShaderBuilder now has a routine emitCustomTextureLookup, which takes a
description of the type of texture lookup, and emits the appropriate code.  
 - GrGLShaderBuilder also has a new routine, keyForTextureUse(...) that
generates a key snippet based on the texture use parameters.  At the moment this
routine is very simple, and generates a single 'bit' worth of information.
 - The GrGLCaps structure is now passed along to all of the key generation
routines, for use in the above routine.
 
Discussion Points:
 - The mixing of the keys generated by texture use, and the keys generated by
the GrGLProgramStage instances isn't really well defined.  As we allow for
multi-texture effects, I think we may need to specify how these bits are mixed,
to prevent collisions.
 - I did not remove the existing 'default' texture fetch code yet with this
change, because of magic behind the default texture coordinate generation.  For
all of the custom stages, I generally see that the custom stage will manage the
texture coordinates to be used for texture accesses.  With this change in place,
I think we could work towards removing the current code.

[bsalomon, 2012-08-01 14:05:52]

Jeff, I think this is totally the right direction. I have some questions,
suggestions, bike-shedding, and silly nits.

> - The mixing of the keys generated by texture use, and the keys generated by
>the GrGLProgramStage instances isn't really well defined.  As we allow for
>multi-texture effects, I think we may need to specify how these bits are mixed,
>to prevent collisions.

I think we should probably go ahead and increase the per-stage key size and
reserve range of the bits for texture access. This may imply that we cap the
number of textures per stage (which seems like a reasonable thing to do. 4 or 8
texture should be enough for anybody. :) ).


>- I did not remove the existing 'default' texture fetch code yet with this
>change, because of magic behind the default texture coordinate generation.  For
>all of the custom stages, I generally see that the custom stage will manage the
>texture coordinates to be used for texture accesses.  With this change in
place,
>I think we could work towards removing the current code.

Yeah, I that should be done in tandem with moving the burden of inserting a
texture matrix / coord varying onto GrGLProgramStage as well as moving the
texture matrix out of GrSampleState and onto GrCustomEffects that require a
matrix. But I agree that this CL moves us closer.

http://codereview.appspot.com/6446072/diff/4002/include/gpu/GrCustomStage.h
File include/gpu/GrCustomStage.h (right):

http://codereview.appspot.com/6446072/diff/4002/include/gpu/GrCustomStage.h#n...
include/gpu/GrCustomStage.h:19: /** A structure representing the swizzle access
pattern for a texture.
I think we should class-ify this a bit.

Also, why don't we replace the GrCustomStage::texture() function entirely with
GrCustomStage::textureAccess() and rename numTextures() to numTextureAccesses()
or numTexturesAccessed().

I will soon want to make GrCustomStage provide a GrTextureParams for each
texture. This object will be a convenient place to hang that. (Not an ask for
this CL, just FYI).

Do we want to consider the case of having multiple swizzles for the same texture
(i.e. making fSwizzle an array or list)?

http://codereview.appspot.com/6446072/diff/4002/include/gpu/GrCustomStage.h#n...
include/gpu/GrCustomStage.h:93: in generated shader code. */
nit: here and in several other files a tab got inserted.

http://codereview.appspot.com/6446072/diff/4002/include/gpu/GrProgramStageFac...
File include/gpu/GrProgramStageFactory.h (right):

http://codereview.appspot.com/6446072/diff/4002/include/gpu/GrProgramStageFac...
include/gpu/GrProgramStageFactory.h:32: const GrGLCaps& caps ) const = 0;
tabs

http://codereview.appspot.com/6446072/diff/4002/src/effects/gradients/SkTwoPo...
File src/effects/gradients/SkTwoPointConicalGradient.cpp (left):

http://codereview.appspot.com/6446072/diff/4002/src/effects/gradients/SkTwoPo...
src/effects/gradients/SkTwoPointConicalGradient.cpp:661: 
Are you removing the trailing LFs in the files or were there extra ones? If the
file ends on } (or anything but LF) clang will yell at us (even though its a cpp
not an h file that is never included anywhere).

http://codereview.appspot.com/6446072/diff/4002/src/gpu/GrCustomStage.cpp
File src/gpu/GrCustomStage.cpp (right):

http://codereview.appspot.com/6446072/diff/4002/src/gpu/GrCustomStage.cpp#new...
src/gpu/GrCustomStage.cpp:38: for (unsigned int offset = 0; offset <
swizzle.size(); ++offset)
style nit: always use { and }

http://codereview.appspot.com/6446072/diff/4002/src/gpu/GrCustomStage.cpp#new...
src/gpu/GrCustomStage.cpp:40: if (swizzle.size() < 4)
ditto

http://codereview.appspot.com/6446072/diff/4002/src/gpu/effects/GrColorTableE...
File src/gpu/effects/GrColorTableEffect.cpp (right):

http://codereview.appspot.com/6446072/diff/4002/src/gpu/effects/GrColorTableE...
src/gpu/effects/GrColorTableEffect.cpp:64: access,
tabs

http://codereview.appspot.com/6446072/diff/4002/src/gpu/gl/GrGLProgramStage.h
File src/gpu/gl/GrGLProgramStage.h (right):

http://codereview.appspot.com/6446072/diff/4002/src/gpu/gl/GrGLProgramStage.h...
src/gpu/gl/GrGLProgramStage.h:81: static StageKey GenKey(const GrCustomStage&,
const GrGLCaps&);
I'm probably being dense, but I don't get how this gets called.

The templated factory calls GLProgramStage::GenKey where GLProgramStage (no
Gr-prefix) is a typedef (or nested class) on a GrCustomStage subclass which must
be a subclass of GrGLProgramStage.

So, it seems like the factory function will go to the GrGLProgramStage
subclass's static function not this one. What am I missing?

http://codereview.appspot.com/6446072/diff/4002/src/gpu/gl/GrGLShaderBuilder.cpp
File src/gpu/gl/GrGLShaderBuilder.cpp (right):

http://codereview.appspot.com/6446072/diff/4002/src/gpu/gl/GrGLShaderBuilder....
src/gpu/gl/GrGLShaderBuilder.cpp:43: if
(GrPixelConfigIsAlphaOnly(access.fTexture->config()) && caps.textureRedSupport()
style nit: && at the end of a line rather than beginning 

Also, would it be simpler as:
 
return GrPixelConfigIsAlphaOnly(access.fTexture->config()) &&
caps.textureRedSupport() && access.referencesAlpha();

?

http://codereview.appspot.com/6446072/diff/4002/src/gpu/gl/GrGLShaderBuilder.h
File src/gpu/gl/GrGLShaderBuilder.h (right):

http://codereview.appspot.com/6446072/diff/4002/src/gpu/gl/GrGLShaderBuilder....
src/gpu/gl/GrGLShaderBuilder.h:72: static GrCustomStage::StageKey
keyForTextureUse(const GrTextureAccess& access,
style nit: static function begins with a capital letter.

Would calling it KeyForTextureAccess() be more consistent naming?

http://codereview.appspot.com/6446072/diff/4002/src/gpu/gl/GrGpuGL_program.cpp
File src/gpu/gl/GrGpuGL_program.cpp (right):

http://codereview.appspot.com/6446072/diff/4002/src/gpu/gl/GrGpuGL_program.cp...
src/gpu/gl/GrGpuGL_program.cpp:750: setup_custom_stage(&stage, sampler,
this->glCaps(), customStages, fCurrentProgram.get(), s);
style-nit: long line

[bsalomon, 2012-08-01 14:15:50]

Adding Rile and Rob to the CC in case they want to have a look.

[bsalomon, 2012-08-01 14:17:21]

On 2012/08/01 14:15:50, bsalomon wrote:
> Adding Rile and Rob to the CC in case they want to have a look.

Sorry, Riley, for the typo. :)

[robertphillips, 2012-08-01 14:44:58]

http://codereview.appspot.com/6446072/diff/4002/include/gpu/GrProgramStageFac...
File include/gpu/GrProgramStageFactory.h (right):

http://codereview.appspot.com/6446072/diff/4002/include/gpu/GrProgramStageFac...
include/gpu/GrProgramStageFactory.h:84: GLSL code generation. */
Is the left spacing still correct here?

[twiz1, 2012-08-01 18:11:54]

Thanks for the reviews.

I made all of the style changes.  I will upload another patch that addresses:
 - Converting TextureAccess to a class.
 - Removing the getTexture(...) api on custom effects, and replacing it with
getTextureAccess(...)

http://codereview.appspot.com/6446072/diff/4002/include/gpu/GrCustomStage.h
File include/gpu/GrCustomStage.h (right):

http://codereview.appspot.com/6446072/diff/4002/include/gpu/GrCustomStage.h#n...
include/gpu/GrCustomStage.h:19: /** A structure representing the swizzle access
pattern for a texture.
On 2012/08/01 14:05:52, bsalomon wrote:
> I think we should class-ify this a bit.

I agree.  I left it as a struct because it started off simple, and because of
the precidence for occasionally using structs in ganesh.  Will convert to a
class. 

> 
> Also, why don't we replace the GrCustomStage::texture() function entirely with
> GrCustomStage::textureAccess() and rename numTextures() to
numTextureAccesses()
> or numTexturesAccessed().

I considered that change, but wondered if it was the right fit.  I will make the
change you suggest.

> 
> I will soon want to make GrCustomStage provide a GrTextureParams for each
> texture. This object will be a convenient place to hang that. (Not an ask for
> this CL, just FYI).
> 
> Do we want to consider the case of having multiple swizzles for the same
texture
> (i.e. making fSwizzle an array or list)?
> 

I also thought about this.  My understanding is that for all of our foreseeable
uses, we will always be sampling a given texture in the same way.  Arguably, if
we have separate fetches for r, g, b, wouldn't we want to do a single fetch, and
cache all of the channels?  

Perhaps instead of an array, this swizzle should be understood to be the union
of all channels used during texture fetches.

http://codereview.appspot.com/6446072/diff/4002/include/gpu/GrCustomStage.h#n...
include/gpu/GrCustomStage.h:93: in generated shader code. */
On 2012/08/01 14:05:52, bsalomon wrote:
> nit: here and in several other files a tab got inserted.

My emacs got into a weird state, and started inserting tabs.  I thought I had
removed them.  

I will untabify.

http://codereview.appspot.com/6446072/diff/4002/include/gpu/GrProgramStageFac...
File include/gpu/GrProgramStageFactory.h (right):

http://codereview.appspot.com/6446072/diff/4002/include/gpu/GrProgramStageFac...
include/gpu/GrProgramStageFactory.h:32: const GrGLCaps& caps ) const = 0;
On 2012/08/01 14:05:52, bsalomon wrote:
> tabs

Done.

http://codereview.appspot.com/6446072/diff/4002/include/gpu/GrProgramStageFac...
include/gpu/GrProgramStageFactory.h:84: GLSL code generation. */
On 2012/08/01 14:44:58, robertphillips wrote:
> Is the left spacing still correct here?

Done.

http://codereview.appspot.com/6446072/diff/4002/src/effects/gradients/SkTwoPo...
File src/effects/gradients/SkTwoPointConicalGradient.cpp (left):

http://codereview.appspot.com/6446072/diff/4002/src/effects/gradients/SkTwoPo...
src/effects/gradients/SkTwoPointConicalGradient.cpp:661: 
On 2012/08/01 14:05:52, bsalomon wrote:
> Are you removing the trailing LFs in the files or were there extra ones? If
the
> file ends on } (or anything but LF) clang will yell at us (even though its a
cpp
> not an h file that is never included anywhere).

That's a good point.  I transitioned to the official google3 .emacs file for
this CL, and it automatically 'corrects' trailing white-space and end of files. 


I looked closely at this file, and with my change, it now ends on a single line
feed.  Previously, it was ending with two line feeds.

http://codereview.appspot.com/6446072/diff/4002/src/gpu/GrCustomStage.cpp
File src/gpu/GrCustomStage.cpp (right):

http://codereview.appspot.com/6446072/diff/4002/src/gpu/GrCustomStage.cpp#new...
src/gpu/GrCustomStage.cpp:38: for (unsigned int offset = 0; offset <
swizzle.size(); ++offset)
On 2012/08/01 14:05:52, bsalomon wrote:
> style nit: always use { and }

Done.

http://codereview.appspot.com/6446072/diff/4002/src/gpu/GrCustomStage.cpp#new...
src/gpu/GrCustomStage.cpp:40: if (swizzle.size() < 4)
On 2012/08/01 14:05:52, bsalomon wrote:
> ditto

Done.

http://codereview.appspot.com/6446072/diff/4002/src/gpu/effects/GrColorTableE...
File src/gpu/effects/GrColorTableEffect.cpp (right):

http://codereview.appspot.com/6446072/diff/4002/src/gpu/effects/GrColorTableE...
src/gpu/effects/GrColorTableEffect.cpp:64: access,
On 2012/08/01 14:05:52, bsalomon wrote:
> tabs

Done.

http://codereview.appspot.com/6446072/diff/4002/src/gpu/gl/GrGLProgramStage.h
File src/gpu/gl/GrGLProgramStage.h (right):

http://codereview.appspot.com/6446072/diff/4002/src/gpu/gl/GrGLProgramStage.h...
src/gpu/gl/GrGLProgramStage.h:81: static StageKey GenKey(const GrCustomStage&,
const GrGLCaps&);
On 2012/08/01 14:05:52, bsalomon wrote:
> I'm probably being dense, but I don't get how this gets called.
> 
> The templated factory calls GLProgramStage::GenKey where GLProgramStage (no
> Gr-prefix) is a typedef (or nested class) on a GrCustomStage subclass which
must
> be a subclass of GrGLProgramStage.
> 
> So, it seems like the factory function will go to the GrGLProgramStage
> subclass's static function not this one. What am I missing?

I should have elaborated on this change in my first CL description.

This GenKey routine is not invoked by the factory, but it is the responsibility
of the custom stage to explicitly invoke it.

I'm presently only calling it from the GrGLColorTableEffect::GenKey call.  We
could pull this up so that the factory level routine calls this for all custom
stages, which would then certainly require better key-mixing logic.

http://codereview.appspot.com/6446072/diff/4002/src/gpu/gl/GrGLShaderBuilder.cpp
File src/gpu/gl/GrGLShaderBuilder.cpp (right):

http://codereview.appspot.com/6446072/diff/4002/src/gpu/gl/GrGLShaderBuilder....
src/gpu/gl/GrGLShaderBuilder.cpp:43: if
(GrPixelConfigIsAlphaOnly(access.fTexture->config()) && caps.textureRedSupport()
On 2012/08/01 14:05:52, bsalomon wrote:
> style nit: && at the end of a line rather than beginning 
> 
> Also, would it be simpler as:
>  
> return GrPixelConfigIsAlphaOnly(access.fTexture->config()) &&
> caps.textureRedSupport() && access.referencesAlpha();
> 
> ?

Done.

http://codereview.appspot.com/6446072/diff/4002/src/gpu/gl/GrGLShaderBuilder.h
File src/gpu/gl/GrGLShaderBuilder.h (right):

http://codereview.appspot.com/6446072/diff/4002/src/gpu/gl/GrGLShaderBuilder....
src/gpu/gl/GrGLShaderBuilder.h:72: static GrCustomStage::StageKey
keyForTextureUse(const GrTextureAccess& access,
On 2012/08/01 14:05:52, bsalomon wrote:
> style nit: static function begins with a capital letter.
> 
> Would calling it KeyForTextureAccess() be more consistent naming?

Done.

http://codereview.appspot.com/6446072/diff/4002/src/gpu/gl/GrGpuGL_program.cpp
File src/gpu/gl/GrGpuGL_program.cpp (right):

http://codereview.appspot.com/6446072/diff/4002/src/gpu/gl/GrGpuGL_program.cp...
src/gpu/gl/GrGpuGL_program.cpp:750: setup_custom_stage(&stage, sampler,
this->glCaps(), customStages, fCurrentProgram.get(), s);
On 2012/08/01 14:05:52, bsalomon wrote:
> style-nit: long line

Done.

[bsalomon, 2012-08-01 18:27:22]

On 2012/08/01 18:11:54, twiz1 wrote:
> Thanks for the reviews.
> 
> I made all of the style changes.  I will upload another patch that addresses:
>  - Converting TextureAccess to a class.
>  - Removing the getTexture(...) api on custom effects, and replacing it with
> getTextureAccess(...)
> 
> http://codereview.appspot.com/6446072/diff/4002/include/gpu/GrCustomStage.h
> File include/gpu/GrCustomStage.h (right):
> 
>
http://codereview.appspot.com/6446072/diff/4002/include/gpu/GrCustomStage.h#n...
> include/gpu/GrCustomStage.h:19: /** A structure representing the swizzle
access
> pattern for a texture.
> On 2012/08/01 14:05:52, bsalomon wrote:
> > I think we should class-ify this a bit.
> 
> I agree.  I left it as a struct because it started off simple, and because of
> the precidence for occasionally using structs in ganesh.  Will convert to a
> class. 
> 
> > 
> > Also, why don't we replace the GrCustomStage::texture() function entirely
with
> > GrCustomStage::textureAccess() and rename numTextures() to
> numTextureAccesses()
> > or numTexturesAccessed().
> 
> I considered that change, but wondered if it was the right fit.  I will make
the
> change you suggest.
> 
> > 
> > I will soon want to make GrCustomStage provide a GrTextureParams for each
> > texture. This object will be a convenient place to hang that. (Not an ask
for
> > this CL, just FYI).
> > 
> > Do we want to consider the case of having multiple swizzles for the same
> texture
> > (i.e. making fSwizzle an array or list)?
> > 
> 
> I also thought about this.  My understanding is that for all of our
foreseeable
> uses, we will always be sampling a given texture in the same way.  Arguably,
if
> we have separate fetches for r, g, b, wouldn't we want to do a single fetch,
and
> cache all of the channels?  
> 

My thought was maybe a stage would access the same texture using "rgb" with one
set of coords and "a" different coords. It seems unlikely and probably not worth
worrying about at this juncture.



> Perhaps instead of an array, this swizzle should be understood to be the union
> of all channels used during texture fetches.

That's a pretty good idea. Would it make sense to simplify the object to use a
bitmask rather than a string converted to array in that case? 

> 
>
http://codereview.appspot.com/6446072/diff/4002/include/gpu/GrCustomStage.h#n...
> include/gpu/GrCustomStage.h:93: in generated shader code. */
> On 2012/08/01 14:05:52, bsalomon wrote:
> > nit: here and in several other files a tab got inserted.
> 
> My emacs got into a weird state, and started inserting tabs.  I thought I had
> removed them.  
> 
> I will untabify.
> 
>
http://codereview.appspot.com/6446072/diff/4002/include/gpu/GrProgramStageFac...
> File include/gpu/GrProgramStageFactory.h (right):
> 
>
http://codereview.appspot.com/6446072/diff/4002/include/gpu/GrProgramStageFac...
> include/gpu/GrProgramStageFactory.h:32: const GrGLCaps& caps ) const = 0;
> On 2012/08/01 14:05:52, bsalomon wrote:
> > tabs
> 
> Done.
> 
>
http://codereview.appspot.com/6446072/diff/4002/include/gpu/GrProgramStageFac...
> include/gpu/GrProgramStageFactory.h:84: GLSL code generation. */
> On 2012/08/01 14:44:58, robertphillips wrote:
> > Is the left spacing still correct here?
> 
> Done.
> 
>
http://codereview.appspot.com/6446072/diff/4002/src/effects/gradients/SkTwoPo...
> File src/effects/gradients/SkTwoPointConicalGradient.cpp (left):
> 
>
http://codereview.appspot.com/6446072/diff/4002/src/effects/gradients/SkTwoPo...
> src/effects/gradients/SkTwoPointConicalGradient.cpp:661: 
> On 2012/08/01 14:05:52, bsalomon wrote:
> > Are you removing the trailing LFs in the files or were there extra ones? If
> the
> > file ends on } (or anything but LF) clang will yell at us (even though its a
> cpp
> > not an h file that is never included anywhere).
> 
> That's a good point.  I transitioned to the official google3 .emacs file for
> this CL, and it automatically 'corrects' trailing white-space and end of
files. 
> 
> 
> I looked closely at this file, and with my change, it now ends on a single
line
> feed.  Previously, it was ending with two line feeds.
> 
> http://codereview.appspot.com/6446072/diff/4002/src/gpu/GrCustomStage.cpp
> File src/gpu/GrCustomStage.cpp (right):
> 
>
http://codereview.appspot.com/6446072/diff/4002/src/gpu/GrCustomStage.cpp#new...
> src/gpu/GrCustomStage.cpp:38: for (unsigned int offset = 0; offset <
> swizzle.size(); ++offset)
> On 2012/08/01 14:05:52, bsalomon wrote:
> > style nit: always use { and }
> 
> Done.
> 
>
http://codereview.appspot.com/6446072/diff/4002/src/gpu/GrCustomStage.cpp#new...
> src/gpu/GrCustomStage.cpp:40: if (swizzle.size() < 4)
> On 2012/08/01 14:05:52, bsalomon wrote:
> > ditto
> 
> Done.
> 
>
http://codereview.appspot.com/6446072/diff/4002/src/gpu/effects/GrColorTableE...
> File src/gpu/effects/GrColorTableEffect.cpp (right):
> 
>
http://codereview.appspot.com/6446072/diff/4002/src/gpu/effects/GrColorTableE...
> src/gpu/effects/GrColorTableEffect.cpp:64: access,
> On 2012/08/01 14:05:52, bsalomon wrote:
> > tabs
> 
> Done.
> 
> http://codereview.appspot.com/6446072/diff/4002/src/gpu/gl/GrGLProgramStage.h
> File src/gpu/gl/GrGLProgramStage.h (right):
> 
>
http://codereview.appspot.com/6446072/diff/4002/src/gpu/gl/GrGLProgramStage.h...
> src/gpu/gl/GrGLProgramStage.h:81: static StageKey GenKey(const GrCustomStage&,
> const GrGLCaps&);
> On 2012/08/01 14:05:52, bsalomon wrote:
> > I'm probably being dense, but I don't get how this gets called.
> > 
> > The templated factory calls GLProgramStage::GenKey where GLProgramStage (no
> > Gr-prefix) is a typedef (or nested class) on a GrCustomStage subclass which
> must
> > be a subclass of GrGLProgramStage.
> > 
> > So, it seems like the factory function will go to the GrGLProgramStage
> > subclass's static function not this one. What am I missing?
> 
> I should have elaborated on this change in my first CL description.
> 
> This GenKey routine is not invoked by the factory, but it is the
responsibility
> of the custom stage to explicitly invoke it.
> 
> I'm presently only calling it from the GrGLColorTableEffect::GenKey call.  We
> could pull this up so that the factory level routine calls this for all custom
> stages, which would then certainly require better key-mixing logic.
> 
>
http://codereview.appspot.com/6446072/diff/4002/src/gpu/gl/GrGLShaderBuilder.cpp
> File src/gpu/gl/GrGLShaderBuilder.cpp (right):
> 
>
http://codereview.appspot.com/6446072/diff/4002/src/gpu/gl/GrGLShaderBuilder....
> src/gpu/gl/GrGLShaderBuilder.cpp:43: if
> (GrPixelConfigIsAlphaOnly(access.fTexture->config()) &&
caps.textureRedSupport()
> On 2012/08/01 14:05:52, bsalomon wrote:
> > style nit: && at the end of a line rather than beginning 
> > 
> > Also, would it be simpler as:
> >  
> > return GrPixelConfigIsAlphaOnly(access.fTexture->config()) &&
> > caps.textureRedSupport() && access.referencesAlpha();
> > 
> > ?
> 
> Done.
> 
> http://codereview.appspot.com/6446072/diff/4002/src/gpu/gl/GrGLShaderBuilder.h
> File src/gpu/gl/GrGLShaderBuilder.h (right):
> 
>
http://codereview.appspot.com/6446072/diff/4002/src/gpu/gl/GrGLShaderBuilder....
> src/gpu/gl/GrGLShaderBuilder.h:72: static GrCustomStage::StageKey
> keyForTextureUse(const GrTextureAccess& access,
> On 2012/08/01 14:05:52, bsalomon wrote:
> > style nit: static function begins with a capital letter.
> > 
> > Would calling it KeyForTextureAccess() be more consistent naming?
> 
> Done.
> 
> http://codereview.appspot.com/6446072/diff/4002/src/gpu/gl/GrGpuGL_program.cpp
> File src/gpu/gl/GrGpuGL_program.cpp (right):
> 
>
http://codereview.appspot.com/6446072/diff/4002/src/gpu/gl/GrGpuGL_program.cp...
> src/gpu/gl/GrGpuGL_program.cpp:750: setup_custom_stage(&stage, sampler,
> this->glCaps(), customStages, fCurrentProgram.get(), s);
> On 2012/08/01 14:05:52, bsalomon wrote:
> > style-nit: long line
> 
> Done.

[bsalomon, 2012-08-01 18:33:00]

It seems like it'd be good to make the inclusion of the texture access parts of
the key automatic. I agree that it pushes towards having a range of bits in the
key reserved for texture accesses and not directly settable by the subclass.

http://codereview.appspot.com/6446072/diff/4002/src/gpu/gl/GrGLShaderBuilder.h
File src/gpu/gl/GrGLShaderBuilder.h (right):

http://codereview.appspot.com/6446072/diff/4002/src/gpu/gl/GrGLShaderBuilder....
src/gpu/gl/GrGLShaderBuilder.h:72: static GrCustomStage::StageKey
keyForTextureUse(const GrTextureAccess& access,
On 2012/08/01 18:11:55, twiz1 wrote:
> On 2012/08/01 14:05:52, bsalomon wrote:
> > style nit: static function begins with a capital letter.
> > 
> > Would calling it KeyForTextureAccess() be more consistent naming?
> 
> Done.

Sorry, should have been more clear. There were two parts: capital letter and
changing "Use" To "Access". I'm OK with not doing the latter but it seems a bit
more consistent with the naming of GrTexture"Access".

[TomH, 2012-08-01 18:34:40]

https://codereview.appspot.com/6446072/diff/4002/include/gpu/GrCustomStage.h
File include/gpu/GrCustomStage.h (right):

https://codereview.appspot.com/6446072/diff/4002/include/gpu/GrCustomStage.h#...
include/gpu/GrCustomStage.h:19: /** A structure representing the swizzle access
pattern for a texture.
On 2012/08/01 14:05:52, bsalomon wrote:
> I think we should class-ify this a bit.

Does it actually need classifying?

> Also, why don't we replace the GrCustomStage::texture() function entirely with
> GrCustomStage::textureAccess() and rename numTextures() to
numTextureAccesses()
> or numTexturesAccessed().

I'm tempted to bikeshed on 'Access', but it's probably not worth it.

> Do we want to consider the case of having multiple swizzles for the same
texture
> (i.e. making fSwizzle an array or list)?

YAGNI?

https://codereview.appspot.com/6446072/diff/4002/include/gpu/GrCustomStage.h#...
include/gpu/GrCustomStage.h:19: /** A structure representing the swizzle access
pattern for a texture.
On 2012/08/01 18:11:55, twiz1 wrote:
> On 2012/08/01 14:05:52, bsalomon wrote:
> > I think we should class-ify this a bit.
> 
> I agree.  I left it as a struct because it started off simple, and because of
> the precidence for occasionally using structs in ganesh.  Will convert to a
> class. 

That's what I get for allowing my code review to be interrupted.

https://codereview.appspot.com/6446072/diff/13001/src/gpu/effects/GrColorTabl...
File src/gpu/effects/GrColorTableEffect.cpp (right):

https://codereview.appspot.com/6446072/diff/13001/src/gpu/effects/GrColorTabl...
src/gpu/effects/GrColorTableEffect.cpp:91: const GrSingleTextureEffect& lut =
static_cast<const GrSingleTextureEffect&>(s);
You perform this cast, but never use the result?

https://codereview.appspot.com/6446072/diff/13001/src/gpu/gl/GrGLShaderBuilde...
File src/gpu/gl/GrGLShaderBuilder.cpp (right):

https://codereview.appspot.com/6446072/diff/13001/src/gpu/gl/GrGLShaderBuilde...
src/gpu/gl/GrGLShaderBuilder.cpp:48: const GrGLCaps& caps) {
Couldn't this function be rewritten much more compactly, something like:
SkString output = textureAccess.fSwizzle;
bool replaceAWithR = texture_requires...
for (int offset = ...) {
  if ('a' == output[offset] && replaceAWithR) {
     output[offset] = 'r';
  }
}
return output

You've written something that's really general.

https://codereview.appspot.com/6446072/diff/13001/src/gpu/gl/GrGLShaderBuilde...
src/gpu/gl/GrGLShaderBuilder.cpp:54: // If the sampled texture is alpha-only,
and but R8 formats are supported,
grammar nit: 'but'? Also, missing word in next line.

https://codereview.appspot.com/6446072/diff/13001/src/gpu/gl/GrGLShaderBuilde...
src/gpu/gl/GrGLShaderBuilder.cpp:185: void
GrGLShaderBuilder::emitCustomTextureLookup(SamplerMode samplerMode,
Do we need 'Custom' in the name? Isn't this supposed to end up as the only way
of getting a texture lookup?

https://codereview.appspot.com/6446072/diff/13001/src/gpu/gl/GrGLShaderBuilder.h
File src/gpu/gl/GrGLShaderBuilder.h (right):

https://codereview.appspot.com/6446072/diff/13001/src/gpu/gl/GrGLShaderBuilde...
src/gpu/gl/GrGLShaderBuilder.h:62: The generated swizzle state is built based on
the format of the texture, and the requested
nit: unnnecessary comma

[bsalomon, 2012-08-01 18:39:40]

https://codereview.appspot.com/6446072/diff/13001/src/gpu/gl/GrGLShaderBuilde...
File src/gpu/gl/GrGLShaderBuilder.cpp (right):

https://codereview.appspot.com/6446072/diff/13001/src/gpu/gl/GrGLShaderBuilde...
src/gpu/gl/GrGLShaderBuilder.cpp:185: void
GrGLShaderBuilder::emitCustomTextureLookup(SamplerMode samplerMode,
On 2012/08/01 18:34:41, TomH wrote:
> Do we need 'Custom' in the name? Isn't this supposed to end up as the only way
> of getting a texture lookup?

I agree... the same is true of Gr"Custom"Stage as well :P

[twiz1, 2012-08-01 19:36:41]

On 2012/08/01 18:27:22, bsalomon wrote:
> On 2012/08/01 18:11:54, twiz1 wrote:
> > Thanks for the reviews.
> > 
> > I made all of the style changes.  I will upload another patch that
addresses:
> >  - Converting TextureAccess to a class.
> >  - Removing the getTexture(...) api on custom effects, and replacing it with
> > getTextureAccess(...)
> > 
> > http://codereview.appspot.com/6446072/diff/4002/include/gpu/GrCustomStage.h
> > File include/gpu/GrCustomStage.h (right):
> > 
> >
>
http://codereview.appspot.com/6446072/diff/4002/include/gpu/GrCustomStage.h#n...
> > include/gpu/GrCustomStage.h:19: /** A structure representing the swizzle
> access
> > pattern for a texture.
> > On 2012/08/01 14:05:52, bsalomon wrote:
> > > I think we should class-ify this a bit.
> > 
> > I agree.  I left it as a struct because it started off simple, and because
of
> > the precidence for occasionally using structs in ganesh.  Will convert to a
> > class. 
> > 
> > > 
> > > Also, why don't we replace the GrCustomStage::texture() function entirely
> with
> > > GrCustomStage::textureAccess() and rename numTextures() to
> > numTextureAccesses()
> > > or numTexturesAccessed().
> > 
> > I considered that change, but wondered if it was the right fit.  I will make
> the
> > change you suggest.
> > 
> > > 
> > > I will soon want to make GrCustomStage provide a GrTextureParams for each
> > > texture. This object will be a convenient place to hang that. (Not an ask
> for
> > > this CL, just FYI).
> > > 
> > > Do we want to consider the case of having multiple swizzles for the same
> > texture
> > > (i.e. making fSwizzle an array or list)?
> > > 
> > 
> > I also thought about this.  My understanding is that for all of our
> foreseeable
> > uses, we will always be sampling a given texture in the same way.  Arguably,
> if
> > we have separate fetches for r, g, b, wouldn't we want to do a single fetch,
> and
> > cache all of the channels?  
> > 
> 
> My thought was maybe a stage would access the same texture using "rgb" with
one
> set of coords and "a" different coords. It seems unlikely and probably not
worth
> worrying about at this juncture.
> 
> 
> 
> > Perhaps instead of an array, this swizzle should be understood to be the
union
> > of all channels used during texture fetches.
> 
> That's a pretty good idea. Would it make sense to simplify the object to use a
> bitmask rather than a string converted to array in that case? 

Now that I think about it, since the actual texture swizzle is being generated
from the structure, a bitmask would not be sufficient - the specific ordering of
the channels is relevant, not just there presence.  This means my union idea
wouldn't quite work.

I second that supporting an array of accesses isn't worth following at this
point.

> 
> > 
> >
>
http://codereview.appspot.com/6446072/diff/4002/include/gpu/GrCustomStage.h#n...
> > include/gpu/GrCustomStage.h:93: in generated shader code. */
> > On 2012/08/01 14:05:52, bsalomon wrote:
> > > nit: here and in several other files a tab got inserted.
> > 
> > My emacs got into a weird state, and started inserting tabs.  I thought I
had
> > removed them.  
> > 
> > I will untabify.
> > 
> >
>
http://codereview.appspot.com/6446072/diff/4002/include/gpu/GrProgramStageFac...
> > File include/gpu/GrProgramStageFactory.h (right):
> > 
> >
>
http://codereview.appspot.com/6446072/diff/4002/include/gpu/GrProgramStageFac...
> > include/gpu/GrProgramStageFactory.h:32: const GrGLCaps& caps ) const = 0;
> > On 2012/08/01 14:05:52, bsalomon wrote:
> > > tabs
> > 
> > Done.
> > 
> >
>
http://codereview.appspot.com/6446072/diff/4002/include/gpu/GrProgramStageFac...
> > include/gpu/GrProgramStageFactory.h:84: GLSL code generation. */
> > On 2012/08/01 14:44:58, robertphillips wrote:
> > > Is the left spacing still correct here?
> > 
> > Done.
> > 
> >
>
http://codereview.appspot.com/6446072/diff/4002/src/effects/gradients/SkTwoPo...
> > File src/effects/gradients/SkTwoPointConicalGradient.cpp (left):
> > 
> >
>
http://codereview.appspot.com/6446072/diff/4002/src/effects/gradients/SkTwoPo...
> > src/effects/gradients/SkTwoPointConicalGradient.cpp:661: 
> > On 2012/08/01 14:05:52, bsalomon wrote:
> > > Are you removing the trailing LFs in the files or were there extra ones?
If
> > the
> > > file ends on } (or anything but LF) clang will yell at us (even though its
a
> > cpp
> > > not an h file that is never included anywhere).
> > 
> > That's a good point.  I transitioned to the official google3 .emacs file for
> > this CL, and it automatically 'corrects' trailing white-space and end of
> files. 
> > 
> > 
> > I looked closely at this file, and with my change, it now ends on a single
> line
> > feed.  Previously, it was ending with two line feeds.
> > 
> > http://codereview.appspot.com/6446072/diff/4002/src/gpu/GrCustomStage.cpp
> > File src/gpu/GrCustomStage.cpp (right):
> > 
> >
>
http://codereview.appspot.com/6446072/diff/4002/src/gpu/GrCustomStage.cpp#new...
> > src/gpu/GrCustomStage.cpp:38: for (unsigned int offset = 0; offset <
> > swizzle.size(); ++offset)
> > On 2012/08/01 14:05:52, bsalomon wrote:
> > > style nit: always use { and }
> > 
> > Done.
> > 
> >
>
http://codereview.appspot.com/6446072/diff/4002/src/gpu/GrCustomStage.cpp#new...
> > src/gpu/GrCustomStage.cpp:40: if (swizzle.size() < 4)
> > On 2012/08/01 14:05:52, bsalomon wrote:
> > > ditto
> > 
> > Done.
> > 
> >
>
http://codereview.appspot.com/6446072/diff/4002/src/gpu/effects/GrColorTableE...
> > File src/gpu/effects/GrColorTableEffect.cpp (right):
> > 
> >
>
http://codereview.appspot.com/6446072/diff/4002/src/gpu/effects/GrColorTableE...
> > src/gpu/effects/GrColorTableEffect.cpp:64: access,
> > On 2012/08/01 14:05:52, bsalomon wrote:
> > > tabs
> > 
> > Done.
> > 
> >
http://codereview.appspot.com/6446072/diff/4002/src/gpu/gl/GrGLProgramStage.h
> > File src/gpu/gl/GrGLProgramStage.h (right):
> > 
> >
>
http://codereview.appspot.com/6446072/diff/4002/src/gpu/gl/GrGLProgramStage.h...
> > src/gpu/gl/GrGLProgramStage.h:81: static StageKey GenKey(const
GrCustomStage&,
> > const GrGLCaps&);
> > On 2012/08/01 14:05:52, bsalomon wrote:
> > > I'm probably being dense, but I don't get how this gets called.
> > > 
> > > The templated factory calls GLProgramStage::GenKey where GLProgramStage
(no
> > > Gr-prefix) is a typedef (or nested class) on a GrCustomStage subclass
which
> > must
> > > be a subclass of GrGLProgramStage.
> > > 
> > > So, it seems like the factory function will go to the GrGLProgramStage
> > > subclass's static function not this one. What am I missing?
> > 
> > I should have elaborated on this change in my first CL description.
> > 
> > This GenKey routine is not invoked by the factory, but it is the
> responsibility
> > of the custom stage to explicitly invoke it.
> > 
> > I'm presently only calling it from the GrGLColorTableEffect::GenKey call. 
We
> > could pull this up so that the factory level routine calls this for all
custom
> > stages, which would then certainly require better key-mixing logic.
> > 
> >
>
http://codereview.appspot.com/6446072/diff/4002/src/gpu/gl/GrGLShaderBuilder.cpp
> > File src/gpu/gl/GrGLShaderBuilder.cpp (right):
> > 
> >
>
http://codereview.appspot.com/6446072/diff/4002/src/gpu/gl/GrGLShaderBuilder....
> > src/gpu/gl/GrGLShaderBuilder.cpp:43: if
> > (GrPixelConfigIsAlphaOnly(access.fTexture->config()) &&
> caps.textureRedSupport()
> > On 2012/08/01 14:05:52, bsalomon wrote:
> > > style nit: && at the end of a line rather than beginning 
> > > 
> > > Also, would it be simpler as:
> > >  
> > > return GrPixelConfigIsAlphaOnly(access.fTexture->config()) &&
> > > caps.textureRedSupport() && access.referencesAlpha();
> > > 
> > > ?
> > 
> > Done.
> > 
> >
http://codereview.appspot.com/6446072/diff/4002/src/gpu/gl/GrGLShaderBuilder.h
> > File src/gpu/gl/GrGLShaderBuilder.h (right):
> > 
> >
>
http://codereview.appspot.com/6446072/diff/4002/src/gpu/gl/GrGLShaderBuilder....
> > src/gpu/gl/GrGLShaderBuilder.h:72: static GrCustomStage::StageKey
> > keyForTextureUse(const GrTextureAccess& access,
> > On 2012/08/01 14:05:52, bsalomon wrote:
> > > style nit: static function begins with a capital letter.
> > > 
> > > Would calling it KeyForTextureAccess() be more consistent naming?
> > 
> > Done.
> > 
> >
http://codereview.appspot.com/6446072/diff/4002/src/gpu/gl/GrGpuGL_program.cpp
> > File src/gpu/gl/GrGpuGL_program.cpp (right):
> > 
> >
>
http://codereview.appspot.com/6446072/diff/4002/src/gpu/gl/GrGpuGL_program.cp...
> > src/gpu/gl/GrGpuGL_program.cpp:750: setup_custom_stage(&stage, sampler,
> > this->glCaps(), customStages, fCurrentProgram.get(), s);
> > On 2012/08/01 14:05:52, bsalomon wrote:
> > > style-nit: long line
> > 
> > Done.

[twiz1, 2012-08-01 20:14:12]

Thx again for the reviews.

I addressed the nits, and converted GrTextureAccess to a class.

Next patch will fix the texture key generation so that all stages will pull in
key generation - noting that only GrColorTableEffect will return texture access
specifications.  We will need to retrofit access specs on the existing stages.

https://codereview.appspot.com/6446072/diff/13001/src/gpu/effects/GrColorTabl...
File src/gpu/effects/GrColorTableEffect.cpp (right):

https://codereview.appspot.com/6446072/diff/13001/src/gpu/effects/GrColorTabl...
src/gpu/effects/GrColorTableEffect.cpp:91: const GrSingleTextureEffect& lut =
static_cast<const GrSingleTextureEffect&>(s);
On 2012/08/01 18:34:41, TomH wrote:
> You perform this cast, but never use the result?

Done.

https://codereview.appspot.com/6446072/diff/13001/src/gpu/gl/GrGLShaderBuilde...
File src/gpu/gl/GrGLShaderBuilder.cpp (right):

https://codereview.appspot.com/6446072/diff/13001/src/gpu/gl/GrGLShaderBuilde...
src/gpu/gl/GrGLShaderBuilder.cpp:48: const GrGLCaps& caps) {
On 2012/08/01 18:34:41, TomH wrote:
> Couldn't this function be rewritten much more compactly, something like:
> SkString output = textureAccess.fSwizzle;
> bool replaceAWithR = texture_requires...
> for (int offset = ...) {
>   if ('a' == output[offset] && replaceAWithR) {
>      output[offset] = 'r';
>   }
> }
> return output
> 
> You've written something that's really general.

Done.

https://codereview.appspot.com/6446072/diff/13001/src/gpu/gl/GrGLShaderBuilde...
src/gpu/gl/GrGLShaderBuilder.cpp:54: // If the sampled texture is alpha-only,
and but R8 formats are supported,
On 2012/08/01 18:34:41, TomH wrote:
> grammar nit: 'but'? Also, missing word in next line.

Comment removed with the conversion to simplified code.

https://codereview.appspot.com/6446072/diff/13001/src/gpu/gl/GrGLShaderBuilde...
src/gpu/gl/GrGLShaderBuilder.cpp:185: void
GrGLShaderBuilder::emitCustomTextureLookup(SamplerMode samplerMode,
On 2012/08/01 18:39:40, bsalomon wrote:
> On 2012/08/01 18:34:41, TomH wrote:
> > Do we need 'Custom' in the name? Isn't this supposed to end up as the only
way
> > of getting a texture lookup?
> 
> I agree... the same is true of Gr"Custom"Stage as well :P
> 

At the moment it's custom - because it's not a default fetch.  Once this gets
cleaned up, we can drop the custom.  (I'd make a meme here . . . )

https://codereview.appspot.com/6446072/diff/13001/src/gpu/gl/GrGLShaderBuilder.h
File src/gpu/gl/GrGLShaderBuilder.h (right):

https://codereview.appspot.com/6446072/diff/13001/src/gpu/gl/GrGLShaderBuilde...
src/gpu/gl/GrGLShaderBuilder.h:62: The generated swizzle state is built based on
the format of the texture, and the requested
On 2012/08/01 18:34:41, TomH wrote:
> nit: unnnecessary comma

Done.

[twiz1, 2012-08-01 20:59:52]

I made a small modification to the key generation code in
GrProgramStageFactory.h to include a separate set of bits for texture-use keys.

Instead of relying on each custom stage to explicitly call the parent GenKey to
compute the texture component of the keys, the template now does this for all
classes.  

I see the follow-up changes for this work being the following:
 - Removing numTextures, and texture from custom effect, and relying entirely on
GrTextureAccess for texture information.
 - Ensuring that all custom effects correctly report their texture access
pattern.
 - Finally, piping the existing default fetch through this system.

http://codereview.appspot.com/6446072/diff/4002/src/gpu/gl/GrGLShaderBuilder.h
File src/gpu/gl/GrGLShaderBuilder.h (right):

http://codereview.appspot.com/6446072/diff/4002/src/gpu/gl/GrGLShaderBuilder....
src/gpu/gl/GrGLShaderBuilder.h:72: static GrCustomStage::StageKey
keyForTextureUse(const GrTextureAccess& access,
On 2012/08/01 18:33:01, bsalomon wrote:
> On 2012/08/01 18:11:55, twiz1 wrote:
> > On 2012/08/01 14:05:52, bsalomon wrote:
> > > style nit: static function begins with a capital letter.
> > > 
> > > Would calling it KeyForTextureAccess() be more consistent naming?
> > 
> > Done.
> 
> Sorry, should have been more clear. There were two parts: capital letter and
> changing "Use" To "Access". I'm OK with not doing the latter but it seems a
bit
> more consistent with the naming of GrTexture"Access".

Done.

[TomH, 2012-08-01 21:01:28]

LGTM.

Exciting list of follow-up changes. I'd be happy to do any of those if you've
got other things that need doing.

[bsalomon, 2012-08-01 21:10:04]

On 2012/08/01 21:01:28, TomH wrote:
> LGTM.
> 
> Exciting list of follow-up changes. I'd be happy to do any of those if you've
> got other things that need doing.

LGTM

[bsalomon, 2012-08-01 21:10:22]

Forgot to publish this Q:

http://codereview.appspot.com/6446072/diff/13004/src/gpu/effects/GrColorTable...
File src/gpu/effects/GrColorTableEffect.cpp (right):

http://codereview.appspot.com/6446072/diff/13004/src/gpu/effects/GrColorTable...
src/gpu/effects/GrColorTableEffect.cpp:90: const GrGLCaps& caps) {
Do we need caps passed to GenKey anymore?

[twiz1, 2012-08-01 21:22:45]

http://codereview.appspot.com/6446072/diff/13004/src/gpu/effects/GrColorTable...
File src/gpu/effects/GrColorTableEffect.cpp (right):

http://codereview.appspot.com/6446072/diff/13004/src/gpu/effects/GrColorTable...
src/gpu/effects/GrColorTableEffect.cpp:90: const GrGLCaps& caps) {
On 2012/08/01 21:10:23, bsalomon wrote:
> Do we need caps passed to GenKey anymore?

No, the caps is no longer required.  I left it in this revision, because it
feels like the caps is full of information that may be required during key
generation.  

Do we have a preference for if the caps changes should be reverted?  

(I also forgot to highlight this in my last code upload.)

[bsalomon, 2012-08-01 21:24:23]

On 2012/08/01 21:22:45, twiz1 wrote:
>
http://codereview.appspot.com/6446072/diff/13004/src/gpu/effects/GrColorTable...
> File src/gpu/effects/GrColorTableEffect.cpp (right):
> 
>
http://codereview.appspot.com/6446072/diff/13004/src/gpu/effects/GrColorTable...
> src/gpu/effects/GrColorTableEffect.cpp:90: const GrGLCaps& caps) {
> On 2012/08/01 21:10:23, bsalomon wrote:
> > Do we need caps passed to GenKey anymore?
> 
> No, the caps is no longer required.  I left it in this revision, because it
> feels like the caps is full of information that may be required during key
> generation.  
> 

I think you're right. We'd probably wind up adding it back in the future anyway.
Let's leave it.

> Do we have a preference for if the caps changes should be reverted?  
> 
> (I also forgot to highlight this in my last code upload.)

[twiz1, 2012-08-01 21:39:44]

On 2012/08/01 21:01:28, TomH wrote:
> LGTM.
> 
> Exciting list of follow-up changes. I'd be happy to do any of those if you've
> got other things that need doing.

Anyone is welcome to take on those tasks.  I'm moving my focus over to win8
work, so I won't be able to immediately continue this work.

[twiz1, 2012-08-01 22:14:16]

On 2012/08/01 21:39:44, twiz1 wrote:
> On 2012/08/01 21:01:28, TomH wrote:
> > LGTM.
> > 
> > Exciting list of follow-up changes. I'd be happy to do any of those if
you've
> > got other things that need doing.
> 
> Anyone is welcome to take on those tasks.  I'm moving my focus over to win8
> work, so I won't be able to immediately continue this work.

The LGTMs are a little premature.  It seems that somehow the ImageLightingGM
slide is breaking with this change.  The following assert is triggered with this
change.

Will investigate.

../src/effects/gradients/SkTwoPointRadialGradient.cpp(578) : 
data.isDegenerate() == fIsDegenerate failed

Program received signal SIGSEGV, Segmentation fault.
0x0000000000541fa6 in GrGLRadial2Gradient::setData (this=0xdee310, uman=...,
baseData=..., stageNum=0)
    at ../src/effects/gradients/SkTwoPointRadialGradient.cpp:578
578	    GrAssert(data.isDegenerate() == fIsDegenerate);
(gdb) bt
#0  0x0000000000541fa6 in GrGLRadial2Gradient::setData (this=0xdee310, uman=...,
baseData=..., stageNum=0)
    at ../src/effects/gradients/SkTwoPointRadialGradient.cpp:578
#1  0x0000000000588aab in GrGpuGL::flushGraphicsState (this=0xa2a7b0,
type=GrGpu::kDrawTriangles_DrawType)
    at ../src/gpu/gl/GrGpuGL_program.cpp:419
#2  0x000000000055c1bb in GrGpu::setupClipAndFlushState (this=0xa2a7b0,
type=GrGpu::kDrawTriangles_DrawType)
    at ../src/gpu/GrGpu.cpp:372
#3  0x000000000055c3f9 in GrGpu::onDrawNonIndexed (this=0xa2a7b0,
type=kTriangleFan_GrPrimitiveType, 
    startVertex=0, vertexCount=4) at ../src/gpu/GrGpu.cpp:431
#4  0x000000000055790e in GrDrawTarget::drawNonIndexed (this=0xa2a7b0,
type=kTriangleFan_GrPrimitiveType, 
    startVertex=0, vertexCount=4) at ../src/gpu/GrDrawTarget.cpp:805
#5  0x00000000005581a6 in GrDrawTarget::drawRect (this=0xa2a7b0, rect=...,
matrix=0x0, srcRects=0x0, 
    srcMatrices=0x0) at ../src/gpu/GrDrawTarget.cpp:1067
#6  0x0000000000553d51 in GrDrawTarget::drawSimpleRect (this=0xa2a7b0, rect=...,
matrix=0x0)
    at ../src/gpu/GrDrawTarget.h:496
#7  0x000000000054d21a in GrContext::drawRect (this=0xa25e20, paint=...,
rect=..., width=-1, matrix=0x0)
    at ../src/gpu/GrContext.cpp:847
#8  0x00000000005b4b81 in (anonymous namespace)::apply_custom_stage
(context=0xa25e20, srcTexture=0x11f6750, 
    dstTexture=0x112dcb0, rect=..., stage=0xb7e570) at
../src/gpu/SkGpuDevice.cpp:1495
#9  0x00000000005b4d84 in filter_texture (context=0xa25e20, texture=0x11f6750,
filter=0x1981b10, rect=...)
    at ../src/gpu/SkGpuDevice.cpp:1515
#10 0x00000000005b50e3 in SkGpuDevice::drawSprite (this=0xb70390, draw=...,
bitmap=..., left=0, top=0, paint=
    ...) at ../src/gpu/SkGpuDevice.cpp:1553
#11 0x000000000046efe4 in SkCanvas::drawSprite (this=0x1459610, bitmap=..., x=0,
y=0, paint=0x7fffffffdf60)
    at ../src/core/SkCanvas.cpp:1036
#12 0x000000000042c535 in skiagm::ImageLightingGM::onDraw (this=0x13c26f0,
canvas=0x1459610)
    at ../gm/lighting.cpp:67
#13 0x000000000040579e in skiagm::GM::drawContent (this=0x13c26f0,
canvas=0x1459610) at ../gm/gm.cpp:24
#14 0x000000000040576e in skiagm::GM::draw (this=0x13c26f0, canvas=0x1459610) at
../gm/gm.cpp:20
#15 0x00000000004068e3 in invokeGM (gm=0x13c26f0, canvas=0x1459610, isPDF=false)
at ../gm/gmmain.cpp:288
#16 0x0000000000406c0f in generate_image (gm=0x13c26f0, gRec=...,
context=0xa25e20, rt=0x153c090, 
    bitmap=0x7fffffffe270, deferred=false) at ../gm/gmmain.cpp:323
#17 0x0000000000407900 in test_drawing (gm=0x13c26f0, gRec=..., writePath=0x0,
readPath=0x0, diffPath=0x0, 
    context=0xa25e20, rt=0x153c090, bitmap=0x7fffffffe270) at
../gm/gmmain.cpp:545
#18 0x0000000000409423 in main (argc=1, argv=0x7fffffffe540) at
../gm/gmmain.cpp:1062
(gdb)

[twiz1, 2012-08-01 23:09:15]

On 2012/08/01 22:14:16, twiz1 wrote:
> On 2012/08/01 21:39:44, twiz1 wrote:
> > On 2012/08/01 21:01:28, TomH wrote:
> > > LGTM.
> > > 
> > > Exciting list of follow-up changes. I'd be happy to do any of those if
> you've
> > > got other things that need doing.
> > 
> > Anyone is welcome to take on those tasks.  I'm moving my focus over to win8
> > work, so I won't be able to immediately continue this work.
> 
> The LGTMs are a little premature.  It seems that somehow the ImageLightingGM
> slide is breaking with this change.  The following assert is triggered with
this
> change.
> 
> Will investigate.

Patch set 8 corrects the problem.  StageDesc::fCustomStageKey was using a
uint16_t, which was causing hash collisions with the larger key space.  

I corrected it to reference GrProgramStageFactory::StageKey to tie it to the
correct key size.

> 
> ../src/effects/gradients/SkTwoPointRadialGradient.cpp(578) : 
> data.isDegenerate() == fIsDegenerate failed
> 
> Program received signal SIGSEGV, Segmentation fault.
> 0x0000000000541fa6 in GrGLRadial2Gradient::setData (this=0xdee310, uman=...,
> baseData=..., stageNum=0)
>     at ../src/effects/gradients/SkTwoPointRadialGradient.cpp:578
> 578	    GrAssert(data.isDegenerate() == fIsDegenerate);
> (gdb) bt
> #0  0x0000000000541fa6 in GrGLRadial2Gradient::setData (this=0xdee310,
uman=...,
> baseData=..., stageNum=0)
>     at ../src/effects/gradients/SkTwoPointRadialGradient.cpp:578
> #1  0x0000000000588aab in GrGpuGL::flushGraphicsState (this=0xa2a7b0,
> type=GrGpu::kDrawTriangles_DrawType)
>     at ../src/gpu/gl/GrGpuGL_program.cpp:419
> #2  0x000000000055c1bb in GrGpu::setupClipAndFlushState (this=0xa2a7b0,
> type=GrGpu::kDrawTriangles_DrawType)
>     at ../src/gpu/GrGpu.cpp:372
> #3  0x000000000055c3f9 in GrGpu::onDrawNonIndexed (this=0xa2a7b0,
> type=kTriangleFan_GrPrimitiveType, 
>     startVertex=0, vertexCount=4) at ../src/gpu/GrGpu.cpp:431
> #4  0x000000000055790e in GrDrawTarget::drawNonIndexed (this=0xa2a7b0,
> type=kTriangleFan_GrPrimitiveType, 
>     startVertex=0, vertexCount=4) at ../src/gpu/GrDrawTarget.cpp:805
> #5  0x00000000005581a6 in GrDrawTarget::drawRect (this=0xa2a7b0, rect=...,
> matrix=0x0, srcRects=0x0, 
>     srcMatrices=0x0) at ../src/gpu/GrDrawTarget.cpp:1067
> #6  0x0000000000553d51 in GrDrawTarget::drawSimpleRect (this=0xa2a7b0,
rect=...,
> matrix=0x0)
>     at ../src/gpu/GrDrawTarget.h:496
> #7  0x000000000054d21a in GrContext::drawRect (this=0xa25e20, paint=...,
> rect=..., width=-1, matrix=0x0)
>     at ../src/gpu/GrContext.cpp:847
> #8  0x00000000005b4b81 in (anonymous namespace)::apply_custom_stage
> (context=0xa25e20, srcTexture=0x11f6750, 
>     dstTexture=0x112dcb0, rect=..., stage=0xb7e570) at
> ../src/gpu/SkGpuDevice.cpp:1495
> #9  0x00000000005b4d84 in filter_texture (context=0xa25e20, texture=0x11f6750,
> filter=0x1981b10, rect=...)
>     at ../src/gpu/SkGpuDevice.cpp:1515
> #10 0x00000000005b50e3 in SkGpuDevice::drawSprite (this=0xb70390, draw=...,
> bitmap=..., left=0, top=0, paint=
>     ...) at ../src/gpu/SkGpuDevice.cpp:1553
> #11 0x000000000046efe4 in SkCanvas::drawSprite (this=0x1459610, bitmap=...,
x=0,
> y=0, paint=0x7fffffffdf60)
>     at ../src/core/SkCanvas.cpp:1036
> #12 0x000000000042c535 in skiagm::ImageLightingGM::onDraw (this=0x13c26f0,
> canvas=0x1459610)
>     at ../gm/lighting.cpp:67
> #13 0x000000000040579e in skiagm::GM::drawContent (this=0x13c26f0,
> canvas=0x1459610) at ../gm/gm.cpp:24
> #14 0x000000000040576e in skiagm::GM::draw (this=0x13c26f0, canvas=0x1459610)
at
> ../gm/gm.cpp:20
> #15 0x00000000004068e3 in invokeGM (gm=0x13c26f0, canvas=0x1459610,
isPDF=false)
> at ../gm/gmmain.cpp:288
> #16 0x0000000000406c0f in generate_image (gm=0x13c26f0, gRec=...,
> context=0xa25e20, rt=0x153c090, 
>     bitmap=0x7fffffffe270, deferred=false) at ../gm/gmmain.cpp:323
> #17 0x0000000000407900 in test_drawing (gm=0x13c26f0, gRec=..., writePath=0x0,
> readPath=0x0, diffPath=0x0, 
>     context=0xa25e20, rt=0x153c090, bitmap=0x7fffffffe270) at
> ../gm/gmmain.cpp:545
> #18 0x0000000000409423 in main (argc=1, argv=0x7fffffffe540) at
> ../gm/gmmain.cpp:1062
> (gdb)

[bsalomon, 2012-08-02 13:07:11]

Nice catch, thanks for the thorough testing! LGTM

[twiz1, 2012-08-02 15:15:07]

Introduction of set of functions to manage generation of texture fetch shader
code.  

A new set of routines have been added to GrGLShaderBuilder to emit texture
fetches, taking into consideration the format of the texture to be accessed, and
the channel swizzle.