Pull out Effect Shaders in GPU Path Renderer

Pull out Effect Shaders in GPU Path Renderer

Move the effects in HairLinePathRender (and eventually others into their
own class called GrBezierEffects. This will allow for more sharing of
code between different path renderers on GPU.

BUG=

Committed: http://code.google.com/p/skia/source/detail?r=10877

Committed: http://code.google.com/p/skia/source/detail?r=10884

[egdaniel, 2013-08-21 19:48:32]

Here's the first version of having a separate effects class for the path
shaders. Just looking for general feed back as of now. I still need to do some
testing using Brian's gpu gm tool.

[bsalomon, 2013-08-21 20:04:38]

https://codereview.chromium.org/23004010/diff/1/src/gpu/effects/GrBezierEffect.h
File src/gpu/effects/GrBezierEffect.h (right):

https://codereview.chromium.org/23004010/diff/1/src/gpu/effects/GrBezierEffec...
src/gpu/effects/GrBezierEffect.h:13: enum BezierEffectType {
I'm wondering if there is something more specific we could call this.
BezierEdgeType?
BezierEdgeFill?
BezierEdgeStyle?


Also should be GrFoo {
kA_GrFoo,
kB_GrFoo,...
}
if it's not scoped by a Gr class or namespace

https://codereview.chromium.org/23004010/diff/1/src/gpu/effects/GrBezierEffec...
src/gpu/effects/GrBezierEffect.h:20: * Shader is based off of Loop-Blinn
Quadratic GPU Rendering
Should we merge this comment block with the next one?

https://codereview.chromium.org/23004010/diff/1/src/gpu/effects/GrBezierEffec...
src/gpu/effects/GrBezierEffect.h:66: GR_CREATE_STATIC_EFFECT(gConicFillNoAA,
GrConicEffect, (false, true));
It seems like we should just plumb the enum value all the way down rather than
converting it to two bools here (and repeating this for the other curve types).

could have helpers:
static inline bool GrBezierEdgeTypeIsFill(BezierEdgeType);
static inline bool GrBezierEdgeTypeIsAA(BezierEdgeType);

if it helps.

https://codereview.chromium.org/23004010/diff/1/src/gpu/effects/GrBezierEffec...
src/gpu/effects/GrBezierEffect.h:115: * Coverage is min(0, 1-distance). 3rd &
4th component unused.
maybe we should say somewhere in here that uv is {0,0}, {1/2, 0} and {1, 1} at
the control pts.

[bsalomon, 2013-08-21 20:11:29]

https://codereview.chromium.org/23004010/diff/1/src/gpu/effects/GrBezierEffect.h
File src/gpu/effects/GrBezierEffect.h (right):

https://codereview.chromium.org/23004010/diff/1/src/gpu/effects/GrBezierEffec...
src/gpu/effects/GrBezierEffect.h:65: static GrEffectRef* Create(BezierEffectType
effectType) {
Do we need to check or assert that we have derivative support in these
factories?

[egdaniel, 2013-08-21 20:18:06]

https://codereview.chromium.org/23004010/diff/1/src/gpu/effects/GrBezierEffect.h
File src/gpu/effects/GrBezierEffect.h (right):

https://codereview.chromium.org/23004010/diff/1/src/gpu/effects/GrBezierEffec...
src/gpu/effects/GrBezierEffect.h:13: enum BezierEffectType {
I say either GrBezierEdgeType or Style...wish there was a shorter and more
descriptive way since we end up with things like, kFillAA_GrBezierEdgeType which
seems so wordy 

On 2013/08/21 20:04:39, bsalomon wrote:
> I'm wondering if there is something more specific we could call this.
> BezierEdgeType?
> BezierEdgeFill?
> BezierEdgeStyle?
> 
> 
> Also should be GrFoo {
> kA_GrFoo,
> kB_GrFoo,...
> }
> if it's not scoped by a Gr class or namespace
>

https://codereview.chromium.org/23004010/diff/1/src/gpu/effects/GrBezierEffec...
src/gpu/effects/GrBezierEffect.h:20: * Shader is based off of Loop-Blinn
Quadratic GPU Rendering
Yeah, I just left them separate at first since the top part was more explaining
the implemented function where as the bottom was all about additional non
implemented test we tried.
On 2013/08/21 20:04:39, bsalomon wrote:
> Should we merge this comment block with the next one?

https://codereview.chromium.org/23004010/diff/1/src/gpu/effects/GrBezierEffec...
src/gpu/effects/GrBezierEffect.h:65: static GrEffectRef* Create(BezierEffectType
effectType) {
I suppose we should. Before we relied on the PathRenderer to do that check for
us, but since these are stand alone we should. Also the non-AA doesn't need it

On 2013/08/21 20:11:29, bsalomon wrote:
> Do we need to check or assert that we have derivative support in these
> factories?

https://codereview.chromium.org/23004010/diff/1/src/gpu/effects/GrBezierEffec...
src/gpu/effects/GrBezierEffect.h:66: GR_CREATE_STATIC_EFFECT(gConicFillNoAA,
GrConicEffect, (false, true));
Will do
On 2013/08/21 20:04:39, bsalomon wrote:
> It seems like we should just plumb the enum value all the way down rather than
> converting it to two bools here (and repeating this for the other curve
types).
> 
> could have helpers:
> static inline bool GrBezierEdgeTypeIsFill(BezierEdgeType);
> static inline bool GrBezierEdgeTypeIsAA(BezierEdgeType);
> 
> if it helps.

https://codereview.chromium.org/23004010/diff/1/src/gpu/effects/GrBezierEffec...
src/gpu/effects/GrBezierEffect.h:115: * Coverage is min(0, 1-distance). 3rd &
4th component unused.
Adding it.

On 2013/08/21 20:04:39, bsalomon wrote:
> maybe we should say somewhere in here that uv is {0,0}, {1/2, 0} and {1, 1} at
> the control pts.

[bsalomon, 2013-08-22 13:01:43]

lgtm with a few minor comments. I would use the enum as a member of the class
rather than the two bools, but it's fine either way.

https://codereview.chromium.org/23004010/diff/8001/src/gpu/effects/GrBezierEf...
File src/gpu/effects/GrBezierEffect.h (right):

https://codereview.chromium.org/23004010/diff/8001/src/gpu/effects/GrBezierEf...
src/gpu/effects/GrBezierEffect.h:20: static inline bool
GrBezierEdgeTypeIsFill(const GrBezierEdgeType edgeType) {
Probably doesn't matter, but each of these could be done with one != check.

https://codereview.chromium.org/23004010/diff/8001/src/gpu/effects/GrBezierEf...
src/gpu/effects/GrBezierEffect.h:129: * Uses unsigned distance.
Probably need to strike out this uses unsigned distance business.

https://codereview.chromium.org/23004010/diff/8001/src/gpu/effects/GrBezierEf...
src/gpu/effects/GrBezierEffect.h:176: GrQuadEffect(const GrBezierEdgeType);
const?

https://codereview.chromium.org/23004010/diff/8001/src/gpu/effects/GrBezierEf...
src/gpu/effects/GrBezierEffect.h:242: GrCubicEffect(const GrBezierEdgeType);
const?

[egdaniel, 2013-08-22 14:55:51]

Small change to flip orientation of cubic loops. Also I may need another lgtm
since somehow my cl was closed during first one....

[bsalomon, 2013-08-22 14:57:42]

On 2013/08/22 14:55:51, egdaniel wrote:
> Small change to flip orientation of cubic loops. Also I may need another lgtm
> since somehow my cl was closed during first one....

lgtm

[commit-bot: I haz the power, 2013-08-22 17:18:53]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/egdaniel@google.com/23004010/1005

[commit-bot: I haz the power, 2013-08-22 17:31:12]

Change committed as 10877

[commit-bot: I haz the power, 2013-08-22 20:22:41]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/egdaniel@google.com/23004010/30001

[commit-bot: I haz the power, 2013-08-22 20:41:29]

Change committed as 10884