Avoid regenerating cached textblobs on integer scrolls

Avoid regenerating cached textblobs on integer scrolls

BUG=skia:

Committed: https://skia.googlesource.com/skia/+/2a0e9f36c6e823d2c0472ef23b4c5a247427807f

[joshualitt, 2015-04-06 19:23:08]

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

[joshualitt, 2015-04-06 19:23:42]

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

[joshualitt, 2015-04-06 19:23:43]

Take a close look at this.  I'm sure I made mistakes.  I'm going to add a GM for
the filters stuff because it exposed bugs in this implementation.

[bsalomon, 2015-04-08 15:43:00]

Does this change include bits of other CLs too? Wondering if it was uploaded
with the wrong tracking branch.

https://codereview.chromium.org/1062863002/diff/60001/src/gpu/GrAtlasTextCont...
File src/gpu/GrAtlasTextContext.cpp (right):

https://codereview.chromium.org/1062863002/diff/60001/src/gpu/GrAtlasTextCont...
src/gpu/GrAtlasTextContext.cpp:132: SkScalar transX = viewMatrix.getTranslateX()
+
Do we need to worry about presence/absence of subpixel pos?

https://codereview.chromium.org/1062863002/diff/60001/src/gpu/GrAtlasTextCont...
src/gpu/GrAtlasTextContext.cpp:143: (*outTransY) = transY;
why have intermediates?

[joshualitt, 2015-04-08 15:52:28]

Whoops, sorry, there was some noise in this CL, should be cleaned up now.

https://codereview.chromium.org/1062863002/diff/60001/src/gpu/GrAtlasTextCont...
File src/gpu/GrAtlasTextContext.cpp (right):

https://codereview.chromium.org/1062863002/diff/60001/src/gpu/GrAtlasTextCont...
src/gpu/GrAtlasTextContext.cpp:132: SkScalar transX = viewMatrix.getTranslateX()
+
On 2015/04/08 15:42:59, bsalomon wrote:
> Do we need to worry about presence/absence of subpixel pos?

Jim, thoughts?  I don't really know enough about subpixel text to have an
informed opinion.

https://codereview.chromium.org/1062863002/diff/60001/src/gpu/GrAtlasTextCont...
src/gpu/GrAtlasTextContext.cpp:143: (*outTransY) = transY;
On 2015/04/08 15:43:00, bsalomon wrote:
> why have intermediates?

done

[robertphillips, 2015-04-09 13:15:32]

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

https://codereview.chromium.org/1062863002/diff/100001/src/gpu/GrAtlasTextCon...
src/gpu/GrAtlasTextContext.cpp:93: 
kMinDiscernableTranslation ?
Can this be moved into MustRegenerateBlob?

https://codereview.chromium.org/1062863002/diff/100001/src/gpu/GrAtlasTextCon...
src/gpu/GrAtlasTextContext.cpp:121: // only for integer translations.
That's an ominous todo
Shouldn't this be something more like:

(newX - oldX) > kMinDiscernableTranslation ||
(newY - oldY) > kMinDiscernableTranslation

?

https://codereview.chromium.org/1062863002/diff/100001/src/gpu/GrAtlasTextCon...
src/gpu/GrAtlasTextContext.cpp:131: 
// This cool math does the following ...
?

https://codereview.chromium.org/1062863002/diff/100001/src/gpu/GrAtlasTextCon...
src/gpu/GrAtlasTextContext.cpp:159: // TODO start caching these, mix bits into
the key
Would this be more natural as "canCache" ?

[joshualitt, 2015-04-09 14:59:00]

feedback inc

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

https://codereview.chromium.org/1062863002/diff/100001/src/gpu/GrAtlasTextCon...
src/gpu/GrAtlasTextContext.cpp:93: 
On 2015/04/09 13:15:32, robertphillips wrote:
> kMinDiscernableTranslation ?
> Can this be moved into MustRegenerateBlob?

Acknowledged.

https://codereview.chromium.org/1062863002/diff/100001/src/gpu/GrAtlasTextCon...
src/gpu/GrAtlasTextContext.cpp:121: // only for integer translations.
On 2015/04/09 13:15:32, robertphillips wrote:
> That's an ominous todo
> Shouldn't this be something more like:
> 
> (newX - oldX) > kMinDiscernableTranslation ||
> (newY - oldY) > kMinDiscernableTranslation
> 
> ?

I took another stab at it.  I *think* I can allow arbitrary integer
translations, as long as the fractional part is < kMinDiscernableTranslation.

https://codereview.chromium.org/1062863002/diff/100001/src/gpu/GrAtlasTextCon...
src/gpu/GrAtlasTextContext.cpp:131: 
On 2015/04/09 13:15:32, robertphillips wrote:
> // This cool math does the following ...
> ?

Acknowledged.

https://codereview.chromium.org/1062863002/diff/100001/src/gpu/GrAtlasTextCon...
src/gpu/GrAtlasTextContext.cpp:159: // TODO start caching these, mix bits into
the key
On 2015/04/09 13:15:32, robertphillips wrote:
> Would this be more natural as "canCache" ?

Acknowledged.

[joshualitt, 2015-04-09 15:40:48]

Feedback incorporated

[joshualitt, 2015-04-10 15:05:04]

On 2015/04/09 15:40:48, joshualitt wrote:
> Feedback incorporated

Jim, any feedback on this CL?

[jvanverth1, 2015-04-10 15:46:31]

In general it looks okay, just one concern and one comment.

https://codereview.chromium.org/1062863002/diff/320001/src/gpu/GrAtlasTextCon...
File src/gpu/GrAtlasTextContext.cpp (left):

https://codereview.chromium.org/1062863002/diff/320001/src/gpu/GrAtlasTextCon...
src/gpu/GrAtlasTextContext.cpp:539: // check if we clipped out
I recall a discussion about removing this, but upon reflection, I'm concerned.
This can be a big perf regression on some pages. One thing that got distance
fields closer to bitmap text perf was adding this check. Have you tried running
a comparison in Clankium with
repaint.gpu_rasterization.key_mobile_sites_repaint?

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

https://codereview.chromium.org/1062863002/diff/320001/src/gpu/GrAtlasTextCon...
src/gpu/GrAtlasTextContext.cpp:136: #ifdef SK_DEBUG
A comment on what this all means would be useful (e.g., debug output for
Chrome).

[joshualitt, 2015-04-10 18:17:00]

On 2015/04/10 15:46:31, jvanverth1 wrote:
> In general it looks okay, just one concern and one comment.
> 
>
https://codereview.chromium.org/1062863002/diff/320001/src/gpu/GrAtlasTextCon...
> File src/gpu/GrAtlasTextContext.cpp (left):
> 
>
https://codereview.chromium.org/1062863002/diff/320001/src/gpu/GrAtlasTextCon...
> src/gpu/GrAtlasTextContext.cpp:539: // check if we clipped out
> I recall a discussion about removing this, but upon reflection, I'm concerned.
> This can be a big perf regression on some pages. One thing that got distance
> fields closer to bitmap text perf was adding this check. Have you tried
running
> a comparison in Clankium with
> repaint.gpu_rasterization.key_mobile_sites_repaint?
> 
>
https://codereview.chromium.org/1062863002/diff/320001/src/gpu/GrAtlasTextCon...
> File src/gpu/GrAtlasTextContext.cpp (right):
> 
>
https://codereview.chromium.org/1062863002/diff/320001/src/gpu/GrAtlasTextCon...
> src/gpu/GrAtlasTextContext.cpp:136: #ifdef SK_DEBUG
> A comment on what this all means would be useful (e.g., debug output for
> Chrome).

Okay, comment added, and I reintroduced the clip rect test behind a flag, until
I verify the performance impact.  Anything else?

[jvanverth1, 2015-04-13 12:50:59]

lgtm

[joshualitt, 2015-04-13 12:51:47]

The CQ bit was checked by joshualitt@google.com

[commit-bot: I haz the power, 2015-04-13 12:52:00]

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

[commit-bot: I haz the power, 2015-04-13 12:56:02]

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

[commit-bot: I haz the power, 2015-04-13 12:56:03]

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-13 13:05:15]

The CQ bit was checked by joshualitt@google.com

[joshualitt, 2015-04-13 13:05:15]

The patchset sent to the CQ was uploaded after l-g-t-m from jvanverth@google.com
Link to the patchset: https://codereview.chromium.org/1062863002/#ps420001
(title: "tweak")

[commit-bot: I haz the power, 2015-04-13 13:05:24]

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

[commit-bot: I haz the power, 2015-04-13 13:12:25]

Committed patchset #22 (id:420001) as
https://skia.googlesource.com/skia/+/2a0e9f36c6e823d2c0472ef23b4c5a247427807f