Add a GrPathRange class

Add a GrPathRange class

Adds a GrPathRange object that represents a range of paths on the gpu.
Updates GrDrawTarget::drawPaths and supporting code to use GrPathRange
instead of an array of GrPath objects.

Change-Id: I67845f3893cd4d955db947d699aa3733cbb081e0

BUG=skia:

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

[bsalomon, 2014-07-17 17:27:28]

https://codereview.chromium.org/400713003/diff/1/include/gpu/GrTypes.h
File include/gpu/GrTypes.h (right):

https://codereview.chromium.org/400713003/diff/1/include/gpu/GrTypes.h#newcod...
include/gpu/GrTypes.h:217: static inline int GrTransformSize(GrTransformFormat
format) {
Can we call it Type rather than Format?

Also, maybe PathTransformType since we're only using it for paths and we have
GrCoordTransform?

Should we put this inside GrDrawTarget?

https://codereview.chromium.org/400713003/diff/1/src/gpu/GrDrawTarget.cpp
File src/gpu/GrDrawTarget.cpp (right):

https://codereview.chromium.org/400713003/diff/1/src/gpu/GrDrawTarget.cpp#new...
src/gpu/GrDrawTarget.cpp:562: // support NV_blend_equation_advanced.
Where's that CL? :)

https://codereview.chromium.org/400713003/diff/1/src/gpu/GrDrawTarget.h
File src/gpu/GrDrawTarget.h (right):

https://codereview.chromium.org/400713003/diff/1/src/gpu/GrDrawTarget.h#newco...
src/gpu/GrDrawTarget.h:353: * @param pathRange        Source of paths to draw
from, stroke must not be
Is this comment right or should the hairline text be part of the fill param?

https://codereview.chromium.org/400713003/diff/1/src/gpu/GrInOrderDrawBuffer.cpp
File src/gpu/GrInOrderDrawBuffer.cpp (right):

https://codereview.chromium.org/400713003/diff/1/src/gpu/GrInOrderDrawBuffer....
src/gpu/GrInOrderDrawBuffer.cpp:472: dp->fIndices = SkNEW_ARRAY(uint32_t,
count); // TODO: Accomplish this without a malloc
What's the plan here? An immutable ref-cnt'ed array of indices?

https://codereview.chromium.org/400713003/diff/1/src/gpu/GrPathRange.h
File src/gpu/GrPathRange.h (right):

https://codereview.chromium.org/400713003/diff/1/src/gpu/GrPathRange.h#newcode26
src/gpu/GrPathRange.h:26: GrPathRange(GrGpu* gpu, bool isWrapped, size_t size,
const SkStrokeRec& stroke)
given that we don't have a GrContext::wrapPathRange function should we just omit
the isWrapped param?

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

https://codereview.chromium.org/400713003/diff/1/src/gpu/GrStencilAndCoverTex...
src/gpu/GrStencilAndCoverTextContext.cpp:31:
GrResourceKey::GenerateResourceType();
I think the type should come from GrPathRange itself. The domain below will
guarantee that the key won't collide with keys for other path arrays from other
domains. It'll fit better in with the future evolution of the cache.

[Chris Dalton, 2014-07-17 18:15:19]

https://codereview.chromium.org/400713003/diff/1/include/gpu/GrTypes.h
File include/gpu/GrTypes.h (right):

https://codereview.chromium.org/400713003/diff/1/include/gpu/GrTypes.h#newcod...
include/gpu/GrTypes.h:217: static inline int GrTransformSize(GrTransformFormat
format) {
On 2014/07/17 17:27:28, bsalomon wrote:
> Can we call it Type rather than Format?
> 
> Also, maybe PathTransformType since we're only using it for paths and we have
> GrCoordTransform?
> 
> Should we put this inside GrDrawTarget?

My motivation for calling it Format was because it describes the format of a
float* array of transforms: 'drawPaths(..., transforms, transformsFormat, ...)',
but then again, the parameter is named Type in nvpr :). I guess don't have a
strong preference.

I'll add Path to the name.

I thought GrTypes would be a good spot because it's alongside other enums that
basically map GLenums, complete with helper functions. But it is a bit weird
that it's in the include/ directory and there are no public APIs that use it.
Are all the other enums in this file used publicly? Were you thinking of moving
it to a nested enum within GrDrawTarget?

https://codereview.chromium.org/400713003/diff/1/src/gpu/GrDrawTarget.cpp
File src/gpu/GrDrawTarget.cpp (right):

https://codereview.chromium.org/400713003/diff/1/src/gpu/GrDrawTarget.cpp#new...
src/gpu/GrDrawTarget.cpp:562: // support NV_blend_equation_advanced.
On 2014/07/17 17:27:28, bsalomon wrote:
> Where's that CL? :)

That's what I keep wondering :-\

https://codereview.chromium.org/400713003/diff/1/src/gpu/GrDrawTarget.h
File src/gpu/GrDrawTarget.h (right):

https://codereview.chromium.org/400713003/diff/1/src/gpu/GrDrawTarget.h#newco...
src/gpu/GrDrawTarget.h:353: * @param pathRange        Source of paths to draw
from, stroke must not be
On 2014/07/17 17:27:28, bsalomon wrote:
> Is this comment right or should the hairline text be part of the fill param?

The old comment wasn't correct, hairline is a stroking style. The fill type is
winding/even-odd/inverse/etc.

But now that I think about it, we don't pass a stroke style to this method
anymore, so GrPathRange seems like the more natural place to enforce the no
hairline requirement. That's where we setup the nvpr stroking parameters, which
are what don't have hairline.

https://codereview.chromium.org/400713003/diff/1/src/gpu/GrInOrderDrawBuffer.cpp
File src/gpu/GrInOrderDrawBuffer.cpp (right):

https://codereview.chromium.org/400713003/diff/1/src/gpu/GrInOrderDrawBuffer....
src/gpu/GrInOrderDrawBuffer.cpp:472: dp->fIndices = SkNEW_ARRAY(uint32_t,
count); // TODO: Accomplish this without a malloc
On 2014/07/17 17:27:28, bsalomon wrote:
> What's the plan here? An immutable ref-cnt'ed array of indices?

I don't quite see where you are going with the immutable ref-cnt'ed array.. I
might need more explanation.

I was thinking something along the lines of hopefully storing the
index/transform data alongside the DrawPaths struct in memory (so having
variable sized DrawPaths entries), and when we run out of space in the queue,
flushing instead of allocating more.

https://codereview.chromium.org/400713003/diff/1/src/gpu/GrPathRange.h
File src/gpu/GrPathRange.h (right):

https://codereview.chromium.org/400713003/diff/1/src/gpu/GrPathRange.h#newcode26
src/gpu/GrPathRange.h:26: GrPathRange(GrGpu* gpu, bool isWrapped, size_t size,
const SkStrokeRec& stroke)
On 2014/07/17 17:27:28, bsalomon wrote:
> given that we don't have a GrContext::wrapPathRange function should we just
omit
> the isWrapped param?

Yeah that seems fine. I'm not too familiar with the concept of wrapping, I was
just copying GrPath.

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

https://codereview.chromium.org/400713003/diff/1/src/gpu/GrStencilAndCoverTex...
src/gpu/GrStencilAndCoverTextContext.cpp:31:
GrResourceKey::GenerateResourceType();
On 2014/07/17 17:27:28, bsalomon wrote:
> I think the type should come from GrPathRange itself. The domain below will
> guarantee that the key won't collide with keys for other path arrays from
other
> domains. It'll fit better in with the future evolution of the cache.

Acknowledged.

[Chris Dalton, 2014-07-17 22:08:23]

This one should address all the comments up to now.

[Chris Dalton, 2014-07-18 20:56:52]

Adding Kimmo (currently out of office) as an FYI

[bsalomon, 2014-07-21 14:11:08]

lgtm

https://codereview.chromium.org/400713003/diff/1/src/gpu/GrInOrderDrawBuffer.cpp
File src/gpu/GrInOrderDrawBuffer.cpp (right):

https://codereview.chromium.org/400713003/diff/1/src/gpu/GrInOrderDrawBuffer....
src/gpu/GrInOrderDrawBuffer.cpp:472: dp->fIndices = SkNEW_ARRAY(uint32_t,
count); // TODO: Accomplish this without a malloc
On 2014/07/17 18:15:19, Chris Dalton wrote:
> On 2014/07/17 17:27:28, bsalomon wrote:
> > What's the plan here? An immutable ref-cnt'ed array of indices?
> 
> I don't quite see where you are going with the immutable ref-cnt'ed array.. I

I was thinking that drawPaths() could take a index array class type instead of a
ptr/count. The object would be immutable and refcnted. Then the DrawPaths struct
could just ref it, knowing that the client can't change the contents of the
index array, and no copy would needed.

> might need more explanation.
> 
> I was thinking something along the lines of hopefully storing the
> index/transform data alongside the DrawPaths struct in memory (so having
> variable sized DrawPaths entries), and when we run out of space in the queue,
> flushing instead of allocating more.

[Chris Dalton, 2014-07-21 17:14:38]

The CQ bit was checked by cdalton@nvidia.com

[Chris Dalton, 2014-07-21 17:14:50]

The CQ bit was unchecked by cdalton@nvidia.com

[Chris Dalton, 2014-07-21 17:14:51]

The CQ bit was checked by cdalton@nvidia.com

[commit-bot: I haz the power, 2014-07-21 17:15:02]

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

[commit-bot: I haz the power, 2014-07-21 17:27:23]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  Build-Mac10.8-Clang-x86-Release-Trybot on tryserver.skia
(http://108.170.220.76:10117/builders/Build-Mac10.8-Clang-x86-Release-Trybot/b...)
  Build-Mac10.8-Clang-x86_64-Release-Trybot on tryserver.skia
(http://108.170.220.76:10117/builders/Build-Mac10.8-Clang-x86_64-Release-Trybo...)
  Build-Ubuntu13.10-Clang-x86_64-Debug-Trybot on tryserver.skia
(http://108.170.220.76:10117/builders/Build-Ubuntu13.10-Clang-x86_64-Debug-Try...)
  Build-Ubuntu13.10-GCC4.8-Arm7-Debug-Android-Trybot on tryserver.skia
(http://108.170.220.76:10117/builders/Build-Ubuntu13.10-GCC4.8-Arm7-Debug-Andr...)
  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-...)
  Build-Win-VS2013-x86-Debug-Trybot on tryserver.skia
(http://108.170.220.76:10117/builders/Build-Win-VS2013-x86-Debug-Trybot/builds...)

[commit-bot: I haz the power, 2014-07-21 17:29:32]

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

[commit-bot: I haz the power, 2014-07-21 17:29:33]

Try jobs failed on following builders:
  Build-Ubuntu13.10-GCC4.8-Arm7-Debug-Android-Trybot on tryserver.skia
(http://108.170.220.76:10117/builders/Build-Ubuntu13.10-GCC4.8-Arm7-Debug-Andr...)

[Chris Dalton, 2014-07-21 17:52:13]

The CQ bit was checked by cdalton@nvidia.com

[commit-bot: I haz the power, 2014-07-21 17:52:42]

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

[commit-bot: I haz the power, 2014-07-21 18:04:22]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  Build-Mac10.8-Clang-x86-Release-Trybot on tryserver.skia
(http://108.170.220.76:10117/builders/Build-Mac10.8-Clang-x86-Release-Trybot/b...)
  Build-Mac10.8-Clang-x86_64-Release-Trybot on tryserver.skia
(http://108.170.220.76:10117/builders/Build-Mac10.8-Clang-x86_64-Release-Trybo...)
  Build-Ubuntu13.10-Clang-x86_64-Debug-Trybot on tryserver.skia
(http://108.170.220.76:10117/builders/Build-Ubuntu13.10-Clang-x86_64-Debug-Try...)
  Build-Ubuntu13.10-GCC4.8-Arm7-Debug-Android-Trybot on tryserver.skia
(http://108.170.220.76:10117/builders/Build-Ubuntu13.10-GCC4.8-Arm7-Debug-Andr...)
  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-...)
  Build-Win-VS2013-x86-Debug-Trybot on tryserver.skia
(http://108.170.220.76:10117/builders/Build-Win-VS2013-x86-Debug-Trybot/builds...)

[commit-bot: I haz the power, 2014-07-21 18:06:24]

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

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

Try jobs failed on following builders:
  Build-Ubuntu13.10-GCC4.8-Arm7-Debug-Android-Trybot on tryserver.skia
(http://108.170.220.76:10117/builders/Build-Ubuntu13.10-GCC4.8-Arm7-Debug-Andr...)

[Chris Dalton, 2014-07-21 21:03:42]

I'll wait a minute in case anybody wants to review the SkRefPtr<> ->
SkAutoTUnref<> cahnges, but if not I'll just go ahead and submit later toady.
They are very trivial.

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

On 2014/07/21 21:03:42, Chris Dalton wrote:
> I'll wait a minute in case anybody wants to review the SkRefPtr<> ->
> SkAutoTUnref<> cahnges, but if not I'll just go ahead and submit later toady.
> They are very trivial.

lgtm

[Chris Dalton, 2014-07-21 21:17:26]

The CQ bit was checked by cdalton@nvidia.com

[commit-bot: I haz the power, 2014-07-21 21:18:16]

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

[commit-bot: I haz the power, 2014-07-21 21:29:40]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  Build-Mac10.8-Clang-x86-Release-Trybot on tryserver.skia
(http://108.170.220.76:10117/builders/Build-Mac10.8-Clang-x86-Release-Trybot/b...)
  Build-Ubuntu13.10-Clang-x86_64-Debug-Trybot on tryserver.skia
(http://108.170.220.76:10117/builders/Build-Ubuntu13.10-Clang-x86_64-Debug-Try...)
  Build-Win-VS2013-x86-Debug-Trybot on tryserver.skia
(http://108.170.220.76:10117/builders/Build-Win-VS2013-x86-Debug-Trybot/builds...)

[commit-bot: I haz the power, 2014-07-21 21:33:49]

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

[commit-bot: I haz the power, 2014-07-21 21:33:50]

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

[Chris Dalton, 2014-07-21 21:34:47]

The CQ bit was checked by cdalton@nvidia.com

[commit-bot: I haz the power, 2014-07-21 21:35:29]

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

[commit-bot: I haz the power, 2014-07-21 21:47:50]

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

[commit-bot: I haz the power, 2014-07-21 21:54:29]

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

[commit-bot: I haz the power, 2014-07-21 21:54:30]

Try jobs failed on following builders:
  Build-Win-VS2013-x86-Debug-Trybot on tryserver.skia
(http://108.170.220.76:10117/builders/Build-Win-VS2013-x86-Debug-Trybot/builds...)

[Chris Dalton, 2014-07-21 22:02:14]

The CQ bit was checked by cdalton@nvidia.com

[commit-bot: I haz the power, 2014-07-21 22:02:40]

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

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

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

[Chris Dalton, 2014-07-21 22:18:55]

The CQ bit was checked by cdalton@nvidia.com

[commit-bot: I haz the power, 2014-07-21 22:19:49]

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

[commit-bot: I haz the power, 2014-07-21 22:32:50]

Change committed as b85a0aab6905af8b329539b7573a7555b727d5e5

[Nico, 2014-08-12 04:20:21]

https://codereview.chromium.org/400713003/diff/140001/src/gpu/GrDrawTarget.h
File src/gpu/GrDrawTarget.h (right):

https://codereview.chromium.org/400713003/diff/140001/src/gpu/GrDrawTarget.h#...
src/gpu/GrDrawTarget.h:358: PathTransformSize(transformsType) × count elements
Can you replace this with ascii please? On some locales, windows's cl.exe has
trouble with non-ascii.