Hook in rough distance field support for fonts

Hook in rough distance field support for fonts

R=bsalomon@google.com

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

[jvanverth1, 2013-10-24 19:36:02]

This is a first pass at getting distance fields working for fonts, at least on
the GPU side. It loads in the glyphs normally on the raster side, then runs them
through a process that turns them into distance fields. I can't say I'm happy
about the way it's set up, and it also doesn't work in the more unusual cases
(the colortype and texteffects GMs in particular look rather strange), but it's
a start.

The magic number 32 is in a few places -- it uses a 32-point font as the basis
for the distance field and scales up and down from there.

[bungeman-skia, 2013-10-24 19:50:48]

Commenting on the one thing I know about.

https://codereview.chromium.org/41213003/diff/1/src/core/SkDraw.cpp
File src/core/SkDraw.cpp (right):

https://codereview.chromium.org/41213003/diff/1/src/core/SkDraw.cpp#newcode1689
src/core/SkDraw.cpp:1689: paintCopy.setTextSize(32);
32 is quite a magic number. If you want a 'canonical' size, using the typeface's
em size would be good. Note that you'll also want to turn off hinting if you're
looking for 'scalable' outlines.

[jvanverth1, 2013-11-04 20:23:31]

On 2013/10/24 19:50:48, bungeman1 wrote:
> Commenting on the one thing I know about.
> 
> https://codereview.chromium.org/41213003/diff/1/src/core/SkDraw.cpp
> File src/core/SkDraw.cpp (right):
> 
>
https://codereview.chromium.org/41213003/diff/1/src/core/SkDraw.cpp#newcode1689
> src/core/SkDraw.cpp:1689: paintCopy.setTextSize(32);
> 32 is quite a magic number. If you want a 'canonical' size, using the
typeface's
> em size would be good. Note that you'll also want to turn off hinting if
you're
> looking for 'scalable' outlines.

The number 32 was chosen because it was recommended that we use 32x32 glyphs,
and that
seemed the best way to achieve that.

I'll look into disabling hinting, maybe that will solve some of the issues I'm
seeing.

I've since added the Euclidean distance transform code as a separate library
(it's under
the MIT license), and done some additional cleanup.

[bsalomon, 2013-11-04 20:34:52]

https://codereview.chromium.org/41213003/diff/270001/gyp/gpu.gyp
File gyp/gpu.gyp (right):

https://codereview.chromium.org/41213003/diff/270001/gyp/gpu.gyp#newcode86
gyp/gpu.gyp:86: 'edtaa.gyp:*',
Can we make this dependency require a gyp/compile flag? It seems like this is
still in the experimental stage and should be opt-in.

https://codereview.chromium.org/41213003/diff/270001/src/gpu/GrDistanceFieldT...
File src/gpu/GrDistanceFieldTextContext.cpp (right):

https://codereview.chromium.org/41213003/diff/270001/src/gpu/GrDistanceFieldT...
src/gpu/GrDistanceFieldTextContext.cpp:62: transform.reset();
why reset? also do we need to make a copy of the matrix or should we just pass
fContext->getMatrix()? It returns by ref.

https://codereview.chromium.org/41213003/diff/270001/src/gpu/GrDistanceFieldT...
src/gpu/GrDistanceFieldTextContext.cpp:275:
fVertices[2*fCurrVertex+1].setRectFan(SkFixedToScalar(texture->normalizeFixedX(tx)),
In this case I think ToFloat is correct since the data is being written to a
VBO.

[jvanverth1, 2013-11-05 14:59:11]

https://codereview.chromium.org/41213003/diff/270001/gyp/gpu.gyp
File gyp/gpu.gyp (right):

https://codereview.chromium.org/41213003/diff/270001/gyp/gpu.gyp#newcode86
gyp/gpu.gyp:86: 'edtaa.gyp:*',
On 2013/11/04 20:34:53, bsalomon wrote:
> Can we make this dependency require a gyp/compile flag? It seems like this is
> still in the experimental stage and should be opt-in.

I didn't put the dependency behind a flag, but did set it up so the library
isn't linked in. I also hid some of the cpp files in GPU and used the GYP flag
to set a #define.

https://codereview.chromium.org/41213003/diff/270001/src/gpu/GrDistanceFieldT...
File src/gpu/GrDistanceFieldTextContext.cpp (right):

https://codereview.chromium.org/41213003/diff/270001/src/gpu/GrDistanceFieldT...
src/gpu/GrDistanceFieldTextContext.cpp:62: transform.reset();
On 2013/11/04 20:34:53, bsalomon wrote:
> why reset? also do we need to make a copy of the matrix or should we just pass
> fContext->getMatrix()? It returns by ref.

No idea why this was this way. Done.

https://codereview.chromium.org/41213003/diff/270001/src/gpu/GrDistanceFieldT...
src/gpu/GrDistanceFieldTextContext.cpp:275:
fVertices[2*fCurrVertex+1].setRectFan(SkFixedToScalar(texture->normalizeFixedX(tx)),
On 2013/11/04 20:34:53, bsalomon wrote:
> In this case I think ToFloat is correct since the data is being written to a
> VBO.

Done.

[bsalomon, 2013-11-05 15:28:51]

lgtm if reed@ is good with the SkDraw changes. Some minor stuff noted.

https://codereview.chromium.org/41213003/diff/340001/src/gpu/GrTextStrike.cpp
File src/gpu/GrTextStrike.cpp (right):

https://codereview.chromium.org/41213003/diff/340001/src/gpu/GrTextStrike.cpp...
src/gpu/GrTextStrike.cpp:317: memset(dfstorage.get(), 0, dfsize);
not a blocker, but there is an sk_bzero, not sure if it's beneficial. Also I
wonder if we should (or maybe do now?) have a variant of SkAutoSMalloc that
callocs instead of mallocs.

https://codereview.chromium.org/41213003/diff/340001/src/gpu/GrTextStrike.cpp...
src/gpu/GrTextStrike.cpp:333: short* distx = new short[dfwidth*dfheight];
SkNEW(), SkDELETE()

https://codereview.chromium.org/41213003/diff/340001/src/gpu/effects/GrDistan...
File src/gpu/effects/GrDistanceFieldTextureEffect.cpp (right):

https://codereview.chromium.org/41213003/diff/340001/src/gpu/effects/GrDistan...
src/gpu/effects/GrDistanceFieldTextureEffect.cpp:48:
builder->fsCodeAppend("\tfloat afwidth = 0.7071*length(vec2(dFdx(distance),
dFdy(distance)));\n");
Maybe a comment here or somewhere about how this works.

[robertphillips, 2013-11-05 15:30:25]

https://codereview.chromium.org/41213003/diff/340001/gm/scaletext.cpp
File gm/scaletext.cpp (right):

https://codereview.chromium.org/41213003/diff/340001/gm/scaletext.cpp#newcode1
gm/scaletext.cpp:1: /*
Update this?

https://codereview.chromium.org/41213003/diff/340001/gm/scaletext.cpp#newcode11
gm/scaletext.cpp:11: 
// raison d'etre

https://codereview.chromium.org/41213003/diff/340001/gm/scaletext.cpp#newcode50
gm/scaletext.cpp:50: 
Do we need the 'j' loop anymore?

https://codereview.chromium.org/41213003/diff/340001/gm/scaletext.cpp#newcode51
gm/scaletext.cpp:51: for (int j = 0; j < 1; ++j) {
Remove this comment?

https://codereview.chromium.org/41213003/diff/340001/gm/scaletext.cpp#newcode70
gm/scaletext.cpp:70: 
What is index for?

https://codereview.chromium.org/41213003/diff/340001/gm/scaletext.cpp#newcode71
gm/scaletext.cpp:71: int index = 0;
ps seems like a bad name
Should 6 & 50 be constants?

https://codereview.chromium.org/41213003/diff/340001/gm/scaletext.cpp#newcode78
gm/scaletext.cpp:78: }
Are the next two lines required any more?

https://codereview.chromium.org/41213003/diff/340001/gm/scaletext.cpp#newcode89
gm/scaletext.cpp:89: 
DEF_GM?

https://codereview.chromium.org/41213003/diff/340001/include/gpu/GrDistanceFi...
File include/gpu/GrDistanceFieldTextContext.h (right):

https://codereview.chromium.org/41213003/diff/340001/include/gpu/GrDistanceFi...
include/gpu/GrDistanceFieldTextContext.h:15: /*
remove "standard"?

https://codereview.chromium.org/41213003/diff/340001/include/gpu/GrDistanceFi...
include/gpu/GrDistanceFieldTextContext.h:23: virtual void
drawPackedGlyph(GrGlyph::PackedID, GrFixed left, GrFixed top,
indent here?

https://codereview.chromium.org/41213003/diff/340001/include/gpu/GrDistanceFi...
include/gpu/GrDistanceFieldTextContext.h:31: 
If this is required in both GrBitmapTextContext and GrDistanceFieldTextContext
would it make sense to move it into GrTextContext?

https://codereview.chromium.org/41213003/diff/340001/src/core/SkDraw.cpp
File src/core/SkDraw.cpp (right):

https://codereview.chromium.org/41213003/diff/340001/src/core/SkDraw.cpp#newc...
src/core/SkDraw.cpp:1688: paintCopy = paint;
Would it make sense to make 32 a constant and document where the value came
from?

https://codereview.chromium.org/41213003/diff/340001/src/core/SkDraw.cpp#newc...
src/core/SkDraw.cpp:1757: 
fscale is a bit confusing since it seems like a typo of fScale. Would fixedScale
be any better?

https://codereview.chromium.org/41213003/diff/340001/src/core/SkDraw.cpp#newc...
src/core/SkDraw.cpp:1759: if (procFlags & SkDrawProcs::kUseScaledGlyphs_Flag) {
Merge these two into 1 line?

https://codereview.chromium.org/41213003/diff/340001/src/gpu/GrDistanceFieldT...
File src/gpu/GrDistanceFieldTextContext.cpp (right):

https://codereview.chromium.org/41213003/diff/340001/src/gpu/GrDistanceFieldT...
src/gpu/GrDistanceFieldTextContext.cpp:29: SkScalar textSize) :
: GrTextContext(...)
, fTextSize(...) {

https://codereview.chromium.org/41213003/diff/340001/src/gpu/GrTextStrike.cpp
File src/gpu/GrTextStrike.cpp (right):

https://codereview.chromium.org/41213003/diff/340001/src/gpu/GrTextStrike.cpp...
src/gpu/GrTextStrike.cpp:299: // but must shrink back down to get the packed
glyph data
dfWidth/dfHeight?

https://codereview.chromium.org/41213003/diff/340001/src/gpu/GrTextStrike.cpp...
src/gpu/GrTextStrike.cpp:312: // alloc storage for distance field glyph
dfSize?

https://codereview.chromium.org/41213003/diff/340001/src/gpu/GrTextStrike.cpp...
src/gpu/GrTextStrike.cpp:319: unsigned char* ptr = (unsigned char*)
storage.get();
dfPtr?

https://codereview.chromium.org/41213003/diff/340001/src/gpu/GrTextStrike.cpp...
src/gpu/GrTextStrike.cpp:332: // generate distance field data
distX\distY? outerDist?
some form of SkNEW?
some sort of AutoDelete guy?

https://codereview.chromium.org/41213003/diff/340001/src/gpu/GrTextStrike.cpp...
src/gpu/GrTextStrike.cpp:338: dfptr = (unsigned char*) dfstorage.get();
is it possible to namespace these guys to make it clear we're calling into
another library?

https://codereview.chromium.org/41213003/diff/340001/src/gpu/GrTextStrike.cpp...
src/gpu/GrTextStrike.cpp:354: double outerval = outerdist[i];
newlines on {}s?

https://codereview.chromium.org/41213003/diff/340001/src/gpu/GrTextStrike.cpp...
src/gpu/GrTextStrike.cpp:375: plot = fAtlasMgr->addToAtlas(&fAtlas, dfwidth,
dfheight, dfstorage.get(),
indent?

https://codereview.chromium.org/41213003/diff/340001/src/gpu/GrTextStrike.cpp...
src/gpu/GrTextStrike.cpp:389: plot = fAtlasMgr->addToAtlas(&fAtlas,
glyph->width(), glyph->height(), storage.get(),
indent?

https://codereview.chromium.org/41213003/diff/340001/src/gpu/effects/GrDistan...
File src/gpu/effects/GrDistanceFieldTextureEffect.cpp (right):

https://codereview.chromium.org/41213003/diff/340001/src/gpu/effects/GrDistan...
src/gpu/effects/GrDistanceFieldTextureEffect.cpp:15: 
// words of wisdom?

https://codereview.chromium.org/41213003/diff/340001/src/gpu/effects/GrDistan...
File src/gpu/effects/GrDistanceFieldTextureEffect.h (right):

https://codereview.chromium.org/41213003/diff/340001/src/gpu/effects/GrDistan...
src/gpu/effects/GrDistanceFieldTextureEffect.h:16: /**
Update comment?

[jvanverth1, 2013-11-05 17:09:21]

https://codereview.chromium.org/41213003/diff/340001/gm/scaletext.cpp
File gm/scaletext.cpp (right):

https://codereview.chromium.org/41213003/diff/340001/gm/scaletext.cpp#newcode1
gm/scaletext.cpp:1: /*
On 2013/11/05 15:30:26, robertphillips wrote:
> Update this?

Removed -- it shouldn't have been committed.

https://codereview.chromium.org/41213003/diff/340001/include/gpu/GrDistanceFi...
File include/gpu/GrDistanceFieldTextContext.h (right):

https://codereview.chromium.org/41213003/diff/340001/include/gpu/GrDistanceFi...
include/gpu/GrDistanceFieldTextContext.h:15: /*
On 2013/11/05 15:30:26, robertphillips wrote:
> remove "standard"?

Done.

https://codereview.chromium.org/41213003/diff/340001/include/gpu/GrDistanceFi...
include/gpu/GrDistanceFieldTextContext.h:23: virtual void
drawPackedGlyph(GrGlyph::PackedID, GrFixed left, GrFixed top,
On 2013/11/05 15:30:26, robertphillips wrote:
> indent here?

Done.

https://codereview.chromium.org/41213003/diff/340001/include/gpu/GrDistanceFi...
include/gpu/GrDistanceFieldTextContext.h:31: 
On 2013/11/05 15:30:26, robertphillips wrote:
> If this is required in both GrBitmapTextContext and GrDistanceFieldTextContext
> would it make sense to move it into GrTextContext?

Not necessarily -- GrNVPathRenderingTextContext (or whatever it's called)
probably won't use this.

https://codereview.chromium.org/41213003/diff/340001/src/core/SkDraw.cpp
File src/core/SkDraw.cpp (right):

https://codereview.chromium.org/41213003/diff/340001/src/core/SkDraw.cpp#newc...
src/core/SkDraw.cpp:1688: paintCopy = paint;
On 2013/11/05 15:30:26, robertphillips wrote:
> Would it make sense to make 32 a constant and document where the value came
> from?

It would -- still trying to figure out the right place to put this so it can be
accessed by all the places it's used (SkDraw, SkGpuDevice,
GrDistanceFieldTextContext).

https://codereview.chromium.org/41213003/diff/340001/src/core/SkDraw.cpp#newc...
src/core/SkDraw.cpp:1757: 
On 2013/11/05 15:30:26, robertphillips wrote:
> fscale is a bit confusing since it seems like a typo of fScale. Would
fixedScale
> be any better?

Done.

https://codereview.chromium.org/41213003/diff/340001/src/core/SkDraw.cpp#newc...
src/core/SkDraw.cpp:1759: if (procFlags & SkDrawProcs::kUseScaledGlyphs_Flag) {
On 2013/11/05 15:30:26, robertphillips wrote:
> Merge these two into 1 line?

Done.

https://codereview.chromium.org/41213003/diff/340001/src/gpu/GrDistanceFieldT...
File src/gpu/GrDistanceFieldTextContext.cpp (right):

https://codereview.chromium.org/41213003/diff/340001/src/gpu/GrDistanceFieldT...
src/gpu/GrDistanceFieldTextContext.cpp:29: SkScalar textSize) :
On 2013/11/05 15:30:26, robertphillips wrote:
> : GrTextContext(...)
> , fTextSize(...) {

Done.

https://codereview.chromium.org/41213003/diff/340001/src/gpu/GrTextStrike.cpp
File src/gpu/GrTextStrike.cpp (right):

https://codereview.chromium.org/41213003/diff/340001/src/gpu/GrTextStrike.cpp...
src/gpu/GrTextStrike.cpp:299: // but must shrink back down to get the packed
glyph data
On 2013/11/05 15:30:26, robertphillips wrote:
> dfWidth/dfHeight?

Done.

https://codereview.chromium.org/41213003/diff/340001/src/gpu/GrTextStrike.cpp...
src/gpu/GrTextStrike.cpp:312: // alloc storage for distance field glyph
On 2013/11/05 15:30:26, robertphillips wrote:
> dfSize?

Done.

https://codereview.chromium.org/41213003/diff/340001/src/gpu/GrTextStrike.cpp...
src/gpu/GrTextStrike.cpp:317: memset(dfstorage.get(), 0, dfsize);
On 2013/11/05 15:28:52, bsalomon wrote:
> not a blocker, but there is an sk_bzero, not sure if it's beneficial. Also I
> wonder if we should (or maybe do now?) have a variant of SkAutoSMalloc that
> callocs instead of mallocs.

Done.

https://codereview.chromium.org/41213003/diff/340001/src/gpu/GrTextStrike.cpp...
src/gpu/GrTextStrike.cpp:319: unsigned char* ptr = (unsigned char*)
storage.get();
On 2013/11/05 15:30:26, robertphillips wrote:
> dfPtr?

Done.

https://codereview.chromium.org/41213003/diff/340001/src/gpu/GrTextStrike.cpp...
src/gpu/GrTextStrike.cpp:332: // generate distance field data
On 2013/11/05 15:30:26, robertphillips wrote:
> distX\distY? outerDist?
> some form of SkNEW?
> some sort of AutoDelete guy?

Done.

https://codereview.chromium.org/41213003/diff/340001/src/gpu/GrTextStrike.cpp...
src/gpu/GrTextStrike.cpp:333: short* distx = new short[dfwidth*dfheight];
On 2013/11/05 15:28:52, bsalomon wrote:
> SkNEW(), SkDELETE()

Done.

https://codereview.chromium.org/41213003/diff/340001/src/gpu/GrTextStrike.cpp...
src/gpu/GrTextStrike.cpp:354: double outerval = outerdist[i];
On 2013/11/05 15:30:26, robertphillips wrote:
> newlines on {}s?

Done.

https://codereview.chromium.org/41213003/diff/340001/src/gpu/GrTextStrike.cpp...
src/gpu/GrTextStrike.cpp:375: plot = fAtlasMgr->addToAtlas(&fAtlas, dfwidth,
dfheight, dfstorage.get(),
On 2013/11/05 15:30:26, robertphillips wrote:
> indent?

Done.

https://codereview.chromium.org/41213003/diff/340001/src/gpu/GrTextStrike.cpp...
src/gpu/GrTextStrike.cpp:389: plot = fAtlasMgr->addToAtlas(&fAtlas,
glyph->width(), glyph->height(), storage.get(),
On 2013/11/05 15:30:26, robertphillips wrote:
> indent?

Done.

https://codereview.chromium.org/41213003/diff/340001/src/gpu/effects/GrDistan...
File src/gpu/effects/GrDistanceFieldTextureEffect.cpp (right):

https://codereview.chromium.org/41213003/diff/340001/src/gpu/effects/GrDistan...
src/gpu/effects/GrDistanceFieldTextureEffect.cpp:15: 
On 2013/11/05 15:30:26, robertphillips wrote:
> // words of wisdom?

Done.

https://codereview.chromium.org/41213003/diff/340001/src/gpu/effects/GrDistan...
src/gpu/effects/GrDistanceFieldTextureEffect.cpp:48:
builder->fsCodeAppend("\tfloat afwidth = 0.7071*length(vec2(dFdx(distance),
dFdy(distance)));\n");
On 2013/11/05 15:28:52, bsalomon wrote:
> Maybe a comment here or somewhere about how this works.

Done.

https://codereview.chromium.org/41213003/diff/340001/src/gpu/effects/GrDistan...
File src/gpu/effects/GrDistanceFieldTextureEffect.h (right):

https://codereview.chromium.org/41213003/diff/340001/src/gpu/effects/GrDistan...
src/gpu/effects/GrDistanceFieldTextureEffect.h:16: /**
On 2013/11/05 15:30:26, robertphillips wrote:
> Update comment?

Done.

[jvanverth1, 2013-11-11 20:54:29]

Committed patchset #8 manually as r12229 (presubmit successful).