Start canonicalizing color for all A8 textblobs

Start canonicalizing color for all A8 textblobs

BUG=skia:

Committed: https://skia.googlesource.com/skia/+/9e36c1a9306f052331550dab4728b9875127bfb5

[joshualitt, 2015-04-13 13:01:46]

joshualitt@google.com changed reviewers:
+ bsalomon@google.com, bungeman@google.com, fmalita@chromium.org,
joshualitt@google.com, reed@google.com

[joshualitt, 2015-04-13 13:01:46]

A change to only regenerate textblobs if the canonical color changes.

reed for SkPaint changes
fmalita for textblob
bungeman for computation of canonical color
bsalomon for general atlascontext changes.

[joshualitt, 2015-04-13 13:02:37]

joshualitt@google.com changed reviewers:
+ jvanverth@google.com

[bsalomon, 2015-04-13 15:16:38]

lgtm aside from requests for comments.

https://codereview.chromium.org/1076593002/diff/290001/gm/textblobcolortrans.cpp
File gm/textblobcolortrans.cpp (right):

https://codereview.chromium.org/1076593002/diff/290001/gm/textblobcolortrans....
gm/textblobcolortrans.cpp:19: static void add_to_text_blob(SkTextBlobBuilder*
builder, const char* text, const SkPaint& origPaint,
This looks familiar... should we add this to some GM utility file somewhere?

https://codereview.chromium.org/1076593002/diff/290001/gm/textblobcolortrans....
gm/textblobcolortrans.cpp:34: class TextBlobColorTrans : public GM {
// This GM tests some stuff about gamma text blah blah blah 

It's just not obvious why this test exists.

https://codereview.chromium.org/1076593002/diff/290001/gm/textblobcolortrans....
gm/textblobcolortrans.cpp:85: // Colors were chosen to map to pairs of canonical
colors
not obvious to me what canonical colors are.

https://codereview.chromium.org/1076593002/diff/290001/include/core/SkPaint.h
File include/core/SkPaint.h (right):

https://codereview.chromium.org/1076593002/diff/290001/include/core/SkPaint.h...
include/core/SkPaint.h:1079: SkColor computeLuminanceColor() const;
comment about what luminance color is? Or is this something that already has a
clear meaning in Skia?

https://codereview.chromium.org/1076593002/diff/290001/src/gpu/GrAtlasTextCon...
File src/gpu/GrAtlasTextContext.h (right):

https://codereview.chromium.org/1076593002/diff/290001/src/gpu/GrAtlasTextCon...
src/gpu/GrAtlasTextContext.h:158: // There are a certain number of buckets, and
within those buckets we can reassign the
Replace second sentence with

Each color is assigned to on of a fixed number of buckets based o nits
luminance. For each luminance bucket there is a "canonical color" that
represents the bucket.

??

[jvanverth1, 2015-04-13 16:51:51]

Are there plans to support LCD? That seems like the more interesting case.

[reed1, 2015-04-13 17:05:51]

https://codereview.chromium.org/1076593002/diff/290001/include/core/SkTextBlob.h
File include/core/SkTextBlob.h (right):

https://codereview.chromium.org/1076593002/diff/290001/include/core/SkTextBlo...
include/core/SkTextBlob.h:71: uint32_t flags() const;
I'm nervous about an API that promises to return "paint" flags per-run, as we
certainly hope in the future to not store so much. Can we get away with a more
narrow, semantic getter, like isLCD() or return an enum for AA-type (e.g. BW,
A8, LCD)?

https://codereview.chromium.org/1076593002/diff/290001/src/gpu/GrAtlasTextCon...
File src/gpu/GrAtlasTextContext.h (right):

https://codereview.chromium.org/1076593002/diff/290001/src/gpu/GrAtlasTextCon...
src/gpu/GrAtlasTextContext.h:156: bool fHasBlur;
I know there is a memset(0) at the beginning (btw - could use sk_bzero), but
still the fields in this Key are not explicitly packed, allowing for gaps in the
in-memory representation. Could we be more explicit in the order and size of
each?

[joshualitt, 2015-04-14 18:37:12]

On 2015/04/13 17:05:51, reed1 wrote:
>
https://codereview.chromium.org/1076593002/diff/290001/include/core/SkTextBlob.h
> File include/core/SkTextBlob.h (right):
> 
>
https://codereview.chromium.org/1076593002/diff/290001/include/core/SkTextBlo...
> include/core/SkTextBlob.h:71: uint32_t flags() const;
> I'm nervous about an API that promises to return "paint" flags per-run, as we
> certainly hope in the future to not store so much. Can we get away with a more
> narrow, semantic getter, like isLCD() or return an enum for AA-type (e.g. BW,
> A8, LCD)?
> 
>
https://codereview.chromium.org/1076593002/diff/290001/src/gpu/GrAtlasTextCon...
> File src/gpu/GrAtlasTextContext.h (right):
> 
>
https://codereview.chromium.org/1076593002/diff/290001/src/gpu/GrAtlasTextCon...
> src/gpu/GrAtlasTextContext.h:156: bool fHasBlur;
> I know there is a memset(0) at the beginning (btw - could use sk_bzero), but
> still the fields in this Key are not explicitly packed, allowing for gaps in
the
> in-memory representation. Could we be more explicit in the order and size of
> each?

@Reed, I have packed the key bytes a bit better.  I should pack the owning
struct as well.
@jvanverth, canonical color for LCD is a whole can of worms because of the issue
of A8 fallback glyphs.  I will tackle it at some point, that or sRGB.

[reed1, 2015-04-14 18:51:06]

lgtm

https://codereview.chromium.org/1076593002/diff/330001/include/core/SkTextBlob.h
File include/core/SkTextBlob.h (right):

https://codereview.chromium.org/1076593002/diff/330001/include/core/SkTextBlo...
include/core/SkTextBlob.h:71: bool hasLCD() const;
nit: isLCD() since we're talking about the entire run for this predicate, but
hasLCD is fine.

[joshualitt, 2015-04-14 19:09:31]

The CQ bit was checked by joshualitt@google.com

[joshualitt, 2015-04-14 19:09:32]

The patchset sent to the CQ was uploaded after l-g-t-m from bsalomon@google.com,
reed@google.com
Link to the patchset: https://codereview.chromium.org/1076593002/#ps350001
(title: "nits")

[commit-bot: I haz the power, 2015-04-14 19:09:50]

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

[commit-bot: I haz the power, 2015-04-14 19:17:31]

Committed patchset #19 (id:350001) as
https://skia.googlesource.com/skia/+/9e36c1a9306f052331550dab4728b9875127bfb5