Fix GrAtlasTextBlob bounds management

Fix GrAtlasTextBlob bounds management

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1605013002

Committed: https://skia.googlesource.com/skia/+/ae473fdfc3db2d9dd82b05b2568767d6d4038fcd

Committed: https://skia.googlesource.com/skia/+/7481e75830dfae1e9084ff62e3d8438879e31aaa

[joshua.litt, 2016-01-19 16:47:17]

Description was changed from

==========
Fix GrAtlasTextBlob bounds management

BUG=skia:
==========

to

==========
Fix GrAtlasTextBlob bounds management

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[joshualitt, 2016-01-19 21:02:51]

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

[joshualitt, 2016-01-19 21:02:52]

Take a close look at this one, make sure it makes sense.  Brian you might want
to try your patch with this to see if it helps.

[bsalomon, 2016-01-20 16:32:29]

https://codereview.chromium.org/1605013002/diff/100001/src/gpu/text/GrAtlasTe...
File src/gpu/text/GrAtlasTextBlob.h (right):

https://codereview.chromium.org/1605013002/diff/100001/src/gpu/text/GrAtlasTe...
src/gpu/text/GrAtlasTextBlob.h:250: void setupViewMatrix(const SkMatrix&
viewMatrix, SkScalar x, SkScalar y) {
what are x and y here?

https://codereview.chromium.org/1605013002/diff/100001/src/gpu/text/GrAtlasTe...
src/gpu/text/GrAtlasTextBlob.h:253: fInitialViewMatrixInverse = SkMatrix::I();
Is this really initial if we update it every time this is called? Seems like it
is always the inverse of fViewMatrix.

[joshualitt, 2016-01-20 16:43:50]

https://codereview.chromium.org/1605013002/diff/100001/src/gpu/text/GrAtlasTe...
File src/gpu/text/GrAtlasTextBlob.h (right):

https://codereview.chromium.org/1605013002/diff/100001/src/gpu/text/GrAtlasTe...
src/gpu/text/GrAtlasTextBlob.h:250: void setupViewMatrix(const SkMatrix&
viewMatrix, SkScalar x, SkScalar y) {
On 2016/01/20 16:32:29, bsalomon wrote:
> what are x and y here?

I'm not 100% sure myself, but drawTextBlob(blob, x, y), for example.  They
become part of the masks.  I think its the offsets passed to the drawText call

https://codereview.chromium.org/1605013002/diff/100001/src/gpu/text/GrAtlasTe...
src/gpu/text/GrAtlasTextBlob.h:253: fInitialViewMatrixInverse = SkMatrix::I();
On 2016/01/20 16:32:29, bsalomon wrote:
> Is this really initial if we update it every time this is called? Seems like
it
> is always the inverse of fViewMatrix.

We only call setupViewMatrix when we fully regenerate the blob.  Otherwise, we
just update fViewMatrix and fX / fY directly.

[bsalomon, 2016-01-20 16:51:41]

https://codereview.chromium.org/1605013002/diff/100001/src/gpu/text/GrAtlasTe...
File src/gpu/text/GrAtlasTextBlob.h (right):

https://codereview.chromium.org/1605013002/diff/100001/src/gpu/text/GrAtlasTe...
src/gpu/text/GrAtlasTextBlob.h:250: void setupViewMatrix(const SkMatrix&
viewMatrix, SkScalar x, SkScalar y) {
On 2016/01/20 16:43:50, joshualitt wrote:
> On 2016/01/20 16:32:29, bsalomon wrote:
> > what are x and y here?
> 
> I'm not 100% sure myself, but drawTextBlob(blob, x, y), for example.  They
> become part of the masks.  I think its the offsets passed to the drawText call

I see, that makes sense. Add a comment?

https://codereview.chromium.org/1605013002/diff/100001/src/gpu/text/GrAtlasTe...
src/gpu/text/GrAtlasTextBlob.h:253: fInitialViewMatrixInverse = SkMatrix::I();
On 2016/01/20 16:43:50, joshualitt wrote:
> On 2016/01/20 16:32:29, bsalomon wrote:
> > Is this really initial if we update it every time this is called? Seems like
> it
> > is always the inverse of fViewMatrix.
> 
> We only call setupViewMatrix when we fully regenerate the blob.  Otherwise, we
> just update fViewMatrix and fX / fY directly.

Ok it'd be nice for that to be clearer. Maybe a comment?

[joshualitt, 2016-01-20 17:49:16]

On 2016/01/20 16:51:41, bsalomon wrote:
>
https://codereview.chromium.org/1605013002/diff/100001/src/gpu/text/GrAtlasTe...
> File src/gpu/text/GrAtlasTextBlob.h (right):
> 
>
https://codereview.chromium.org/1605013002/diff/100001/src/gpu/text/GrAtlasTe...
> src/gpu/text/GrAtlasTextBlob.h:250: void setupViewMatrix(const SkMatrix&
> viewMatrix, SkScalar x, SkScalar y) {
> On 2016/01/20 16:43:50, joshualitt wrote:
> > On 2016/01/20 16:32:29, bsalomon wrote:
> > > what are x and y here?
> > 
> > I'm not 100% sure myself, but drawTextBlob(blob, x, y), for example.  They
> > become part of the masks.  I think its the offsets passed to the drawText
call
> 
> I see, that makes sense. Add a comment?
> 
>
https://codereview.chromium.org/1605013002/diff/100001/src/gpu/text/GrAtlasTe...
> src/gpu/text/GrAtlasTextBlob.h:253: fInitialViewMatrixInverse = SkMatrix::I();
> On 2016/01/20 16:43:50, joshualitt wrote:
> > On 2016/01/20 16:32:29, bsalomon wrote:
> > > Is this really initial if we update it every time this is called? Seems
like
> > it
> > > is always the inverse of fViewMatrix.
> > 
> > We only call setupViewMatrix when we fully regenerate the blob.  Otherwise,
we
> > just update fViewMatrix and fX / fY directly.
> 
> Ok it'd be nice for that to be clearer. Maybe a comment?

comment added.

[bsalomon, 2016-01-20 18:33:44]

lgtm

[joshualitt, 2016-01-20 20:22:48]

The CQ bit was checked by joshualitt@google.com

[commit-bot: I haz the power, 2016-01-20 20:22:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1605013002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1605013002/140001

[commit-bot: I haz the power, 2016-01-20 20:35:25]

Description was changed from

==========
Fix GrAtlasTextBlob bounds management

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
Fix GrAtlasTextBlob bounds management

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...

Committed:
https://skia.googlesource.com/skia/+/ae473fdfc3db2d9dd82b05b2568767d6d4038fcd
==========

[commit-bot: I haz the power, 2016-01-20 20:35:25]

Committed patchset #8 (id:140001) as
https://skia.googlesource.com/skia/+/ae473fdfc3db2d9dd82b05b2568767d6d4038fcd

[joshualitt, 2016-01-20 21:54:18]

A revert of this CL (patchset #8 id:140001) has been created in
https://codereview.chromium.org/1615473002/ by joshualitt@google.com.

The reason for reverting is: seems to break upright matrix.

[joshualitt, 2016-01-21 16:20:56]

Description was changed from

==========
Fix GrAtlasTextBlob bounds management

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...

Committed:
https://skia.googlesource.com/skia/+/ae473fdfc3db2d9dd82b05b2568767d6d4038fcd
==========

to

==========
Fix GrAtlasTextBlob bounds management

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...

Committed:
https://skia.googlesource.com/skia/+/ae473fdfc3db2d9dd82b05b2568767d6d4038fcd
==========

[joshualitt, 2016-01-21 16:21:08]

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

[joshualitt, 2016-01-21 16:21:25]

okay, this should be fixed now.  PTAL, adding Robert because he helped me work
this out.

[joshualitt, 2016-01-21 16:24:23]

Patchset #9 (id:160001) has been deleted

[robertphillips, 2016-01-21 17:46:44]

looks reasonable to me

https://codereview.chromium.org/1605013002/diff/180001/src/gpu/batches/GrAtla...
File src/gpu/batches/GrAtlasTextBatch.h (right):

https://codereview.chromium.org/1605013002/diff/180001/src/gpu/batches/GrAtla...
src/gpu/batches/GrAtlasTextBatch.h:104: 
boundsMatrix.postTranslate(-geo.fBlob->fInitialX, -geo.fBlob->fInitialY);

?

https://codereview.chromium.org/1605013002/diff/180001/src/gpu/batches/GrAtla...
src/gpu/batches/GrAtlasTextBatch.h:108: 
same here ?

[joshualitt, 2016-01-21 18:39:55]

The CQ bit was checked by joshualitt@google.com

[joshualitt, 2016-01-21 18:39:55]

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

[commit-bot: I haz the power, 2016-01-21 18:40:07]

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

[joshualitt, 2016-01-21 18:49:41]

The CQ bit was unchecked by joshualitt@google.com

[joshualitt, 2016-01-21 20:55:00]

The CQ bit was checked by joshualitt@google.com

[joshualitt, 2016-01-21 20:55:00]

The patchset sent to the CQ was uploaded after l-g-t-m from bsalomon@google.com
Link to the patchset: https://codereview.chromium.org/1605013002/#ps220001
(title: "fix glprograms")

[commit-bot: I haz the power, 2016-01-21 20:55:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1605013002/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1605013002/220001

[joshualitt, 2016-01-21 21:43:00]

The CQ bit was unchecked by joshualitt@google.com

[joshualitt, 2016-01-22 14:08:03]

The CQ bit was checked by joshualitt@google.com

[commit-bot: I haz the power, 2016-01-22 14:08:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1605013002/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1605013002/220001

[commit-bot: I haz the power, 2016-01-22 14:08:51]

Description was changed from

==========
Fix GrAtlasTextBlob bounds management

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...

Committed:
https://skia.googlesource.com/skia/+/ae473fdfc3db2d9dd82b05b2568767d6d4038fcd
==========

to

==========
Fix GrAtlasTextBlob bounds management

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...

Committed:
https://skia.googlesource.com/skia/+/ae473fdfc3db2d9dd82b05b2568767d6d4038fcd

Committed:
https://skia.googlesource.com/skia/+/7481e75830dfae1e9084ff62e3d8438879e31aaa
==========

[commit-bot: I haz the power, 2016-01-22 14:08:52]

Committed patchset #11 (id:220001) as
https://skia.googlesource.com/skia/+/7481e75830dfae1e9084ff62e3d8438879e31aaa