Add subpixel text.

Add subpixel text.

For the moment, this only supports horizontal subpixel renderering. It
also splits apart the idea of subpixel positioning and subpixel
rendering.

In the near future, I'm looking to support the full range of GTK
options. So the SubpixelGlyphs flag will turn into two calls: one to
set the hinting level (none, light, normal, full) and one to set the
anti-aliasing level (mono, aa and subpixel).

[reed, 2009-07-09 16:57:22]

Very excited to see this effort begun.

1) What are the other changes that will be required for other depths (i.e. if I
drew to a 16bit device, will it revert to simple A8, or will it look wacky (or
crash)?

2) How should Skia handle different RGB ordering on LCDs? Compile-time flag?
runtime? Where would those changes affect?

3) Related to (1), if I request LCD style, how can I downgrade in different
parts of the code (e.g. calling an xfermode, which assumes a single byte per
pixel)?

4) Is subpixelpositioning handled for free? i.e. do we just generate different
masks for different positions as we do for A8? If so, that's nice since it
simplifies our code, but I wonder if we could instead reuse the same mask but
just change the phase in which we access it based on a 1/3 portion of the
fractional X position...

5) As an idea for 1,3, what if the mask format was semi-planar: Begins with a
plane of just A8, followed by what you have now. This would both remove the need
for calling max twice inside the blit, and would make it trivial for us to
down-grade for things like xfermodes, since we'd have a ready-made A8 mask to
use.

Really cool that you started this. Perhaps we could have a VC sometime to talk
more.

http://codereview.appspot.com/91088/diff/1/2
File include/core/SkBitmap.h (left):

http://codereview.appspot.com/91088/diff/1/2#oldcode47
Line 47: kA8_Config,         //!< 8-bits per pixel, with only alpha specified (0
is transparent, 0xFF is opaque)
Maybe that's the right name. kA8x3 ? kALCD ?

http://codereview.appspot.com/91088/diff/1/3
File include/core/SkPaint.h (left):

http://codereview.appspot.com/91088/diff/1/3#oldcode83
Line 83: kDevKernText_Flag     = 0x100,  //!< mask to enable device kerning text
Not sure the Glyphs suffix is any clearer to me. I like your orthogonal
distinction between position and rendering. Perhaps we deprecate subpixelText,
and create two clear names (e.g. kSubpixelPositionText, kSubpixelRenderText)?

http://codereview.appspot.com/91088/diff/1/3#oldcode752
Line 752: unsigned        fFlags : 9;
do we need 11 bits, or 10?

http://codereview.appspot.com/91088/diff/1/4
File include/core/SkScalerContext.h (right):

http://codereview.appspot.com/91088/diff/1/4#newcode50
Line 50: } else if (SkMask::kLCD_Format == fMaskFormat) {
do we need to align the rowbytes in the LCD case?

http://codereview.appspot.com/91088/diff/1/5
File src/core/SkBlitter_ARGB32.cpp (left):

http://codereview.appspot.com/91088/diff/1/5#oldcode209
Line 209: 
Did you know about the getters for the components (e.g. SkGetPackedR32(c) and
the constructor SkPackARGB32(a, r, g, b) in SkColorPriv.h, rather than doing the
explicit shifts and masks?

http://codereview.appspot.com/91088/diff/1/5
File src/core/SkBlitter_ARGB32.cpp (right):

http://codereview.appspot.com/91088/diff/1/5#newcode238
Line 238: unsigned alphaBlue = *alpha++;
I had never thought about what to do for the alpha component of the dst. If we
really have to call max (twice), perhaps we could store that in the mask itself
(making it 4 channels instead of 3)?

[agl, 2009-07-09 18:00:42]

(partial reply. Other points addressed via email)

http://codereview.appspot.com/91088/diff/1/2
File include/core/SkBitmap.h (left):

http://codereview.appspot.com/91088/diff/1/2#oldcode47
Line 47: kA8_Config,         //!< 8-bits per pixel, with only alpha specified (0
is transparent, 0xFF is opaque)
On 2009/07/09 16:57:22, reed wrote:
> Maybe that's the right name. kA8x3 ? kALCD ?

kALCD sounds good, because we'll need kVerticalALCD soon.

http://codereview.appspot.com/91088/diff/1/3
File include/core/SkPaint.h (left):

http://codereview.appspot.com/91088/diff/1/3#oldcode83
Line 83: kDevKernText_Flag     = 0x100,  //!< mask to enable device kerning text
On 2009/07/09 16:57:22, reed wrote:
> Not sure the Glyphs suffix is any clearer to me. I like your orthogonal
> distinction between position and rendering. Perhaps we deprecate subpixelText,
> and create two clear names (e.g. kSubpixelPositionText, kSubpixelRenderText)?

Done.

http://codereview.appspot.com/91088/diff/1/3#oldcode752
Line 752: unsigned        fFlags : 9;
On 2009/07/09 16:57:22, reed wrote:
> do we need 11 bits, or 10?

Thanks.

http://codereview.appspot.com/91088/diff/1/4
File include/core/SkScalerContext.h (right):

http://codereview.appspot.com/91088/diff/1/4#newcode50
Line 50: } else if (SkMask::kLCD_Format == fMaskFormat) {
On 2009/07/09 16:57:22, reed wrote:
> do we need to align the rowbytes in the LCD case?

I'll defer to you here. We do for other formats. Should we not for LCD formats
too?

http://codereview.appspot.com/91088/diff/1/5
File src/core/SkBlitter_ARGB32.cpp (left):

http://codereview.appspot.com/91088/diff/1/5#oldcode209
Line 209: 
On 2009/07/09 16:57:22, reed wrote:
> Did you know about the getters for the components (e.g. SkGetPackedR32(c) and
> the constructor SkPackARGB32(a, r, g, b) in SkColorPriv.h, rather than doing
the
> explicit shifts and masks?

I'll switch to using them now that I know. Thanks

[agl, 2009-07-14 20:00:56]

Have updated with discussed design.

At the moment, non-opaque text falls back to the A8 plane because it seems to
take a very different path through the blitting code.

[reed, 2009-07-17 19:46:39]

http://codereview.appspot.com/91088/diff/3012/2005
File include/core/SkFontHost.h (right):

http://codereview.appspot.com/91088/diff/3012/2005#newcode204
Line 204: 
Capitalize these, since they are static methods (see GetGammaTables)

http://codereview.appspot.com/91088/diff/3012/2007
File include/core/SkPaint.h (left):

http://codereview.appspot.com/91088/diff/3012/2007#oldcode71
Line 71: 
In general, this seems like a lot of levels for a multi-fontscaler graphics
engine to publicize. Especially the distinction (which I don't really get)
between Normal and Full. Since we have an orthogonal LCD flag, do we need to
have (and explain) these two in hintlevel?

Additionally (and this is a very minor comment) - it seems odd to have a value
called Normal, but not use it as our default :)

http://codereview.appspot.com/91088/diff/3012/2007
File include/core/SkPaint.h (right):

http://codereview.appspot.com/91088/diff/3012/2007#newcode82
Line 82: */
Hinting, or HintingLevel? Just trying to match the enum name to the
getter/setter convention if possible.

http://codereview.appspot.com/91088/diff/3012/2007#newcode90
Line 90: 
return type should be the enum name

http://codereview.appspot.com/91088/diff/3012/2007#newcode95
Line 95: 
parameter type should be the enum name

http://codereview.appspot.com/91088/diff/3012/2008
File include/core/SkScalerContext.h (right):

http://codereview.appspot.com/91088/diff/3012/2008#newcode159
Line 159: uint8_t     fHintingLevel;  //!< See HintingLevel in SkPaint.h
Can we just make this another bit in Flags, rather than a whole bool/int?

http://codereview.appspot.com/91088/diff/3012/2009
File src/core/SkBlitter_ARGB32.cpp (left):

http://codereview.appspot.com/91088/diff/3012/2009#oldcode23
Line 23: 
Don't use any namespace yet in skia, but assuming this is supported everywhere,
fine.

http://codereview.appspot.com/91088/diff/3012/2009#oldcode209
Line 209: 
For performance, and possibly better isolation, perhaps we can move the loops
themselves also into the external function. Just a thought. Easy to refactor
later if that proves useful.

http://codereview.appspot.com/91088/diff/3012/2013
File src/core/SkScalerContext.cpp (left):

http://codereview.appspot.com/91088/diff/3012/2013#oldcode17
Line 17: 
General comment about the mask format

What do we implicitly add 2 to either the width or the height everywhere. Why
don't we just add that value to fWidth/fHeight up front?

Separate topic: When I suggested the planar-a8 layer (thanks for trying it), I
did not also think we needed 4-bytes to follow. I has presumed the a8-plane
would be the max, and that what would follow would just need to be (either
planar or interleaved) 3 more bytes per pixel. We needed resolve this before the
checkin per-se.

[reed, 2009-07-21 02:09:15]

LGTM

I think there are more internal changes I would like to try, but I think the
public API part (esp. SkPaint.h) looks good, so I suggest we submit it, and then
we can refine it in place.