Add stroking support to distance field path renderer

Add stroking support to distance field path renderer

Also slightly increases sizes of paths accepted for distance field
caching.

Committed: https://skia.googlesource.com/skia/+/5ce76efd1c847308c7bcac17bd87d567c42cd786

Committed: https://skia.googlesource.com/skia/+/73ee6770260dbeeabc4a78eee4f13533f0031211

Committed: https://skia.googlesource.com/skia/+/512e437e1e07159a258dd3c5b907576bd1aefc1e

[jvanverth1, 2015-11-19 18:51:04]

jvanverth@google.com changed reviewers:
+ robertphillips@google.com

[robertphillips, 2015-11-19 19:03:29]

lgtm + nits & stuff

https://codereview.chromium.org/1460873002/diff/80001/src/gpu/batches/GrAADis...
File src/gpu/batches/GrAADistanceFieldPathRenderer.cpp (right):

https://codereview.chromium.org/1460873002/diff/80001/src/gpu/batches/GrAADis...
src/gpu/batches/GrAADistanceFieldPathRenderer.cpp:103: SkScalar maxDim =
SkMaxScalar(bounds.width(), bounds.height());
// Outset by half the stroke width when stroking
?

if (SkStrokeRect::kFill_Stype != args.fStroke->getStyle())
?

Don't we have to take into account the mitre limit too in some cases ?

https://codereview.chromium.org/1460873002/diff/80001/src/gpu/batches/GrAADis...
src/gpu/batches/GrAADistanceFieldPathRenderer.cpp:127: SkPath fPath;
// The unique ID of the path involved in this draw. This may be different than
the unique ID in 'fPath' since that path may have resulted from a
SkStrokeRec::applyToPath call.

?

https://codereview.chromium.org/1460873002/diff/80001/src/gpu/batches/GrAADis...
src/gpu/batches/GrAADistanceFieldPathRenderer.cpp:538: } else {
Do we need 'newPath'? Can't we pass "&geometry.fPath" into applyToPath ?

https://codereview.chromium.org/1460873002/diff/80001/src/gpu/batches/GrAADis...
src/gpu/batches/GrAADistanceFieldPathRenderer.cpp:543: geometry.fAntiAlias =
args.fAntiAlias;
// Note: this is the generation ID of the _original_ path. When a new path is
generated due to stroking it is important that the original path's id is used
for caching.

https://codereview.chromium.org/1460873002/diff/80001/src/gpu/batches/GrAADis...
File src/gpu/batches/GrAADistanceFieldPathRenderer.h (right):

https://codereview.chromium.org/1460873002/diff/80001/src/gpu/batches/GrAADis...
src/gpu/batches/GrAADistanceFieldPathRenderer.h:37: public:
Why do we need the default ctor ?

https://codereview.chromium.org/1460873002/diff/80001/src/gpu/batches/GrAADis...
src/gpu/batches/GrAADistanceFieldPathRenderer.h:46: return other.fGenID ==
fGenID && other.fDimension == fDimension &&
tab this over ?

https://codereview.chromium.org/1460873002/diff/80001/src/gpu/batches/GrAADis...
src/gpu/batches/GrAADistanceFieldPathRenderer.h:55: SkStrokeRec fStroke;
extra \n ?

[jvanverth1, 2015-11-19 20:33:23]

Updated to take miter limit into account, plus the comments below:

https://codereview.chromium.org/1460873002/diff/80001/src/gpu/batches/GrAADis...
File src/gpu/batches/GrAADistanceFieldPathRenderer.cpp (right):

https://codereview.chromium.org/1460873002/diff/80001/src/gpu/batches/GrAADis...
src/gpu/batches/GrAADistanceFieldPathRenderer.cpp:103: SkScalar maxDim =
SkMaxScalar(bounds.width(), bounds.height());
On 2015/11/19 19:03:28, robertphillips wrote:
> // Outset by half the stroke width when stroking
> ?
> 
> if (SkStrokeRect::kFill_Stype != args.fStroke->getStyle())
> ?
> 
> Don't we have to take into account the mitre limit too in some cases ?

Done.

https://codereview.chromium.org/1460873002/diff/80001/src/gpu/batches/GrAADis...
src/gpu/batches/GrAADistanceFieldPathRenderer.cpp:127: SkPath fPath;
On 2015/11/19 19:03:28, robertphillips wrote:
> // The unique ID of the path involved in this draw. This may be different than
> the unique ID in 'fPath' since that path may have resulted from a
> SkStrokeRec::applyToPath call.
> 
> ?

Done.

https://codereview.chromium.org/1460873002/diff/80001/src/gpu/batches/GrAADis...
src/gpu/batches/GrAADistanceFieldPathRenderer.cpp:538: } else {
On 2015/11/19 19:03:28, robertphillips wrote:
> Do we need 'newPath'? Can't we pass "&geometry.fPath" into applyToPath ?

Done.

https://codereview.chromium.org/1460873002/diff/80001/src/gpu/batches/GrAADis...
src/gpu/batches/GrAADistanceFieldPathRenderer.cpp:543: geometry.fAntiAlias =
args.fAntiAlias;
On 2015/11/19 19:03:28, robertphillips wrote:
> // Note: this is the generation ID of the _original_ path. When a new path is
> generated due to stroking it is important that the original path's id is used
> for caching.

Done.

https://codereview.chromium.org/1460873002/diff/80001/src/gpu/batches/GrAADis...
File src/gpu/batches/GrAADistanceFieldPathRenderer.h (right):

https://codereview.chromium.org/1460873002/diff/80001/src/gpu/batches/GrAADis...
src/gpu/batches/GrAADistanceFieldPathRenderer.h:37: public:
On 2015/11/19 19:03:28, robertphillips wrote:
> Why do we need the default ctor ?

Added a comment. It's needed when allocating PathData, since fStroke has no
default constructor.

https://codereview.chromium.org/1460873002/diff/80001/src/gpu/batches/GrAADis...
src/gpu/batches/GrAADistanceFieldPathRenderer.h:46: return other.fGenID ==
fGenID && other.fDimension == fDimension &&
On 2015/11/19 19:03:28, robertphillips wrote:
> tab this over ?

Done.

https://codereview.chromium.org/1460873002/diff/80001/src/gpu/batches/GrAADis...
src/gpu/batches/GrAADistanceFieldPathRenderer.h:55: SkStrokeRec fStroke;
On 2015/11/19 19:03:28, robertphillips wrote:
> extra \n ?

Done.

[jvanverth1, 2015-11-19 20:35:06]

The CQ bit was checked by jvanverth@google.com

[jvanverth1, 2015-11-19 20:35:06]

The patchset sent to the CQ was uploaded after l-g-t-m from
robertphillips@google.com
Link to the patchset: https://codereview.chromium.org/1460873002/#ps120001
(title: "Take miter limit into account")

[commit-bot: I haz the power, 2015-11-19 20:35:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1460873002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1460873002/120001

[commit-bot: I haz the power, 2015-11-19 20:47:12]

Committed patchset #7 (id:120001) as
https://skia.googlesource.com/skia/+/5ce76efd1c847308c7bcac17bd87d567c42cd786

[jvanverth1, 2015-11-19 21:18:38]

A revert of this CL (patchset #7 id:120001) has been created in
https://codereview.chromium.org/1458233002/ by jvanverth@google.com.

The reason for reverting is: Asserting in nanobench and dm..

[jvanverth1, 2015-11-20 15:46:43]

Description was changed from

==========
Add stroking support to distance field path renderer

Also slightly increases sizes of paths accepted for distance field
caching.

Committed:
https://skia.googlesource.com/skia/+/5ce76efd1c847308c7bcac17bd87d567c42cd786
==========

to

==========
Add stroking support to distance field path renderer

Also slightly increases sizes of paths accepted for distance field
caching.

Committed:
https://skia.googlesource.com/skia/+/5ce76efd1c847308c7bcac17bd87d567c42cd786
==========

[jvanverth1, 2015-11-20 15:50:21]

Patchset #8 (id:140001) has been deleted

[jvanverth1, 2015-11-20 20:10:49]

The CQ bit was checked by jvanverth@google.com

[jvanverth1, 2015-11-20 20:10:49]

The patchset sent to the CQ was uploaded after l-g-t-m from
robertphillips@google.com
Link to the patchset: https://codereview.chromium.org/1460873002/#ps160001
(title: "Fix hash equality")

[commit-bot: I haz the power, 2015-11-20 20:10:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1460873002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1460873002/160001

[commit-bot: I haz the power, 2015-11-20 22:06:16]

Committed patchset #8 (id:160001) as
https://skia.googlesource.com/skia/+/73ee6770260dbeeabc4a78eee4f13533f0031211

[f(malita), 2015-11-22 16:44:59]

A revert of this CL (patchset #8 id:160001) has been created in
https://codereview.chromium.org/1465053002/ by fmalita@chromium.org.

The reason for reverting is: Valgrind failures:

http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-ShuttleA-GPU...

==7062== Use of uninitialised value of size 8
==7062==    at 0x8ECA9A:
AADistanceFieldPathBatch::onPrepareDraws(GrVertexBatch::Target*)
(SkTDynamicHash.h:94)
==7062==    by 0x90DCF5: GrVertexBatch::onPrepare(GrBatchFlushState*)
(GrVertexBatch.cpp:16)
==7062==    by 0x8B592A: GrDrawTarget::prepareBatches(GrBatchFlushState*)
(GrBatch.h:100)
==7062==    by 0x8B51EA: GrDrawingManager::flush() (GrDrawingManager.cpp:78)
==7062==    by 0x8AE568: GrContext::flush(int) (GrContext.cpp:191)
==7062==    by 0x6C118A: GrContext::getTestTarget(GrTestTarget*,
GrRenderTarget*) (GrTest.cpp:56)
==7062==    by 0x4F4D82: GrDrawingManager::ProgramUnitTest(GrContext*, int)
(GLProgramsTest.cpp:360)
==7062==    by 0x4F58EC: test_GLPrograms(skiatest::Reporter*, GrContextFactory*)
(GLProgramsTest.cpp:450)
==7062==    by 0x40925E: run_test(skiatest::Test*) (DM.cpp:1022)
==7062==    by 0x40BAC2: run_enclave_and_gpu_tests(SkTArray<Task, false>*)
(DM.cpp:1033)
==7062==    by 0x7621A9: SkTaskGroup::wait() (SkTaskGroup.cpp:88)
==7062==    by 0x40B756: dm_main() (DM.cpp:1124)
==7062==    by 0x40BA5F: main (DM.cpp:1211)
==7062==  Uninitialised value was created by a stack allocation
==7062==    at 0x8DB484: GrTest::TestStrokeRec(SkRandom*) (GrTestUtils.cpp:232)
==7062== 
{
   <insert_a_suppression_name_here>
   Memcheck:Value8
   fun:_ZN24AADistanceFieldPathBatch14onPrepareDrawsEPN13GrVertexBatch6TargetE
   fun:_ZN13GrVertexBatch9onPrepareEP17GrBatchFlushState
   fun:_ZN12GrDrawTarget14prepareBatchesEP17GrBatchFlushState
   fun:_ZN16GrDrawingManager5flushEv
   fun:_ZN9GrContext5flushEi
   fun:_ZN9GrContext13getTestTargetEP12GrTestTargetP14GrRenderTarget
   fun:_ZN16GrDrawingManager15ProgramUnitTestEP9GrContexti
   fun:_ZL15test_GLProgramsPN8skiatest8ReporterEP16GrContextFactory
   fun:_ZL8run_testPN8skiatest4TestE
   fun:_ZL25run_enclave_and_gpu_testsP8SkTArrayI4TaskLb0EE
   fun:_ZN11SkTaskGroup4waitEv
   fun:_Z7dm_mainv
   fun:main
}.

[jvanverth1, 2015-11-23 14:43:11]

Description was changed from

==========
Add stroking support to distance field path renderer

Also slightly increases sizes of paths accepted for distance field
caching.

Committed:
https://skia.googlesource.com/skia/+/5ce76efd1c847308c7bcac17bd87d567c42cd786

Committed:
https://skia.googlesource.com/skia/+/73ee6770260dbeeabc4a78eee4f13533f0031211
==========

to

==========
Add stroking support to distance field path renderer

Also slightly increases sizes of paths accepted for distance field
caching.

Committed:
https://skia.googlesource.com/skia/+/5ce76efd1c847308c7bcac17bd87d567c42cd786

Committed:
https://skia.googlesource.com/skia/+/73ee6770260dbeeabc4a78eee4f13533f0031211
==========

[jvanverth1, 2015-11-23 19:37:04]

The CQ bit was checked by jvanverth@google.com

[jvanverth1, 2015-11-23 19:37:05]

The patchset sent to the CQ was uploaded after l-g-t-m from
robertphillips@google.com
Link to the patchset: https://codereview.chromium.org/1460873002/#ps200001
(title: "Fix Valgrind issue")

[commit-bot: I haz the power, 2015-11-23 19:37:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1460873002/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1460873002/200001

[commit-bot: I haz the power, 2015-11-23 19:50:04]

Committed patchset #10 (id:200001) as
https://skia.googlesource.com/skia/+/512e437e1e07159a258dd3c5b907576bd1aefc1e