GrSingleTextureEffect

Creates a new base class for any GrCustomStage that needs a texture. Ports all extant stages to use it.

Verified that new stages that try to use the old texture pointer & not derive from GrSingleTextureEffect will fail gracefully. A follow-on CL will remove the old texture pointer.

Base class has virtual API to support arbitrary numbers of textures, but doesn't provide any storage for them.

[TomH, 2012-06-18 21:01:20]

Prototype. Passes unit tests & GM, but leaks a texture somewhere - perhaps I'm
not cleaning up GrCustomStages correcty yet?

GrCustomStage only stores a count of textures. GrSingleTextureEffect keeps a
texture pointer.
Presumably effects with more textures would have arrays or ???

If we're binding textures for a stage that has a texture associated, uses that
*instead of* the texture stored in the sampler.

[bsalomon, 2012-06-19 12:52:52]

Maybe we should run either the existing code or the patched code in gm or bench
with Rob's instance counter to see if we are leaking effects?

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

http://codereview.appspot.com/6306097/diff/1/include/gpu/GrCustomStage.h#newc...
include/gpu/GrCustomStage.h:67: unsigned int numTextures() const { return
fNumTextures; }
Do we think this has enough flexibility or should we consider either:

a) making numTextures virtual

b) iterator instead of array type access

class GrCustomStage {
    virtual GrTexture* firstTexture();
    virtual GrTexture* nextTexture();
}

for (GrTexture* tex = stage.firstTexture(),
     NULL != tex;
     tex = stage.nextTexture()) {
}

http://codereview.appspot.com/6306097/diff/1/src/gpu/effects/GrConvolutionEff...
File src/gpu/effects/GrConvolutionEffect.cpp (right):

http://codereview.appspot.com/6306097/diff/1/src/gpu/effects/GrConvolutionEff...
src/gpu/effects/GrConvolutionEffect.cpp:125: GrGLTexture& texture =
Does this need to be downcasted?

http://codereview.appspot.com/6306097/diff/1/src/gpu/effects/GrSingleTextureE...
File src/gpu/effects/GrSingleTextureEffect.h (right):

http://codereview.appspot.com/6306097/diff/1/src/gpu/effects/GrSingleTextureE...
src/gpu/effects/GrSingleTextureEffect.h:13: class GrSingleTextureEffect : public
GrCustomStage {
Does this add much value? Won't we sometimes have gradients with 1 texture and
sometimes none (e.g. simple 2 color gradient).

http://codereview.appspot.com/6306097/diff/1/src/gpu/gl/GrGpuGL.cpp
File src/gpu/gl/GrGpuGL.cpp (right):

http://codereview.appspot.com/6306097/diff/1/src/gpu/gl/GrGpuGL.cpp#newcode2123
src/gpu/gl/GrGpuGL.cpp:2123: // HACK - if we're using a new SingleTextureEffect,
override
Does this mean we're doing things in the wrong order? Should we first get
everybody going through customstages so that we can push textures down to the
custom effect and remomve the old array on drawstate in one swoop?

[TomH, 2012-06-19 19:33:14]

No idea how I picked up the gyp change?

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

http://codereview.appspot.com/6306097/diff/1/include/gpu/GrCustomStage.h#newc...
include/gpu/GrCustomStage.h:67: unsigned int numTextures() const { return
fNumTextures; }
On 2012/06/19 12:52:53, bsalomon wrote:
> a) making numTextures virtual

I was going to say "overkill", but your point about simple gradients undoes me.

> b) iterator instead of array type access

If that's all you wanted for an iterator, it's much simpler than what I was
playing with, so maybe supportable, but it's useful to know # of textures so
that we can do things like allocate texture slots intelligently.

http://codereview.appspot.com/6306097/diff/1/src/gpu/effects/GrConvolutionEff...
File src/gpu/effects/GrConvolutionEffect.cpp (right):

http://codereview.appspot.com/6306097/diff/1/src/gpu/effects/GrConvolutionEff...
src/gpu/effects/GrConvolutionEffect.cpp:125: GrGLTexture& texture =
On 2012/06/19 12:52:53, bsalomon wrote:
> Does this need to be downcasted?

Done.

http://codereview.appspot.com/6306097/diff/1/src/gpu/effects/GrSingleTextureE...
File src/gpu/effects/GrSingleTextureEffect.h (right):

http://codereview.appspot.com/6306097/diff/1/src/gpu/effects/GrSingleTextureE...
src/gpu/effects/GrSingleTextureEffect.h:13: class GrSingleTextureEffect : public
GrCustomStage {
On 2012/06/19 12:52:53, bsalomon wrote:
> Does this add much value? Won't we sometimes have gradients with 1 texture and
> sometimes none (e.g. simple 2 color gradient). 

Ouch.
So, I wanted to separate texture storage from the base class exactly because
we'd have all sorts of weird use cases; having nothing but a count there keeps
the base class small while lets people who have multiple or variable add
whatever data type they need.

But I'm not sure how that interacts with the simple 2-color gradient. Maybe
virtual numTextures() suffices; that actually makes the base class more compact,
since we have a default implementation that returns 0 and no need for a member
variable. A priori, I suspect that I'd still want to keep this class around.

http://codereview.appspot.com/6306097/diff/1/src/gpu/gl/GrGpuGL.cpp
File src/gpu/gl/GrGpuGL.cpp (right):

http://codereview.appspot.com/6306097/diff/1/src/gpu/gl/GrGpuGL.cpp#newcode2123
src/gpu/gl/GrGpuGL.cpp:2123: // HACK - if we're using a new SingleTextureEffect,
override
On 2012/06/19 12:52:53, bsalomon wrote:
> Does this mean we're doing things in the wrong order? Should we first get
> everybody going through customstages so that we can push textures down to the
> custom effect and remomve the old array on drawstate in one swoop?
This is not meant to be a finished CL, it was meant to get a temperature reading
for things like the GrSingleTextureEffect.

[TomH, 2012-07-11 19:52:47]

Ready for another look.
Steven, Jeff, this will break your custom effects under development
(gracefully!) once it lands.

[robertphillips, 2012-07-11 20:11:13]

One question.

http://codereview.appspot.com/6306097/diff/12001/src/gpu/effects/GrSingleText...
File src/gpu/effects/GrSingleTextureEffect.h (right):

http://codereview.appspot.com/6306097/diff/12001/src/gpu/effects/GrSingleText...
src/gpu/effects/GrSingleTextureEffect.h:16: GrSingleTextureEffect(GrTexture*
texture);
I don't think we put SK_OVERRIDE on the destructors?

[TomH, 2012-07-11 20:12:09]

http://codereview.appspot.com/6306097/diff/12001/src/gpu/effects/GrSingleText...
File src/gpu/effects/GrSingleTextureEffect.h (right):

http://codereview.appspot.com/6306097/diff/12001/src/gpu/effects/GrSingleText...
src/gpu/effects/GrSingleTextureEffect.h:16: GrSingleTextureEffect(GrTexture*
texture);
On 2012/07/11 20:11:13, robertphillips wrote:
> I don't think we put SK_OVERRIDE on the destructors?

I think you're right, they don't belong on __ct or __dt.

[robertphillips, 2012-07-11 20:15:03]

Also, I should've said LGTM

[bsalomon, 2012-07-11 20:25:27]

Will the defaultfetch / coord dims stuff eventually move to SingleTextureEffect
from Builder? Maybe that depends on also moving matrix to the effect (along with
the boiler plate texture coordinate stuff that is still in GrGLProgram).

*Should* Jeff inherit from this? His effect seems different than the rest in
that it only needs the incoming color and no additional coordinates?

http://codereview.appspot.com/6306097/diff/12001/src/gpu/gl/GrGpuGL.cpp
File src/gpu/gl/GrGpuGL.cpp (right):

http://codereview.appspot.com/6306097/diff/12001/src/gpu/gl/GrGpuGL.cpp#newco...
src/gpu/gl/GrGpuGL.cpp:2142: // HACK - if we're using a new SingleTextureEffect,
override
I take it the next step is to make ALL textures go through custom effect and
remove the texture array from GrPaint/GrDrawState?

[TomH, 2012-07-11 20:28:46]

On 2012/07/11 20:25:27, bsalomon wrote:
> Will the defaultfetch / coord dims stuff eventually move to
SingleTextureEffect
> from Builder? Maybe that depends on also moving matrix to the effect (along
with
> the boiler plate texture coordinate stuff that is still in GrGLProgram).

Yeah, not a small move.

> *Should* Jeff inherit from this? His effect seems different than the rest in
> that it only needs the incoming color and no additional coordinates?

I don't know. If that texture pointer is vestigial, then I suppose he doesn't
need to inherit. But the way I read his GrGLColorTableEffect::emitFS(), he's
doing a texture lookup, right?

> http://codereview.appspot.com/6306097/diff/12001/src/gpu/gl/GrGpuGL.cpp
> File src/gpu/gl/GrGpuGL.cpp (right):
> 
>
http://codereview.appspot.com/6306097/diff/12001/src/gpu/gl/GrGpuGL.cpp#newco...
> src/gpu/gl/GrGpuGL.cpp:2142: // HACK - if we're using a new
SingleTextureEffect,
> override
> I take it the next step is to make ALL textures go through custom effect and
> remove the texture array from GrPaint/GrDrawState?

Yes. We could try to do that as part of this change, although to be clean it
should also involve default fetch, coord dims, matrix (?), which makes the CL
bigger and more complicated.

[bsalomon, 2012-07-11 20:33:22]

On 2012/07/11 20:28:46, TomH wrote:
> On 2012/07/11 20:25:27, bsalomon wrote:
> > Will the defaultfetch / coord dims stuff eventually move to
> SingleTextureEffect
> > from Builder? Maybe that depends on also moving matrix to the effect (along
> with
> > the boiler plate texture coordinate stuff that is still in GrGLProgram).
> 
> Yeah, not a small move.


Agreed, not arguing for doing it as part of this CL.

> 
> > *Should* Jeff inherit from this? His effect seems different than the rest in
> > that it only needs the incoming color and no additional coordinates?
> 
> I don't know. If that texture pointer is vestigial, then I suppose he doesn't
> need to inherit. But the way I read his GrGLColorTableEffect::emitFS(), he's
> doing a texture lookup, right?

Yeah, doing a texture lookup but with different inputs. He doesn't need a
matrix, doesn't need texture coords, doesn't need to worry about whether the
texture is alpha-only or rgba. If the boilerplate stuff related to those
concerns is going to move from GrGLProgram to GrSingleTextureEffect it seems
like he shouldn't inherit from it. I suppose there could be another subclass of
GrSingleTexturEffect that brings in those elements.

> 
> > http://codereview.appspot.com/6306097/diff/12001/src/gpu/gl/GrGpuGL.cpp
> > File src/gpu/gl/GrGpuGL.cpp (right):
> > 
> >
>
http://codereview.appspot.com/6306097/diff/12001/src/gpu/gl/GrGpuGL.cpp#newco...
> > src/gpu/gl/GrGpuGL.cpp:2142: // HACK - if we're using a new
> SingleTextureEffect,
> > override
> > I take it the next step is to make ALL textures go through custom effect and
> > remove the texture array from GrPaint/GrDrawState?
> 
> Yes. We could try to do that as part of this change, although to be clean it
> should also involve default fetch, coord dims, matrix (?), which makes the CL
> bigger and more complicated.

I wouldn't do it as part of this change. Just trying to get a feel for the
roadmap.

[Stephen White, 2012-07-12 14:36:55]

On 2012/07/11 20:25:27, bsalomon wrote:
> *Should* Jeff inherit from this? His effect seems different than the rest in
> that it only needs the incoming color and no additional coordinates?

I don't think it should, in the long term.  Although for now the LUT stage will
probably be doing two texture lookups explicitly (one for the image colour, and
one for the LUT), it should probably be able to take the input of a previous
stage as colour input, rather than an explicit image texture lookup.  (Apologies
in advance if that's already possible).

The reason is, I'd like to re-use the LUT shader to do gamma correction before
and after rendering a primitive, for example.  So its input could be just a
solid colour or the output of a gradient, for example.  We might be able to use
this to do the automatic ColorSpace conversions that WebKit currently implements
only in the CoreGraphics path.

One way to do this might be to have custom stages taken another custom stage as
input.  If they don't specify one, they could generate a texture instruction as
they do now.  Otherwise they just read the value of some variable (or call a
function in the other stage).  Or we could break out the image texture lookup
into its own stage, and always mandate that LUT stages have a connected input,
but that might mean a little too much boilerplate code in the caller.

> http://codereview.appspot.com/6306097/diff/12001/src/gpu/gl/GrGpuGL.cpp
> File src/gpu/gl/GrGpuGL.cpp (right):
> 
>
http://codereview.appspot.com/6306097/diff/12001/src/gpu/gl/GrGpuGL.cpp#newco...
> src/gpu/gl/GrGpuGL.cpp:2142: // HACK - if we're using a new
SingleTextureEffect,
> override
> I take it the next step is to make ALL textures go through custom effect and
> remove the texture array from GrPaint/GrDrawState?

[Stephen White, 2012-07-12 14:52:37]

http://codereview.appspot.com/6306097/diff/21001/include/core/SkImageFilter.h
File include/core/SkImageFilter.h (right):

http://codereview.appspot.com/6306097/diff/21001/include/core/SkImageFilter.h...
include/core/SkImageFilter.h:90: virtual bool asNewCustomStage(GrCustomStage**
stage, GrTexture*) const;
Not quite sure I understand why the custom stage must be created with a texture.
 Theoretically, you might want to apply the same custom stage to a different
texture (although we don't do that now).  On the CPU side of image filters, the
input buffer is specified during processing, not at creation time.  IWBN to do
that for the GPU side as well, if possible.

[TomH, 2012-07-12 15:04:05]

On 2012/07/12 14:52:37, Stephen White wrote:
> http://codereview.appspot.com/6306097/diff/21001/include/core/SkImageFilter.h
> File include/core/SkImageFilter.h (right):
> 
>
http://codereview.appspot.com/6306097/diff/21001/include/core/SkImageFilter.h...
> include/core/SkImageFilter.h:90: virtual bool asNewCustomStage(GrCustomStage**
> stage, GrTexture*) const;
> Not quite sure I understand why the custom stage must be created with a
texture.
>  Theoretically, you might want to apply the same custom stage to a different
> texture (although we don't do that now).  On the CPU side of image filters,
the
> input buffer is specified during processing, not at creation time.  IWBN to do
> that for the GPU side as well, if possible.

GrGLProgramStage equality (comparing StageKeys) is different from GrCustomStage
equality. GrGLProgramStages can be reused with different textures, but
GrCustomStages can *not*.

(Right?)

And the shader cache (should) take care of reusing the compiled GL
implementation with a different texture.

[bsalomon, 2012-07-12 15:06:20]

On 2012/07/12 14:36:55, Stephen White wrote:
> On 2012/07/11 20:25:27, bsalomon wrote:
> > *Should* Jeff inherit from this? His effect seems different than the rest in
> > that it only needs the incoming color and no additional coordinates?
> 
> I don't think it should, in the long term.  Although for now the LUT stage
will
> probably be doing two texture lookups explicitly (one for the image colour,
and
> one for the LUT), it should probably be able to take the input of a previous
> stage as colour input, rather than an explicit image texture lookup. 
(Apologies
> in advance if that's already possible).

I think the LUT already works as you describe. The custom effect always receives
a var that is the current color (or coverage). This is either the
constant/per-vertex color or the color output by the previous stage. The LUT
stage uses the incoming color to compute its sample coordinates. Today the
GrGLProgram creates some boiler plate code that adds a varying texture coord
computed in the vs as matrix * position or matrix * tex coord vertex attrib. The
LUT ignores that varying. We want the insertion of that boiler plate code out of
the stage glue and instead to only be emitted by stages that need varying
texture coords / matrix. It seems like it should either live in a subclass of
GrSingleTextureEffect so the LUT doesn't pick it up or in some helper class that
stages can optionally use.

> 
> The reason is, I'd like to re-use the LUT shader to do gamma correction before
> and after rendering a primitive, for example.  So its input could be just a
> solid colour or the output of a gradient, for example.  We might be able to
use
> this to do the automatic ColorSpace conversions that WebKit currently
implements
> only in the CoreGraphics path.
> 
> One way to do this might be to have custom stages taken another custom stage
as
> input.  If they don't specify one, they could generate a texture instruction
as
> they do now.  Otherwise they just read the value of some variable (or call a
> function in the other stage).  Or we could break out the image texture lookup
> into its own stage, and always mandate that LUT stages have a connected input,
> but that might mean a little too much boilerplate code in the caller.

> 
> > http://codereview.appspot.com/6306097/diff/12001/src/gpu/gl/GrGpuGL.cpp
> > File src/gpu/gl/GrGpuGL.cpp (right):
> > 
> >
>
http://codereview.appspot.com/6306097/diff/12001/src/gpu/gl/GrGpuGL.cpp#newco...
> > src/gpu/gl/GrGpuGL.cpp:2142: // HACK - if we're using a new
> SingleTextureEffect,
> > override
> > I take it the next step is to make ALL textures go through custom effect and
> > remove the texture array from GrPaint/GrDrawState?

[bsalomon, 2012-07-12 15:07:32]

On 2012/07/12 15:04:05, TomH wrote:
> On 2012/07/12 14:52:37, Stephen White wrote:
> >
http://codereview.appspot.com/6306097/diff/21001/include/core/SkImageFilter.h
> > File include/core/SkImageFilter.h (right):
> > 
> >
>
http://codereview.appspot.com/6306097/diff/21001/include/core/SkImageFilter.h...
> > include/core/SkImageFilter.h:90: virtual bool
asNewCustomStage(GrCustomStage**
> > stage, GrTexture*) const;
> > Not quite sure I understand why the custom stage must be created with a
> texture.
> >  Theoretically, you might want to apply the same custom stage to a different
> > texture (although we don't do that now).  On the CPU side of image filters,
> the
> > input buffer is specified during processing, not at creation time.  IWBN to
do
> > that for the GPU side as well, if possible.
> 
> GrGLProgramStage equality (comparing StageKeys) is different from
GrCustomStage
> equality. GrGLProgramStages can be reused with different textures, but
> GrCustomStages can *not*.
> 
> (Right?)
> 
> And the shader cache (should) take care of reusing the compiled GL
> implementation with a different texture.

I'm not sure I get your answer. What's wrong with allowing the texture of a
custom effect to be set post-construction?

[Stephen White, 2012-07-12 15:10:29]

On 2012/07/12 15:04:05, TomH wrote:
> On 2012/07/12 14:52:37, Stephen White wrote:
> >
http://codereview.appspot.com/6306097/diff/21001/include/core/SkImageFilter.h
> > File include/core/SkImageFilter.h (right):
> > 
> >
>
http://codereview.appspot.com/6306097/diff/21001/include/core/SkImageFilter.h...
> > include/core/SkImageFilter.h:90: virtual bool
asNewCustomStage(GrCustomStage**
> > stage, GrTexture*) const;
> > Not quite sure I understand why the custom stage must be created with a
> texture.
> >  Theoretically, you might want to apply the same custom stage to a different
> > texture (although we don't do that now).  On the CPU side of image filters,
> the
> > input buffer is specified during processing, not at creation time.  IWBN to
do
> > that for the GPU side as well, if possible.
> 
> GrGLProgramStage equality (comparing StageKeys) is different from
GrCustomStage
> equality. GrGLProgramStages can be reused with different textures, but
> GrCustomStages can *not*.

That's my point -- why not?  I believe they could before, since they pick up the
texture at process time.  Why make them intimately locked in at GrCustomStage
construction time?

I understand the goal (I think:  to move all texture code out of the monolithic
shader generator).  I just find the API kind of weird.  Could we not have some
kind of texturing GrCustomStage that must be instantiated by those who want
texturing?  Or is that too fine-grained?  Let's discuss before landing this,
perhaps elsewhere than this bug.  :)

> And the shader cache (should) take care of reusing the compiled GL
> implementation with a different texture.

Yes, I understand that it's correct for our current uses.

[TomH, 2012-07-12 15:29:49]

Brian & I are talking. Recording notes:

 - GrCustomStage must not change after it's first drawn, or we break deferred
rendering. +junov@ whenever I see the word "deferred".
 - We could have setters & mark it immutable once drawn, but then we have to
have deferred error handling.
 - Constructors are ugly but give us immutability for free.
 - We could instead make a (deep ?) copy when we draw, but that's more load on
the custom memory management.
    - Brian points out that until we introduced custom effects, we were doing a
deep copy of every parameter - and we wanted to reduce that overhead. Copying
only relevant properties will at least mean fewer bytes copied.

Note: there's probably a bug we're not handling - are all the
SingleTextureEffect subclasses including the texture pointer in isEqual()?

Right now Brian wants to copy, Tom prefers immutability but can be swayed.

[TomH, 2012-07-12 17:25:14]

After much discussion with Brian & Stephen followed by verbal LGTMs, documented
immutability, fixed potential bug in isEquals(), and committed as r4576.

[Stephen White, 2012-07-12 17:35:21]

After our discussion I am Enlightened.  Since the other parameters are set at
construction time and not subsequently changed, it makes perfect sense to do the
same for textures.  (So count me in the "immutable" camp).

As I mentioned, we might want to have the ability to add multiple textures in
the asNewCustomStage() call, but we can revisit the API when we find a need for
it.

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

http://codereview.appspot.com/6306097/diff/15003/include/gpu/GrCustomStage.h#...
include/gpu/GrCustomStage.h:75: virtual bool isEqual(const GrCustomStage&)
const;
It seems strange that the base class needs to know about stuff that's really
particular to GrSingleTextureEffect.

I realize you've already committed, but for posterity:  I would try to remove
these when you remove the old texture pointer, and simply make
GrSingleTextureEffect::isEqual() handle the texture comparisons.  With this and
the calls from GrGpu.cpp gone, I think you could get rid of numTextures() and
texture() from the base class (where IMHO they don't belong).

[Stephen White, 2012-07-12 17:42:14]

http://codereview.appspot.com/6306097/diff/15003/src/gpu/effects/GrMorphology...
File src/gpu/effects/GrMorphologyEffect.cpp (right):

http://codereview.appspot.com/6306097/diff/15003/src/gpu/effects/GrMorphology...
src/gpu/effects/GrMorphologyEffect.cpp:131:
*static_cast<GrGLTexture*>(data.texture(0));
Here (and in the other effects) you're getting the texture from data.texture(0),
rather than from fTexture in the GrSingleTextureEffect base class.  Is there a
reason for that?

[TomH, 2012-07-12 17:59:29]

On 2012/07/12 17:42:14, Stephen White wrote:
>
http://codereview.appspot.com/6306097/diff/15003/src/gpu/effects/GrMorphology...
> File src/gpu/effects/GrMorphologyEffect.cpp (right):
> 
>
http://codereview.appspot.com/6306097/diff/15003/src/gpu/effects/GrMorphology...
> src/gpu/effects/GrMorphologyEffect.cpp:131:
> *static_cast<GrGLTexture*>(data.texture(0));
> Here (and in the other effects) you're getting the texture from
data.texture(0),
> rather than from fTexture in the GrSingleTextureEffect base class.  Is there a
> reason for that?

Just using the interface rather than the implementation - no important reason.

More notes for posterity: there is still a *lot* of directly setting textures on
draw state that will need to be replaced with trivial  custom stages before we
can eliminate the texture array from the draw state.

[twiz1, 2012-07-12 18:57:53]

On 2012/07/11 20:33:22, bsalomon wrote:
> On 2012/07/11 20:28:46, TomH wrote:
> > On 2012/07/11 20:25:27, bsalomon wrote:
> > > Will the defaultfetch / coord dims stuff eventually move to
> > SingleTextureEffect
> > > from Builder? Maybe that depends on also moving matrix to the effect
(along
> > with
> > > the boiler plate texture coordinate stuff that is still in GrGLProgram).
> > 
> > Yeah, not a small move.
> 
> 
> Agreed, not arguing for doing it as part of this CL.
> 
> > 
> > > *Should* Jeff inherit from this? His effect seems different than the rest
in
> > > that it only needs the incoming color and no additional coordinates?
> > 
> > I don't know. If that texture pointer is vestigial, then I suppose he
doesn't
> > need to inherit. But the way I read his GrGLColorTableEffect::emitFS(), he's
> > doing a texture lookup, right?
> 
> Yeah, doing a texture lookup but with different inputs. He doesn't need a
> matrix, doesn't need texture coords, doesn't need to worry about whether the
> texture is alpha-only or rgba. If the boilerplate stuff related to those
> concerns is going to move from GrGLProgram to GrSingleTextureEffect it seems
> like he shouldn't inherit from it. I suppose there could be another subclass
of
> GrSingleTexturEffect that brings in those elements.

I just synced with this change, and it seems that something is going wrong with
my existing CL:  https://codereview.appspot.com/6351081/

After the sync, the lut is not making it's way to the shader - I'm getting an
empty LUT.  

Investigating further right now, and will post an update.

Note that I did not modify my CL to inherit from the new classes, as per this
discussion.

> 
> > 
> > > http://codereview.appspot.com/6306097/diff/12001/src/gpu/gl/GrGpuGL.cpp
> > > File src/gpu/gl/GrGpuGL.cpp (right):
> > > 
> > >
> >
>
http://codereview.appspot.com/6306097/diff/12001/src/gpu/gl/GrGpuGL.cpp#newco...
> > > src/gpu/gl/GrGpuGL.cpp:2142: // HACK - if we're using a new
> > SingleTextureEffect,
> > > override
> > > I take it the next step is to make ALL textures go through custom effect
and
> > > remove the texture array from GrPaint/GrDrawState?
> > 
> > Yes. We could try to do that as part of this change, although to be clean it
> > should also involve default fetch, coord dims, matrix (?), which makes the
CL
> > bigger and more complicated.
> 
> I wouldn't do it as part of this change. Just trying to get a feel for the
> roadmap.

[bsalomon, 2012-07-12 19:37:42]

On 2012/07/12 18:57:53, twiz1 wrote:
> On 2012/07/11 20:33:22, bsalomon wrote:
> > On 2012/07/11 20:28:46, TomH wrote:
> > > On 2012/07/11 20:25:27, bsalomon wrote:
> > > > Will the defaultfetch / coord dims stuff eventually move to
> > > SingleTextureEffect
> > > > from Builder? Maybe that depends on also moving matrix to the effect
> (along
> > > with
> > > > the boiler plate texture coordinate stuff that is still in GrGLProgram).
> > > 
> > > Yeah, not a small move.
> > 
> > 
> > Agreed, not arguing for doing it as part of this CL.
> > 
> > > 
> > > > *Should* Jeff inherit from this? His effect seems different than the
rest
> in
> > > > that it only needs the incoming color and no additional coordinates?
> > > 
> > > I don't know. If that texture pointer is vestigial, then I suppose he
> doesn't
> > > need to inherit. But the way I read his GrGLColorTableEffect::emitFS(),
he's
> > > doing a texture lookup, right?
> > 
> > Yeah, doing a texture lookup but with different inputs. He doesn't need a
> > matrix, doesn't need texture coords, doesn't need to worry about whether the
> > texture is alpha-only or rgba. If the boilerplate stuff related to those
> > concerns is going to move from GrGLProgram to GrSingleTextureEffect it seems
> > like he shouldn't inherit from it. I suppose there could be another subclass
> of
> > GrSingleTexturEffect that brings in those elements.
> 
> I just synced with this change, and it seems that something is going wrong
with
> my existing CL:  https://codereview.appspot.com/6351081/
> 
> After the sync, the lut is not making it's way to the shader - I'm getting an
> empty LUT.  
> 
> Investigating further right now, and will post an update.
> 
> Note that I did not modify my CL to inherit from the new classes, as per this
> discussion.

Hey Jeff, If you'd like, post the merged patch and I can help track it down.

> 
> > 
> > > 
> > > > http://codereview.appspot.com/6306097/diff/12001/src/gpu/gl/GrGpuGL.cpp
> > > > File src/gpu/gl/GrGpuGL.cpp (right):
> > > > 
> > > >
> > >
> >
>
http://codereview.appspot.com/6306097/diff/12001/src/gpu/gl/GrGpuGL.cpp#newco...
> > > > src/gpu/gl/GrGpuGL.cpp:2142: // HACK - if we're using a new
> > > SingleTextureEffect,
> > > > override
> > > > I take it the next step is to make ALL textures go through custom effect
> and
> > > > remove the texture array from GrPaint/GrDrawState?
> > > 
> > > Yes. We could try to do that as part of this change, although to be clean
it
> > > should also involve default fetch, coord dims, matrix (?), which makes the
> CL
> > > bigger and more complicated.
> > 
> > I wouldn't do it as part of this change. Just trying to get a feel for the
> > roadmap.

[twiz1, 2012-07-12 19:45:39]

On 2012/07/12 19:37:42, bsalomon wrote:
> On 2012/07/12 18:57:53, twiz1 wrote:
> > On 2012/07/11 20:33:22, bsalomon wrote:
> > > On 2012/07/11 20:28:46, TomH wrote:
> > > > On 2012/07/11 20:25:27, bsalomon wrote:
> > > > > Will the defaultfetch / coord dims stuff eventually move to
> > > > SingleTextureEffect
> > > > > from Builder? Maybe that depends on also moving matrix to the effect
> > (along
> > > > with
> > > > > the boiler plate texture coordinate stuff that is still in
GrGLProgram).
> > > > 
> > > > Yeah, not a small move.
> > > 
> > > 
> > > Agreed, not arguing for doing it as part of this CL.
> > > 
> > > > 
> > > > > *Should* Jeff inherit from this? His effect seems different than the
> rest
> > in
> > > > > that it only needs the incoming color and no additional coordinates?
> > > > 
> > > > I don't know. If that texture pointer is vestigial, then I suppose he
> > doesn't
> > > > need to inherit. But the way I read his GrGLColorTableEffect::emitFS(),
> he's
> > > > doing a texture lookup, right?
> > > 
> > > Yeah, doing a texture lookup but with different inputs. He doesn't need a
> > > matrix, doesn't need texture coords, doesn't need to worry about whether
the
> > > texture is alpha-only or rgba. If the boilerplate stuff related to those
> > > concerns is going to move from GrGLProgram to GrSingleTextureEffect it
seems
> > > like he shouldn't inherit from it. I suppose there could be another
subclass
> > of
> > > GrSingleTexturEffect that brings in those elements.
> > 
> > I just synced with this change, and it seems that something is going wrong
> with
> > my existing CL:  https://codereview.appspot.com/6351081/
> > 
> > After the sync, the lut is not making it's way to the shader - I'm getting
an
> > empty LUT.  
> > 
> > Investigating further right now, and will post an update.
> > 
> > Note that I did not modify my CL to inherit from the new classes, as per
this
> > discussion.
> 
> Hey Jeff, If you'd like, post the merged patch and I can help track it down.

Patch uploaded as set #4.  (https://codereview.appspot.com/6351081/)

I think the stage is being skipped entirely, as emitFS is not being invoked.

> 
> > 
> > > 
> > > > 
> > > > >
http://codereview.appspot.com/6306097/diff/12001/src/gpu/gl/GrGpuGL.cpp
> > > > > File src/gpu/gl/GrGpuGL.cpp (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
http://codereview.appspot.com/6306097/diff/12001/src/gpu/gl/GrGpuGL.cpp#newco...
> > > > > src/gpu/gl/GrGpuGL.cpp:2142: // HACK - if we're using a new
> > > > SingleTextureEffect,
> > > > > override
> > > > > I take it the next step is to make ALL textures go through custom
effect
> > and
> > > > > remove the texture array from GrPaint/GrDrawState?
> > > > 
> > > > Yes. We could try to do that as part of this change, although to be
clean
> it
> > > > should also involve default fetch, coord dims, matrix (?), which makes
the
> > CL
> > > > bigger and more complicated.
> > > 
> > > I wouldn't do it as part of this change. Just trying to get a feel for the
> > > roadmap.

[twiz1, 2012-07-13 19:50:10]

On 2012/07/12 19:45:39, twiz1 wrote:
> On 2012/07/12 19:37:42, bsalomon wrote:
> > On 2012/07/12 18:57:53, twiz1 wrote:
> > > On 2012/07/11 20:33:22, bsalomon wrote:
> > > > On 2012/07/11 20:28:46, TomH wrote:
> > > > > On 2012/07/11 20:25:27, bsalomon wrote:
> > > > > > Will the defaultfetch / coord dims stuff eventually move to
> > > > > SingleTextureEffect
> > > > > > from Builder? Maybe that depends on also moving matrix to the effect
> > > (along
> > > > > with
> > > > > > the boiler plate texture coordinate stuff that is still in
> GrGLProgram).
> > > > > 
> > > > > Yeah, not a small move.
> > > > 
> > > > 
> > > > Agreed, not arguing for doing it as part of this CL.
> > > > 
> > > > > 
> > > > > > *Should* Jeff inherit from this? His effect seems different than the
> > rest
> > > in
> > > > > > that it only needs the incoming color and no additional coordinates?
> > > > > 
> > > > > I don't know. If that texture pointer is vestigial, then I suppose he
> > > doesn't
> > > > > need to inherit. But the way I read his
GrGLColorTableEffect::emitFS(),
> > he's
> > > > > doing a texture lookup, right?
> > > > 
> > > > Yeah, doing a texture lookup but with different inputs. He doesn't need
a
> > > > matrix, doesn't need texture coords, doesn't need to worry about whether
> the
> > > > texture is alpha-only or rgba. If the boilerplate stuff related to those
> > > > concerns is going to move from GrGLProgram to GrSingleTextureEffect it
> seems
> > > > like he shouldn't inherit from it. I suppose there could be another
> subclass
> > > of
> > > > GrSingleTexturEffect that brings in those elements.
> > > 
> > > I just synced with this change, and it seems that something is going wrong
> > with
> > > my existing CL:  https://codereview.appspot.com/6351081/
> > > 
> > > After the sync, the lut is not making it's way to the shader - I'm getting
> an
> > > empty LUT.  
> > > 
> > > Investigating further right now, and will post an update.
> > > 
> > > Note that I did not modify my CL to inherit from the new classes, as per
> this
> > > discussion.
> > 
> > Hey Jeff, If you'd like, post the merged patch and I can help track it down.
> 
> Patch uploaded as set #4.  (https://codereview.appspot.com/6351081/)
> 
> I think the stage is being skipped entirely, as emitFS is not being invoked.

An update on this conflict.  The regression I've been mentioning is not related
to this change.  After some hand-rolled bisect-builds, I narrowed down the
revision that causes my problem: 
https://code.google.com/p/skia/source/detail?r=4555

Discussion on the problem will continue on the other issue.
(https://codereview.appspot.com/6351081/)

> 
> > 
> > > 
> > > > 
> > > > > 
> > > > > >
> http://codereview.appspot.com/6306097/diff/12001/src/gpu/gl/GrGpuGL.cpp
> > > > > > File src/gpu/gl/GrGpuGL.cpp (right):
> > > > > > 
> > > > > >
> > > > >
> > > >
> > >
> >
>
http://codereview.appspot.com/6306097/diff/12001/src/gpu/gl/GrGpuGL.cpp#newco...
> > > > > > src/gpu/gl/GrGpuGL.cpp:2142: // HACK - if we're using a new
> > > > > SingleTextureEffect,
> > > > > > override
> > > > > > I take it the next step is to make ALL textures go through custom
> effect
> > > and
> > > > > > remove the texture array from GrPaint/GrDrawState?
> > > > > 
> > > > > Yes. We could try to do that as part of this change, although to be
> clean
> > it
> > > > > should also involve default fetch, coord dims, matrix (?), which makes
> the
> > > CL
> > > > > bigger and more complicated.
> > > > 
> > > > I wouldn't do it as part of this change. Just trying to get a feel for
the
> > > > roadmap.