Improved distance field sampling

Improved distance field sampling

There are two different fixes here. First, it computes the distance value properly within the shader. Second, it handles anti-aliasing properly by doing a correction based on the gradient of the texture coordinates.

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

[bsalomon, 2014-02-05 20:19:09]

I'm curious if you've timed the new code vs the old code for the uniform scaling
case, especially on mobile GPUs where shader length is more important. I'm
wondering whether we want to keep the uniform scaling specialization.

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

https://codereview.chromium.org/149853002/diff/150001/src/gpu/GrAtlas.h#newco...
src/gpu/GrAtlas.h:102: SkISize getSize();
const?

https://codereview.chromium.org/149853002/diff/150001/src/gpu/GrDistanceField...
File src/gpu/GrDistanceFieldTextContext.cpp (right):

https://codereview.chromium.org/149853002/diff/150001/src/gpu/GrDistanceField...
src/gpu/GrDistanceFieldTextContext.cpp:276: SkISize texSize =
fStrike->getAtlasSize();
AFAICT this is the same for the lifetime of the program. Perhaps we should use a
uniform rather than a vertex attribute?

https://codereview.chromium.org/149853002/diff/150001/src/gpu/GrDistanceField...
src/gpu/GrDistanceFieldTextContext.cpp:282: 3 * sizeof(SkPoint));
this 3 * sizeof(SkPoint) is repeated here three times and once above. Maybe

static const size_t kVertexSize = 3 * sizeof(SkPoint);

https://codereview.chromium.org/149853002/diff/150001/src/gpu/GrTextStrike.h
File src/gpu/GrTextStrike.h (right):

https://codereview.chromium.org/149853002/diff/150001/src/gpu/GrTextStrike.h#...
src/gpu/GrTextStrike.h:42: SkISize getAtlasSize() { return fAtlas.getSize(); }
const?

[jvanverth1, 2014-02-07 15:04:20]

On 2014/02/05 20:19:09, bsalomon wrote:
> I'm curious if you've timed the new code vs the old code for the uniform
scaling
> case, especially on mobile GPUs where shader length is more important. I'm
> wondering whether we want to keep the uniform scaling specialization.

Here some timing on the Nexus 7 showing the uniform scaling shader vs. the full
one. The data is pretty noisy (AA and LCD should behave the same way, since I've
disabled LCD), so I have to conclude they're roughly the same. The full shader
adds four instructions: a dFdy, two dot products, and a normalize.

    text_16_BW_BK GPU c         31.30      35.54      -4.24   -13.5% 
    text_16_AA_FF GPU c         34.75      36.39      -1.64    -4.7% 
text_16_pos_AA_BK GPU c        118.39     122.74      -4.35    -3.7% 
    text_16_AA_88 GPU c         35.50      35.80      -0.30    -0.8% 
   text_16_LCD_FF GPU c         36.19      36.11      +0.08    +0.2% 
   text_16_LCD_BK GPU c         36.87      36.62      +0.25    +0.7% 
    text_16_AA_BK GPU c         33.59      32.89      +0.70    +2.1% 
   text_16_LCD_88 GPU c        121.67     118.62      +3.05    +2.5% 
    text_16_BW_88 GPU c        121.43     117.91      +3.52    +2.9% 
    text_16_BW_FF GPU c         35.45      31.83      +3.62   +10.2%

[bsalomon, 2014-02-07 15:13:15]

On 2014/02/07 15:04:20, JimVV wrote:
> On 2014/02/05 20:19:09, bsalomon wrote:
> Here some timing on the Nexus 7 showing the uniform scaling shader vs. the
full
> one. The data is pretty noisy (AA and LCD should behave the same way, since
I've
> disabled LCD), so I have to conclude they're roughly the same. The full shader
> adds four instructions: a dFdy, two dot products, and a normalize.
> 
>     text_16_BW_BK GPU c         31.30      35.54      -4.24   -13.5% 
>     text_16_AA_FF GPU c         34.75      36.39      -1.64    -4.7% 
> text_16_pos_AA_BK GPU c        118.39     122.74      -4.35    -3.7% 
>     text_16_AA_88 GPU c         35.50      35.80      -0.30    -0.8% 
>    text_16_LCD_FF GPU c         36.19      36.11      +0.08    +0.2% 
>    text_16_LCD_BK GPU c         36.87      36.62      +0.25    +0.7% 
>     text_16_AA_BK GPU c         33.59      32.89      +0.70    +2.1% 
>    text_16_LCD_88 GPU c        121.67     118.62      +3.05    +2.5% 
>     text_16_BW_88 GPU c        121.43     117.91      +3.52    +2.9% 
>     text_16_BW_FF GPU c         35.45      31.83      +3.62   +10.2%

We should probably check at least one more architecture, maybe sgx or mali. The
other thing to consider is the 4x memory increase. That alone is probably enough
reason to keep both paths. Also, I'm wondering whether we should dedicate a
smaller atlas to the general case since we expect it to be rarely used. We might
also want to consider putting the atlas textures into GrResourceCache so that
they eventually get purged if not used for some time.

[jvanverth1, 2014-02-07 15:16:10]

On 2014/02/07 15:13:15, bsalomon wrote:
> On 2014/02/07 15:04:20, JimVV wrote:
> > On 2014/02/05 20:19:09, bsalomon wrote:
> > Here some timing on the Nexus 7 showing the uniform scaling shader vs. the
> full
> > one. The data is pretty noisy (AA and LCD should behave the same way, since
> I've
> > disabled LCD), so I have to conclude they're roughly the same. The full
shader
> > adds four instructions: a dFdy, two dot products, and a normalize.
> > 
> >     text_16_BW_BK GPU c         31.30      35.54      -4.24   -13.5% 
> >     text_16_AA_FF GPU c         34.75      36.39      -1.64    -4.7% 
> > text_16_pos_AA_BK GPU c        118.39     122.74      -4.35    -3.7% 
> >     text_16_AA_88 GPU c         35.50      35.80      -0.30    -0.8% 
> >    text_16_LCD_FF GPU c         36.19      36.11      +0.08    +0.2% 
> >    text_16_LCD_BK GPU c         36.87      36.62      +0.25    +0.7% 
> >     text_16_AA_BK GPU c         33.59      32.89      +0.70    +2.1% 
> >    text_16_LCD_88 GPU c        121.67     118.62      +3.05    +2.5% 
> >     text_16_BW_88 GPU c        121.43     117.91      +3.52    +2.9% 
> >     text_16_BW_FF GPU c         35.45      31.83      +3.62   +10.2%
> 
> We should probably check at least one more architecture, maybe sgx or mali.
The
> other thing to consider is the 4x memory increase. That alone is probably
enough
> reason to keep both paths. Also, I'm wondering whether we should dedicate a
> smaller atlas to the general case since we expect it to be rarely used. We
might
> also want to consider putting the atlas textures into GrResourceCache so that
> they eventually get purged if not used for some time.

There is no 4x memory increase. The full matrix method uses the same 8-bit
texture as the uniform scaling one.

[bsalomon, 2014-02-07 15:17:14]

On 2014/02/07 15:16:10, JimVV wrote:
> On 2014/02/07 15:13:15, bsalomon wrote:
> > On 2014/02/07 15:04:20, JimVV wrote:
> > > On 2014/02/05 20:19:09, bsalomon wrote:
> > > Here some timing on the Nexus 7 showing the uniform scaling shader vs. the
> > full
> > > one. The data is pretty noisy (AA and LCD should behave the same way,
since
> > I've
> > > disabled LCD), so I have to conclude they're roughly the same. The full
> shader
> > > adds four instructions: a dFdy, two dot products, and a normalize.
> > > 
> > >     text_16_BW_BK GPU c         31.30      35.54      -4.24   -13.5% 
> > >     text_16_AA_FF GPU c         34.75      36.39      -1.64    -4.7% 
> > > text_16_pos_AA_BK GPU c        118.39     122.74      -4.35    -3.7% 
> > >     text_16_AA_88 GPU c         35.50      35.80      -0.30    -0.8% 
> > >    text_16_LCD_FF GPU c         36.19      36.11      +0.08    +0.2% 
> > >    text_16_LCD_BK GPU c         36.87      36.62      +0.25    +0.7% 
> > >     text_16_AA_BK GPU c         33.59      32.89      +0.70    +2.1% 
> > >    text_16_LCD_88 GPU c        121.67     118.62      +3.05    +2.5% 
> > >     text_16_BW_88 GPU c        121.43     117.91      +3.52    +2.9% 
> > >     text_16_BW_FF GPU c         35.45      31.83      +3.62   +10.2%
> > 
> > We should probably check at least one more architecture, maybe sgx or mali.
> The
> > other thing to consider is the 4x memory increase. That alone is probably
> enough
> > reason to keep both paths. Also, I'm wondering whether we should dedicate a
> > smaller atlas to the general case since we expect it to be rarely used. We
> might
> > also want to consider putting the atlas textures into GrResourceCache so
that
> > they eventually get purged if not used for some time.
> 
> There is no 4x memory increase. The full matrix method uses the same 8-bit
> texture as the uniform scaling one.

Oh right, of course. I was thinking of your earlier approach.

[jvanverth1, 2014-02-07 16:17:34]

On 2014/02/07 15:17:14, bsalomon wrote:
> On 2014/02/07 15:16:10, JimVV wrote:
> > On 2014/02/07 15:13:15, bsalomon wrote:
> > > On 2014/02/07 15:04:20, JimVV wrote:
> > > > On 2014/02/05 20:19:09, bsalomon wrote:
> > > > Here some timing on the Nexus 7 showing the uniform scaling shader vs.
the
> > > full
> > > > one. The data is pretty noisy (AA and LCD should behave the same way,
> since
> > > I've
> > > > disabled LCD), so I have to conclude they're roughly the same. The full
> > shader
> > > > adds four instructions: a dFdy, two dot products, and a normalize.
> > > > 
> > > >     text_16_BW_BK GPU c         31.30      35.54      -4.24   -13.5% 
> > > >     text_16_AA_FF GPU c         34.75      36.39      -1.64    -4.7% 
> > > > text_16_pos_AA_BK GPU c        118.39     122.74      -4.35    -3.7% 
> > > >     text_16_AA_88 GPU c         35.50      35.80      -0.30    -0.8% 
> > > >    text_16_LCD_FF GPU c         36.19      36.11      +0.08    +0.2% 
> > > >    text_16_LCD_BK GPU c         36.87      36.62      +0.25    +0.7% 
> > > >     text_16_AA_BK GPU c         33.59      32.89      +0.70    +2.1% 
> > > >    text_16_LCD_88 GPU c        121.67     118.62      +3.05    +2.5% 
> > > >     text_16_BW_88 GPU c        121.43     117.91      +3.52    +2.9% 
> > > >     text_16_BW_FF GPU c         35.45      31.83      +3.62   +10.2%
> > > 
> > > We should probably check at least one more architecture, maybe sgx or
mali.
> > The
> > > other thing to consider is the 4x memory increase. That alone is probably
> > enough
> > > reason to keep both paths. Also, I'm wondering whether we should dedicate
a
> > > smaller atlas to the general case since we expect it to be rarely used. We
> > might
> > > also want to consider putting the atlas textures into GrResourceCache so
> that
> > > they eventually get purged if not used for some time.
> > 
> > There is no 4x memory increase. The full matrix method uses the same 8-bit
> > texture as the uniform scaling one.
> 
> Oh right, of course. I was thinking of your earlier approach.

More numbers, comparing the Bitmap approach with the uniform scaled DF:
   text_16_LCD_88 GPU c         30.91     121.67     -90.76  -293.6% 
    text_16_BW_88 GPU c         30.88     121.43     -90.55  -293.2% 
text_16_pos_AA_BK GPU c         33.71     118.39     -84.68  -251.2% 
    text_16_BW_FF GPU c         27.66      35.45      -7.79   -28.2% 
   text_16_LCD_BK GPU c         30.97      36.87      -5.90   -19.1% 
    text_16_AA_FF GPU c         31.05      34.75      -3.70   -11.9% 
    text_16_AA_88 GPU c         32.55      35.50      -2.95    -9.1% 
   text_16_LCD_FF GPU c         33.37      36.19      -2.82    -8.5% 
    text_16_AA_BK GPU c         31.43      33.59      -2.16    -6.9% 
    text_16_BW_BK GPU c         31.54      31.30      +0.24    +0.8% 

Not so good. Disabling the distance field creation helps the worst cases (the
rest are probably using the cached glyphs), but overall it's still not as fast
as the bitmap method. I'm going to wrap up this change and then dig into
profiling.

[bsalomon, 2014-02-07 16:19:56]

On 2014/02/07 16:17:34, JimVV wrote:
> On 2014/02/07 15:17:14, bsalomon wrote:
> > On 2014/02/07 15:16:10, JimVV wrote:
> > > On 2014/02/07 15:13:15, bsalomon wrote:
> > > > On 2014/02/07 15:04:20, JimVV wrote:
> > > > > On 2014/02/05 20:19:09, bsalomon wrote:
> > > > > Here some timing on the Nexus 7 showing the uniform scaling shader vs.
> the
> > > > full
> > > > > one. The data is pretty noisy (AA and LCD should behave the same way,
> > since
> > > > I've
> > > > > disabled LCD), so I have to conclude they're roughly the same. The
full
> > > shader
> > > > > adds four instructions: a dFdy, two dot products, and a normalize.
> > > > > 
> > > > >     text_16_BW_BK GPU c         31.30      35.54      -4.24   -13.5% 
> > > > >     text_16_AA_FF GPU c         34.75      36.39      -1.64    -4.7% 
> > > > > text_16_pos_AA_BK GPU c        118.39     122.74      -4.35    -3.7% 
> > > > >     text_16_AA_88 GPU c         35.50      35.80      -0.30    -0.8% 
> > > > >    text_16_LCD_FF GPU c         36.19      36.11      +0.08    +0.2% 
> > > > >    text_16_LCD_BK GPU c         36.87      36.62      +0.25    +0.7% 
> > > > >     text_16_AA_BK GPU c         33.59      32.89      +0.70    +2.1% 
> > > > >    text_16_LCD_88 GPU c        121.67     118.62      +3.05    +2.5% 
> > > > >     text_16_BW_88 GPU c        121.43     117.91      +3.52    +2.9% 
> > > > >     text_16_BW_FF GPU c         35.45      31.83      +3.62   +10.2%
> > > > 
> > > > We should probably check at least one more architecture, maybe sgx or
> mali.
> > > The
> > > > other thing to consider is the 4x memory increase. That alone is
probably
> > > enough
> > > > reason to keep both paths. Also, I'm wondering whether we should
dedicate
> a
> > > > smaller atlas to the general case since we expect it to be rarely used.
We
> > > might
> > > > also want to consider putting the atlas textures into GrResourceCache so
> > that
> > > > they eventually get purged if not used for some time.
> > > 
> > > There is no 4x memory increase. The full matrix method uses the same 8-bit
> > > texture as the uniform scaling one.
> > 
> > Oh right, of course. I was thinking of your earlier approach.
> 
> More numbers, comparing the Bitmap approach with the uniform scaled DF:
>    text_16_LCD_88 GPU c         30.91     121.67     -90.76  -293.6% 
>     text_16_BW_88 GPU c         30.88     121.43     -90.55  -293.2% 
> text_16_pos_AA_BK GPU c         33.71     118.39     -84.68  -251.2% 
>     text_16_BW_FF GPU c         27.66      35.45      -7.79   -28.2% 
>    text_16_LCD_BK GPU c         30.97      36.87      -5.90   -19.1% 
>     text_16_AA_FF GPU c         31.05      34.75      -3.70   -11.9% 
>     text_16_AA_88 GPU c         32.55      35.50      -2.95    -9.1% 
>    text_16_LCD_FF GPU c         33.37      36.19      -2.82    -8.5% 
>     text_16_AA_BK GPU c         31.43      33.59      -2.16    -6.9% 
>     text_16_BW_BK GPU c         31.54      31.30      +0.24    +0.8% 
> 
> Not so good. Disabling the distance field creation helps the worst cases (the
> rest are probably using the cached glyphs), but overall it's still not as fast
> as the bitmap method. I'm going to wrap up this change and then dig into
> profiling.

I'm curious why the LCD text benchs changed.

[jvanverth1, 2014-02-10 18:34:01]

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

https://codereview.chromium.org/149853002/diff/150001/src/gpu/GrAtlas.h#newco...
src/gpu/GrAtlas.h:102: SkISize getSize();
On 2014/02/05 20:19:10, bsalomon wrote:
> const?

Done.

https://codereview.chromium.org/149853002/diff/150001/src/gpu/GrDistanceField...
File src/gpu/GrDistanceFieldTextContext.cpp (right):

https://codereview.chromium.org/149853002/diff/150001/src/gpu/GrDistanceField...
src/gpu/GrDistanceFieldTextContext.cpp:276: SkISize texSize =
fStrike->getAtlasSize();
On 2014/02/05 20:19:10, bsalomon wrote:
> AFAICT this is the same for the lifetime of the program. Perhaps we should use
a
> uniform rather than a vertex attribute?

Done.

https://codereview.chromium.org/149853002/diff/150001/src/gpu/GrDistanceField...
src/gpu/GrDistanceFieldTextContext.cpp:282: 3 * sizeof(SkPoint));
On 2014/02/05 20:19:10, bsalomon wrote:
> this 3 * sizeof(SkPoint) is repeated here three times and once above. Maybe
> 
> static const size_t kVertexSize = 3 * sizeof(SkPoint);

Done.

https://codereview.chromium.org/149853002/diff/150001/src/gpu/GrTextStrike.h
File src/gpu/GrTextStrike.h (right):

https://codereview.chromium.org/149853002/diff/150001/src/gpu/GrTextStrike.h#...
src/gpu/GrTextStrike.h:42: SkISize getAtlasSize() { return fAtlas.getSize(); }
On 2014/02/05 20:19:10, bsalomon wrote:
> const?

Done.

[bsalomon, 2014-02-10 21:51:26]

lgtm w/ comment about uniform

https://codereview.chromium.org/149853002/diff/310001/src/gpu/effects/GrDista...
File src/gpu/effects/GrDistanceFieldTextureEffect.cpp (right):

https://codereview.chromium.org/149853002/diff/310001/src/gpu/effects/GrDista...
src/gpu/effects/GrDistanceFieldTextureEffect.cpp:80: if
(fTextureSizeUni.isValid()) {
I don't think we really need the branch here. I also wonder if it makes sense to
store the old value and only upload if it has changed (since in practice today
it will really never change).

[jvanverth1, 2014-02-14 21:35:57]

The CQ bit was checked by jvanverth@google.com

[commit-bot: I haz the power, 2014-02-14 21:36:01]

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

[commit-bot: I haz the power, 2014-02-14 21:48:32]

Change committed as 13461