Initial CL to add distance field support to GrAtlasTextContext

Initial CL to add distance field support to GrAtlasTextContext

BUG=skia:

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

[joshualitt, 2015-04-14 17:12:40]

joshualitt@google.com changed reviewers:
+ bsalomon@google.com, joshualitt@google.com, jvanverth@google.com,
robertphillips@google.com

[joshualitt, 2015-04-14 17:12:41]

Okay, +1000 lines to an already really complicated file is not ideal.  Let me
know if you guys see more opportunities for code sharing.  Consider this a rough
draft, so feel free to be critical.

[joshualitt, 2015-04-15 20:14:31]

Incorporated Jim's update for the Mali bug.

[bsalomon, 2015-04-16 15:03:56]

https://codereview.chromium.org/1082843002/diff/140001/src/gpu/GrAtlasTextCon...
File src/gpu/GrAtlasTextContext.cpp (right):

https://codereview.chromium.org/1082843002/diff/140001/src/gpu/GrAtlasTextCon...
src/gpu/GrAtlasTextContext.cpp:598: if (canDrawAsDistanceFields(skPaint,
viewMatrix)) {
this->

https://codereview.chromium.org/1082843002/diff/140001/src/gpu/GrAtlasTextCon...
src/gpu/GrAtlasTextContext.cpp:606: SkTArray<char> fallbackTxt;
I believe that when using POD and no prealloc SkTDArray is a little faster.

https://codereview.chromium.org/1082843002/diff/140001/src/gpu/GrAtlasTextCon...
src/gpu/GrAtlasTextContext.cpp:614: this->fallbackDrawPosText(rt, clip, paint,
skPaint, viewMatrix, fallbackTxt, fallbackPos,
Wonder if it is worth checking for empty outside of func to avoid func call in
the common case? (Would want to assert int fDPT that non-empty).

https://codereview.chromium.org/1082843002/diff/140001/src/gpu/GrAtlasTextCon...
src/gpu/GrAtlasTextContext.cpp:639: if (canDrawAsDistanceFields(skPaint,
viewMatrix)) {
this->

https://codereview.chromium.org/1082843002/diff/140001/src/gpu/GrAtlasTextCon...
src/gpu/GrAtlasTextContext.cpp:654: this->fallbackDrawPosText(rt, clip, paint,
skPaint, viewMatrix, fallbackTxt, fallbackPos,
same Q as above

https://codereview.chromium.org/1082843002/diff/140001/src/gpu/GrAtlasTextCon...
src/gpu/GrAtlasTextContext.cpp:1124: this->appendBigGlyph(blob, glyph, scaler,
vx, vy);
appendGlyphPath? appendGlyphAsPath?

https://codereview.chromium.org/1082843002/diff/140001/src/gpu/GrAtlasTextCon...
src/gpu/GrAtlasTextContext.cpp:1164: if (NULL == fCurrStrike) {
can just say if (!fCurrStrike)

https://codereview.chromium.org/1082843002/diff/140001/src/gpu/GrAtlasTextCon...
src/gpu/GrAtlasTextContext.cpp:1169: if (NULL == glyph ||
glyph->fBounds.isEmpty()) {
if (glyph || ...)

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

https://codereview.chromium.org/1082843002/diff/140001/src/gpu/GrAtlasTextCon...
src/gpu/GrAtlasTextContext.h:32: bool enableDistanceFields);
Maybe more a question for Jim: Should DF enable be on SkDeviceProperties? That
may be a dumb Q. I don't really know all the ways in which it is used.

https://codereview.chromium.org/1082843002/diff/140001/src/gpu/GrAtlasTextCon...
src/gpu/GrAtlasTextContext.h:37: ~GrAtlasTextContext() {}
override?

https://codereview.chromium.org/1082843002/diff/140001/src/gpu/GrAtlasTextCon...
src/gpu/GrAtlasTextContext.h:91: struct Run {
How much of Run and nested classes really needs to be defined in the header?

Could some of these be declared here and defined in the cpp?

https://codereview.chromium.org/1082843002/diff/140001/src/gpu/GrAtlasTextCon...
src/gpu/GrAtlasTextContext.h:95: , fDrawAsPaths(false){
I liked that space before "{"

https://codereview.chromium.org/1082843002/diff/140001/src/gpu/GrAtlasTextCon...
src/gpu/GrAtlasTextContext.h:239: BitmapTextBlob* setupDfBlob(size_t glyphCount,
const SkPaint& origPaint,
DFBlob? With few exceptions I think we usually cap all acronym letters (when
they aren't the first part of a name that begins lowercase, e.g. dfPaint).

https://codereview.chromium.org/1082843002/diff/140001/src/gpu/GrAtlasTextCon...
src/gpu/GrAtlasTextContext.h:247: inline void appendBigGlyph(BitmapTextBlob*
blob, GrGlyph* glyph,
Does "big" mean path?

https://codereview.chromium.org/1082843002/diff/140001/src/gpu/GrAtlasTextCon...
src/gpu/GrAtlasTextContext.h:249: inline void innerAppendGlyph(BitmapTextBlob*,
Run*, Run::SubRunInfo*,
appendGlyphCommon?

[jvanverth1, 2015-04-16 15:19:22]

Sending this out, then I'll look again with Brian's comments:

https://codereview.chromium.org/1082843002/diff/140001/src/gpu/GrAtlasTextCon...
File src/gpu/GrAtlasTextContext.cpp (right):

https://codereview.chromium.org/1082843002/diff/140001/src/gpu/GrAtlasTextCon...
src/gpu/GrAtlasTextContext.cpp:600: SkMatrix dfViewMatrix;
Where is dfViewMatrix set?

https://codereview.chromium.org/1082843002/diff/140001/src/gpu/GrAtlasTextCon...
src/gpu/GrAtlasTextContext.cpp:644: SkAutoTUnref<BitmapTextBlob>
blob(this->setupDfBlob(glyphCount, skPaint, viewMatrix, &cache,
It seems odd that setupDfBlob creates a BitmapTextBlob, and then we draw said
blob with internalDrawDfPostTextText

[jvanverth1, 2015-04-16 15:26:54]

More followup comments below. I need to do another pass, but I'm wondering if,
rather than checking a lot of conditionals to switch between DF and bitmap text,
it would be possible to set up function ptrs instead.

https://codereview.chromium.org/1082843002/diff/140001/src/gpu/GrAtlasTextCon...
File src/gpu/GrAtlasTextContext.cpp (right):

https://codereview.chromium.org/1082843002/diff/140001/src/gpu/GrAtlasTextCon...
src/gpu/GrAtlasTextContext.cpp:641: SkMatrix dfViewMatrix;
Again, dfViewMatrix is used without being (to my eye) initialized.

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

https://codereview.chromium.org/1082843002/diff/140001/src/gpu/GrAtlasTextCon...
src/gpu/GrAtlasTextContext.h:32: bool enableDistanceFields);
On 2015/04/16 15:03:55, bsalomon wrote:
> Maybe more a question for Jim: Should DF enable be on SkDeviceProperties? That
> may be a dumb Q. I don't really know all the ways in which it is used.

SkDeviceProperties is tied more to the physical device: gamma and pixel
geometry.

https://codereview.chromium.org/1082843002/diff/140001/src/gpu/GrAtlasTextCon...
src/gpu/GrAtlasTextContext.h:239: BitmapTextBlob* setupDfBlob(size_t glyphCount,
const SkPaint& origPaint,
On 2015/04/16 15:03:55, bsalomon wrote:
> DFBlob? With few exceptions I think we usually cap all acronym letters (when
> they aren't the first part of a name that begins lowercase, e.g. dfPaint).

I agree, DF rather than Df.

[joshualitt, 2015-04-16 15:36:12]

RE Using function pointers:

Jim, I think this is a good idea, but I'd like to profile the difference first. 
Given we usually use either df or bmp text on a given platform, and rarely mix
the two, I think CPU branch prediction is probably going to really good, vs a
function call.  However, it might make the code much clearer.

https://codereview.chromium.org/1082843002/diff/140001/src/gpu/GrAtlasTextCon...
File src/gpu/GrAtlasTextContext.cpp (right):

https://codereview.chromium.org/1082843002/diff/140001/src/gpu/GrAtlasTextCon...
src/gpu/GrAtlasTextContext.cpp:598: if (canDrawAsDistanceFields(skPaint,
viewMatrix)) {
On 2015/04/16 15:03:55, bsalomon wrote:
> this->

Acknowledged.

https://codereview.chromium.org/1082843002/diff/140001/src/gpu/GrAtlasTextCon...
src/gpu/GrAtlasTextContext.cpp:600: SkMatrix dfViewMatrix;
On 2015/04/16 15:19:22, jvanverth1 wrote:
> Where is dfViewMatrix set?

I guess df doesn't use the viewmatrix anymore with the new mali fix, so I've
removed this parameter.  I'm still passing the viewmatrix into drawPosText for
the time being because I'd like to do positioning on the cpu.

https://codereview.chromium.org/1082843002/diff/140001/src/gpu/GrAtlasTextCon...
src/gpu/GrAtlasTextContext.cpp:606: SkTArray<char> fallbackTxt;
On 2015/04/16 15:03:55, bsalomon wrote:
> I believe that when using POD and no prealloc SkTDArray is a little faster.

Acknowledged.

https://codereview.chromium.org/1082843002/diff/140001/src/gpu/GrAtlasTextCon...
src/gpu/GrAtlasTextContext.cpp:614: this->fallbackDrawPosText(rt, clip, paint,
skPaint, viewMatrix, fallbackTxt, fallbackPos,
On 2015/04/16 15:03:55, bsalomon wrote:
> Wonder if it is worth checking for empty outside of func to avoid func call in
> the common case? (Would want to assert int fDPT that non-empty).

Acknowledged.

https://codereview.chromium.org/1082843002/diff/140001/src/gpu/GrAtlasTextCon...
src/gpu/GrAtlasTextContext.cpp:639: if (canDrawAsDistanceFields(skPaint,
viewMatrix)) {
On 2015/04/16 15:03:55, bsalomon wrote:
> this->

Acknowledged.

https://codereview.chromium.org/1082843002/diff/140001/src/gpu/GrAtlasTextCon...
src/gpu/GrAtlasTextContext.cpp:641: SkMatrix dfViewMatrix;
On 2015/04/16 15:26:54, jvanverth1 wrote:
> Again, dfViewMatrix is used without being (to my eye) initialized.

Acknowledged.

https://codereview.chromium.org/1082843002/diff/140001/src/gpu/GrAtlasTextCon...
src/gpu/GrAtlasTextContext.cpp:644: SkAutoTUnref<BitmapTextBlob>
blob(this->setupDfBlob(glyphCount, skPaint, viewMatrix, &cache,
On 2015/04/16 15:19:22, jvanverth1 wrote:
> It seems odd that setupDfBlob creates a BitmapTextBlob, and then we draw said
> blob with internalDrawDfPostTextText

I'll rename the blob after I've landed all of my df patches.  I'm hesitant to do
that now because I have like ~8(much smaller) patches ahead of this one.

https://codereview.chromium.org/1082843002/diff/140001/src/gpu/GrAtlasTextCon...
src/gpu/GrAtlasTextContext.cpp:654: this->fallbackDrawPosText(rt, clip, paint,
skPaint, viewMatrix, fallbackTxt, fallbackPos,
On 2015/04/16 15:03:55, bsalomon wrote:
> same Q as above

Acknowledged.

https://codereview.chromium.org/1082843002/diff/140001/src/gpu/GrAtlasTextCon...
src/gpu/GrAtlasTextContext.cpp:1164: if (NULL == fCurrStrike) {
On 2015/04/16 15:03:55, bsalomon wrote:
> can just say if (!fCurrStrike)

Acknowledged.

https://codereview.chromium.org/1082843002/diff/140001/src/gpu/GrAtlasTextCon...
src/gpu/GrAtlasTextContext.cpp:1169: if (NULL == glyph ||
glyph->fBounds.isEmpty()) {
On 2015/04/16 15:03:55, bsalomon wrote:
> if (glyph || ...)

Acknowledged.

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

https://codereview.chromium.org/1082843002/diff/140001/src/gpu/GrAtlasTextCon...
src/gpu/GrAtlasTextContext.h:37: ~GrAtlasTextContext() {}
On 2015/04/16 15:03:55, bsalomon wrote:
> override?

Acknowledged.

https://codereview.chromium.org/1082843002/diff/140001/src/gpu/GrAtlasTextCon...
src/gpu/GrAtlasTextContext.h:91: struct Run {
On 2015/04/16 15:03:55, bsalomon wrote:
> How much of Run and nested classes really needs to be defined in the header?
> 
> Could some of these be declared here and defined in the cpp?
> 
>  

I think they have to be in the header because they are allocated in
GrTextBlobCache.cpp, and used in the cpp.  I could move this whole thing to its
own .h though

https://codereview.chromium.org/1082843002/diff/140001/src/gpu/GrAtlasTextCon...
src/gpu/GrAtlasTextContext.h:95: , fDrawAsPaths(false){
On 2015/04/16 15:03:55, bsalomon wrote:
> I liked that space before "{"

Acknowledged.

https://codereview.chromium.org/1082843002/diff/140001/src/gpu/GrAtlasTextCon...
src/gpu/GrAtlasTextContext.h:239: BitmapTextBlob* setupDfBlob(size_t glyphCount,
const SkPaint& origPaint,
On 2015/04/16 15:03:55, bsalomon wrote:
> DFBlob? With few exceptions I think we usually cap all acronym letters (when
> they aren't the first part of a name that begins lowercase, e.g. dfPaint).

Acknowledged.

https://codereview.chromium.org/1082843002/diff/140001/src/gpu/GrAtlasTextCon...
src/gpu/GrAtlasTextContext.h:247: inline void appendBigGlyph(BitmapTextBlob*
blob, GrGlyph* glyph,
On 2015/04/16 15:03:55, bsalomon wrote:
> Does "big" mean path?

yes

https://codereview.chromium.org/1082843002/diff/140001/src/gpu/GrAtlasTextCon...
src/gpu/GrAtlasTextContext.h:249: inline void innerAppendGlyph(BitmapTextBlob*,
Run*, Run::SubRunInfo*,
On 2015/04/16 15:03:55, bsalomon wrote:
> appendGlyphCommon?

Acknowledged.

[mtklein, 2015-04-16 15:46:09]

On 2015/04/16 15:36:12, joshualitt wrote:
> RE Using function pointers:
> 
> Jim, I think this is a good idea, but I'd like to profile the difference
first. 
> Given we usually use either df or bmp text on a given platform, and rarely mix
> the two, I think CPU branch prediction is probably going to really good, vs a
> function call.  However, it might make the code much clearer.

Just a note: calls through function pointers can generally be branch predicted
too.  You can thank C++ and vtables for this...

[joshualitt, 2015-04-16 19:37:34]

On 2015/04/16 15:46:09, mtklein wrote:
> On 2015/04/16 15:36:12, joshualitt wrote:
> > RE Using function pointers:
> > 
> > Jim, I think this is a good idea, but I'd like to profile the difference
> first. 
> > Given we usually use either df or bmp text on a given platform, and rarely
mix
> > the two, I think CPU branch prediction is probably going to really good, vs
a
> > function call.  However, it might make the code much clearer.
> 
> Just a note: calls through function pointers can generally be branch predicted
> too.  You can thank C++ and vtables for this...

I think for code quality we should definitely experiment.  In the meantime, the
latest patch should have all feedback so far.

[bsalomon, 2015-04-17 13:23:49]

lgtm, I defer to Jim on the details.

[jvanverth1, 2015-04-17 15:22:51]

On 2015/04/17 13:23:49, bsalomon wrote:
> lgtm, I defer to Jim on the details.

lgtm, too. Just one question.

[jvanverth1, 2015-04-17 15:29:04]

Ah yes, that question:

https://codereview.chromium.org/1082843002/diff/160001/src/gpu/GrAtlasTextCon...
File src/gpu/GrAtlasTextContext.cpp (right):

https://codereview.chromium.org/1082843002/diff/160001/src/gpu/GrAtlasTextCon...
src/gpu/GrAtlasTextContext.cpp:323: const SkMatrix* viewMatrix,
Why was this changed to a pointer?

[joshualitt, 2015-04-17 15:30:30]

https://codereview.chromium.org/1082843002/diff/160001/src/gpu/GrAtlasTextCon...
File src/gpu/GrAtlasTextContext.cpp (right):

https://codereview.chromium.org/1082843002/diff/160001/src/gpu/GrAtlasTextCon...
src/gpu/GrAtlasTextContext.cpp:323: const SkMatrix* viewMatrix,
On 2015/04/17 15:29:04, jvanverth1 wrote:
> Why was this changed to a pointer?

From what I could tell, distanceField text doesn't pass in a VM to get a
scalerContextDescriptor, so this takes a pointer so we can pass NULL into
getScalerContextDescriptor.

[joshualitt, 2015-04-17 15:46:16]

The CQ bit was checked by joshualitt@google.com

[joshualitt, 2015-04-17 15:46:16]

The patchset sent to the CQ was uploaded after l-g-t-m from
jvanverth@google.com, bsalomon@google.com
Link to the patchset: https://codereview.chromium.org/1082843002/#ps180001
(title: "any changes?")

[commit-bot: I haz the power, 2015-04-17 15:46:37]

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

[commit-bot: I haz the power, 2015-04-17 16:01:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-04-17 16:01:51]

Try jobs failed on following builders:
  Build-Win-MSVC-x86-Debug-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86-D...)
  Build-Win-MSVC-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86_6...)

[joshualitt, 2015-04-17 16:22:41]

The CQ bit was checked by joshualitt@google.com

[joshualitt, 2015-04-17 16:22:42]

The patchset sent to the CQ was uploaded after l-g-t-m from
jvanverth@google.com, bsalomon@google.com
Link to the patchset: https://codereview.chromium.org/1082843002/#ps200001
(title: "windows warnigns")

[commit-bot: I haz the power, 2015-04-17 16:24:08]

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

[commit-bot: I haz the power, 2015-04-17 16:30:10]

Committed patchset #11 (id:200001) as
https://skia.googlesource.com/skia/+/9bd2daff8e53a6f02c7e5d1e6aa330f89b8542a3