Handle large paths in textblobs

Handle large paths in textblobs

BUG=skia:

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

[joshualitt, 2015-04-01 18:50:57]

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

[joshualitt, 2015-04-01 18:50:58]

This change adds support for runs of large paths in SkTextBlobs

[bsalomon, 2015-04-02 15:37:24]

more up Jim's alley... I had some small suggestions.

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

https://codereview.chromium.org/1057613002/diff/100001/src/gpu/GrAtlasTextCon...
src/gpu/GrAtlasTextContext.h:61: struct BitmapTextBlob : public SkRefCnt {
Rename to TextBlob?

https://codereview.chromium.org/1057613002/diff/100001/src/gpu/GrAtlasTextCon...
src/gpu/GrAtlasTextContext.h:76: * handle, we have a bit to mark the run as
flusahable via path.  It is worth pointing.
"...flushable via rendering as paths." ?

[joshualitt, 2015-04-02 15:52:18]

On 2015/04/02 15:37:24, bsalomon wrote:
> more up Jim's alley... I had some small suggestions.
> 
>
https://codereview.chromium.org/1057613002/diff/100001/src/gpu/GrAtlasTextCon...
> File src/gpu/GrAtlasTextContext.h (right):
> 
>
https://codereview.chromium.org/1057613002/diff/100001/src/gpu/GrAtlasTextCon...
> src/gpu/GrAtlasTextContext.h:61: struct BitmapTextBlob : public SkRefCnt {
> Rename to TextBlob?
> 
>
https://codereview.chromium.org/1057613002/diff/100001/src/gpu/GrAtlasTextCon...
> src/gpu/GrAtlasTextContext.h:76: * handle, we have a bit to mark the run as
> flusahable via path.  It is worth pointing.
> "...flushable via rendering as paths." ?

comment updated.  I'd prefer to do the rename later, if you don't mind.

[joshualitt, 2015-04-02 15:52:32]

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

[joshualitt, 2015-04-02 15:52:32]

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

https://codereview.chromium.org/1057613002/diff/100001/src/gpu/GrAtlasTextCon...
src/gpu/GrAtlasTextContext.h:61: struct BitmapTextBlob : public SkRefCnt {
On 2015/04/02 15:37:24, bsalomon wrote:
> Rename to TextBlob?

Mind if I do this down the road?  I've got a few CLs in flight and it'd be
annoying to do the merge if I rename now.

https://codereview.chromium.org/1057613002/diff/100001/src/gpu/GrAtlasTextCon...
src/gpu/GrAtlasTextContext.h:76: * handle, we have a bit to mark the run as
flusahable via path.  It is worth pointing.
On 2015/04/02 15:37:24, bsalomon wrote:
> "...flushable via rendering as paths." ?

Acknowledged.

[bsalomon, 2015-04-02 16:06:40]

On 2015/04/02 15:52:32, joshualitt wrote:
>
https://codereview.chromium.org/1057613002/diff/100001/src/gpu/GrAtlasTextCon...
> File src/gpu/GrAtlasTextContext.h (right):
> 
>
https://codereview.chromium.org/1057613002/diff/100001/src/gpu/GrAtlasTextCon...
> src/gpu/GrAtlasTextContext.h:61: struct BitmapTextBlob : public SkRefCnt {
> On 2015/04/02 15:37:24, bsalomon wrote:
> > Rename to TextBlob?

sgtm
> 
> Mind if I do this down the road?  I've got a few CLs in flight and it'd be
> annoying to do the merge if I rename now.
> 
>
https://codereview.chromium.org/1057613002/diff/100001/src/gpu/GrAtlasTextCon...
> src/gpu/GrAtlasTextContext.h:76: * handle, we have a bit to mark the run as
> flusahable via path.  It is worth pointing.
> On 2015/04/02 15:37:24, bsalomon wrote:
> > "...flushable via rendering as paths." ?
> 
> Acknowledged.

[bsalomon, 2015-04-02 16:06:41]

On 2015/04/02 15:52:32, joshualitt wrote:
>
https://codereview.chromium.org/1057613002/diff/100001/src/gpu/GrAtlasTextCon...
> File src/gpu/GrAtlasTextContext.h (right):
> 
>
https://codereview.chromium.org/1057613002/diff/100001/src/gpu/GrAtlasTextCon...
> src/gpu/GrAtlasTextContext.h:61: struct BitmapTextBlob : public SkRefCnt {
> On 2015/04/02 15:37:24, bsalomon wrote:
> > Rename to TextBlob?

sgtm
> 
> Mind if I do this down the road?  I've got a few CLs in flight and it'd be
> annoying to do the merge if I rename now.
> 
>
https://codereview.chromium.org/1057613002/diff/100001/src/gpu/GrAtlasTextCon...
> src/gpu/GrAtlasTextContext.h:76: * handle, we have a bit to mark the run as
> flusahable via path.  It is worth pointing.
> On 2015/04/02 15:37:24, bsalomon wrote:
> > "...flushable via rendering as paths." ?
> 
> Acknowledged.

[jvanverth1, 2015-04-02 19:27:31]

One thought: Now that you're including a subrun of path positions, it's not
clear to me why you're using draw{Pos}TextAsPath. Why not stash the positions
for the next time so you can just rip through them?

[robertphillips, 2015-04-02 20:24:56]

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

https://codereview.chromium.org/1057613002/diff/140001/src/gpu/GrAtlasTextCon...
src/gpu/GrAtlasTextContext.cpp:1017: const SkPoint& offset = it.offset();
Do we need this comment here?

https://codereview.chromium.org/1057613002/diff/140001/src/gpu/GrAtlasTextCon...
src/gpu/GrAtlasTextContext.cpp:1048: for (int subRun = 0; subRun <
cacheBlob->fRuns[run].fSubRunInfo.count(); subRun++) {
const PerSubRunInfo& ?

https://codereview.chromium.org/1057613002/diff/140001/src/gpu/GrAtlasTextCon...
src/gpu/GrAtlasTextContext.cpp:1075: for (int i = 0; i <
cacheBlob->fBigGlyphs.count(); i++) {
const BitmapTextBlob::BigGlyph& ?

https://codereview.chromium.org/1057613002/diff/140001/src/gpu/GrAtlasTextCon...
src/gpu/GrAtlasTextContext.cpp:1106: int run = 0;
Why not "for (int run = 0; ... ?

https://codereview.chromium.org/1057613002/diff/140001/src/gpu/GrAtlasTextCon...
src/gpu/GrAtlasTextContext.cpp:1129: GrColor color = grPaint.getColor();
rm runPaint ?

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

https://codereview.chromium.org/1057613002/diff/140001/src/gpu/GrAtlasTextCon...
src/gpu/GrAtlasTextContext.h:75: * handle, we have a bit to mark the run as
flusahable via rendering as paths.  It is worth
It is worth pointing?

[joshualitt, 2015-04-03 15:37:45]

On 2015/04/02 20:24:56, robertphillips wrote:
>
https://codereview.chromium.org/1057613002/diff/140001/src/gpu/GrAtlasTextCon...
> File src/gpu/GrAtlasTextContext.cpp (right):
> 
>
https://codereview.chromium.org/1057613002/diff/140001/src/gpu/GrAtlasTextCon...
> src/gpu/GrAtlasTextContext.cpp:1017: const SkPoint& offset = it.offset();
> Do we need this comment here?
> 
>
https://codereview.chromium.org/1057613002/diff/140001/src/gpu/GrAtlasTextCon...
> src/gpu/GrAtlasTextContext.cpp:1048: for (int subRun = 0; subRun <
> cacheBlob->fRuns[run].fSubRunInfo.count(); subRun++) {
> const PerSubRunInfo& ?
> 
>
https://codereview.chromium.org/1057613002/diff/140001/src/gpu/GrAtlasTextCon...
> src/gpu/GrAtlasTextContext.cpp:1075: for (int i = 0; i <
> cacheBlob->fBigGlyphs.count(); i++) {
> const BitmapTextBlob::BigGlyph& ?
> 
>
https://codereview.chromium.org/1057613002/diff/140001/src/gpu/GrAtlasTextCon...
> src/gpu/GrAtlasTextContext.cpp:1106: int run = 0;
> Why not "for (int run = 0; ... ?
> 
>
https://codereview.chromium.org/1057613002/diff/140001/src/gpu/GrAtlasTextCon...
> src/gpu/GrAtlasTextContext.cpp:1129: GrColor color = grPaint.getColor();
> rm runPaint ?
> 
>
https://codereview.chromium.org/1057613002/diff/140001/src/gpu/GrAtlasTextCon...
> File src/gpu/GrAtlasTextContext.h (right):
> 
>
https://codereview.chromium.org/1057613002/diff/140001/src/gpu/GrAtlasTextCon...
> src/gpu/GrAtlasTextContext.h:75: * handle, we have a bit to mark the run as
> flusahable via rendering as paths.  It is worth
> It is worth pointing?

Feedback incorporated.  After talking it over with Jim, we've decided to punt on
optimizing large text runs.  I've added a comment indicating it might be worth
exploring, but it seems really low priority at this time.

[joshualitt, 2015-04-06 14:03:10]

Anyone have any other comments on this CL?

[jvanverth1, 2015-04-06 17:09:26]

lgtm

[joshualitt, 2015-04-06 17:46:27]

The CQ bit was checked by joshualitt@google.com

[commit-bot: I haz the power, 2015-04-06 17:46:33]

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

[commit-bot: I haz the power, 2015-04-06 17:53:41]

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