Some refactoring of GrCustomStage and friends

Some refactoring of GrCustomStage and friends

[bsalomon, 2012-05-17 01:24:09]

Hey Tom, While I've been out of town I started looking at doing the morphology
rewrite. I wanted to do some cleanup first, though. This isn't finished and
ready for the fine toothed comb but I wanted you to see it before I get any
farther and I may not get to update it before I'm on my way back.

I wanted to do thee things:

1) Templatize the factory as you suggested in your comments so that the
implementor of a custom effect doesn't have to do any work to get a factory. 

2) Make the insertion of the class-specific part of the stage id
automatic/unavoidable.

3) Remove the fData pointer from GrGLProgramStage since I think there are
lifetime issues there. It refers to a GrCustomStage which may only live as long
as the current draw while the GrGLProgramStage has program-lifetime.

Doing this helped me understand how all your classes relate to each other at a
deeper level so it was worth it even if you tell me it's bunk. :)

[TomH, 2012-05-17 14:26:53]

Want!

http://codereview.appspot.com/6209071/diff/1/include/core/SkTemplates.h
File include/core/SkTemplates.h (right):

http://codereview.appspot.com/6209071/diff/1/include/core/SkTemplates.h#newco...
include/core/SkTemplates.h:292: T* get() { return
reinterpret_cast<T*>(fStorage.get()); }
With this change, the comment above is obsolete.
Was the comment wrong?

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

http://codereview.appspot.com/6209071/diff/1/include/gpu/GrCustomStage.h#newc...
include/gpu/GrCustomStage.h:69: }
I didn't entirely understand this at first reading, but on rereading it makes
sense. Not sure how to improve intelligibility except perhaps calling out
explicitly what GenKey() is doing (filling in the lower bits?!)

http://codereview.appspot.com/6209071/diff/1/include/gpu/GrCustomStage.h#newc...
include/gpu/GrCustomStage.h:116: GrGLProgramStage created by the factory.
Ooh, does this mean we need to have GLProgramStage in the GrCustomStage-subclass
header? Fail...

http://codereview.appspot.com/6209071/diff/1/src/gpu/GrProgramStageFactory.h
File src/gpu/GrProgramStageFactory.h (right):

http://codereview.appspot.com/6209071/diff/1/src/gpu/GrProgramStageFactory.h#...
src/gpu/GrProgramStageFactory.h:14: class GrGLProgramStage;
Shouldn't this entire header be dead now?

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

http://codereview.appspot.com/6209071/diff/1/src/gpu/effects/GrConvolutionEff...
src/gpu/effects/GrConvolutionEffect.cpp:40: int            fKernelWidth;
Eh, OK, I was trying to avoid the redundancy, but there's enough win in this I
suppose we can live with it. Too bad I never wrote a design doc where we could
add this...

http://codereview.appspot.com/6209071/diff/1/src/gpu/effects/GrConvolutionEff...
src/gpu/effects/GrConvolutionEffect.cpp:149: GrAssert(conv->width() ==
fKernelWidth);
Good.

http://codereview.appspot.com/6209071/diff/1/src/gpu/effects/GrConvolutionEff...
src/gpu/effects/GrConvolutionEffect.cpp:168: return static_cast<const
GrConvolutionEffect*>(s)->width();
Hmm - can we maybe put the assert back into the caller that GenKey() is only
touching the bits it's allowed to touch?

[bsalomon, 2012-05-18 18:20:22]

http://codereview.appspot.com/6209071/diff/1/include/core/SkTemplates.h
File include/core/SkTemplates.h (right):

http://codereview.appspot.com/6209071/diff/1/include/core/SkTemplates.h#newco...
include/core/SkTemplates.h:292: T* get() { return
reinterpret_cast<T*>(fStorage.get()); }
On 2012/05/17 14:26:53, TomH wrote:
> With this change, the comment above is obsolete.
> Was the comment wrong?

I think the change is probably wrong. I reverted it.

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

http://codereview.appspot.com/6209071/diff/1/include/gpu/GrCustomStage.h#newc...
include/gpu/GrCustomStage.h:69: }
On 2012/05/17 14:26:53, TomH wrote:
> I didn't entirely understand this at first reading, but on rereading it makes
> sense. Not sure how to improve intelligibility except perhaps calling out
> explicitly what GenKey() is doing (filling in the lower bits?!)

I tried rewriting, let me know what you think. [It's now in
GrProgramStageFactory.h]

http://codereview.appspot.com/6209071/diff/1/include/gpu/GrCustomStage.h#newc...
include/gpu/GrCustomStage.h:116: GrGLProgramStage created by the factory.
On 2012/05/17 14:26:53, TomH wrote:
> Ooh, does this mean we need to have GLProgramStage in the
GrCustomStage-subclass
> header? Fail...

It just has to be forward decl'ed. Do you think that is OK?

http://codereview.appspot.com/6209071/diff/1/src/gpu/GrProgramStageFactory.h
File src/gpu/GrProgramStageFactory.h (right):

http://codereview.appspot.com/6209071/diff/1/src/gpu/GrProgramStageFactory.h#...
src/gpu/GrProgramStageFactory.h:14: class GrGLProgramStage;
On 2012/05/17 14:26:53, TomH wrote:
> Shouldn't this entire header be dead now?


I retyped all the program factory stuff in the custom stage header just for
convenience. I think it is too much clutter for a GrCustomStage writer to skip
so I'm moving the factory stuff back here.

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

http://codereview.appspot.com/6209071/diff/1/src/gpu/effects/GrConvolutionEff...
src/gpu/effects/GrConvolutionEffect.cpp:40: int            fKernelWidth;
On 2012/05/17 14:26:53, TomH wrote:
> Eh, OK, I was trying to avoid the redundancy, but there's enough win in this I
> suppose we can live with it. Too bad I never wrote a design doc where we could
> add this...

I added it because I didn't think we can have a persistent fData object to refer
back to for the width. Also, I wanted to assert that later GrConvolutionEffects
used with this GrGLConvolutionProgramStage have a matching kernel width.

http://codereview.appspot.com/6209071/diff/1/src/gpu/effects/GrConvolutionEff...
src/gpu/effects/GrConvolutionEffect.cpp:168: return static_cast<const
GrConvolutionEffect*>(s)->width();
On 2012/05/17 14:26:53, TomH wrote:
> Hmm - can we maybe put the assert back into the caller that GenKey() is only
> touching the bits it's allowed to touch?

GrTProgramStageFactory::stageKey is the caller of GenKey and he does assert
(kIllegalIDMask)

[TomH, 2012-05-18 19:38:39]

LGTM except for:

On 2012/05/18 18:20:22, bsalomon wrote:
> http://codereview.appspot.com/6209071/diff/1/include/gpu/GrCustomStage.h
> File include/gpu/GrCustomStage.h (right):
> 
>
http://codereview.appspot.com/6209071/diff/1/include/gpu/GrCustomStage.h#newc...
> include/gpu/GrCustomStage.h:69: }
> On 2012/05/17 14:26:53, TomH wrote:
> > I didn't entirely understand this at first reading, but on rereading it
makes
> > sense. Not sure how to improve intelligibility except perhaps calling out
> > explicitly what GenKey() is doing (filling in the lower bits?!)
> 
> I tried rewriting, let me know what you think. [It's now in
> GrProgramStageFactory.h]

GrProgramStageFactory.h is empty (but not deleted, unlike the .cpp)

[bsalomon, 2012-05-18 20:28:22]

Closed with r4003 and r4004 (fix mac/win/linux build breaks due to SkTemplates.h
being included in the GrCustomStage.h rather than GrProgramStageFactory.h)