Send less transform data when drawing text with nvpr

Send less transform data when drawing text with nvpr

Before this change, GrStencilAndCoverTextContext would send 6-float
affine transforms to drawPaths for every glyph. This updates it to
concat the text scale onto the context matrix, and then only send
2-float translates (or 1-float x-translates when possible).

Also adds a glyph_pos_align test to gm that exercises the newly added
codepaths, and starts ignoring a few gm tests with benign pixel diffs
until we can rebaseline.

BUG=skia:

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

[Chris Dalton, 2014-07-18 22:23:30]

https://codereview.chromium.org/406523003/diff/1/src/gpu/GrStencilAndCoverTex...
File src/gpu/GrStencilAndCoverTextContext.cpp (right):

https://codereview.chromium.org/406523003/diff/1/src/gpu/GrStencilAndCoverTex...
src/gpu/GrStencilAndCoverTextContext.cpp:266: } else {
We could do a translate-x here too if we could verify every glyph's advanceY
will be zero. That seems to be the usual case, is there a way to know they will
all be zero?

[bsalomon, 2014-07-21 14:13:14]

Adding Jim.

[jvanverth1, 2014-07-21 17:23:13]

lgtm + 1 comment

https://codereview.chromium.org/406523003/diff/1/src/gpu/GrStencilAndCoverTex...
File src/gpu/GrStencilAndCoverTextContext.cpp (right):

https://codereview.chromium.org/406523003/diff/1/src/gpu/GrStencilAndCoverTex...
src/gpu/GrStencilAndCoverTextContext.cpp:266: } else {
On 2014/07/18 22:23:30, Chris Dalton wrote:
> We could do a translate-x here too if we could verify every glyph's advanceY
> will be zero. That seems to be the usual case, is there a way to know they
will
> all be zero?

Not sure -- adding bungeman to see if he knows.

[jvanverth1, 2014-07-21 17:37:45]

On 2014/07/21 17:23:13, jvanverth1 wrote:
> lgtm + 1 comment
> 
>
https://codereview.chromium.org/406523003/diff/1/src/gpu/GrStencilAndCoverTex...
> File src/gpu/GrStencilAndCoverTextContext.cpp (right):
> 
>
https://codereview.chromium.org/406523003/diff/1/src/gpu/GrStencilAndCoverTex...
> src/gpu/GrStencilAndCoverTextContext.cpp:266: } else {
> On 2014/07/18 22:23:30, Chris Dalton wrote:
> > We could do a translate-x here too if we could verify every glyph's advanceY
> > will be zero. That seems to be the usual case, is there a way to know they
> will
> > all be zero?
> 
> Not sure -- adding bungeman to see if he knows.

Actually adding bungeman this time...

[bungeman-skia, 2014-07-21 17:51:28]

https://codereview.chromium.org/406523003/diff/1/src/gpu/GrStencilAndCoverTex...
File src/gpu/GrStencilAndCoverTextContext.cpp (right):

https://codereview.chromium.org/406523003/diff/1/src/gpu/GrStencilAndCoverTex...
src/gpu/GrStencilAndCoverTextContext.cpp:266: } else {
On 2014/07/18 22:23:30, Chris Dalton wrote:
> We could do a translate-x here too if we could verify every glyph's advanceY
> will be zero. That seems to be the usual case, is there a way to know they
will
> all be zero?

I'm not sure I understand. You've used scalersPerPosition above which when 1
means there are just x advances. Is that what you want? It's an odd way to
phrase the question as the glyph advances don't really mean anything here, its
just glyphs at positions (the advances were probably used to generate those
positions, but they might not have been).

[Chris Dalton, 2014-07-21 18:03:27]

On 2014/07/21 17:51:28, bungeman1 wrote:
>
https://codereview.chromium.org/406523003/diff/1/src/gpu/GrStencilAndCoverTex...
> File src/gpu/GrStencilAndCoverTextContext.cpp (right):
> 
>
https://codereview.chromium.org/406523003/diff/1/src/gpu/GrStencilAndCoverTex...
> src/gpu/GrStencilAndCoverTextContext.cpp:266: } else {
> On 2014/07/18 22:23:30, Chris Dalton wrote:
> > We could do a translate-x here too if we could verify every glyph's advanceY
> > will be zero. That seems to be the usual case, is there a way to know they
> will
> > all be zero?
> 
> I'm not sure I understand. You've used scalersPerPosition above which when 1
> means there are just x advances. Is that what you want? It's an odd way to
> phrase the question as the glyph advances don't really mean anything here, its
> just glyphs at positions (the advances were probably used to generate those
> positions, but they might not have been).

If you look at SkTextAlignProcScalar, it does the following:

    void operator()(const SkPoint& loc, const SkGlyph& glyph, SkPoint* dst) {
        if (SkPaint::kLeft_Align == fAlign) {
            dst->set(loc.fX, loc.fY);
        } else if (SkPaint::kCenter_Align == fAlign) {
            dst->set(loc.fX - SkFixedToScalar(glyph.fAdvanceX >> 1),
                     loc.fY - SkFixedToScalar(glyph.fAdvanceY >> 1));
        } else {
            SkASSERT(SkPaint::kRight_Align == fAlign);
            dst->set(loc.fX - SkFixedToScalar(glyph.fAdvanceX),
                     loc.fY - SkFixedToScalar(glyph.fAdvanceY));
        }
    }

So even if scalarsPerPosition is 1, the y-coordinate the gpu needs to draw each
glyph at will not be constant when align is not left and the glyphs have
different y-advances. A nonzero y-advance is rare though, so it seems like there
could be a quick check that detects 99% of right and center aligned cases that
can be drawn with just TranslateX.

[bungeman-skia, 2014-07-21 18:50:51]

On 2014/07/21 18:03:27, Chris Dalton wrote:
> On 2014/07/21 17:51:28, bungeman1 wrote:
> >
>
https://codereview.chromium.org/406523003/diff/1/src/gpu/GrStencilAndCoverTex...
> > File src/gpu/GrStencilAndCoverTextContext.cpp (right):
> > 
> >
>
https://codereview.chromium.org/406523003/diff/1/src/gpu/GrStencilAndCoverTex...
> > src/gpu/GrStencilAndCoverTextContext.cpp:266: } else {
> > On 2014/07/18 22:23:30, Chris Dalton wrote:
> > > We could do a translate-x here too if we could verify every glyph's
advanceY
> > > will be zero. That seems to be the usual case, is there a way to know they
> > will
> > > all be zero?
> > 
> > I'm not sure I understand. You've used scalersPerPosition above which when 1
> > means there are just x advances. Is that what you want? It's an odd way to
> > phrase the question as the glyph advances don't really mean anything here,
its
> > just glyphs at positions (the advances were probably used to generate those
> > positions, but they might not have been).
> 
> If you look at SkTextAlignProcScalar, it does the following:
> 
>     void operator()(const SkPoint& loc, const SkGlyph& glyph, SkPoint* dst) {
>         if (SkPaint::kLeft_Align == fAlign) {
>             dst->set(loc.fX, loc.fY);
>         } else if (SkPaint::kCenter_Align == fAlign) {
>             dst->set(loc.fX - SkFixedToScalar(glyph.fAdvanceX >> 1),
>                      loc.fY - SkFixedToScalar(glyph.fAdvanceY >> 1));
>         } else {
>             SkASSERT(SkPaint::kRight_Align == fAlign);
>             dst->set(loc.fX - SkFixedToScalar(glyph.fAdvanceX),
>                      loc.fY - SkFixedToScalar(glyph.fAdvanceY));
>         }
>     }
> 
> So even if scalarsPerPosition is 1, the y-coordinate the gpu needs to draw
each
> glyph at will not be constant when align is not left and the glyphs have
> different y-advances. A nonzero y-advance is rare though, so it seems like
there
> could be a quick check that detects 99% of right and center aligned cases that
> can be drawn with just TranslateX.

Well, we just had a WTF? moment here, as this makes no sense. It appears this
code has been this way for some time (the entire time this has been open source,
afaict). We now have a vertical bit on the paint, which might be used here to
decide that 'left', 'center', and 'right' apply along the vertical baseline. I'm
pretty sure most of the time we get 'left', and have no tests for anything else,
which is why this hasn't been considered before.

Mike is out at the moment, but we'll try to put our heads together and figure
out what should happen. In any event, I believe the intent of scalersPerPosition
being 1 was supposed to mean that all the glyphs would run along the baseline
(horizontal or vertical with respect to the current matrix).

[bungeman-skia, 2014-07-21 18:59:09]

On 2014/07/21 18:50:51, bungeman1 wrote:
> On 2014/07/21 18:03:27, Chris Dalton wrote:
> > On 2014/07/21 17:51:28, bungeman1 wrote:
> > >
> >
>
https://codereview.chromium.org/406523003/diff/1/src/gpu/GrStencilAndCoverTex...
> > > File src/gpu/GrStencilAndCoverTextContext.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/406523003/diff/1/src/gpu/GrStencilAndCoverTex...
> > > src/gpu/GrStencilAndCoverTextContext.cpp:266: } else {
> > > On 2014/07/18 22:23:30, Chris Dalton wrote:
> > > > We could do a translate-x here too if we could verify every glyph's
> advanceY
> > > > will be zero. That seems to be the usual case, is there a way to know
they
> > > will
> > > > all be zero?
> > > 
> > > I'm not sure I understand. You've used scalersPerPosition above which when
1
> > > means there are just x advances. Is that what you want? It's an odd way to
> > > phrase the question as the glyph advances don't really mean anything here,
> its
> > > just glyphs at positions (the advances were probably used to generate
those
> > > positions, but they might not have been).
> > 
> > If you look at SkTextAlignProcScalar, it does the following:
> > 
> >     void operator()(const SkPoint& loc, const SkGlyph& glyph, SkPoint* dst)
{
> >         if (SkPaint::kLeft_Align == fAlign) {
> >             dst->set(loc.fX, loc.fY);
> >         } else if (SkPaint::kCenter_Align == fAlign) {
> >             dst->set(loc.fX - SkFixedToScalar(glyph.fAdvanceX >> 1),
> >                      loc.fY - SkFixedToScalar(glyph.fAdvanceY >> 1));
> >         } else {
> >             SkASSERT(SkPaint::kRight_Align == fAlign);
> >             dst->set(loc.fX - SkFixedToScalar(glyph.fAdvanceX),
> >                      loc.fY - SkFixedToScalar(glyph.fAdvanceY));
> >         }
> >     }
> > 
> > So even if scalarsPerPosition is 1, the y-coordinate the gpu needs to draw
> each
> > glyph at will not be constant when align is not left and the glyphs have
> > different y-advances. A nonzero y-advance is rare though, so it seems like
> there
> > could be a quick check that detects 99% of right and center aligned cases
that
> > can be drawn with just TranslateX.
> 
> Well, we just had a WTF? moment here, as this makes no sense. It appears this
> code has been this way for some time (the entire time this has been open
source,
> afaict). We now have a vertical bit on the paint, which might be used here to
> decide that 'left', 'center', and 'right' apply along the vertical baseline.
I'm
> pretty sure most of the time we get 'left', and have no tests for anything
else,
> which is why this hasn't been considered before.
> 
> Mike is out at the moment, but we'll try to put our heads together and figure
> out what should happen. In any event, I believe the intent of
scalersPerPosition
> being 1 was supposed to mean that all the glyphs would run along the baseline
> (horizontal or vertical with respect to the current matrix).

Forget all that, turns out we do have the glyph advances in the device space, so
for any rotated, etc text this does make sense. I'll have to take a deeper look
at what's actually going on.

[Chris Dalton, 2014-07-23 19:48:52]

> Forget all that, turns out we do have the glyph advances in the device space,
so
> for any rotated, etc text this does make sense. I'll have to take a deeper
look
> at what's actually going on.

Thanks for checking. Is this something we can understand easily? Or would it be
better to submit the code as-is then optimize for center and right align later?

The typical Chrome usecase (int english, at least) of left-aligned drawPosTextH
is already covered in this patch.

[bungeman-skia, 2014-07-23 20:00:40]

On 2014/07/23 19:48:52, Chris Dalton wrote:
> > Forget all that, turns out we do have the glyph advances in the device
space,
> so
> > for any rotated, etc text this does make sense. I'll have to take a deeper
> look
> > at what's actually going on.
> 
> Thanks for checking. Is this something we can understand easily? Or would it
be
> better to submit the code as-is then optimize for center and right align
later?
> 
> The typical Chrome usecase (int english, at least) of left-aligned
drawPosTextH
> is already covered in this patch.

I'm ok with that, since you're right that Chrome isn't going to use it. I can't
say I understand the entire thing, but note that the glyph advances being used
are in device space already.

[Chris Dalton, 2014-07-23 21:07:43]

Ok. I'll go ahead and submit then. One more question: how do I handle the fact
that this change causes benign pixel diffs to a few tests in gm?


> but note that the glyph advances being used are in device space already.

Ok. This code doesn't mess with glyph advances, but continues to go through
SkTextAlignProcScalar like the previous code when they are needed. There
shouldn't be any change in functionality there.

[bungeman-skia, 2014-07-23 21:15:49]

On 2014/07/23 21:07:43, Chris Dalton wrote:
> Ok. I'll go ahead and submit then. One more question: how do I handle the fact
> that this change causes benign pixel diffs to a few tests in gm?
> 
> 
> > but note that the glyph advances being used are in device space already.
> 
> Ok. This code doesn't mess with glyph advances, but continues to go through
> SkTextAlignProcScalar like the previous code when they are needed. There
> shouldn't be any change in functionality there.

Hmmm... if that's the case we may also need to figure out what the impact will
be on the Blink layout tests. I don't have time right now, but Brian or Jim
might be able to help out with that.

[Chris Dalton, 2014-07-24 21:04:55]

On 2014/07/23 21:15:49, bungeman1 wrote:
> if that's the case we may also need to figure out what the impact will
> be on the Blink layout tests.

Do the blink layout tests run with nvpr configs?

[bungeman-skia, 2014-07-25 18:54:58]

On 2014/07/24 21:04:55, Chris Dalton wrote:
> On 2014/07/23 21:15:49, bungeman1 wrote:
> > if that's the case we may also need to figure out what the impact will
> > be on the Blink layout tests.
> 
> Do the blink layout tests run with nvpr configs?

It turns out the answer is no, so I think you're ok.

[Chris Dalton, 2014-07-25 18:57:29]

On 2014/07/25 18:54:58, bungeman1 wrote:
> On 2014/07/24 21:04:55, Chris Dalton wrote:
> > On 2014/07/23 21:15:49, bungeman1 wrote:
> > > if that's the case we may also need to figure out what the impact will
> > > be on the Blink layout tests.
> > 
> > Do the blink layout tests run with nvpr configs?
> 
> It turns out the answer is no, so I think you're ok.

Perfect, thanks! So do I submit this, and then rebase the gm nvpr expectations
separately?

Just to be clear, this adds a new gm test in addition to causing pixel diffs.

[bungeman-skia, 2014-07-25 19:26:26]

On 2014/07/25 18:57:29, Chris Dalton wrote:
> On 2014/07/25 18:54:58, bungeman1 wrote:
> > On 2014/07/24 21:04:55, Chris Dalton wrote:
> > > On 2014/07/23 21:15:49, bungeman1 wrote:
> > > > if that's the case we may also need to figure out what the impact will
> > > > be on the Blink layout tests.
> > > 
> > > Do the blink layout tests run with nvpr configs?
> > 
> > It turns out the answer is no, so I think you're ok.
> 
> Perfect, thanks! So do I submit this, and then rebase the gm nvpr expectations
> separately?
> 
> Just to be clear, this adds a new gm test in addition to causing pixel diffs.

I think so, you might want to ask/tell/coordinate with the current sheriff (as
seen at http://skia-tree-status.appspot.com/buildbots/console ).

[Chris Dalton, 2014-07-25 20:06:49]

The CQ bit was checked by cdalton@nvidia.com

[commit-bot: I haz the power, 2014-07-25 20:07:06]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/cdalton@nvidia.com/406523003/20001

[commit-bot: I haz the power, 2014-07-25 20:07:18]

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

[commit-bot: I haz the power, 2014-07-25 20:07:23]

Failed to apply patch for expectations/gm/ignored-tests.txt:
While running git apply --index -p1;
  error: patch failed: expectations/gm/ignored-tests.txt:45
  error: expectations/gm/ignored-tests.txt: patch does not apply

Patch:       expectations/gm/ignored-tests.txt
Index: expectations/gm/ignored-tests.txt
diff --git a/expectations/gm/ignored-tests.txt
b/expectations/gm/ignored-tests.txt
index
c97a0b9de23851fa466a54354e0d4064b6aa3709..94ed4de733817c0982335c6432b3fc19c8056c8a
100644
--- a/expectations/gm/ignored-tests.txt
+++ b/expectations/gm/ignored-tests.txt
@@ -45,3 +45,11 @@ bleed
 blurquickreject
 blurrects
 bigblurs
+shadertext3
+shadertext2
+glyph_pos_n_f
+glyph_pos_h_f
+glyph_pos_n_s
+glyph_pos_h_s
+glyph_pos_n_b
+glyph_pos_h_b

[Chris Dalton, 2014-07-25 20:17:13]

The CQ bit was checked by cdalton@nvidia.com

[commit-bot: I haz the power, 2014-07-25 20:18:11]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/cdalton@nvidia.com/406523003/40001

[commit-bot: I haz the power, 2014-07-25 20:30:20]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  Build-Ubuntu13.10-GCC4.8-x86_64-Release-Trybot on tryserver.skia
(http://108.170.220.76:10117/builders/Build-Ubuntu13.10-GCC4.8-x86_64-Release-...)

[Chris Dalton, 2014-07-25 20:36:59]

The CQ bit was checked by cdalton@nvidia.com

[commit-bot: I haz the power, 2014-07-25 20:37:10]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/cdalton@nvidia.com/406523003/60001

[commit-bot: I haz the power, 2014-07-25 20:48:59]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  Build-Ubuntu13.10-GCC4.8-x86_64-Release-Trybot on tryserver.skia
(http://108.170.220.76:10117/builders/Build-Ubuntu13.10-GCC4.8-x86_64-Release-...)

[commit-bot: I haz the power, 2014-07-25 20:53:44]

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

[commit-bot: I haz the power, 2014-07-25 20:53:45]

Try jobs failed on following builders:
  Build-Ubuntu13.10-GCC4.8-x86_64-Release-Trybot on tryserver.skia
(http://108.170.220.76:10117/builders/Build-Ubuntu13.10-GCC4.8-x86_64-Release-...)

[Chris Dalton, 2014-07-25 20:59:55]

The CQ bit was checked by cdalton@nvidia.com

[commit-bot: I haz the power, 2014-07-25 21:00:24]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/cdalton@nvidia.com/406523003/70001

[commit-bot: I haz the power, 2014-07-25 21:14:02]

Change committed as b2808cd0411b5860f04f1364138674463768e414