Improve caching of dashed paths in GrStencilAndCoverPathRenderer

Improve caching of dashed paths in GrStencilAndCoverPathRenderer

Improve caching of dashed paths in GrStencilAndCoverPathRenderer.
Look up the (NVPR specific) GrGLPath based on GrStrokeInfo and
the original path.

Use unique keys for all GrPaths.

Dash the path with Skia dash stroker and use that path geometry for
NVPR path.

NVPR internal dashing stroke is not used, because the dashing
implementation of NVPR does not match Skia implementation.

Committed: https://skia.googlesource.com/skia/+/50b58e6fbcc50785ceffacb2c51b22c6e67a7ab7

[Kimmo Kinnunen, 2015-05-04 11:49:58]

kkinnunen@nvidia.com changed reviewers:
+ egdaniel@google.com

[Kimmo Kinnunen, 2015-05-04 11:49:58]

This would be ready for review.

[egdaniel, 2015-05-05 19:48:44]

https://codereview.chromium.org/1116123003/diff/20001/src/gpu/GrPath.cpp
File src/gpu/GrPath.cpp (right):

https://codereview.chromium.org/1116123003/diff/20001/src/gpu/GrPath.cpp#newc...
src/gpu/GrPath.cpp:56: key ^= get_top_n_float_bits<32>(intervals[i]);
So this doesn't seem like it will give a unique key by just xoring.  When this
function was originally written I'm pretty sure we had a fixed length key, thus
the uint64. Now we can have variable length keys so maybe just add all the
intervals to the key?

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

https://codereview.chromium.org/1116123003/diff/20001/src/gpu/GrStencilAndCov...
src/gpu/GrStencilAndCoverTextContext.cpp:333: if (!fStroke.isDashed() &&
(otherBackendsWillDrawAsPaths || kMaxPerformance_RenderMode == renderMode)) {
100 chars

https://codereview.chromium.org/1116123003/diff/20001/src/gpu/GrStrokeInfo.h
File src/gpu/GrStrokeInfo.h (right):

https://codereview.chromium.org/1116123003/diff/20001/src/gpu/GrStrokeInfo.h#...
src/gpu/GrStrokeInfo.h:64: fIntervals.count() != other.fIntervals.count() ||
align these lines under each other? It could be that there is a tab char that my
viewer is showing as 8 chars. If that is the case just turn into spaces.

[Kimmo Kinnunen, 2015-05-06 08:30:26]

https://codereview.chromium.org/1116123003/diff/20001/src/gpu/GrPath.cpp
File src/gpu/GrPath.cpp (right):

https://codereview.chromium.org/1116123003/diff/20001/src/gpu/GrPath.cpp#newc...
src/gpu/GrPath.cpp:56: key ^= get_top_n_float_bits<32>(intervals[i]);
On 2015/05/05 19:48:44, egdaniel wrote:
> So this doesn't seem like it will give a unique key by just xoring.  When this
> function was originally written I'm pretty sure we had a fixed length key,
thus
> the uint64. Now we can have variable length keys so maybe just add all the
> intervals to the key?

Done.

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

https://codereview.chromium.org/1116123003/diff/20001/src/gpu/GrStencilAndCov...
src/gpu/GrStencilAndCoverTextContext.cpp:333: if (!fStroke.isDashed() &&
(otherBackendsWillDrawAsPaths || kMaxPerformance_RenderMode == renderMode)) {
On 2015/05/05 19:48:44, egdaniel wrote:
> 100 chars

Done.

https://codereview.chromium.org/1116123003/diff/20001/src/gpu/GrStrokeInfo.h
File src/gpu/GrStrokeInfo.h (right):

https://codereview.chromium.org/1116123003/diff/20001/src/gpu/GrStrokeInfo.h#...
src/gpu/GrStrokeInfo.h:64: fIntervals.count() != other.fIntervals.count() ||
On 2015/05/05 19:48:44, egdaniel wrote:
> align these lines under each other? It could be that there is a tab char that
my
> viewer is showing as 8 chars. If that is the case just turn into spaces.

It was intentional -- some reviewers want *me* to hilight the "if" body contents
by indenting the multiline condition irregularly by extra 4 spaces.

Done.

[egdaniel, 2015-05-15 15:28:53]

https://codereview.chromium.org/1116123003/diff/100001/src/gpu/GrPath.cpp
File src/gpu/GrPath.cpp (right):

https://codereview.chromium.org/1116123003/diff/100001/src/gpu/GrPath.cpp#new...
src/gpu/GrPath.cpp:16: stroke.asUniqueKeyFragment(&builder[2]);
We should probably Assert here the computeUniqueKeyFragmentData32Cnt is not 0.

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

https://codereview.chromium.org/1116123003/diff/100001/src/gpu/GrStencilAndCo...
src/gpu/GrStencilAndCoverTextContext.cpp:233:
stroke.asUniqueKeyFragment(&builder[2]);
Same with asserting computeUKFD32C is not zero

https://codereview.chromium.org/1116123003/diff/100001/src/gpu/GrStrokeInfo.h
File src/gpu/GrStrokeInfo.h (right):

https://codereview.chromium.org/1116123003/diff/100001/src/gpu/GrStrokeInfo.h...
src/gpu/GrStrokeInfo.h:148: int computeUniqueKeyFragmentData32Cnt() const {
This function definitely needs some comments

https://codereview.chromium.org/1116123003/diff/100001/src/gpu/GrStrokeInfo.h...
src/gpu/GrStrokeInfo.h:150: int strokeKeyData32Cnt = 1 + 2 * kSkScalarData32Cnt;
Is this trying to account for values that are is SkStrokeRec?

https://codereview.chromium.org/1116123003/diff/100001/src/gpu/GrStrokeInfo.h...
src/gpu/GrStrokeInfo.h:152: strokeKeyData32Cnt += (1 + this->getDashCount()) *
kSkScalarData32Cnt;
is the 1 for the phase here?

[Kimmo Kinnunen, 2015-05-18 06:48:40]

Thanks!

https://codereview.chromium.org/1116123003/diff/100001/src/gpu/GrPath.cpp
File src/gpu/GrPath.cpp (right):

https://codereview.chromium.org/1116123003/diff/100001/src/gpu/GrPath.cpp#new...
src/gpu/GrPath.cpp:16: stroke.asUniqueKeyFragment(&builder[2]);
On 2015/05/15 15:28:53, egdaniel wrote:
> We should probably Assert here the computeUniqueKeyFragmentData32Cnt is not 0.

Hmm. I expect you hint at the Builder operator[] assertion? good point. 
The intention is that the stroke decides how big representation it needs. If it
needs 0, it can request that. If this is the decision, then
"asUniqueKeyFragment" would just not write anything. This should be opaque to
the call site. Asserting non-zero would then rise the question: why is this
call-site expecting the stroke to be represented by this specific length or
above?

I could also do an if?

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

https://codereview.chromium.org/1116123003/diff/100001/src/gpu/GrStencilAndCo...
src/gpu/GrStencilAndCoverTextContext.cpp:233:
stroke.asUniqueKeyFragment(&builder[2]);
On 2015/05/15 15:28:53, egdaniel wrote:
> Same with asserting computeUKFD32C is not zero

Done.

https://codereview.chromium.org/1116123003/diff/100001/src/gpu/GrStrokeInfo.h
File src/gpu/GrStrokeInfo.h (right):

https://codereview.chromium.org/1116123003/diff/100001/src/gpu/GrStrokeInfo.h...
src/gpu/GrStrokeInfo.h:148: int computeUniqueKeyFragmentData32Cnt() const {
On 2015/05/15 15:28:53, egdaniel wrote:
> This function definitely needs some comments

Done.

https://codereview.chromium.org/1116123003/diff/100001/src/gpu/GrStrokeInfo.h...
src/gpu/GrStrokeInfo.h:150: int strokeKeyData32Cnt = 1 + 2 * kSkScalarData32Cnt;
On 2015/05/15 15:28:53, egdaniel wrote:
> Is this trying to account for values that are is SkStrokeRec?

Yeah. I'll add the comments

https://codereview.chromium.org/1116123003/diff/100001/src/gpu/GrStrokeInfo.h...
src/gpu/GrStrokeInfo.h:152: strokeKeyData32Cnt += (1 + this->getDashCount()) *
kSkScalarData32Cnt;
On 2015/05/15 15:28:53, egdaniel wrote:
> is the 1 for the phase here?

Yeah

[egdaniel, 2015-05-18 13:43:47]

lgtm

https://codereview.chromium.org/1116123003/diff/100001/src/gpu/GrPath.cpp
File src/gpu/GrPath.cpp (right):

https://codereview.chromium.org/1116123003/diff/100001/src/gpu/GrPath.cpp#new...
src/gpu/GrPath.cpp:16: stroke.asUniqueKeyFragment(&builder[2]);
On 2015/05/18 06:48:40, Kimmo Kinnunen wrote:
> On 2015/05/15 15:28:53, egdaniel wrote:
> > We should probably Assert here the computeUniqueKeyFragmentData32Cnt is not
0.
> 
> Hmm. I expect you hint at the Builder operator[] assertion? good point. 
> The intention is that the stroke decides how big representation it needs. If
it
> needs 0, it can request that. If this is the decision, then
> "asUniqueKeyFragment" would just not write anything. This should be opaque to
> the call site. Asserting non-zero would then rise the question: why is this
> call-site expecting the stroke to be represented by this specific length or
> above?
> 
> I could also do an if?

Yeah it was the builder[2] that I was worried about hitting (which would
Assert). I like the "if" best. You're right that Asserting != 0 would be
strange.

[Kimmo Kinnunen, 2015-05-19 05:50:47]

The CQ bit was checked by kkinnunen@nvidia.com

[commit-bot: I haz the power, 2015-05-19 05:50:55]

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

[commit-bot: I haz the power, 2015-05-19 06:02:13]

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