Extract mask gamma; share more scaler contexts.

Extract mask gamma; share more scaler contexts.

[bungeman, 2012-06-01 21:50:08]

Assume SK_USE_COLOR_LUMINANCE.

[reed1, 2012-06-04 12:30:19]

http://codereview.appspot.com/6244068/diff/1/src/utils/SkMaskGamma.h
File src/utils/SkMaskGamma.h (right):

http://codereview.appspot.com/6244068/diff/1/src/utils/SkMaskGamma.h#newcode14
src/utils/SkMaskGamma.h:14: */
static function?

http://codereview.appspot.com/6244068/diff/1/src/utils/SkMaskGamma.h#newcode20
src/utils/SkMaskGamma.h:20: */
Where is the storage for the returned table?

[reed1, 2012-06-04 12:35:20]

It *seems* that fonthosts shouldn't have to know about indices -vs- colors.
Can't we just pre-discretize the color components (to have finite keys) and give
those to the Host, and then offer a single function

ColorComponentToTable (or some other name)

that returns a table given a color component?

http://codereview.appspot.com/6244068/diff/3001/src/ports/SkFontHost_FreeType...
File src/ports/SkFontHost_FreeType.cpp (right):

http://codereview.appspot.com/6244068/diff/3001/src/ports/SkFontHost_FreeType...
src/ports/SkFontHost_FreeType.cpp:665: 
Can this be done in SkPaint, instead of here?

[bungeman, 2012-06-04 13:41:13]

http://codereview.appspot.com/6244068/diff/1/src/utils/SkMaskGamma.h
File src/utils/SkMaskGamma.h (right):

http://codereview.appspot.com/6244068/diff/1/src/utils/SkMaskGamma.h#newcode14
src/utils/SkMaskGamma.h:14: */
On 2012/06/04 12:30:19, reed1 wrote:
> static function?

Done. Already in Patch set 2.

http://codereview.appspot.com/6244068/diff/1/src/utils/SkMaskGamma.h#newcode20
src/utils/SkMaskGamma.h:20: */
On 2012/06/04 12:30:19, reed1 wrote:
> Where is the storage for the returned table?

This one is also static. Currently the storage is just a global scoped to the
table function (like before).

http://codereview.appspot.com/6244068/diff/3001/src/ports/SkFontHost_FreeType...
File src/ports/SkFontHost_FreeType.cpp (right):

http://codereview.appspot.com/6244068/diff/3001/src/ports/SkFontHost_FreeType...
src/ports/SkFontHost_FreeType.cpp:665: 
On 2012/06/04 12:35:20, reed1 wrote:
> Can this be done in SkPaint, instead of here?

Once all the other users are updated.

[bungeman, 2012-06-04 13:42:47]

On 2012/06/04 12:35:20, reed1 wrote:
> It *seems* that fonthosts shouldn't have to know about indices -vs- colors.
> Can't we just pre-discretize the color components (to have finite keys) and
give
> those to the Host, and then offer a single function
> 
> ColorComponentToTable (or some other name)
> 
> that returns a table given a color component?

This is more or less what was done in patch set 1 with cannonicalLuminance, but
I thought you didn't like that for some reason.

[bungeman, 2012-06-27 17:55:24]

Initial ideas on piping the information down, both FreeType and GDI back-ends
updated. CoreGraphics back-end shows a method that would be ideal if it were
publicly documented.

[reed1, 2012-06-27 18:22:31]

http://codereview.appspot.com/6244068/diff/13001/src/core/SkMaskGamma.h
File src/core/SkMaskGamma.h (right):

http://codereview.appspot.com/6244068/diff/13001/src/core/SkMaskGamma.h#newco...
src/core/SkMaskGamma.h:21: 
should this be a Static method?

http://codereview.appspot.com/6244068/diff/13001/src/core/SkMaskGamma.h#newco...
src/core/SkMaskGamma.h:22: U8CPU computeLuminance(SkColor c) {
why do we divide by 255, and then multiply by 255?

[bungeman, 2012-07-18 16:21:45]

Take a look, still rough around the edges, but may do what it is we want it to
do.

[bungeman, 2012-07-19 21:02:41]

By keeping the changes to gray on mac and linux behind the SK_GAMMA_APPLY_TO_A8,
only minimal rebaselines will be needed with this change.

http://codereview.appspot.com/6244068/diff/32001/src/core/SkScalerContext.cpp
File src/core/SkScalerContext.cpp (right):

http://codereview.appspot.com/6244068/diff/32001/src/core/SkScalerContext.cpp...
src/core/SkScalerContext.cpp:593: applyLUTToA8Glyph(origGlyph,
fMaskPreBlend->fR);
One of the larger questions is, do we want to do this? In theory it's the right
thing to do. On the other hand it doesn't match current expectations, and green
shadows will look off. Also, this is applying contrast which we don't want to
do.

A similar (though weaker) argument can be made above, for the rasterizer case.

[reed1, 2012-07-20 17:24:14]

In general, this is a big (important) change, so more clarity/dox/obviousness
would help, esp. when you introduce new class/abstractions.

http://codereview.appspot.com/6244068/diff/32001/include/core/SkFontHost.h
File include/core/SkFontHost.h (left):

http://codereview.appspot.com/6244068/diff/32001/include/core/SkFontHost.h#ol...
include/core/SkFontHost.h:234: */
Is Android fine with removing this?

http://codereview.appspot.com/6244068/diff/32001/include/core/SkScalerContext.h
File include/core/SkScalerContext.h (left):

http://codereview.appspot.com/6244068/diff/32001/include/core/SkScalerContext...
include/core/SkScalerContext.h:212: #endif
Since this is in a cache-key, do these really need to be floats, or is it
sufficient to store scaled-integer versions, where we can more easily control
discretizing them before we stick them into the key?

http://codereview.appspot.com/6244068/diff/32001/src/core/SkMaskGamma.cpp
File src/core/SkMaskGamma.cpp (right):

http://codereview.appspot.com/6244068/diff/32001/src/core/SkMaskGamma.cpp#new...
src/core/SkMaskGamma.cpp:30: }
LF

http://codereview.appspot.com/6244068/diff/32001/src/core/SkMaskGamma.cpp#new...
src/core/SkMaskGamma.cpp:33: }
LF

http://codereview.appspot.com/6244068/diff/32001/src/core/SkMaskGamma.cpp#new...
src/core/SkMaskGamma.cpp:42: static void build_gamma_table(uint8_t table[256],
int srcI, float contrast, const SkColorSpaceLuminance& srcConvert, const
SkColorSpaceLuminance& dstConvert) {
line-break this definition?

http://codereview.appspot.com/6244068/diff/32001/src/core/SkMaskGamma.cpp#new...
src/core/SkMaskGamma.cpp:53: //Remove discontinuity and instability when src is
close to dst.
// comment on how 0.01 was chosen? 1/256.0?

http://codereview.appspot.com/6244068/diff/32001/src/core/SkMaskGamma.cpp#new...
src/core/SkMaskGamma.cpp:57: float srca = apply_contrast(rawSrca,
adjustedContrast);
SkToU8(...) to get debugging check that we don't exceed [0...255]

http://codereview.appspot.com/6244068/diff/32001/src/core/SkMaskGamma.cpp#new...
src/core/SkMaskGamma.cpp:75: 
SkToU8(...) also checks for negatives...

http://codereview.appspot.com/6244068/diff/32001/src/core/SkMaskGamma.h
File src/core/SkMaskGamma.h (right):

http://codereview.appspot.com/6244068/diff/32001/src/core/SkMaskGamma.h#newco...
src/core/SkMaskGamma.h:25: 
const?

http://codereview.appspot.com/6244068/diff/32001/src/core/SkMaskGamma.h#newco...
src/core/SkMaskGamma.h:29: float b = toLuma(SkColorGetB(c) / 255.0f);
Can we have some defines/something to capture these luminance coefficients?

http://codereview.appspot.com/6244068/diff/32001/src/core/SkMaskGamma.h#newco...
src/core/SkMaskGamma.h:36: class SkSrgbLuminance : public SkColorSpaceLuminance
{
SkSRGBLuminence ?

http://codereview.appspot.com/6244068/diff/32001/src/core/SkMaskGamma.h#newco...
src/core/SkMaskGamma.h:53: 
/**
 * What is this class, and what is it used for?
 */

http://codereview.appspot.com/6244068/diff/32001/src/core/SkMaskGamma.h#newco...
src/core/SkMaskGamma.h:57: * Create an sRGB mask gamma.
How are the 3 parameters used?

http://codereview.appspot.com/6244068/diff/32001/src/core/SkMaskGamma.h#newco...
src/core/SkMaskGamma.h:77: 
/**
 * What is the class, and what is it used for?
*
* Is it defined to be immutable?
 */

http://codereview.appspot.com/6244068/diff/32001/src/core/SkPaint.cpp
File src/core/SkPaint.cpp (left):

http://codereview.appspot.com/6244068/diff/32001/src/core/SkPaint.cpp#oldcode...
src/core/SkPaint.cpp:1610: 
Should this new code live in its own .cpp file?

http://codereview.appspot.com/6244068/diff/32001/src/core/SkPaint.cpp
File src/core/SkPaint.cpp (right):

http://codereview.appspot.com/6244068/diff/32001/src/core/SkPaint.cpp#newcode...
src/core/SkPaint.cpp:1448: #ifndef SK_GAMMA_CONTRAST
THe default 2.2 for gamma is not a surprise. Can you document contrast, and why
its default value should be 0.5?

http://codereview.appspot.com/6244068/diff/32001/src/core/SkPaint.cpp#newcode...
src/core/SkPaint.cpp:1558: #ifdef SK_GAMMA_SRGB
What is this ifdef for? Will we ship builds with this defined, or is it just for
testing?

http://codereview.appspot.com/6244068/diff/32001/src/core/SkScalerContext.cpp
File src/core/SkScalerContext.cpp (left):

http://codereview.appspot.com/6244068/diff/32001/src/core/SkScalerContext.cpp...
src/core/SkScalerContext.cpp:356: static void copyToLCD16(const SkBitmap& src,
const SkMask& dst) {
does A means 'hasAlpha' or something like that? clearer name?

http://codereview.appspot.com/6244068/diff/32001/src/ports/SkFontHost_mac_cor...
File src/ports/SkFontHost_mac_coretext.cpp (right):

http://codereview.appspot.com/6244068/diff/32001/src/ports/SkFontHost_mac_cor...
src/ports/SkFontHost_mac_coretext.cpp:1836: #ifndef SK_GAMMA_APPLY_TO_A8
These 4 lines have been seen before. Can this be captured in some helper on Rec?

[bungeman, 2012-07-20 22:50:25]

Still quite rough, but uploading something to look at.

https://codereview.appspot.com/6244068/diff/32001/include/core/SkFontHost.h
File include/core/SkFontHost.h (left):

https://codereview.appspot.com/6244068/diff/32001/include/core/SkFontHost.h#o...
include/core/SkFontHost.h:234: */
On 2012/07/20 17:24:15, reed1 wrote:
> Is Android fine with removing this?

I believe I asked Derek about this a while ago and he was fine with it. I'll ask
him again when he's available.

https://codereview.appspot.com/6244068/diff/32001/include/core/SkScalerContext.h
File include/core/SkScalerContext.h (left):

https://codereview.appspot.com/6244068/diff/32001/include/core/SkScalerContex...
include/core/SkScalerContext.h:212: #endif
On 2012/07/20 17:24:15, reed1 wrote:
> Since this is in a cache-key, do these really need to be floats, or is it
> sufficient to store scaled-integer versions, where we can more easily control
> discretizing them before we stick them into the key?

Good point, these bits will be compared more often than they are actually be
acted on, and small changes are imperceptible. I reduced these to uint8_t with
getters and setters.

https://codereview.appspot.com/6244068/diff/32001/src/core/SkMaskGamma.cpp
File src/core/SkMaskGamma.cpp (right):

https://codereview.appspot.com/6244068/diff/32001/src/core/SkMaskGamma.cpp#ne...
src/core/SkMaskGamma.cpp:19: }
Added LF here too.

https://codereview.appspot.com/6244068/diff/32001/src/core/SkMaskGamma.cpp#ne...
src/core/SkMaskGamma.cpp:30: }
On 2012/07/20 17:24:15, reed1 wrote:
> LF

Done.

https://codereview.appspot.com/6244068/diff/32001/src/core/SkMaskGamma.cpp#ne...
src/core/SkMaskGamma.cpp:33: }
On 2012/07/20 17:24:15, reed1 wrote:
> LF

Done.

https://codereview.appspot.com/6244068/diff/32001/src/core/SkMaskGamma.cpp#ne...
src/core/SkMaskGamma.cpp:42: static void build_gamma_table(uint8_t table[256],
int srcI, float contrast, const SkColorSpaceLuminance& srcConvert, const
SkColorSpaceLuminance& dstConvert) {
On 2012/07/20 17:24:15, reed1 wrote:
> line-break this definition?

Done.

https://codereview.appspot.com/6244068/diff/32001/src/core/SkMaskGamma.cpp#ne...
src/core/SkMaskGamma.cpp:53: //Remove discontinuity and instability when src is
close to dst.
On 2012/07/20 17:24:15, reed1 wrote:
> // comment on how 0.01 was chosen? 1/256.0?

Well, it was completely arbitrary. The function gets a bit unstable around this
point, and the discontinuity is difficult to remove. I doubt we're actually
going down this path right now, but we did while I was testing continuous mask
gamma (to see how this would all look when ported to GPU). Let's try 1/256 and
see what happens.

Added comment on arbitrariness.

https://codereview.appspot.com/6244068/diff/32001/src/core/SkMaskGamma.cpp#ne...
src/core/SkMaskGamma.cpp:57: float srca = apply_contrast(rawSrca,
adjustedContrast);
On 2012/07/20 17:24:15, reed1 wrote:
> SkToU8(...) to get debugging check that we don't exceed [0...255]

Done.

https://codereview.appspot.com/6244068/diff/32001/src/core/SkMaskGamma.cpp#ne...
src/core/SkMaskGamma.cpp:75: 
On 2012/07/20 17:24:15, reed1 wrote:
> SkToU8(...) also checks for negatives...

Done.

https://codereview.appspot.com/6244068/diff/32001/src/core/SkMaskGamma.h
File src/core/SkMaskGamma.h (right):

https://codereview.appspot.com/6244068/diff/32001/src/core/SkMaskGamma.h#newc...
src/core/SkMaskGamma.h:25: 
On 2012/07/20 17:24:15, reed1 wrote:
> const?

Done.

https://codereview.appspot.com/6244068/diff/32001/src/core/SkMaskGamma.h#newc...
src/core/SkMaskGamma.h:29: float b = toLuma(SkColorGetB(c) / 255.0f);
On 2012/07/20 17:24:15, reed1 wrote:
> Can we have some defines/something to capture these luminance coefficients?

Added defines of the form SK_ITU_BT709_LUM_X with a comment about where they
come from. I'm not really sure where to put these, they seem a little more
general than this header.

https://codereview.appspot.com/6244068/diff/32001/src/core/SkMaskGamma.h#newc...
src/core/SkMaskGamma.h:36: class SkSrgbLuminance : public SkColorSpaceLuminance
{
On 2012/07/20 17:24:15, reed1 wrote:
> SkSRGBLuminence ?

Why not? I went though SksRGBLuminance, SKsRGBLuminance,  and SkSRGBLuminance
before giving up after trying SkSrgbLuminance. Sometimes camel case just doesn't
satisfy.

https://codereview.appspot.com/6244068/diff/32001/src/core/SkMaskGamma.h#newc...
src/core/SkMaskGamma.h:53: 
On 2012/07/20 17:24:15, reed1 wrote:
> /**
>  * What is this class, and what is it used for?
>  */

Done.

https://codereview.appspot.com/6244068/diff/32001/src/core/SkMaskGamma.h#newc...
src/core/SkMaskGamma.h:57: * Create an sRGB mask gamma.
On 2012/07/20 17:24:15, reed1 wrote:
> How are the 3 parameters used?

Done.

https://codereview.appspot.com/6244068/diff/32001/src/core/SkMaskGamma.h#newc...
src/core/SkMaskGamma.h:77: 
On 2012/07/20 17:24:15, reed1 wrote:
> /**
>  * What is the class, and what is it used for?
> *
> * Is it defined to be immutable?
>  */

Done.

https://codereview.appspot.com/6244068/diff/32001/src/core/SkPaint.cpp
File src/core/SkPaint.cpp (left):

https://codereview.appspot.com/6244068/diff/32001/src/core/SkPaint.cpp#oldcod...
src/core/SkPaint.cpp:1610: 
On 2012/07/20 17:24:15, reed1 wrote:
> Should this new code live in its own .cpp file?

I assume you mean the above three functions which are caches of size one.
They're rather heavily tied to SkPaint at the moment, though that could possibly
change. Part of the reason for this is that currently I've just pulled
everything up to SkPaint (which seems high enough for this CL). In the future
when we plumb in the ability for users to set these values we may want to
re-think some of this. In other words, I want to put it off.

https://codereview.appspot.com/6244068/diff/32001/src/core/SkPaint.cpp
File src/core/SkPaint.cpp (right):

https://codereview.appspot.com/6244068/diff/32001/src/core/SkPaint.cpp#newcod...
src/core/SkPaint.cpp:1448: #ifndef SK_GAMMA_CONTRAST
On 2012/07/20 17:24:15, reed1 wrote:
> THe default 2.2 for gamma is not a surprise. Can you document contrast, and
why
> its default value should be 0.5?

It is arbitrary, but I've added a comment with justification.

https://codereview.appspot.com/6244068/diff/32001/src/core/SkPaint.cpp#newcod...
src/core/SkPaint.cpp:1558: #ifdef SK_GAMMA_SRGB
On 2012/07/20 17:24:15, reed1 wrote:
> What is this ifdef for? Will we ship builds with this defined, or is it just
for
> testing?

This ifdef will go away when we allow the value to be set at runtime. I would
like to always ship with this on, as it gives better black text (since most
displays are actually sRGB these days anyway). I have it set always through gyp,
and would like it to always be defined in chrome, for the time being.

https://codereview.appspot.com/6244068/diff/32001/src/core/SkScalerContext.cpp
File src/core/SkScalerContext.cpp (left):

https://codereview.appspot.com/6244068/diff/32001/src/core/SkScalerContext.cp...
src/core/SkScalerContext.cpp:356: static void copyToLCD16(const SkBitmap& src,
const SkMask& dst) {
On 2012/07/20 17:24:15, reed1 wrote:
> does A means 'hasAlpha' or something like that? clearer name?

I think I originally had something in mind for it, but now I have forgotten. I
think it stood for 'Apply'. Basically 'A' can be read as 'applyPreBlend'. If
it's false, the pre-blend won't be applied, resulting in a linear mask being
produced. The linear mask is needed when there will be a filter applied, as the
filter expects a linear mask to work on.

https://codereview.appspot.com/6244068/diff/32001/src/ports/SkFontHost_mac_co...
File src/ports/SkFontHost_mac_coretext.cpp (right):

https://codereview.appspot.com/6244068/diff/32001/src/ports/SkFontHost_mac_co...
src/ports/SkFontHost_mac_coretext.cpp:1836: #ifndef SK_GAMMA_APPLY_TO_A8
On 2012/07/20 17:24:15, reed1 wrote:
> These 4 lines have been seen before. Can this be captured in some helper on
Rec?

Yes, they're also in freetype. They're there for slightly different reasons and
have slightly different effects. I might be able to put this somewhere, but
ideally this code block wouldn't exist at all. I will take a look.

[DerekS, 2012-07-23 12:36:18]

http://codereview.appspot.com/6244068/diff/32001/include/core/SkFontHost.h
File include/core/SkFontHost.h (left):

http://codereview.appspot.com/6244068/diff/32001/include/core/SkFontHost.h#ol...
include/core/SkFontHost.h:234: */
There are no callers of this function in Android.

On 2012/07/20 22:50:25, bungeman wrote:
> On 2012/07/20 17:24:15, reed1 wrote:
> > Is Android fine with removing this?
> 
> I believe I asked Derek about this a while ago and he was fine with it. I'll
ask
> him again when he's available.

[bungeman, 2012-07-25 17:18:51]

Now seems to do what we want it to do, with minimal rebaselines required.

[reed1, 2012-07-25 19:14:02]

https://codereview.appspot.com/6244068/diff/43001/include/core/SkScalerContext.h
File include/core/SkScalerContext.h (left):

https://codereview.appspot.com/6244068/diff/43001/include/core/SkScalerContex...
include/core/SkScalerContext.h:80: #endif
Is there a perf win to not make all of these base-255 instead of base 64 or 128?

https://codereview.appspot.com/6244068/diff/43001/include/core/SkScalerContext.h
File include/core/SkScalerContext.h (right):

https://codereview.appspot.com/6244068/diff/43001/include/core/SkScalerContex...
include/core/SkScalerContext.h:71: 
Seems like the 4, 64, 1 and 128 should all be private enums

Below, none of the SkIntToScalar() macros are needed for the literals except in
the asserts, where we compare against a skscalar input.

https://codereview.appspot.com/6244068/diff/43001/src/core/SkMaskGamma.cpp
File src/core/SkMaskGamma.cpp (right):

https://codereview.appspot.com/6244068/diff/43001/src/core/SkMaskGamma.cpp#ne...
src/core/SkMaskGamma.cpp:13: 
Where do any of these literal float values come from? (next 10 lines or so)

https://codereview.appspot.com/6244068/diff/43001/src/core/SkMaskGamma.cpp#ne...
src/core/SkMaskGamma.cpp:87: }
Do we have a comment somewhere (in .h or .cpp) that talks about our decision to
discretize luminance to 2 bits (and therefore only 4 cached tables)? I'm fine
with the choice, but it should be described somewhere.

https://codereview.appspot.com/6244068/diff/43001/src/core/SkMaskGamma.h
File src/core/SkMaskGamma.h (right):

https://codereview.appspot.com/6244068/diff/43001/src/core/SkMaskGamma.h#newc...
src/core/SkMaskGamma.h:31: U8CPU computeLuminance(SkColor c) const {
SkIntToScalar(255) can just become 255 (or faster, if we want to hard-code using
floats for this local computation: * 0.003921568627451

https://codereview.appspot.com/6244068/diff/43001/src/core/SkPaint.cpp
File src/core/SkPaint.cpp (right):

https://codereview.appspot.com/6244068/diff/43001/src/core/SkPaint.cpp#newcod...
src/core/SkPaint.cpp:1709: // Other functions may work better.
The above comment isn't very clear. Should this be moved into some tiny helper
function, and give it a somewhat meaningful name (with a big comment above the
function)?

[bungeman, 2012-07-26 20:51:05]

https://codereview.appspot.com/6244068/diff/43001/include/core/SkScalerContext.h
File include/core/SkScalerContext.h (right):

https://codereview.appspot.com/6244068/diff/43001/include/core/SkScalerContex...
include/core/SkScalerContext.h:71: 
On 2012/07/25 19:14:02, reed1 wrote:
> Seems like the 4, 64, 1 and 128 should all be private enums
> 
> Below, none of the SkIntToScalar() macros are needed for the literals except
in
> the asserts, where we compare against a skscalar input.

fDeviceGamma and fPaintGamma really are 0 - 3.99. Changed fContrast to base 255.

https://codereview.appspot.com/6244068/diff/43001/src/core/SkMaskGamma.cpp
File src/core/SkMaskGamma.cpp (right):

https://codereview.appspot.com/6244068/diff/43001/src/core/SkMaskGamma.cpp#ne...
src/core/SkMaskGamma.cpp:13: 
On 2012/07/25 19:14:02, reed1 wrote:
> Where do any of these literal float values come from? (next 10 lines or so)

These magic numbers come from the sRGB specification. Added comment.

https://codereview.appspot.com/6244068/diff/43001/src/core/SkMaskGamma.cpp#ne...
src/core/SkMaskGamma.cpp:87: }
On 2012/07/25 19:14:02, reed1 wrote:
> Do we have a comment somewhere (in .h or .cpp) that talks about our decision
to
> discretize luminance to 2 bits (and therefore only 4 cached tables)? I'm fine
> with the choice, but it should be described somewhere.

Made the bits configurable.

https://codereview.appspot.com/6244068/diff/43001/src/core/SkMaskGamma.h
File src/core/SkMaskGamma.h (right):

https://codereview.appspot.com/6244068/diff/43001/src/core/SkMaskGamma.h#newc...
src/core/SkMaskGamma.h:31: U8CPU computeLuminance(SkColor c) const {
On 2012/07/25 19:14:02, reed1 wrote:
> SkIntToScalar(255) can just become 255 (or faster, if we want to hard-code
using
> floats for this local computation: * 0.003921568627451

It looks like gcc takes advantage of the symmetrical divide by 255 multiply by
255 when it optimizes. vc++ generated more or less the same code regardless of
dividing by 255 or multiplying by the reciprocal. This function is only called
once per scaler context creation, so it isn't in a hot spot or anything.

https://codereview.appspot.com/6244068/diff/43001/src/core/SkPaint.cpp
File src/core/SkPaint.cpp (right):

https://codereview.appspot.com/6244068/diff/43001/src/core/SkPaint.cpp#newcod...
src/core/SkPaint.cpp:1709: // Other functions may work better.
On 2012/07/25 19:14:02, reed1 wrote:
> The above comment isn't very clear. Should this be moved into some tiny helper
> function, and give it a somewhat meaningful name (with a big comment above the
> function)?

How about just a shorter, clearer comment?

[reed1, 2012-07-27 14:29:07]

mostly requests for more clarity, some requests for code-sharing

https://codereview.appspot.com/6244068/diff/44018/include/core/SkScalerContext.h
File include/core/SkScalerContext.h (right):

https://codereview.appspot.com/6244068/diff/44018/include/core/SkScalerContex...
include/core/SkScalerContext.h:26: 
Small block-comment describing this discretization trick, and why 2-bits is a
decent guess.

https://codereview.appspot.com/6244068/diff/44018/src/core/SkMaskGamma.h
File src/core/SkMaskGamma.h (right):

https://codereview.appspot.com/6244068/diff/44018/src/core/SkMaskGamma.h#newc...
src/core/SkMaskGamma.h:71: */
Perhaps these template params should be renamed for clarity?

R -> R_BITS

Same comment for SkTMaskPreBlend

https://codereview.appspot.com/6244068/diff/44018/src/core/SkMaskGamma.h#newc...
src/core/SkMaskGamma.h:144: 
Does this really need to be in a header, instead of a .cpp?

https://codereview.appspot.com/6244068/diff/44018/src/core/SkMaskGamma.h#newc...
src/core/SkMaskGamma.h:211: 
Are all of these R G B really R_BITS, G_BITS, B_BITS ?

Comment above each function, describing what it does?

https://codereview.appspot.com/6244068/diff/44018/src/core/SkScalerContext.cpp
File src/core/SkScalerContext.cpp (right):

https://codereview.appspot.com/6244068/diff/44018/src/core/SkScalerContext.cp...
src/core/SkScalerContext.cpp:77: SkScalerContext::SkScalerContext(const
SkDescriptor* desc)
Off hand, I do not find moving this much logic into the constructor helpful. Is
this change necessary as part of this CL?

https://codereview.appspot.com/6244068/diff/44018/src/ports/SkFontHost_FreeTy...
File src/ports/SkFontHost_FreeType.cpp (right):

https://codereview.appspot.com/6244068/diff/44018/src/ports/SkFontHost_FreeTy...
src/ports/SkFontHost_FreeType.cpp:1165: template<bool A>
Did I just see these two in SkScalerContext.cpp

https://codereview.appspot.com/6244068/diff/44018/src/ports/SkFontHost_mac_co...
File src/ports/SkFontHost_mac_coretext.cpp (right):

https://codereview.appspot.com/6244068/diff/44018/src/ports/SkFontHost_mac_co...
src/ports/SkFontHost_mac_coretext.cpp:1196: 
We should consider promoting this to SkTypes.h or somewhere handy later on

https://codereview.appspot.com/6244068/diff/44018/src/ports/SkFontHost_mac_co...
src/ports/SkFontHost_mac_coretext.cpp:1837: #ifndef SK_GAMMA_APPLY_TO_A8
I've seen this somewhere before. Helper function on Rec?

https://codereview.appspot.com/6244068/diff/44018/src/ports/SkFontHost_win.cpp
File src/ports/SkFontHost_win.cpp (right):

https://codereview.appspot.com/6244068/diff/44018/src/ports/SkFontHost_win.cp...
src/ports/SkFontHost_win.cpp:957: template<bool A>
Now I KNOW I've seen these before...

https://codereview.appspot.com/6244068/diff/44018/src/ports/SkFontHost_win.cp...
src/ports/SkFontHost_win.cpp:1057: 
Comment what A is, or rename it somehow... HAS_ALPHA? IS_OPAQUE?

[bungeman, 2012-07-27 19:30:52]

I think I've gotten to all of the existing comments now.

https://codereview.appspot.com/6244068/diff/44018/include/core/SkScalerContext.h
File include/core/SkScalerContext.h (right):

https://codereview.appspot.com/6244068/diff/44018/include/core/SkScalerContex...
include/core/SkScalerContext.h:26: 
On 2012/07/27 14:29:07, reed1 wrote:
> Small block-comment describing this discretization trick, and why 2-bits is a
> decent guess.

Done.

https://codereview.appspot.com/6244068/diff/44018/src/core/SkMaskGamma.h
File src/core/SkMaskGamma.h (right):

https://codereview.appspot.com/6244068/diff/44018/src/core/SkMaskGamma.h#newc...
src/core/SkMaskGamma.h:71: */
On 2012/07/27 14:29:07, reed1 wrote:
> Perhaps these template params should be renamed for clarity?
> 
> R -> R_BITS
> 
> Same comment for SkTMaskPreBlend

Changed to X_LUM_BITS, though it is rather verbose.

https://codereview.appspot.com/6244068/diff/44018/src/core/SkMaskGamma.h#newc...
src/core/SkMaskGamma.h:144: 
On 2012/07/27 14:29:07, reed1 wrote:
> Does this really need to be in a header, instead of a .cpp?

The user is the SkTMaskGamma constructor, and logically this code is part of
SkTMaskGamma. I've moved the code to be inside the class where it logically
belongs (getting it out of the global namespace), though we may be able to move
it out at some point.

https://codereview.appspot.com/6244068/diff/44018/src/core/SkMaskGamma.h#newc...
src/core/SkMaskGamma.h:211: 
On 2012/07/27 14:29:07, reed1 wrote:
> Are all of these R G B really R_BITS, G_BITS, B_BITS ?
> 
> Comment above each function, describing what it does?

Changed to X_LUM_BITS.

The functions are commented up where they are defined, and duplicating that
seems error-prone. They are specified out-of-line like this due to the
definitions of both SkTMaskGamma and SkTMaskPreBlend both being needed.

https://codereview.appspot.com/6244068/diff/44018/src/core/SkScalerContext.cpp
File src/core/SkScalerContext.cpp (right):

https://codereview.appspot.com/6244068/diff/44018/src/core/SkScalerContext.cp...
src/core/SkScalerContext.cpp:77: SkScalerContext::SkScalerContext(const
SkDescriptor* desc)
On 2012/07/27 14:29:07, reed1 wrote:
> Off hand, I do not find moving this much logic into the constructor helpful.
Is
> this change necessary as part of this CL?

fMaskPreBlend must be done in the initializer list to prevent it from being
default constructed before being assigned.

Otherwise, this is the same logic as before, we're initializing everything
instead of assigning it. I'm mostly of this guy's opinion,
http://www.parashift.com/c%2B%2B-faq-lite/init-lists.html

https://codereview.appspot.com/6244068/diff/44018/src/ports/SkFontHost_FreeTy...
File src/ports/SkFontHost_FreeType.cpp (right):

https://codereview.appspot.com/6244068/diff/44018/src/ports/SkFontHost_FreeTy...
src/ports/SkFontHost_FreeType.cpp:1165: template<bool A>
On 2012/07/27 14:29:07, reed1 wrote:
> Did I just see these two in SkScalerContext.cpp

Yes. Moved to SkMaskGamma.h.

https://codereview.appspot.com/6244068/diff/44018/src/ports/SkFontHost_mac_co...
File src/ports/SkFontHost_mac_coretext.cpp (right):

https://codereview.appspot.com/6244068/diff/44018/src/ports/SkFontHost_mac_co...
src/ports/SkFontHost_mac_coretext.cpp:1196: 
On 2012/07/27 14:29:07, reed1 wrote:
> We should consider promoting this to SkTypes.h or somewhere handy later on

Yes, and it should also be used in at least 6 places above.

https://codereview.appspot.com/6244068/diff/44018/src/ports/SkFontHost_mac_co...
src/ports/SkFontHost_mac_coretext.cpp:1837: #ifndef SK_GAMMA_APPLY_TO_A8
On 2012/07/27 14:29:07, reed1 wrote:
> I've seen this somewhere before. Helper function on Rec?

Added ingorePreBlend to SkScalarContext::Rec.

https://codereview.appspot.com/6244068/diff/44018/src/ports/SkFontHost_win.cpp
File src/ports/SkFontHost_win.cpp (right):

https://codereview.appspot.com/6244068/diff/44018/src/ports/SkFontHost_win.cp...
src/ports/SkFontHost_win.cpp:957: template<bool A>
On 2012/07/27 14:29:07, reed1 wrote:
> Now I KNOW I've seen these before...

Ah, but you haven't, this is the Windows one, the specialization is 'static'
(grr vc++ and gcc).

https://codereview.appspot.com/6244068/diff/44018/src/ports/SkFontHost_win.cp...
src/ports/SkFontHost_win.cpp:1057: 
On 2012/07/27 14:29:07, reed1 wrote:
> Comment what A is, or rename it somehow... HAS_ALPHA? IS_OPAQUE?

Renamed all A to APPLY_PREBLEND.

[reed1, 2012-07-27 20:32:41]

lets move that big guy into a .cpp, but after that, lgtm

[bungeman, 2012-08-01 19:16:35]

Committed revision r4841.
Rebaselined r4843.
Fixed r4889.

Chromium roll http://codereview.chromium.org/10825089/ .

[thakis, 2012-08-27 05:11:10]

https://codereview.appspot.com/6244068/diff/52002/src/core/SkPaint.cpp
File src/core/SkPaint.cpp (right):

https://codereview.appspot.com/6244068/diff/52002/src/core/SkPaint.cpp#newcod...
src/core/SkPaint.cpp:1628: gDeviceLuminance = SkNEW_ARGS(SkGammaLuminance,
(gammaExponent));
This leaks if called repeatedly with different gamma exponents. I'm filing a
valgrind bug for this.

[bungeman, 2012-08-31 21:17:58]

> This leaks if called repeatedly with different gamma exponents.

True, but this cannot currently happen. The code was written this way as a
stopping off point to show future intent. (This change was already rather
large.)

On the other hand this should be future proofed and we should free the resources
on Term. See https://codereview.appspot.com/6495071/

[thakis, 2012-08-31 21:19:56]

On Fri, Aug 31, 2012 at 2:17 PM, <bungeman@google.com> wrote:

> This leaks if called repeatedly with different gamma exponents.
>>
>
> True, but this cannot currently happen.


http://crbug.com/144930 is valgrind complaining about this leak, so I guess
it does happen somehow.


> The code was written this way as
> a stopping off point to show future intent. (This change was already
> rather large.)
>
> On the other hand this should be future proofed and we should free the
> resources on Term. See
https://codereview.appspot.**com/6495071/<https://codereview.appspot.com/6495...
>
>
https://codereview.appspot.**com/6244068/<https://codereview.appspot.com/6244...
>

[bungeman, 2012-08-31 21:56:28]

> so I guess it does happen somehow.

I now understand why I haven't seen this before. It doesn't happen in Skia, but
it does happen in Chrome. This is because Chrome does not define
SK_GAMMA_APPLY_TO_A8. When this is not defined, on Linux the regular
anti-aliased text is not gamma corrected, mostly because there is currently no
good means of re-base-lining the layout tests. I will need to re-think how
ignorePreBlend works or increase the cache size to 2.

I have opened http://code.google.com/p/skia/issues/detail?id=825 to track this.