Move distance field font code into SkGpuDevice

Move distance field font code into GrDistanceFieldTextContext.

This avoids the SkDraw path and renders the distance field glyphs directly from
GrDistanceFieldTextContext. It also disables LCD, subpixel and autohinting, and
removes the supporting code when rendering DF fonts.

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

[jvanverth1, 2013-11-25 20:25:17]

One note on this: the spacing between characters doesn't seem quite right after
these changes. I'm not sure if that is due to the removal of the autokerning, or
the removal of subpixel support.

[robertphillips, 2013-11-25 20:39:43]

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

https://codereview.chromium.org/85653004/diff/1/src/core/SkDraw.cpp#newcode1446
src/core/SkDraw.cpp:1446: // we don't cache hairlines in the cache
line up? or 1 line?
Shouldn't this comment be "// drawing hairlines is fast so it doesn't make any
sense to cache them"?

https://codereview.chromium.org/85653004/diff/1/src/core/SkDraw.cpp#newcode1711
src/core/SkDraw.cpp:1711: // will fill if its frame-width is 0.
this->

https://codereview.chromium.org/85653004/diff/1/src/core/SkDraw.cpp#newcode1941
src/core/SkDraw.cpp:1941: 
this->

https://codereview.chromium.org/85653004/diff/1/src/gpu/SkGpuDevice.cpp
File src/gpu/SkGpuDevice.cpp (right):

https://codereview.chromium.org/85653004/diff/1/src/gpu/SkGpuDevice.cpp#newco...
src/gpu/SkGpuDevice.cpp:1883: } else {
Is all this duplicate code just for the shift?

[bsalomon, 2013-11-25 20:50:22]

Any thoughts about how this aligns or doesn't with other device independent text
rendering approaches?

https://codereview.chromium.org/85653004/diff/1/src/gpu/SkGpuDevice.cpp
File src/gpu/SkGpuDevice.cpp (right):

https://codereview.chromium.org/85653004/diff/1/src/gpu/SkGpuDevice.cpp#newco...
src/gpu/SkGpuDevice.cpp:1815: drawPosDFText((const char *)text, byteLength, pos,
constY, scalarsPerPos, paint);
this->

[jvanverth1, 2013-11-25 21:01:11]

On 2013/11/25 20:50:22, bsalomon wrote:
> Any thoughts about how this aligns or doesn't with other device independent
text
> rendering approaches?

My initial thought was that we could add another check to
SkGpuDevice::drawText(). However, if NV path rendering is enabled, we shouldn't
need to check that every time. So perhaps the right solution is to replace
drawText() with a function pointer, and set it to the correct one depending on
what's available. Alternatively we could do it the C++ way and subclass
SkGpuDevice and replace those functions.

[reed1, 2013-11-25 21:24:33]

https://codereview.chromium.org/85653004/diff/1/include/core/SkDraw.h
File include/core/SkDraw.h (right):

https://codereview.chromium.org/85653004/diff/1/include/core/SkDraw.h#newcode110
include/core/SkDraw.h:110: bool    shouldDrawTextAsPaths(const SkPaint&, const
SkMatrix&) const;
why did this go from static to member?

[jvanverth1, 2013-11-26 13:30:35]

On 2013/11/25 21:24:33, reed1 wrote:
> https://codereview.chromium.org/85653004/diff/1/include/core/SkDraw.h
> File include/core/SkDraw.h (right):
> 
>
https://codereview.chromium.org/85653004/diff/1/include/core/SkDraw.h#newcode110
> include/core/SkDraw.h:110: bool    shouldDrawTextAsPaths(const SkPaint&, const
> SkMatrix&) const;
> why did this go from static to member?

To make it possible for SkGpuDevice to do the check, and thereby call
drawText_asPaths() directly.

[reed1, 2013-11-26 15:28:52]

On 2013/11/26 13:30:35, JimVV wrote:
> On 2013/11/25 21:24:33, reed1 wrote:
> > https://codereview.chromium.org/85653004/diff/1/include/core/SkDraw.h
> > File include/core/SkDraw.h (right):
> > 
> >
>
https://codereview.chromium.org/85653004/diff/1/include/core/SkDraw.h#newcode110
> > include/core/SkDraw.h:110: bool    shouldDrawTextAsPaths(const SkPaint&,
const
> > SkMatrix&) const;
> > why did this go from static to member?
> 
> To make it possible for SkGpuDevice to do the check, and thereby call
> drawText_asPaths() directly.

Isn't that just a visibility problem, not a static -vs- member issue?

[jvanverth1, 2013-11-26 16:05:09]

On 2013/11/26 15:28:52, reed1 wrote:
> On 2013/11/26 13:30:35, JimVV wrote:
> > On 2013/11/25 21:24:33, reed1 wrote:
> > > https://codereview.chromium.org/85653004/diff/1/include/core/SkDraw.h
> > > File include/core/SkDraw.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/85653004/diff/1/include/core/SkDraw.h#newcode110
> > > include/core/SkDraw.h:110: bool    shouldDrawTextAsPaths(const SkPaint&,
> const
> > > SkMatrix&) const;
> > > why did this go from static to member?
> > 
> > To make it possible for SkGpuDevice to do the check, and thereby call
> > drawText_asPaths() directly.
> 
> Isn't that just a visibility problem, not a static -vs- member issue?

That is true -- I can make it static again.

That said, after a discussion with Brian, I may be revising this change to have
the Gr*TextContext take the text string and handle the layout that (in the case
of GrBitmapTextContext) is normally handled by the SkDraw. The different layout
subfunctions (autokern, subpixel) could be handled by new utility functions. One
thing this would give us (in GPU) is that we could build our vertex buffers more
efficiently, and also it may give us an easy way to override the draw function,
e.g. for NV path rendering. I still have to work through the ramifications of
this idea.

[jvanverth1, 2013-12-18 14:41:47]

So I've moved as much as I can at the moment into GrDistanceFieldTextContext.
The only pieces that I couldn't easy move were the creation of the glyph cache
and the fontScaler, as I'd have to duplicate a lot of code from SkGpuDevice.

The other thought I have is that after this the GrDistanceFieldTextContext
doesn't have much in common with GrBitmapTextContext, and there's no need for a
virtual class in this case. So I may just remove it as a subclass of
GrTextContext.

PTAL,
Jim

[bsalomon, 2013-12-18 20:44:11]

lgtm

On 2013/12/18 14:41:47, JimVV wrote:
> So I've moved as much as I can at the moment into GrDistanceFieldTextContext.
> The only pieces that I couldn't easy move were the creation of the glyph cache
> and the fontScaler, as I'd have to duplicate a lot of code from SkGpuDevice.
> 
> The other thought I have is that after this the GrDistanceFieldTextContext
> doesn't have much in common with GrBitmapTextContext, and there's no need for
a
> virtual class in this case. So I may just remove it as a subclass of
> GrTextContext.

Another alternative mighting be keeping the virtual and having the SkGpuDevice
create a long-lived text context via a factory when the device is constructed. I
see that we still fall back to the old text context when there is a
rasterizer.... but I don't think we really care about supporting rasterizers.

> 
> PTAL,
> Jim

[jvanverth1, 2013-12-18 20:59:09]

On 2013/12/18 20:44:11, bsalomon wrote:
> > The other thought I have is that after this the GrDistanceFieldTextContext
> > doesn't have much in common with GrBitmapTextContext, and there's no need
for
> a
> > virtual class in this case. So I may just remove it as a subclass of
> > GrTextContext.
> 
> Another alternative mighting be keeping the virtual and having the SkGpuDevice
> create a long-lived text context via a factory when the device is constructed.
I
> see that we still fall back to the old text context when there is a
> rasterizer.... but I don't think we really care about supporting rasterizers.

What we could do is have an fDefaultTextContext pointer which is a pointer to
the standard TextContext for that mode (Bitmap, DistanceField or NVPath) and
then create local TextContexts for the special cases, e.g. when using
rasterizers. TextContext could have a canDrawText() method which takes the
current paint and matrix, or maybe just return a boolean from
drawText()/drawPosText().

[commit-bot: I haz the power, 2013-12-19 15:03:22]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/jvanverth@google.com/85653004/40001

[commit-bot: I haz the power, 2013-12-19 16:12:32]

Change committed as 12770