Implement text rendering with NVPR

Implement text rendering with NVPR

Use path rendering to render the text from outlines if supported by the
GPU. Implement this in GrStencilAndCoverTextContext by copying chunks of code
from GrBitmapTextContext.

The drawing is implemented with "instanced" path drawing functions.

Moves the creation of the "main" text context from SkGpuDevice to the
GrContext::createTextContext. This is done because the decision of which text
renderer is optimal can be made only with the internal implementation-specific
information of the context.

Remove a windows assertion from SkScalerContext_GDI::getGDIGlyphPath.  The
GetGlyphOutlineW fails in fontmgr_match for the initial space char in the string
" [700] ...". According to MSDN, this is a known problem.  Just return that the
glyph has no path data in these cases.

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

[Kimmo Kinnunen, 2014-03-20 13:55:54]

Jim, Brian,
Did I understand the intention of the text refactor correctly, eg. it was meant
to support also this, in this way?

Any thoughts on how to handle following:
 * The polymorphism related the text context selection
 * Code-reuse between all the different parts which measure text

Is there something especially wrong with the patch, apart from cleanup?

I also filed bug skia:2308 which tries to discuss a bit about the code
duplication / reuse..

[jvanverth1, 2014-03-20 15:50:28]

On 2014/03/20 13:55:54, kkinnunen wrote:
> Jim, Brian,
> Did I understand the intention of the text refactor correctly, eg. it was
meant
> to support also this, in this way?
> 
> Any thoughts on how to handle following:
>  * The polymorphism related the text context selection
>  * Code-reuse between all the different parts which measure text
> 
> Is there something especially wrong with the patch, apart from cleanup?
> 
> I also filed bug skia:2308 which tries to discuss a bit about the code
> duplication / reuse..

Sorry, I haven't had time to look at the entire patch yet. But a couple of
initial thoughts:

1) While C++ will handle converting a non-virtual method in a child class if the
base method is declared virtual, I believe it's Skia coding style to continue to
declare the child methods virtual.

2) The Main and Fallback TextContexts are necessary for
GrDistanceFieldTextContext -- it can't support all text modes, so it needs to
fall back to GrBitmapTextContext in those cases.

Jim

[jvanverth1, 2014-03-20 20:30:24]

On 2014/03/20 15:50:28, JimVV wrote:
> On 2014/03/20 13:55:54, kkinnunen wrote:
> > Jim, Brian,
> > Did I understand the intention of the text refactor correctly, eg. it was
> meant
> > to support also this, in this way?
> > 
> > Any thoughts on how to handle following:
> >  * The polymorphism related the text context selection
> >  * Code-reuse between all the different parts which measure text
> > 
> > Is there something especially wrong with the patch, apart from cleanup?
> > 
> > I also filed bug skia:2308 which tries to discuss a bit about the code
> > duplication / reuse..
> 
> Sorry, I haven't had time to look at the entire patch yet. But a couple of
> initial thoughts:
> 
> 1) While C++ will handle converting a non-virtual method in a child class if
the
> base method is declared virtual, I believe it's Skia coding style to continue
to
> declare the child methods virtual.
> 
> 2) The Main and Fallback TextContexts are necessary for
> GrDistanceFieldTextContext -- it can't support all text modes, so it needs to
> fall back to GrBitmapTextContext in those cases.
> 
> Jim

Ah, I see how you're supporting 2) now. I still have to go through the whole
patch, but after my second glance that looks much cleaner than what was there
before, thanks.

As far as 1), while it's not explicitly in the style doc, the de facto standard
is to use 'virtual' and 'SK_OVERRIDE' for subclasses, even in the case of a pure
virtual method.

[Kimmo Kinnunen, 2014-03-21 08:29:35]

On 2014/03/20 20:30:24, JimVV wrote:
> far as 1), while it's not explicitly in the style doc, the de facto standard
> is to use 'virtual' and 'SK_OVERRIDE' for subclasses, even in the case of a
pure
> virtual method.

Updated the patch and the description to highlight the answer to this: there's
no pure virtual methods that would break the rule. The SkBaseTextContext, from
which the existing GrBitmapTextContext and GrDistanceFieldTextContext inherit,
does not have any virtual methods.

[jvanverth1, 2014-03-21 12:56:47]

On 2014/03/21 08:29:35, kkinnunen wrote:
> On 2014/03/20 20:30:24, JimVV wrote:
> > far as 1), while it's not explicitly in the style doc, the de facto standard
> > is to use 'virtual' and 'SK_OVERRIDE' for subclasses, even in the case of a
> pure
> > virtual method.
> 
> Updated the patch and the description to highlight the answer to this: there's
> no pure virtual methods that would break the rule. The SkBaseTextContext, from
> which the existing GrBitmapTextContext and GrDistanceFieldTextContext inherit,
> does not have any virtual methods.

Ah, I see -- you split GrTextContext into two classes. And that, kids, is why
you shouldn't review code when you're jetlagged.

I'll get you full comments shortly.

[jvanverth1, 2014-03-21 15:47:59]

On 2014/03/21 12:56:47, JimVV wrote:
> On 2014/03/21 08:29:35, kkinnunen wrote:
> > On 2014/03/20 20:30:24, JimVV wrote:
> > > far as 1), while it's not explicitly in the style doc, the de facto
standard
> > > is to use 'virtual' and 'SK_OVERRIDE' for subclasses, even in the case of
a
> > pure
> > > virtual method.
> > 
> > Updated the patch and the description to highlight the answer to this:
there's
> > no pure virtual methods that would break the rule. The SkBaseTextContext,
from
> > which the existing GrBitmapTextContext and GrDistanceFieldTextContext
inherit,
> > does not have any virtual methods.
> 
> Ah, I see -- you split GrTextContext into two classes. And that, kids, is why
> you shouldn't review code when you're jetlagged.
> 
> I'll get you full comments shortly.

After thinking about it with a clear head, I'm not sure this is quite the right
approach. One problem I see is that it looks like the StencilAndCoverTextContext
falls back to drawText_asPaths() -- it should probably fall back to the
BitmapTextContext first, as that will handle cases which are smaller than 256
point more efficiently as well as color bitmap fonts. That was I was originally
thinking when I set up the fMainTextContext and the fFallbackTextContext -- that
the main context would either be NVPR or distance fields, the fallback from
there would be the original bitmap code, and the final fallback would be to call
drawText_asPaths().

The other issue is that I still find the difference between GrTextContext and
GrBaseTextContext confusing. In my opinion, if we stuck with this it'd be better
to rename GrBaseTextContext to GrTextRenderer and rename the subclasses
accordingly, just to make it clearer.

However, I'm thinking an overall better approach would be what Brian originally
suggested -- set up a chain of text renderers, just like we have a
GrPathRendererChain. So SkGpuDevice would request an appropriate text renderer
from its GrTextContext (or perhaps GrTextRendererChain, to be more consistent)
depending on the current parameters. If it doesn't get one, it would call
drawText_asPaths() as it does now. I'm not sure if the chain would be created in
the SkGpuDevice or the GrContext -- was there a reason you moved the
GrTextContext creation to GrContext?

I also think it'd be better if these changes were split up into separate patches
just for clarity -- first do the work to set up the chain, then add
GrDrawTarget::drawPaths(), then finally add the GrStencilAndCoverTextContext (or
whatever it ends up being called). I'd be happy to do the rewrite to set up the
chain.

The changes to add GrDrawTarget::drawPaths() look fine to me, but Brian should
probably take a look and he's out today.

[Kimmo Kinnunen, 2014-03-24 09:56:22]

On 2014/03/21 15:47:59, JimVV wrote:
> After thinking about it with a clear head, I'm not sure this is quite the
right
> approach. One problem I see is that it looks like the
StencilAndCoverTextContext
> falls back to drawText_asPaths() -- it should probably fall back to the
> BitmapTextContext first

Ok. I simplified the patch.

> The other issue is that I still find the difference between GrTextContext and
> GrBaseTextContext confusing.

Reverted.

> However, I'm thinking an overall better approach would be what Brian
originally
> suggested -- set up a chain of text renderers, just like we have a
> GrPathRendererChain.

I'd be cautious to replicate GrPathRendererChain. FWIW, my finding is that it's
quite poor abstraction. It incurs measurable performance penalty. Also it does
not seem to be able to provide the configurability or polymorphism it tries to
achieve. Eg. for every path draw we pay for something we never benefit of in
practice.

Of course, the GrTextContextChain would not need to replicate the bad parts, but
I just thought to point this out just in case...

> drawText_asPaths() as it does now. I'm not sure if the chain would be created
in
> the SkGpuDevice or the GrContext -- was there a reason you moved the
> GrTextContext creation to GrContext?

The text renderer selection can not be made based on external traits of the
context or render target, rather it is internal information of these concepts.

I tried to add this to the commit msg.

> I also think it'd be better if these changes were split up into separate
patches
> just for clarity -- first do the work to set up the chain, then add
> GrDrawTarget::drawPaths(), then finally add the GrStencilAndCoverTextContext
(or
> whatever it ends up being called). I'd be happy to do the rewrite to set up
the
> chain.

Currently it doesn't need the chain, I reverted the bigger changes.

If the chain is going to be there, I hope it doesn't cost as much as the path
version..
 
> The changes to add GrDrawTarget::drawPaths() look fine to me, but Brian should
> probably take a look and he's out today.

GrDrawTarget::drawPaths now separate:
https://chromiumcodereview.appspot.com/209413006/

[bsalomon, 2014-03-24 13:53:37]

https://codereview.chromium.org/196133014/diff/100001/src/gpu/GrStencilAndCov...
File src/gpu/GrStencilAndCoverTextContext.cpp (right):

https://codereview.chromium.org/196133014/diff/100001/src/gpu/GrStencilAndCov...
src/gpu/GrStencilAndCoverTextContext.cpp:236: default:
tiny style nit: we indent the case/defaults (much to my chagrin).

[jvanverth1, 2014-03-24 14:51:05]

LGTM, though see comments for some issues that may need to be addressed.

https://codereview.chromium.org/196133014/diff/100001/src/gpu/GrStencilAndCov...
File src/gpu/GrStencilAndCoverTextContext.cpp (right):

https://codereview.chromium.org/196133014/diff/100001/src/gpu/GrStencilAndCov...
src/gpu/GrStencilAndCoverTextContext.cpp:92: const SkGlyph& glyph =
glyphCacheProc(cache, &text, fx, fy);
I believe fx, fy should always be 0 in this case. Otherwise you'll create
subpixel glyphs when you don't need to.

https://codereview.chromium.org/196133014/diff/100001/src/gpu/GrStencilAndCov...
src/gpu/GrStencilAndCoverTextContext.cpp:207: fSkPaint.setSubpixelText(false);
You may want to always enable subpixel text so you get the correct positioning.
I had to do this recently in the distance field code.

[Kimmo Kinnunen, 2014-03-25 12:32:48]

There was a hunk missing wrong in the previous patch, would prevent Chromium
from compiling.. It's added here. Is this still ok to go?

https://codereview.chromium.org/196133014/diff/100001/src/gpu/GrStencilAndCov...
File src/gpu/GrStencilAndCoverTextContext.cpp (right):

https://codereview.chromium.org/196133014/diff/100001/src/gpu/GrStencilAndCov...
src/gpu/GrStencilAndCoverTextContext.cpp:92: const SkGlyph& glyph =
glyphCacheProc(cache, &text, fx, fy);
On 2014/03/24 14:51:05, JimVV wrote:
> I believe fx, fy should always be 0 in this case. Otherwise you'll create
> subpixel glyphs when you don't need to.

Done.

https://codereview.chromium.org/196133014/diff/100001/src/gpu/GrStencilAndCov...
src/gpu/GrStencilAndCoverTextContext.cpp:207: fSkPaint.setSubpixelText(false);
On 2014/03/24 14:51:05, JimVV wrote:
> You may want to always enable subpixel text so you get the correct
positioning.
> I had to do this recently in the distance field code.

Done.

https://codereview.chromium.org/196133014/diff/100001/src/gpu/GrStencilAndCov...
src/gpu/GrStencilAndCoverTextContext.cpp:236: default:
On 2014/03/24 13:53:37, bsalomon wrote:
> tiny style nit: we indent the case/defaults (much to my chagrin).

Done.

https://codereview.chromium.org/196133014/diff/120001/src/gpu/SkGpuDevice.cpp
File src/gpu/SkGpuDevice.cpp (right):

https://codereview.chromium.org/196133014/diff/120001/src/gpu/SkGpuDevice.cpp...
src/gpu/SkGpuDevice.cpp:310: fMainTextContext =
fContext->createTextContext(fRenderTarget, fLeakyProperties);
Jim, this hunk was wrong before. Eg. it wasn't even compiling, but I didn't
notice it since the define was off.

[jvanverth1, 2014-03-26 12:23:18]

On 2014/03/25 12:32:48, kkinnunen wrote:
> There was a hunk missing wrong in the previous patch, would prevent Chromium
> from compiling.. It's added here. Is this still ok to go?

Yes, it's good to go.

[Kimmo Kinnunen, 2014-03-26 13:47:11]

The CQ bit was checked by kkinnunen@nvidia.com

[commit-bot: I haz the power, 2014-03-26 13:47:17]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/kkinnunen@nvidia.com/196133014/120001

[commit-bot: I haz the power, 2014-03-26 13:47:21]

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

[commit-bot: I haz the power, 2014-03-26 13:47:22]

Failed to apply patch for src/gpu/GrContext.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file src/gpu/GrContext.cpp
  Hunk #1 FAILED at 14.
  Hunk #2 succeeded at 227 (offset 3 lines).
  1 out of 2 hunks FAILED -- saving rejects to file src/gpu/GrContext.cpp.rej

Patch:       src/gpu/GrContext.cpp
Index: src/gpu/GrContext.cpp
diff --git a/src/gpu/GrContext.cpp b/src/gpu/GrContext.cpp
index
c480a1f718dac47a5704979c47f8efd4bae8d050..2eca88b190dbf175f4bdcf2095b425fcb81ae8b1
100644
--- a/src/gpu/GrContext.cpp
+++ b/src/gpu/GrContext.cpp
@@ -14,22 +14,24 @@
 
 #include "GrAARectRenderer.h"
 #include "GrBufferAllocPool.h"
-#include "GrGpu.h"
+#include "GrDistanceFieldTextContext.h"
 #include "GrDrawTargetCaps.h"
-#include "GrIndexBuffer.h"
+#include "GrGpu.h"
 #include "GrInOrderDrawBuffer.h"
+#include "GrIndexBuffer.h"
 #include "GrOvalRenderer.h"
 #include "GrPathRenderer.h"
 #include "GrPathUtils.h"
 #include "GrResourceCache.h"
 #include "GrSoftwarePathRenderer.h"
+#include "GrStencilAndCoverTextContext.h"
 #include "GrStencilBuffer.h"
 #include "GrTextStrike.h"
-#include "SkRTConf.h"
 #include "SkRRect.h"
+#include "SkRTConf.h"
 #include "SkStrokeRec.h"
-#include "SkTLazy.h"
 #include "SkTLS.h"
+#include "SkTLazy.h"
 #include "SkTrace.h"
 
 // It can be useful to set this to false to test whether a bug is caused by
using the
@@ -224,6 +226,17 @@ int GrContext::getGpuTextureCacheResourceCount() const {
   return fTextureCache->getCachedResourceCount();
 }
 
+GrTextContext* GrContext::createTextContext(GrRenderTarget* renderTarget,
+                                            const SkDeviceProperties&
+                                            leakyProperties) {
+    if (fGpu->caps()->pathRenderingSupport()) {
+        if (renderTarget->getStencilBuffer() && renderTarget->isMultisampled())
{
+            return SkNEW_ARGS(GrStencilAndCoverTextContext, (this,
leakyProperties));
+        }
+    }
+    return SkNEW_ARGS(GrDistanceFieldTextContext, (this, leakyProperties));
+}
+

////////////////////////////////////////////////////////////////////////////////
 
 GrTexture* GrContext::findAndRefTexture(const GrTextureDesc& desc,

[Kimmo Kinnunen, 2014-03-26 13:58:36]

The CQ bit was checked by kkinnunen@nvidia.com

[commit-bot: I haz the power, 2014-03-26 13:58:37]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/kkinnunen@nvidia.com/196133014/200001

[commit-bot: I haz the power, 2014-03-26 13:58:41]

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

[commit-bot: I haz the power, 2014-03-26 13:58:41]

Presubmit check for 196133014-200001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Since the CL is editing public API, you must have an LGTM from one of:
('reed@chromium.org', 'reed@google.com', 'bsalomon@chromium.org',
'bsalomon@google.com')

[Kimmo Kinnunen, 2014-03-26 14:00:23]

Brian, when you have time, can you check out this patch? (Changes the API ->
needs your approval)

[bsalomon, 2014-03-26 14:02:52]

On 2014/03/26 14:00:23, kkinnunen wrote:
> Brian, when you have time, can you check out this patch? (Changes the API ->
> needs your approval)

lgtm

[Kimmo Kinnunen, 2014-03-26 14:07:05]

The CQ bit was checked by kkinnunen@nvidia.com

[commit-bot: I haz the power, 2014-03-26 14:07:07]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/kkinnunen@nvidia.com/196133014/200001

[commit-bot: I haz the power, 2014-03-26 14:49:20]

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

[commit-bot: I haz the power, 2014-03-26 14:49:21]

Retried try job too often on Build-Mac10.8-Clang-x86-Release-Trybot for step(s)
BuildSkiaLib
http://skia-tree-status-staging.appspot.com/redirect/compile-buildbots/builds...

[Kimmo Kinnunen, 2014-03-27 07:56:30]

The CQ bit was checked by kkinnunen@nvidia.com

[commit-bot: I haz the power, 2014-03-27 07:56:40]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/kkinnunen@nvidia.com/196133014/200001

[commit-bot: I haz the power, 2014-03-27 08:04:46]

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

[commit-bot: I haz the power, 2014-03-27 08:04:47]

Retried try job too often on Build-Mac10.8-Clang-x86-Release-Trybot for step(s)
BuildBench, BuildEverything, BuildTools
http://skia-tree-status-staging.appspot.com/redirect/compile-buildbots/builds...

[Kimmo Kinnunen, 2014-03-27 11:07:48]

The CQ bit was checked by kkinnunen@nvidia.com

[commit-bot: I haz the power, 2014-03-27 11:07:51]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/kkinnunen@nvidia.com/196133014/390001

[commit-bot: I haz the power, 2014-03-27 11:26:13]

Change committed as 13962

[Kimmo Kinnunen, 2014-03-27 14:12:32]

This probably caused Win8 "test" bot redness:
http://108.170.217.252:10117/builders/Test-Win8-ShuttleA-GTX660-x86-Debug/bui...

mtklein, maybe this should be reverted?

[Kimmo Kinnunen, 2014-03-27 14:16:15]

A revert of this CL has been created in
https://codereview.chromium.org/213123014/ by kkinnunen@nvidia.com.

The reason for reverting is: This probably caused Win8 test bot fail during gm
run:

http://108.170.217.252:10117/builders/Test-Win8-ShuttleA-GTX660-x86-Debug/bui...

I can not identify any errors from the logs, so maybe it hung or something?.

[mtklein, 2014-03-27 14:18:37]

On 2014/03/27 14:16:15, kkinnunen wrote:
> A revert of this CL has been created in
> https://codereview.chromium.org/213123014/ by mailto:kkinnunen@nvidia.com.
> 
> The reason for reverting is: This probably caused Win8 test bot fail during gm
> run:
> 
>
http://108.170.217.252:10117/builders/Test-Win8-ShuttleA-GTX660-x86-Debug/bui...
> 
> I can not identify any errors from the logs, so maybe it hung or something?.

SGTM.  I had been assuming you were gearing up for a rebaseline, but looking at
the actual changes, it's clear revert is the way to go.  :)

This log
http://108.170.217.252:10117/builders/Test-Win8-ShuttleA-GTX660-x86-Debug/bui...
makes me think GM crashed or ran out of memory while running.  Sadly we don't
get stack traces on Windows when things die.

[Kimmo Kinnunen, 2014-06-09 12:26:46]

Could this get another review?

Things that have changed:
 - Fixed the glyphs to match the default, e.g. bitmap glyphs. Previously, they
matched the same glyphs as the distance field fonts matched.

 - Fixed normal the glyph positions to match the default, e.g. bitmap fonts.
Previously they matched the distance field fonts, which do not match the correct
positions.

 - Fixed the big glyph positions to match the default, e.g. the _asPaths
codepath in SkDraw.

Now the NVPR -drawn text will still have different pixels than the default gpu
text, but the glyph positions are the same.

[jvanverth1, 2014-06-09 13:48:07]

https://codereview.chromium.org/196133014/diff/490001/src/core/SkDrawProcs.h
File src/core/SkDrawProcs.h (right):

https://codereview.chromium.org/196133014/diff/490001/src/core/SkDrawProcs.h#...
src/core/SkDrawProcs.h:100: if (SkPaint::kLeft_Align == fAlign) {
If I understand this correctly, this seems inefficient. The point of the procs
is to do the conditional check once and then just call the appropriate function.
Have you done a comparison between this and the old Bitmap code using 'bench' or
'bench_pictures'?

https://codereview.chromium.org/196133014/diff/490001/src/gpu/GrStencilAndCov...
File src/gpu/GrStencilAndCoverTextContext.cpp (right):

https://codereview.chromium.org/196133014/diff/490001/src/gpu/GrStencilAndCov...
src/gpu/GrStencilAndCoverTextContext.cpp:101: SkFixed fx = SkScalarToFixed(x) +
halfSampleX;
Doesn't this lead to glyphs being off by half a pixel? I had to remove it in the
distance field code.

https://codereview.chromium.org/196133014/diff/490001/src/gpu/GrStencilAndCov...
src/gpu/GrStencilAndCoverTextContext.cpp:176: SkScalar x = tms.fLoc.fX +
halfSampleX;
Same issue with half-pixel alignment as above?  (and others, below)

https://codereview.chromium.org/196133014/diff/490001/src/gpu/GrStencilAndCov...
src/gpu/GrStencilAndCoverTextContext.cpp:212: if (SkPaint::kLeft_Align ==
fSkPaint.getTextAlign()) {
Do we need to support non-subpixel layout when drawing with path rendering?

https://codereview.chromium.org/196133014/diff/490001/src/gpu/GrStencilAndCov...
src/gpu/GrStencilAndCoverTextContext.cpp:306: if
(fContext->getMatrix().invert(&inv)) {
This won't work for non-uniform scale. And if you're assuming uniform scale, you
don't have to invert the whole matrix, just take 1/length of SkVector(kScaleX,
kSkewY).

[bungeman-skia, 2014-06-09 15:00:08]

https://codereview.chromium.org/196133014/diff/490001/src/gpu/GrStencilAndCov...
File src/gpu/GrStencilAndCoverTextContext.cpp (right):

https://codereview.chromium.org/196133014/diff/490001/src/gpu/GrStencilAndCov...
src/gpu/GrStencilAndCoverTextContext.cpp:101: SkFixed fx = SkScalarToFixed(x) +
halfSampleX;
On 2014/06/09 13:48:07, jvanverth1 wrote:
> Doesn't this lead to glyphs being off by half a pixel? I had to remove it in
the
> distance field code.

Yeah, this was done in SkDraw because of SkDraw1Glyph::Proc and SkGlyph::MakeID.
See the documentation there, but the basic reason was to speed up rounding. By
adding half the sampling frequency on the outside means that below it truncation
is rounding. Since you're not calling into SkDraw1Glyph::Proc or SkDrawCacheProc
here, perhaps this is unnecessary. On the other hand GrGlyph::Pack appears to be
depending on this for its truncated x,y rounding? I'm not familiar enough with
the Gr code to say what's really going on through the pipeline, but perhaps this
is enough information to understand the motivation for the half sampling
frequency addition up front. Note that if you get it wrong, subpixel text tends
to just look subtly kerned incorrectly, but it is noticeable.

[Kimmo Kinnunen, 2014-06-11 12:33:24]

Perf tries:

bench_pictures -r $skps/ --config 8888 --repeat 30 

bench --match ext --config 8888


X86 results bench:
                text_16_BW_BK 8888 c          1.93       1.96      -0.03   
-1.6% 
               text_16_LCD_BK 8888 c          4.89       4.92      -0.03   
-0.6% 
                text_16_BW_FF 8888 c          1.94       1.95      -0.01   
-0.5% 
 picture_playback_drawPosText 8888 c        118.92     119.04      -0.12   
-0.1% 
                text_16_AA_FF 8888 c          3.75       3.75      +0.00   
+0.0% 
                text_16_BW_88 8888 c          2.01       2.01      +0.00   
+0.0% 
                text_16_AA_88 8888 c          3.75       3.74      +0.01   
+0.3% 
            text_16_pos_AA_BK 8888 c          3.63       3.62      +0.01   
+0.3% 
               text_16_LCD_88 8888 c          5.25       5.22      +0.03   
+0.6% 
    picture_playback_drawText 8888 c        108.49     107.64      +0.85   
+0.8% 
               text_16_LCD_FF 8888 c          4.95       4.91      +0.04   
+0.8% 
                text_16_AA_BK 8888 c          3.49       3.46      +0.03   
+0.9% 
      cmap_paint_textToGlyphs 8888 c          4.60       4.54      +0.06   
+1.3% 
      cmap_paint_containsText 8888 c          4.64       4.50      +0.14   
+3.0% 
picture_playback_drawPosTextH 8888 c        120.18     109.45     +10.73   
+8.9% 

X86 results bench_pictures:
              tabl_gspro.skp simple c          4.60       4.67      -0.07   
-1.5% 
            desk_booking.skp simple c          9.26       9.34      -0.08   
-0.9% 
         tabl_deviantart.skp simple c          5.98       6.03      -0.05   
-0.8% 
         desk_forecastio.skp simple c          3.69       3.72      -0.03   
-0.8% 
             tabl_nofolo.skp simple c         16.31      16.43      -0.12   
-0.7% 
          tabl_vnexpress.skp simple c          9.27       9.30      -0.03   
-0.3% 
               desk_espn.skp simple c          7.05       7.07      -0.02   
-0.3% 
                tabl_mlb.skp simple c         10.92      10.95      -0.03   
-0.3% 
            tabl_nytimes.skp simple c          3.81       3.82      -0.01   
-0.3% 
               tabl_digg.skp simple c         30.82      30.90      -0.08   
-0.3% 
               desk_ebay.skp simple c         22.52      22.53      -0.01   
-0.0% 
  desk_googlespreadsheet.skp simple c          4.68       4.68      +0.00   
+0.0% 
 desk_jsfiddlehumperclip.skp simple c          0.83       0.83      +0.00   
+0.0% 
desk_rectangletransition.skp simple c          0.32       0.32      +0.00   
+0.0% 
             desk_sfgate.skp simple c         12.72      12.72      +0.00   
+0.0% 
            desk_weather.skp simple c          7.82       7.82      +0.00   
+0.0% 
       desk_yahooanswers.skp simple c          4.32       4.32      +0.00   
+0.0% 
           desk_facebook.skp simple c         49.68      49.63      +0.05   
+0.1% 
         desk_techcrunch.skp simple c         23.67      23.60      +0.07   
+0.3% 
            desk_twitter.skp simple c          9.02       8.99      +0.03   
+0.3% 
            tabl_sahadan.skp simple c          2.92       2.91      +0.01   
+0.3% 
            desk_blogger.skp simple c         21.63      21.47      +0.16   
+0.7% 
               tabl_hsfi.skp simple c          1.30       1.29      +0.01   
+0.8% 
        desk_pokemonwiki.skp simple c         78.01      77.31      +0.70   
+0.9% 
         desk_mobilenews.skp simple c          9.30       9.20      +0.10   
+1.1% 
          desk_pinterest.skp simple c          2.02       1.99      +0.03   
+1.5% 
           desk_fontwipe.skp simple c          0.38       0.37      +0.01   
+2.6% 


For ARM, (-d nvidia_logan) I get pretty random bench results, going both ways.
It's likely that my measurement setup is a bit unreliable atm.. The
bench_pictures results are consistently not having any impact.

Bench ARM:

RESULT 9
picture_playback_drawPosTextH 8888 c        862.54    2222.03   -1359.49 
-157.6% 
 picture_playback_drawPosText 8888 c       1725.73    1870.70    -144.97   
-8.4% 
            text_16_pos_AA_BK 8888 c          8.47       8.57      -0.10   
-1.2% 
               text_16_LCD_88 8888 c         11.41      11.48      -0.07   
-0.6% 
      cmap_paint_textToGlyphs 8888 c          9.46       9.51      -0.05   
-0.5% 
                text_16_AA_FF 8888 c         10.91      10.96      -0.05   
-0.5% 
    picture_playback_drawText 8888 c        614.57     616.93      -2.36   
-0.4% 
                text_16_AA_BK 8888 c          8.81       8.82      -0.01   
-0.1% 
               text_16_LCD_FF 8888 c         11.03      11.04      -0.01   
-0.1% 
                text_16_AA_88 8888 c         11.47      11.48      -0.01   
-0.1% 
      cmap_paint_containsText 8888 c          9.30       9.26      +0.04   
+0.4% 
               text_16_LCD_BK 8888 c          8.94       8.90      +0.04   
+0.4% 
                text_16_BW_BK 8888 c          3.98       3.96      +0.02   
+0.5% 
                text_16_BW_FF 8888 c          3.98       3.95      +0.03   
+0.8% 
                text_16_BW_88 8888 c          6.15       6.10      +0.05   
+0.8% 
RESULT 10
                text_16_BW_FF 8888 c          3.93       3.96      -0.03   
-0.8% 
                text_16_AA_FF 8888 c         10.14      10.19      -0.05   
-0.5% 
                text_16_BW_BK 8888 c          3.96       3.97      -0.01   
-0.3% 
      cmap_paint_textToGlyphs 8888 c          9.49       9.49      +0.00   
+0.0% 
      cmap_paint_containsText 8888 c          9.27       9.25      +0.02   
+0.2% 
                text_16_BW_88 8888 c          5.86       5.82      +0.04   
+0.7% 
               text_16_LCD_88 8888 c         10.81      10.71      +0.10   
+0.9% 
               text_16_LCD_BK 8888 c          8.42       8.34      +0.08   
+1.0% 
            text_16_pos_AA_BK 8888 c          8.62       8.53      +0.09   
+1.0% 
                text_16_AA_BK 8888 c          8.33       8.24      +0.09   
+1.1% 
picture_playback_drawPosTextH 8888 c       1475.11    1457.98     +17.13   
+1.2% 
    picture_playback_drawText 8888 c        450.14     440.30      +9.84   
+2.2% 
               text_16_LCD_FF 8888 c         10.74      10.27      +0.47   
+4.4% 
                text_16_AA_88 8888 c         11.03      10.51      +0.52   
+4.7% 
 picture_playback_drawPosText 8888 c       2065.00    1679.77    +385.23  
+18.7%

https://codereview.chromium.org/196133014/diff/490001/src/core/SkDrawProcs.h
File src/core/SkDrawProcs.h (right):

https://codereview.chromium.org/196133014/diff/490001/src/core/SkDrawProcs.h#...
src/core/SkDrawProcs.h:100: if (SkPaint::kLeft_Align == fAlign) {
On 2014/06/09 13:48:07, jvanverth1 wrote:
> If I understand this correctly, this seems inefficient. The point of the procs
> is to do the conditional check once and then just call the appropriate
function.
> Have you done a comparison between this and the old Bitmap code using 'bench'
or
> 'bench_pictures'?

I'm not an expert, but afaik function pointers aren't really efficient with
modern compilers and processors. The compiler can avoid the call and the checks
if function pointers are not used.

The change is neutral or very slight perf increase, from what I can test.

If we are micro-optimizing now, then it probably would make sense to transform
the TextMapState to same pattern. I changed the new patch to this. Results are
in the main comments.

https://codereview.chromium.org/196133014/diff/490001/src/gpu/GrStencilAndCov...
File src/gpu/GrStencilAndCoverTextContext.cpp (right):

https://codereview.chromium.org/196133014/diff/490001/src/gpu/GrStencilAndCov...
src/gpu/GrStencilAndCoverTextContext.cpp:101: SkFixed fx = SkScalarToFixed(x) +
halfSampleX;
On 2014/06/09 13:48:07, jvanverth1 wrote:
> Doesn't this lead to glyphs being off by half a pixel? I had to remove it in
the
> distance field code.

Done.

https://codereview.chromium.org/196133014/diff/490001/src/gpu/GrStencilAndCov...
src/gpu/GrStencilAndCoverTextContext.cpp:176: SkScalar x = tms.fLoc.fX +
halfSampleX;
On 2014/06/09 13:48:07, jvanverth1 wrote:
> Same issue with half-pixel alignment as above?  (and others, below)

Done.

https://codereview.chromium.org/196133014/diff/490001/src/gpu/GrStencilAndCov...
src/gpu/GrStencilAndCoverTextContext.cpp:212: if (SkPaint::kLeft_Align ==
fSkPaint.getTextAlign()) {
On 2014/06/09 13:48:07, jvanverth1 wrote:
> Do we need to support non-subpixel layout when drawing with path rendering?

Done.

https://codereview.chromium.org/196133014/diff/490001/src/gpu/GrStencilAndCov...
src/gpu/GrStencilAndCoverTextContext.cpp:306: if
(fContext->getMatrix().invert(&inv)) {
On 2014/06/09 13:48:07, jvanverth1 wrote:
> This won't work for non-uniform scale.

Right.. 

> And if you're assuming uniform scale, you
> don't have to invert the whole matrix, just take 1/length of SkVector(kScaleX,
> kSkewY).

I changed to what you suggested. 

I added a test to test transformed hairline text drawing. Since it doesn't seem
to be that stellar with the default implementations, I'm hoping the buggy
non-uniform case suffices for now for NVPR initial implementation.

[Kimmo Kinnunen, 2014-06-11 12:39:21]

https://codereview.chromium.org/196133014/diff/490001/src/gpu/GrStencilAndCov...
File src/gpu/GrStencilAndCoverTextContext.cpp (right):

https://codereview.chromium.org/196133014/diff/490001/src/gpu/GrStencilAndCov...
src/gpu/GrStencilAndCoverTextContext.cpp:212: if (SkPaint::kLeft_Align ==
fSkPaint.getTextAlign()) {
On 2014/06/11 12:33:24, kkinnunen wrote:
> On 2014/06/09 13:48:07, jvanverth1 wrote:
> > Do we need to support non-subpixel layout when drawing with path rendering?
> 
> Done.

Actually, I mean to say that I couldn't get the glyph positions match the 8888
or gpu rendering in all cases without this case.

[Kimmo Kinnunen, 2014-06-11 12:53:35]

And note, there's something still inconsistent with the stroked hairline text
rendered with nvpr, since that produces the same 'error' as 8888/gpu/msaa, where
as filled hairline text with nvpr produces a different result.

These are seen by contrasting glyph_pos_h_s_nvprmsaa4.png vs
glyph_pos_h_f_nvprmsaa4.png

glyph_pos_h_s_nvprmsaa4.png
 - glyph positions do not match what SkPaint::getTextWidths say, but match the
8888/gpu/msaa

glyph_pos_h_f_nvprmsaa4.png
 - glyph positions do match what SkPaint::getTextWidths say, but do not match
the 8888/gpu/msaa

If this is a problem, maybe you would have insight which of the rendering is
correct? E.g is the testcase wrong with using the SkPaint APIs or is the
8888/gpu/msaa image wrong?

[Kimmo Kinnunen, 2014-06-12 12:59:16]

I moved the code not strictly related to nvpr to these patches:
https://codereview.chromium.org/335573002/
https://codereview.chromium.org/335603003/

I think those patches could be reviewed first. 

This patch produces different rendering compared to 8888/gpu, so I'll work on
this a bit longer before it needs to be reviewed again.

[Kimmo Kinnunen, 2014-06-13 12:52:40]

Now I think I got all the variants matching.. Does it make sense now?

[jvanverth1, 2014-06-19 15:18:12]

On 2014/06/13 12:52:40, kkinnunen wrote:
> Now I think I got all the variants matching.. Does it make sense now?

lgtm

[Kimmo Kinnunen, 2014-06-23 05:57:34]

The CQ bit was checked by kkinnunen@nvidia.com

[commit-bot: I haz the power, 2014-06-23 05:57:42]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/kkinnunen@nvidia.com/196133014/590001

[commit-bot: I haz the power, 2014-06-23 06:11:52]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  Build-Win7-VS2010-x86-Debug-Trybot on tryserver.skia
(http://108.170.220.76:10117/builders/Build-Win7-VS2010-x86-Debug-Trybot/build...)

[commit-bot: I haz the power, 2014-06-23 06:32:01]

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

[commit-bot: I haz the power, 2014-06-23 06:32:04]

Try jobs failed on following builders:
  Build-Win7-VS2010-x86-Debug-Trybot on tryserver.skia
(http://108.170.220.76:10117/builders/Build-Win7-VS2010-x86-Debug-Trybot/build...)

[Kimmo Kinnunen, 2014-06-24 06:56:18]

The CQ bit was checked by kkinnunen@nvidia.com

[commit-bot: I haz the power, 2014-06-24 06:57:22]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/kkinnunen@nvidia.com/196133014/610001

[commit-bot: I haz the power, 2014-06-24 07:12:34]

Change committed as c6cb56f36c4aad8ed45486a3bb4de614bb822f1b