Add GrAASmallPathRenderer.

Add GrAASmallPathRenderer.

Uses cached signed distance fields to render scaled and rotated versions
of small paths.

BUG=skia:2935
Committed: https://skia.googlesource.com/skia/+/fa38a30897ceda3e93355d69b8a6812c823f41f6

[jvanverth1, 2014-09-25 19:11:26]

jvanverth@google.com changed reviewers:
+ egdaniel@google.com, robertphillips@chromium.org

[jvanverth1, 2014-09-25 19:11:26]

I need to do more testing for perf and quality, but overall this looks like it's
ready to go. PTAL.

[egdaniel, 2014-09-25 20:34:27]

On 2014/09/25 19:11:26, jvanverth1 wrote:
> I need to do more testing for perf and quality, but overall this looks like
it's
> ready to go. PTAL.

First quick thought...should this GrAA<distance field>PathRender versus Small
path? Seems like this is all about how to do DF paths where one requirement is
it must be small.

[jvanverth1, 2014-09-26 12:55:04]

On 2014/09/25 20:34:27, egdaniel wrote:
> On 2014/09/25 19:11:26, jvanverth1 wrote:
> > I need to do more testing for perf and quality, but overall this looks like
> it's
> > ready to go. PTAL.
> 
> First quick thought...should this GrAA<distance field>PathRender versus Small
> path? Seems like this is all about how to do DF paths where one requirement is
> it must be small.

Done.

[egdaniel, 2014-09-26 13:30:25]

https://codereview.chromium.org/589103004/diff/200001/src/gpu/GrAADistanceFie...
File src/gpu/GrAADistanceFieldPathRenderer.cpp (right):

https://codereview.chromium.org/589103004/diff/200001/src/gpu/GrAADistanceFie...
src/gpu/GrAADistanceFieldPathRenderer.cpp:59: // currently don't support scaling
more than 4x
But check is on for max scale of 3?

https://codereview.chromium.org/589103004/diff/200001/src/gpu/GrAADistanceFie...
src/gpu/GrAADistanceFieldPathRenderer.cpp:318:
reinterpret_cast<intptr_t>(positions)+vertSize - sizeof(SkPoint));
fix line wrap

https://codereview.chromium.org/589103004/diff/200001/src/gpu/effects/GrDista...
File src/gpu/effects/GrDistanceFieldTextureEffect.cpp (right):

https://codereview.chromium.org/589103004/diff/200001/src/gpu/effects/GrDista...
src/gpu/effects/GrDistanceFieldTextureEffect.cpp:393: const
GrDistanceFieldNoGammaTextureEffect& cte =
other.cast<GrDistanceFieldNoGammaTextureEffect>();
line wrap

[robertphillips, 2014-09-26 14:00:27]

robertphillips@google.com changed reviewers:
+ robertphillips@google.com

[robertphillips, 2014-09-26 14:00:32]

https://codereview.chromium.org/589103004/diff/200001/src/gpu/GrAADistanceFie...
File src/gpu/GrAADistanceFieldPathRenderer.cpp (right):

https://codereview.chromium.org/589103004/diff/200001/src/gpu/GrAADistanceFie...
src/gpu/GrAADistanceFieldPathRenderer.cpp:8: 
Is it kosher to include a gl header in gpu?

https://codereview.chromium.org/589103004/diff/200001/src/gpu/GrAADistanceFie...
src/gpu/GrAADistanceFieldPathRenderer.cpp:40: fPathList.remove(pathData);
SkDELETE

https://codereview.chromium.org/589103004/diff/200001/src/gpu/GrAADistanceFie...
src/gpu/GrAADistanceFieldPathRenderer.cpp:43: 
SkDELETE

https://codereview.chromium.org/589103004/diff/200001/src/gpu/GrAADistanceFie...
src/gpu/GrAADistanceFieldPathRenderer.cpp:48: bool
GrAADistanceFieldPathRenderer::canDrawPath(const SkPath& path,
make these line up?

https://codereview.chromium.org/589103004/diff/200001/src/gpu/GrAADistanceFie...
src/gpu/GrAADistanceFieldPathRenderer.cpp:58: 
3x ?

https://codereview.chromium.org/589103004/diff/200001/src/gpu/GrAADistanceFie...
src/gpu/GrAADistanceFieldPathRenderer.cpp:89: bool
GrAADistanceFieldPathRenderer::onDrawPath(const SkPath& path,
line these up?

https://codereview.chromium.org/589103004/diff/200001/src/gpu/GrAADistanceFie...
src/gpu/GrAADistanceFieldPathRenderer.cpp:104: if (NULL == pathData) {
this-> ?

https://codereview.chromium.org/589103004/diff/200001/src/gpu/GrAADistanceFie...
src/gpu/GrAADistanceFieldPathRenderer.cpp:111: // use signed distance field to
render
this-> ?

https://codereview.chromium.org/589103004/diff/200001/src/gpu/GrAADistanceFie...
src/gpu/GrAADistanceFieldPathRenderer.cpp:114: 
// This is the factor by which we scale the path prior to building the DF ?

https://codereview.chromium.org/589103004/diff/200001/src/gpu/GrAADistanceFie...
src/gpu/GrAADistanceFieldPathRenderer.cpp:150: // pad by 1 to allow room for
antialiasing
Not CeilToInt ?

https://codereview.chromium.org/589103004/diff/200001/src/gpu/GrAADistanceFie...
src/gpu/GrAADistanceFieldPathRenderer.cpp:180: if (NULL == plot) {
this-> ?

https://codereview.chromium.org/589103004/diff/200001/src/gpu/GrAADistanceFie...
src/gpu/GrAADistanceFieldPathRenderer.cpp:198: 
this-> ?

https://codereview.chromium.org/589103004/diff/200001/src/gpu/GrAADistanceFie...
src/gpu/GrAADistanceFieldPathRenderer.cpp:210: HAS_ATLAS:
Guard this ?!

https://codereview.chromium.org/589103004/diff/200001/src/gpu/GrAADistanceFie...
src/gpu/GrAADistanceFieldPathRenderer.cpp:214: // add to cache
SkNEW ?

https://codereview.chromium.org/589103004/diff/200001/src/gpu/GrAADistanceFie...
src/gpu/GrAADistanceFieldPathRenderer.cpp:240: // find an unused plot
I don't think this fAtlas alias buys us much.

https://codereview.chromium.org/589103004/diff/200001/src/gpu/GrAADistanceFie...
src/gpu/GrAADistanceFieldPathRenderer.cpp:256: fPathList.remove(pathData);
SkDELETE ?

https://codereview.chromium.org/589103004/diff/200001/src/gpu/GrAADistanceFie...
src/gpu/GrAADistanceFieldPathRenderer.cpp:267: bool
GrAADistanceFieldPathRenderer::internalDrawPath(const SkPath& path,
line these up ?

https://codereview.chromium.org/589103004/diff/200001/src/gpu/GrAADistanceFie...
src/gpu/GrAADistanceFieldPathRenderer.cpp:282: void* indices = NULL;
Will this fit on one line ?

https://codereview.chromium.org/589103004/diff/200001/src/gpu/GrAADistanceFie...
src/gpu/GrAADistanceFieldPathRenderer.cpp:293: 
invScale ?

https://codereview.chromium.org/589103004/diff/200001/src/gpu/GrAADistanceFie...
src/gpu/GrAADistanceFieldPathRenderer.cpp:304: 
SkRect::MakeWH(dx, dy, width, height); ?

https://codereview.chromium.org/589103004/diff/200001/src/gpu/GrAADistanceFie...
src/gpu/GrAADistanceFieldPathRenderer.cpp:317: SkPoint* textureCoords =
reinterpret_cast<SkPoint*>(
overlength.

https://codereview.chromium.org/589103004/diff/200001/src/gpu/GrAADistanceFie...
src/gpu/GrAADistanceFieldPathRenderer.cpp:342: 
one line ?

https://codereview.chromium.org/589103004/diff/200001/src/gpu/GrSWMaskHelper.h
File src/gpu/GrSWMaskHelper.h (right):

https://codereview.chromium.org/589103004/diff/200001/src/gpu/GrSWMaskHelper....
src/gpu/GrSWMaskHelper.h:70: 
Spell out SignedDistanceField in comment ?

https://codereview.chromium.org/589103004/diff/200001/src/gpu/GrSWMaskHelper....
src/gpu/GrSWMaskHelper.h:71: // Convert mask generation results to an SDF
Do we have something less opaque then a void* ?

https://codereview.chromium.org/589103004/diff/200001/src/gpu/effects/GrDista...
File src/gpu/effects/GrDistanceFieldTextureEffect.cpp (right):

https://codereview.chromium.org/589103004/diff/200001/src/gpu/effects/GrDista...
src/gpu/effects/GrDistanceFieldTextureEffect.cpp:266:
GrGLDistanceFieldNoGammaTextureEffect(const GrBackendProcessorFactory& factory,
line this up ?

https://codereview.chromium.org/589103004/diff/200001/src/gpu/effects/GrDista...
src/gpu/effects/GrDistanceFieldTextureEffect.cpp:269: ,
fTextureSize(SkISize::Make(-1, -1))
Have you switched over to Mike R.'s bracket style ?

https://codereview.chromium.org/589103004/diff/200001/src/gpu/effects/GrDista...
src/gpu/effects/GrDistanceFieldTextureEffect.cpp:272: virtual void
emitCode(GrGLFullProgramBuilder* builder,
line up ?

https://codereview.chromium.org/589103004/diff/200001/src/gpu/effects/GrDista...
src/gpu/effects/GrDistanceFieldTextureEffect.cpp:279: const
GrDistanceFieldNoGammaTextureEffect& dfTexEffect =
tab over ?

https://codereview.chromium.org/589103004/diff/200001/src/gpu/effects/GrDista...
src/gpu/effects/GrDistanceFieldTextureEffect.cpp:293: GrGLVertexShaderBuilder*
vsBuilder = builder->getVertexShaderBuilder();
rm '\t's and '\n's ?

https://codereview.chromium.org/589103004/diff/200001/src/gpu/effects/GrDista...
src/gpu/effects/GrDistanceFieldTextureEffect.cpp:297: fTextureSizeUni =
builder->addUniform(GrGLProgramBuilder::kFragment_Visibility,
line up ?

https://codereview.chromium.org/589103004/diff/200001/src/gpu/effects/GrDista...
src/gpu/effects/GrDistanceFieldTextureEffect.cpp:349: virtual void setData(const
GrGLProgramDataManager& pdman,
line up ?

https://codereview.chromium.org/589103004/diff/200001/src/gpu/effects/GrDista...
src/gpu/effects/GrDistanceFieldTextureEffect.cpp:353: GrTexture* texture =
effect.texture(0);
one line ?

https://codereview.chromium.org/589103004/diff/200001/src/gpu/effects/GrDista...
src/gpu/effects/GrDistanceFieldTextureEffect.cpp:357:
pdman.set2f(fTextureSizeUni,
line up ?

https://codereview.chromium.org/589103004/diff/200001/src/gpu/effects/GrDista...
src/gpu/effects/GrDistanceFieldTextureEffect.cpp:363: static inline void
GenKey(const GrProcessor& effect, const GrGLCaps&,
line up ?

https://codereview.chromium.org/589103004/diff/200001/src/gpu/effects/GrDista...
src/gpu/effects/GrDistanceFieldTextureEffect.cpp:385: ,
fInTextureCoords(this->addVertexAttrib(GrShaderVar("inTextureCoords",
this indenting (or lack thereof) is confusing.

https://codereview.chromium.org/589103004/diff/200001/src/gpu/effects/GrDista...
src/gpu/effects/GrDistanceFieldTextureEffect.cpp:394: return fTextureAccess ==
cte.fTextureAccess &&
one line ?

https://codereview.chromium.org/589103004/diff/200001/src/gpu/effects/GrDista...
src/gpu/effects/GrDistanceFieldTextureEffect.cpp:398: void
GrDistanceFieldNoGammaTextureEffect::getConstantColorComponents(GrColor* color,
line up?

https://codereview.chromium.org/589103004/diff/200001/src/gpu/effects/GrDista...
src/gpu/effects/GrDistanceFieldTextureEffect.cpp:403: }
put else on prior line ?

https://codereview.chromium.org/589103004/diff/200001/src/gpu/effects/GrDista...
src/gpu/effects/GrDistanceFieldTextureEffect.cpp:417: GrGeometryProcessor*
GrDistanceFieldNoGammaTextureEffect::TestCreate(SkRandom* random,
line up ?

https://codereview.chromium.org/589103004/diff/200001/src/gpu/effects/GrDista...
src/gpu/effects/GrDistanceFieldTextureEffect.cpp:420: GrTexture* textures[]) {
put ':' on next line and line up with '?' ?

https://codereview.chromium.org/589103004/diff/200001/src/gpu/effects/GrDista...
src/gpu/effects/GrDistanceFieldTextureEffect.cpp:432: GrTextureParams
params(tileModes, random->nextBool() ? GrTextureParams::kBilerp_FilterMode :
same here with ':' and '?' ?

https://codereview.chromium.org/589103004/diff/200001/src/gpu/effects/GrDista...
File src/gpu/effects/GrDistanceFieldTextureEffect.h (right):

https://codereview.chromium.org/589103004/diff/200001/src/gpu/effects/GrDista...
src/gpu/effects/GrDistanceFieldTextureEffect.h:105: static GrGeometryProcessor*
Create(GrTexture* tex, const GrTextureParams& params,
line this up ?

https://codereview.chromium.org/589103004/diff/200001/src/gpu/effects/GrDista...
src/gpu/effects/GrDistanceFieldTextureEffect.h:118: 
Where is this used ?

https://codereview.chromium.org/589103004/diff/200001/src/gpu/effects/GrDista...
src/gpu/effects/GrDistanceFieldTextureEffect.h:124:
GrDistanceFieldNoGammaTextureEffect(GrTexture* texture, const GrTextureParams&
params,
line this up ?

https://codereview.chromium.org/589103004/diff/200001/src/gpu/effects/GrDista...
src/gpu/effects/GrDistanceFieldTextureEffect.h:134: 
GrGeometryProcessor ?

[bsalomon, 2014-09-26 17:32:50]

bsalomon@google.com changed reviewers:
+ bsalomon@google.com

[bsalomon, 2014-09-26 17:32:51]

https://codereview.chromium.org/589103004/diff/200001/src/gpu/GrAADistanceFie...
File src/gpu/GrAADistanceFieldPathRenderer.cpp (right):

https://codereview.chromium.org/589103004/diff/200001/src/gpu/GrAADistanceFie...
src/gpu/GrAADistanceFieldPathRenderer.cpp:8: 
On 2014/09/26 14:00:30, robertphillips wrote:
> Is it kosher to include a gl header in gpu?

We do this for processors all over the place, otherwise GrGLFragmentProcessor
subclasses would have to be apart from the rendering code that uses them. (Which
isn't terrible but not current practice.)

https://codereview.chromium.org/589103004/diff/200001/src/gpu/GrAADistanceFie...
src/gpu/GrAADistanceFieldPathRenderer.cpp:21: #define GR_ATLAS_TEXTURE_WIDTH
1024
Let's drop the GR_ prefix on these macros. They don't escape this file and
aren't GR-wide but rather are specific to the impl here.

https://codereview.chromium.org/589103004/diff/200001/src/gpu/GrAADistanceFie...
src/gpu/GrAADistanceFieldPathRenderer.cpp:52: // TODO: Support inverse fill
You might want to check out the GrGLPath stuff... there is some code there for
building a key out of these style factors, maybe could be shared.

https://codereview.chromium.org/589103004/diff/200001/src/gpu/GrAADistanceFie...
src/gpu/GrAADistanceFieldPathRenderer.cpp:72: //*** not sure if this is needed
I think it's not, but we should have a GM that clips against a path that can be
rendered using this PR to make sure everything works correctly.

https://codereview.chromium.org/589103004/diff/200001/src/gpu/GrAADistanceFie...
src/gpu/GrAADistanceFieldPathRenderer.cpp:93: if (path.isEmpty()) {
maybe a comment that we already bailed on inverse filled paths (so this is
safe)?

https://codereview.chromium.org/589103004/diff/200001/src/gpu/GrAADistanceFie...
src/gpu/GrAADistanceFieldPathRenderer.cpp:97: if (NULL == fContext) {
Do we need this?

[jvanverth1, 2014-10-03 17:28:24]

I think this is good to go. I've addressed the xfermode issues, and set a
#define to block this in Chrome and Android for the moment. Plus comments:

https://codereview.chromium.org/589103004/diff/200001/src/gpu/GrAADistanceFie...
File src/gpu/GrAADistanceFieldPathRenderer.cpp (right):

https://codereview.chromium.org/589103004/diff/200001/src/gpu/GrAADistanceFie...
src/gpu/GrAADistanceFieldPathRenderer.cpp:8: 
On 2014/09/26 17:32:51, bsalomon wrote:
> On 2014/09/26 14:00:30, robertphillips wrote:
> > Is it kosher to include a gl header in gpu?
> 
> We do this for processors all over the place, otherwise GrGLFragmentProcessor
> subclasses would have to be apart from the rendering code that uses them.
(Which
> isn't terrible but not current practice.)

Removed anyway, it's not needed.

https://codereview.chromium.org/589103004/diff/200001/src/gpu/GrAADistanceFie...
src/gpu/GrAADistanceFieldPathRenderer.cpp:21: #define GR_ATLAS_TEXTURE_WIDTH
1024
On 2014/09/26 17:32:51, bsalomon wrote:
> Let's drop the GR_ prefix on these macros. They don't escape this file and
> aren't GR-wide but rather are specific to the impl here.

Done.

https://codereview.chromium.org/589103004/diff/200001/src/gpu/GrAADistanceFie...
src/gpu/GrAADistanceFieldPathRenderer.cpp:40: fPathList.remove(pathData);
On 2014/09/26 14:00:29, robertphillips wrote:
> SkDELETE

Done.

https://codereview.chromium.org/589103004/diff/200001/src/gpu/GrAADistanceFie...
src/gpu/GrAADistanceFieldPathRenderer.cpp:43: 
On 2014/09/26 14:00:30, robertphillips wrote:
> SkDELETE

Done.

https://codereview.chromium.org/589103004/diff/200001/src/gpu/GrAADistanceFie...
src/gpu/GrAADistanceFieldPathRenderer.cpp:48: bool
GrAADistanceFieldPathRenderer::canDrawPath(const SkPath& path,
On 2014/09/26 14:00:29, robertphillips wrote:
> make these line up?

Done.

https://codereview.chromium.org/589103004/diff/200001/src/gpu/GrAADistanceFie...
src/gpu/GrAADistanceFieldPathRenderer.cpp:52: // TODO: Support inverse fill
On 2014/09/26 17:32:51, bsalomon wrote:
> You might want to check out the GrGLPath stuff... there is some code there for
> building a key out of these style factors, maybe could be shared.

Acknowledged.

https://codereview.chromium.org/589103004/diff/200001/src/gpu/GrAADistanceFie...
src/gpu/GrAADistanceFieldPathRenderer.cpp:58: 
On 2014/09/26 14:00:30, robertphillips wrote:
> 3x ?

Done.

https://codereview.chromium.org/589103004/diff/200001/src/gpu/GrAADistanceFie...
src/gpu/GrAADistanceFieldPathRenderer.cpp:59: // currently don't support scaling
more than 4x
On 2014/09/26 13:30:25, egdaniel wrote:
> But check is on for max scale of 3?

Done.

https://codereview.chromium.org/589103004/diff/200001/src/gpu/GrAADistanceFie...
src/gpu/GrAADistanceFieldPathRenderer.cpp:72: //*** not sure if this is needed
On 2014/09/26 17:32:51, bsalomon wrote:
> I think it's not, but we should have a GM that clips against a path that can
be
> rendered using this PR to make sure everything works correctly.

I think it is needed -- GrSoftwarePathRenderer has this set.

https://codereview.chromium.org/589103004/diff/200001/src/gpu/GrAADistanceFie...
src/gpu/GrAADistanceFieldPathRenderer.cpp:89: bool
GrAADistanceFieldPathRenderer::onDrawPath(const SkPath& path,
On 2014/09/26 14:00:29, robertphillips wrote:
> line these up?

Done.

https://codereview.chromium.org/589103004/diff/200001/src/gpu/GrAADistanceFie...
src/gpu/GrAADistanceFieldPathRenderer.cpp:93: if (path.isEmpty()) {
On 2014/09/26 17:32:51, bsalomon wrote:
> maybe a comment that we already bailed on inverse filled paths (so this is
> safe)?

Done.

https://codereview.chromium.org/589103004/diff/200001/src/gpu/GrAADistanceFie...
src/gpu/GrAADistanceFieldPathRenderer.cpp:97: if (NULL == fContext) {
On 2014/09/26 17:32:51, bsalomon wrote:
> Do we need this?

Done.

https://codereview.chromium.org/589103004/diff/200001/src/gpu/GrAADistanceFie...
src/gpu/GrAADistanceFieldPathRenderer.cpp:104: if (NULL == pathData) {
On 2014/09/26 14:00:29, robertphillips wrote:
> this-> ?

Done.

https://codereview.chromium.org/589103004/diff/200001/src/gpu/GrAADistanceFie...
src/gpu/GrAADistanceFieldPathRenderer.cpp:111: // use signed distance field to
render
On 2014/09/26 14:00:29, robertphillips wrote:
> this-> ?

Done.

https://codereview.chromium.org/589103004/diff/200001/src/gpu/GrAADistanceFie...
src/gpu/GrAADistanceFieldPathRenderer.cpp:114: 
On 2014/09/26 14:00:30, robertphillips wrote:
> // This is the factor by which we scale the path prior to building the DF ?

Done.

https://codereview.chromium.org/589103004/diff/200001/src/gpu/GrAADistanceFie...
src/gpu/GrAADistanceFieldPathRenderer.cpp:150: // pad by 1 to allow room for
antialiasing
On 2014/09/26 14:00:30, robertphillips wrote:
> Not CeilToInt ?

Done.

https://codereview.chromium.org/589103004/diff/200001/src/gpu/GrAADistanceFie...
src/gpu/GrAADistanceFieldPathRenderer.cpp:180: if (NULL == plot) {
On 2014/09/26 14:00:29, robertphillips wrote:
> this-> ?

Done.

https://codereview.chromium.org/589103004/diff/200001/src/gpu/GrAADistanceFie...
src/gpu/GrAADistanceFieldPathRenderer.cpp:198: 
On 2014/09/26 14:00:29, robertphillips wrote:
> this-> ?

Done.

https://codereview.chromium.org/589103004/diff/200001/src/gpu/GrAADistanceFie...
src/gpu/GrAADistanceFieldPathRenderer.cpp:210: HAS_ATLAS:
On 2014/09/26 14:00:29, robertphillips wrote:
> Guard this ?!

Whoops, removed.

https://codereview.chromium.org/589103004/diff/200001/src/gpu/GrAADistanceFie...
src/gpu/GrAADistanceFieldPathRenderer.cpp:214: // add to cache
On 2014/09/26 14:00:29, robertphillips wrote:
> SkNEW ?

Done.

https://codereview.chromium.org/589103004/diff/200001/src/gpu/GrAADistanceFie...
src/gpu/GrAADistanceFieldPathRenderer.cpp:240: // find an unused plot
On 2014/09/26 14:00:30, robertphillips wrote:
> I don't think this fAtlas alias buys us much.

Done.

https://codereview.chromium.org/589103004/diff/200001/src/gpu/GrAADistanceFie...
src/gpu/GrAADistanceFieldPathRenderer.cpp:256: fPathList.remove(pathData);
On 2014/09/26 14:00:29, robertphillips wrote:
> SkDELETE ?

Done.

https://codereview.chromium.org/589103004/diff/200001/src/gpu/GrAADistanceFie...
src/gpu/GrAADistanceFieldPathRenderer.cpp:267: bool
GrAADistanceFieldPathRenderer::internalDrawPath(const SkPath& path,
On 2014/09/26 14:00:29, robertphillips wrote:
> line these up ?

Done.

https://codereview.chromium.org/589103004/diff/200001/src/gpu/GrAADistanceFie...
src/gpu/GrAADistanceFieldPathRenderer.cpp:282: void* indices = NULL;
On 2014/09/26 14:00:28, robertphillips wrote:
> Will this fit on one line ?

Done.

https://codereview.chromium.org/589103004/diff/200001/src/gpu/GrAADistanceFie...
src/gpu/GrAADistanceFieldPathRenderer.cpp:293: 
On 2014/09/26 14:00:29, robertphillips wrote:
> invScale ?

Done.

https://codereview.chromium.org/589103004/diff/200001/src/gpu/GrAADistanceFie...
src/gpu/GrAADistanceFieldPathRenderer.cpp:304: 
On 2014/09/26 14:00:29, robertphillips wrote:
> SkRect::MakeWH(dx, dy, width, height); ?

Done.

https://codereview.chromium.org/589103004/diff/200001/src/gpu/GrAADistanceFie...
src/gpu/GrAADistanceFieldPathRenderer.cpp:317: SkPoint* textureCoords =
reinterpret_cast<SkPoint*>(
On 2014/09/26 14:00:29, robertphillips wrote:
> overlength.

Done.

https://codereview.chromium.org/589103004/diff/200001/src/gpu/GrAADistanceFie...
src/gpu/GrAADistanceFieldPathRenderer.cpp:318:
reinterpret_cast<intptr_t>(positions)+vertSize - sizeof(SkPoint));
On 2014/09/26 13:30:25, egdaniel wrote:
> fix line wrap

Done.

https://codereview.chromium.org/589103004/diff/200001/src/gpu/GrAADistanceFie...
src/gpu/GrAADistanceFieldPathRenderer.cpp:342: 
On 2014/09/26 14:00:29, robertphillips wrote:
> one line ?

Done.

https://codereview.chromium.org/589103004/diff/200001/src/gpu/GrSWMaskHelper.h
File src/gpu/GrSWMaskHelper.h (right):

https://codereview.chromium.org/589103004/diff/200001/src/gpu/GrSWMaskHelper....
src/gpu/GrSWMaskHelper.h:70: 
On 2014/09/26 14:00:30, robertphillips wrote:
> Spell out SignedDistanceField in comment ?

Done.

https://codereview.chromium.org/589103004/diff/200001/src/gpu/GrSWMaskHelper....
src/gpu/GrSWMaskHelper.h:71: // Convert mask generation results to an SDF
On 2014/09/26 14:00:30, robertphillips wrote:
> Do we have something less opaque then a void* ?

Done.

https://codereview.chromium.org/589103004/diff/200001/src/gpu/effects/GrDista...
File src/gpu/effects/GrDistanceFieldTextureEffect.cpp (right):

https://codereview.chromium.org/589103004/diff/200001/src/gpu/effects/GrDista...
src/gpu/effects/GrDistanceFieldTextureEffect.cpp:266:
GrGLDistanceFieldNoGammaTextureEffect(const GrBackendProcessorFactory& factory,
On 2014/09/26 14:00:31, robertphillips wrote:
> line this up ?

Done.

https://codereview.chromium.org/589103004/diff/200001/src/gpu/effects/GrDista...
src/gpu/effects/GrDistanceFieldTextureEffect.cpp:269: ,
fTextureSize(SkISize::Make(-1, -1))
On 2014/09/26 14:00:30, robertphillips wrote:
> Have you switched over to Mike R.'s bracket style ?

Done.

https://codereview.chromium.org/589103004/diff/200001/src/gpu/effects/GrDista...
src/gpu/effects/GrDistanceFieldTextureEffect.cpp:272: virtual void
emitCode(GrGLFullProgramBuilder* builder,
On 2014/09/26 14:00:31, robertphillips wrote:
> line up ?

Done.

https://codereview.chromium.org/589103004/diff/200001/src/gpu/effects/GrDista...
src/gpu/effects/GrDistanceFieldTextureEffect.cpp:279: const
GrDistanceFieldNoGammaTextureEffect& dfTexEffect =
On 2014/09/26 14:00:31, robertphillips wrote:
> tab over ?

Done.

https://codereview.chromium.org/589103004/diff/200001/src/gpu/effects/GrDista...
src/gpu/effects/GrDistanceFieldTextureEffect.cpp:293: GrGLVertexShaderBuilder*
vsBuilder = builder->getVertexShaderBuilder();
On 2014/09/26 14:00:30, robertphillips wrote:
> rm '\t's and '\n's ?

Done.

https://codereview.chromium.org/589103004/diff/200001/src/gpu/effects/GrDista...
src/gpu/effects/GrDistanceFieldTextureEffect.cpp:297: fTextureSizeUni =
builder->addUniform(GrGLProgramBuilder::kFragment_Visibility,
On 2014/09/26 14:00:30, robertphillips wrote:
> line up ?

Done.

https://codereview.chromium.org/589103004/diff/200001/src/gpu/effects/GrDista...
src/gpu/effects/GrDistanceFieldTextureEffect.cpp:349: virtual void setData(const
GrGLProgramDataManager& pdman,
On 2014/09/26 14:00:31, robertphillips wrote:
> line up ?

Done.

https://codereview.chromium.org/589103004/diff/200001/src/gpu/effects/GrDista...
src/gpu/effects/GrDistanceFieldTextureEffect.cpp:353: GrTexture* texture =
effect.texture(0);
On 2014/09/26 14:00:31, robertphillips wrote:
> one line ?
Too wide...

https://codereview.chromium.org/589103004/diff/200001/src/gpu/effects/GrDista...
src/gpu/effects/GrDistanceFieldTextureEffect.cpp:357:
pdman.set2f(fTextureSizeUni,
On 2014/09/26 14:00:31, robertphillips wrote:
> line up ?

Done.

https://codereview.chromium.org/589103004/diff/200001/src/gpu/effects/GrDista...
src/gpu/effects/GrDistanceFieldTextureEffect.cpp:363: static inline void
GenKey(const GrProcessor& effect, const GrGLCaps&,
On 2014/09/26 14:00:31, robertphillips wrote:
> line up ?

Done.

https://codereview.chromium.org/589103004/diff/200001/src/gpu/effects/GrDista...
src/gpu/effects/GrDistanceFieldTextureEffect.cpp:385: ,
fInTextureCoords(this->addVertexAttrib(GrShaderVar("inTextureCoords",
On 2014/09/26 14:00:31, robertphillips wrote:
> this indenting (or lack thereof) is confusing.

Done.

https://codereview.chromium.org/589103004/diff/200001/src/gpu/effects/GrDista...
src/gpu/effects/GrDistanceFieldTextureEffect.cpp:393: const
GrDistanceFieldNoGammaTextureEffect& cte =
other.cast<GrDistanceFieldNoGammaTextureEffect>();
On 2014/09/26 13:30:25, egdaniel wrote:
> line wrap

Done.

https://codereview.chromium.org/589103004/diff/200001/src/gpu/effects/GrDista...
src/gpu/effects/GrDistanceFieldTextureEffect.cpp:394: return fTextureAccess ==
cte.fTextureAccess &&
On 2014/09/26 14:00:31, robertphillips wrote:
> one line ?

Done.

https://codereview.chromium.org/589103004/diff/200001/src/gpu/effects/GrDista...
src/gpu/effects/GrDistanceFieldTextureEffect.cpp:398: void
GrDistanceFieldNoGammaTextureEffect::getConstantColorComponents(GrColor* color,
On 2014/09/26 14:00:31, robertphillips wrote:
> line up?

Done.

https://codereview.chromium.org/589103004/diff/200001/src/gpu/effects/GrDista...
src/gpu/effects/GrDistanceFieldTextureEffect.cpp:403: }
On 2014/09/26 14:00:31, robertphillips wrote:
> put else on prior line ?

Done.

https://codereview.chromium.org/589103004/diff/200001/src/gpu/effects/GrDista...
src/gpu/effects/GrDistanceFieldTextureEffect.cpp:417: GrGeometryProcessor*
GrDistanceFieldNoGammaTextureEffect::TestCreate(SkRandom* random,
On 2014/09/26 14:00:31, robertphillips wrote:
> line up ?

Done.

https://codereview.chromium.org/589103004/diff/200001/src/gpu/effects/GrDista...
src/gpu/effects/GrDistanceFieldTextureEffect.cpp:420: GrTexture* textures[]) {
On 2014/09/26 14:00:30, robertphillips wrote:
> put ':' on next line and line up with '?' ?

Done.

https://codereview.chromium.org/589103004/diff/200001/src/gpu/effects/GrDista...
src/gpu/effects/GrDistanceFieldTextureEffect.cpp:432: GrTextureParams
params(tileModes, random->nextBool() ? GrTextureParams::kBilerp_FilterMode :
On 2014/09/26 14:00:30, robertphillips wrote:
> same here with ':' and '?' ?

Done.

https://codereview.chromium.org/589103004/diff/200001/src/gpu/effects/GrDista...
File src/gpu/effects/GrDistanceFieldTextureEffect.h (right):

https://codereview.chromium.org/589103004/diff/200001/src/gpu/effects/GrDista...
src/gpu/effects/GrDistanceFieldTextureEffect.h:105: static GrGeometryProcessor*
Create(GrTexture* tex, const GrTextureParams& params,
On 2014/09/26 14:00:31, robertphillips wrote:
> line this up ?

Done.

https://codereview.chromium.org/589103004/diff/200001/src/gpu/effects/GrDista...
src/gpu/effects/GrDistanceFieldTextureEffect.h:118: 
On 2014/09/26 14:00:31, robertphillips wrote:
> Where is this used ?

In GrTBackendProcessorFactory.

https://codereview.chromium.org/589103004/diff/200001/src/gpu/effects/GrDista...
src/gpu/effects/GrDistanceFieldTextureEffect.h:124:
GrDistanceFieldNoGammaTextureEffect(GrTexture* texture, const GrTextureParams&
params,
On 2014/09/26 14:00:31, robertphillips wrote:
> line this up ?

Done.

https://codereview.chromium.org/589103004/diff/200001/src/gpu/effects/GrDista...
src/gpu/effects/GrDistanceFieldTextureEffect.h:134: 
On 2014/09/26 14:00:31, robertphillips wrote:
> GrGeometryProcessor ?

Done.

[bsalomon, 2014-10-03 17:46:27]

Are you worried about this slowing down dynamic paths? Should we have a volatile
bit on SkPath?

https://codereview.chromium.org/589103004/diff/220001/gyp/skia_for_android_fr...
File gyp/skia_for_android_framework_defines.gypi (right):

https://codereview.chromium.org/589103004/diff/220001/gyp/skia_for_android_fr...
gyp/skia_for_android_framework_defines.gypi:25:
'SK_LEGACY_NO_DISTANCE_FIELD_PATHS'
nit: alignment

https://codereview.chromium.org/589103004/diff/220001/gyp/skia_for_chromium_d...
File gyp/skia_for_chromium_defines.gypi (right):

https://codereview.chromium.org/589103004/diff/220001/gyp/skia_for_chromium_d...
gyp/skia_for_chromium_defines.gypi:17: 'SK_LEGACY_NO_DISTANCE_FIELD_PATHS'
alignment

[robertphillips, 2014-10-03 19:23:41]

lgtm + nits

https://codereview.chromium.org/589103004/diff/220001/gyp/skia_for_android_fr...
File gyp/skia_for_android_framework_defines.gypi (right):

https://codereview.chromium.org/589103004/diff/220001/gyp/skia_for_android_fr...
gyp/skia_for_android_framework_defines.gypi:24: 'SK_LEGACY_PICTURE_DRAW_API',
extra tab ?

https://codereview.chromium.org/589103004/diff/220001/gyp/skia_for_chromium_d...
File gyp/skia_for_chromium_defines.gypi (right):

https://codereview.chromium.org/589103004/diff/220001/gyp/skia_for_chromium_d...
gyp/skia_for_chromium_defines.gypi:16: 'SK_SUPPORT_LEGACY_TEXTRENDERMODE',
xtra tab ?

https://codereview.chromium.org/589103004/diff/220001/src/gpu/GrAADistanceFie...
File src/gpu/GrAADistanceFieldPathRenderer.cpp (right):

https://codereview.chromium.org/589103004/diff/220001/src/gpu/GrAADistanceFie...
src/gpu/GrAADistanceFieldPathRenderer.cpp:125: if (NULL == fAtlas) {
one line ?

https://codereview.chromium.org/589103004/diff/220001/src/gpu/effects/GrDista...
File src/gpu/effects/GrDistanceFieldTextureEffect.cpp (right):

https://codereview.chromium.org/589103004/diff/220001/src/gpu/effects/GrDista...
src/gpu/effects/GrDistanceFieldTextureEffect.cpp:317: }
move to prior line ?

https://codereview.chromium.org/589103004/diff/220001/src/gpu/effects/GrDista...
src/gpu/effects/GrDistanceFieldTextureEffect.cpp:331: }
move to prior line ?

https://codereview.chromium.org/589103004/diff/220001/src/gpu/effects/GrDista...
src/gpu/effects/GrDistanceFieldTextureEffect.cpp:347: virtual void setData(const
GrGLProgramDataManager& pdman,
add ' ' before '{' ?

https://codereview.chromium.org/589103004/diff/220001/src/gpu/effects/GrDista...
src/gpu/effects/GrDistanceFieldTextureEffect.cpp:432: return
GrDistanceFieldNoGammaTextureEffect::Create(textures[texIdx], params,
fix indent here ?

https://codereview.chromium.org/589103004/diff/220001/src/gpu/effects/GrDista...
File src/gpu/effects/GrDistanceFieldTextureEffect.h (right):

https://codereview.chromium.org/589103004/diff/220001/src/gpu/effects/GrDista...
src/gpu/effects/GrDistanceFieldTextureEffect.h:106: uint32_t flags) {
extra ' ' here ?

[jvanverth1, 2014-10-03 20:43:30]

On 2014/10/03 17:46:27, bsalomon wrote:
> Are you worried about this slowing down dynamic paths? Should we have a
volatile
> bit on SkPath?

I agree we should -- can add that when it's clear when this will drop into
Chrome.

[bsalomon, 2014-10-03 21:32:46]

lgtm

[jvanverth1, 2014-10-06 12:51:37]

The CQ bit was checked by jvanverth@google.com

[commit-bot: I haz the power, 2014-10-06 12:52:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/589103004/260001

[commit-bot: I haz the power, 2014-10-06 12:59:31]

Committed patchset #14 (id:260001) as fa38a30897ceda3e93355d69b8a6812c823f41f6