GPU Font Cache improvements:

GPU Font Cache improvements:
- If a strike has multiple atlases, check all for room for a new glyph
- Mark remaining atlases unused after a purge, then check for an unused
atlas before purging (reduces TextContext flushes and ghosting)
- Hide Atlas management a little better inside AtlasMgr

Committed: http://code.google.com/p/skia/source/detail?r=10544

[robertphillips, 2013-08-02 15:23:50]

lgtm afai understand it

https://codereview.chromium.org/21594005/diff/3001/src/gpu/GrAtlas.h
File src/gpu/GrAtlas.h (right):

https://codereview.chromium.org/21594005/diff/3001/src/gpu/GrAtlas.h#newcode41
src/gpu/GrAtlas.h:41: 
Does this need to be out here with the availability of nextAtlas?

https://codereview.chromium.org/21594005/diff/3001/src/gpu/GrTextStrike.cpp
File src/gpu/GrTextStrike.cpp (right):

https://codereview.chromium.org/21594005/diff/3001/src/gpu/GrTextStrike.cpp#n...
src/gpu/GrTextStrike.cpp:173: 
invalidate_glyph?
GrGlyph*?

https://codereview.chromium.org/21594005/diff/3001/src/gpu/GrTextStrike.cpp#n...
src/gpu/GrTextStrike.cpp:204: bool GrTextStrike::removeUnusedAtlas() {
Why is prev a GrAtlas**?

https://codereview.chromium.org/21594005/diff/3001/src/gpu/GrTextStrike.cpp#n...
src/gpu/GrTextStrike.cpp:224: GrAtlas* atlas = fAtlas;
NULL !=?

[bsalomon, 2013-08-02 15:30:35]

https://codereview.chromium.org/21594005/diff/3001/src/gpu/GrAtlas.h
File src/gpu/GrAtlas.h (right):

https://codereview.chromium.org/21594005/diff/3001/src/gpu/GrAtlas.h#newcode41
src/gpu/GrAtlas.h:41: 
On 2013/08/02 15:23:50, robertphillips wrote:
> Does this need to be out here with the availability of nextAtlas?

I'd feel better if this and fUsed were private, even if they have dumb set/get
wrappers.

https://codereview.chromium.org/21594005/diff/3001/src/gpu/GrTextStrike.cpp
File src/gpu/GrTextStrike.cpp (right):

https://codereview.chromium.org/21594005/diff/3001/src/gpu/GrTextStrike.cpp#n...
src/gpu/GrTextStrike.cpp:173: 
On 2013/08/02 15:23:50, robertphillips wrote:
> invalidate_glyph?
> GrGlyph*?

ref to a ptr feels weird to me. We aren't assigning to glyph so does the ref
matter?

[jvanverth1, 2013-08-02 21:49:27]

In addition to the changes below, I also removed the multiple PlotMgrs. I
discovered some differences in GMs I hadn't seen before. It doesn't appear to
affect the reduction in flushes much, and I'll fix it in a later change.

https://codereview.chromium.org/21594005/diff/3001/src/gpu/GrAtlas.h
File src/gpu/GrAtlas.h (right):

https://codereview.chromium.org/21594005/diff/3001/src/gpu/GrAtlas.h#newcode41
src/gpu/GrAtlas.h:41: 
On 2013/08/02 15:30:35, bsalomon wrote:
> On 2013/08/02 15:23:50, robertphillips wrote:
> > Does this need to be out here with the availability of nextAtlas?
> 
> I'd feel better if this and fUsed were private, even if they have dumb set/get
> wrappers.

Done.

https://codereview.chromium.org/21594005/diff/3001/src/gpu/GrTextStrike.cpp
File src/gpu/GrTextStrike.cpp (right):

https://codereview.chromium.org/21594005/diff/3001/src/gpu/GrTextStrike.cpp#n...
src/gpu/GrTextStrike.cpp:173: 
The ref is needed because this function is used in visitAll(), below.

On 2013/08/02 15:30:35, bsalomon wrote:
> On 2013/08/02 15:23:50, robertphillips wrote:
> > invalidate_glyph?
> > GrGlyph*?
> 
> ref to a ptr feels weird to me. We aren't assigning to glyph so does the ref
> matter?

https://codereview.chromium.org/21594005/diff/3001/src/gpu/GrTextStrike.cpp#n...
src/gpu/GrTextStrike.cpp:204: bool GrTextStrike::removeUnusedAtlas() {
On 2013/08/02 15:23:50, robertphillips wrote:
> Why is prev a GrAtlas**?

Moved to GrAtlas.cpp, but that's done so I don't have to check whether we're at
the head of the list or not. I renamed it.

https://codereview.chromium.org/21594005/diff/3001/src/gpu/GrTextStrike.cpp#n...
src/gpu/GrTextStrike.cpp:224: GrAtlas* atlas = fAtlas;
On 2013/08/02 15:23:50, robertphillips wrote:
> NULL !=?

Done.

[bsalomon, 2013-08-05 13:09:54]

lgtm

https://codereview.chromium.org/21594005/diff/3001/src/gpu/GrTextStrike.cpp
File src/gpu/GrTextStrike.cpp (right):

https://codereview.chromium.org/21594005/diff/3001/src/gpu/GrTextStrike.cpp#n...
src/gpu/GrTextStrike.cpp:173: 
On 2013/08/02 21:49:27, JimVV wrote:
> The ref is needed because this function is used in visitAll(), below.

Gotcha, maybe a comment // this is used with SkTDArray::vistAll()
? Otherwise the signature will look odd I to a reader I think.


> 
> On 2013/08/02 15:30:35, bsalomon wrote:
> > On 2013/08/02 15:23:50, robertphillips wrote:
> > > invalidate_glyph?
> > > GrGlyph*?
> > 
> > ref to a ptr feels weird to me. We aren't assigning to glyph so does the ref
> > matter?
>

[robertphillips, 2013-08-05 15:11:40]

lgtm + 1 question & 1 suggestion

https://codereview.chromium.org/21594005/diff/27001/src/gpu/GrAtlas.cpp
File src/gpu/GrAtlas.cpp (right):

https://codereview.chromium.org/21594005/diff/27001/src/gpu/GrAtlas.cpp#newco...
src/gpu/GrAtlas.cpp:83: bool GrAtlas::RemoveUnusedAtlases(GrAtlasMgr* atlasMgr,
GrAtlas** startAtlas) {
// add comment here why a **?

https://codereview.chromium.org/21594005/diff/27001/src/gpu/GrAtlas.h
File src/gpu/GrAtlas.h (right):

https://codereview.chromium.org/21594005/diff/27001/src/gpu/GrAtlas.h#newcode47
src/gpu/GrAtlas.h:47: 
const?

[jvanverth1, 2013-08-05 19:34:23]

On 2013/08/05 13:09:54, bsalomon wrote:
> lgtm
> 
> https://codereview.chromium.org/21594005/diff/3001/src/gpu/GrTextStrike.cpp
> File src/gpu/GrTextStrike.cpp (right):
> 
>
https://codereview.chromium.org/21594005/diff/3001/src/gpu/GrTextStrike.cpp#n...
> src/gpu/GrTextStrike.cpp:173: 
> On 2013/08/02 21:49:27, JimVV wrote:
> > The ref is needed because this function is used in visitAll(), below.
> 
> Gotcha, maybe a comment // this is used with SkTDArray::vistAll()
> ? Otherwise the signature will look odd I to a reader I think.

Done.

[jvanverth1, 2013-08-05 19:34:40]

On 2013/08/05 15:11:40, robertphillips wrote:
> lgtm + 1 question & 1 suggestion
> 
> https://codereview.chromium.org/21594005/diff/27001/src/gpu/GrAtlas.cpp
> File src/gpu/GrAtlas.cpp (right):
> 
>
https://codereview.chromium.org/21594005/diff/27001/src/gpu/GrAtlas.cpp#newco...
> src/gpu/GrAtlas.cpp:83: bool GrAtlas::RemoveUnusedAtlases(GrAtlasMgr*
atlasMgr,
> GrAtlas** startAtlas) {
> // add comment here why a **?

Done.

> https://codereview.chromium.org/21594005/diff/27001/src/gpu/GrAtlas.h
> File src/gpu/GrAtlas.h (right):
> 
>
https://codereview.chromium.org/21594005/diff/27001/src/gpu/GrAtlas.h#newcode47
> src/gpu/GrAtlas.h:47: 
> const?

Done.

[commit-bot: I haz the power, 2013-08-05 19:34:52]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/jvanverth@google.com/21594005/34001

[commit-bot: I haz the power, 2013-08-05 19:43:00]

Change committed as 10544