SkCanvas interface for drawing a patch.

SkCanvas interface for drawing a patch.

Added function SkCanvas::drawPatch to the API. This function
receives the patch to draw and the paint.

Added function SkBaseDevice::drawPatch to the API. This function also receives the patch to draw and the paint.

Currently SkGpuDevice and SkBitmapDevice generate the mesh taking into
account the scale factor and call the corresponding device's drawVertices.

BUG=skia:

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

[dandov, 2014-07-28 15:50:56]

https://codereview.chromium.org/424663006/diff/1/gm/patch.cpp
File gm/patch.cpp (right):

https://codereview.chromium.org/424663006/diff/1/gm/patch.cpp#newcode40
gm/patch.cpp:40: return kSkipTiled_Flag | kSkipPipe_Flag | kSkipPicture_Flag;
Added this flags to avoid dm failing for now. 

The skps are not generated correctly when the gm is drawing with a pipe or
picture. Only the control points and lines connecting these points are drawn and
the actual patch is missing, so it fails when it tries to compare it with the
reference image which is drawn correctly.

[bsalomon, 2014-07-28 20:56:26]

https://codereview.chromium.org/424663006/diff/1/src/core/SkBitmapDevice.cpp
File src/core/SkBitmapDevice.cpp (right):

https://codereview.chromium.org/424663006/diff/1/src/core/SkBitmapDevice.cpp#...
src/core/SkBitmapDevice.cpp:348: patch.getVertexData(&data,
draw.fMatrix->getScaleX() * 10, draw.fMatrix->getScaleY() * 10);
I don't think getScaleX and getScaleY are what you want. I think that will give
you 0 for this matrix:

0   100 0
100   0 0
0     0 1

You might want SkMatrix::getMaxScale().

The 10x scale factor should have some kind of comment. Does the tess. level not
depend on the size of the patch?

This implementation could be in SkDevice and then all the other subclasses can
share it (until they have their own specializations where it makes sense to do
so).

https://codereview.chromium.org/424663006/diff/1/src/core/SkCanvas.cpp
File src/core/SkCanvas.cpp (right):

https://codereview.chromium.org/424663006/diff/1/src/core/SkCanvas.cpp#newcod...
src/core/SkCanvas.cpp:2279: LOOPER_BEGIN(paint, SkDrawFilter::kPath_Type, NULL)
Should we add a new SkDrawFilter::kPatch? Not really sure what this enum is used
for, though.

[dandov, 2014-07-29 18:45:46]

Added a default implementation in SkBaseDevice::drawPatch, this takes into
account the size of the patch to determine the level of detail in for each axis.

https://codereview.chromium.org/424663006/diff/1/src/core/SkBitmapDevice.cpp
File src/core/SkBitmapDevice.cpp (right):

https://codereview.chromium.org/424663006/diff/1/src/core/SkBitmapDevice.cpp#...
src/core/SkBitmapDevice.cpp:348: patch.getVertexData(&data,
draw.fMatrix->getScaleX() * 10, draw.fMatrix->getScaleY() * 10);
On 2014/07/28 20:56:26, bsalomon wrote:
> I don't think getScaleX and getScaleY are what you want. I think that will
give
> you 0 for this matrix:
> 
> 0   100 0
> 100   0 0
> 0     0 1
> 
> You might want SkMatrix::getMaxScale().
> 
> The 10x scale factor should have some kind of comment. Does the tess. level
not
> depend on the size of the patch?
> 
> This implementation could be in SkDevice and then all the other subclasses can
> share it (until they have their own specializations where it makes sense to do
> so).
> 

I moved this to SkBaseDevice and created a default implementation, the level of
detail now takes into account the size of the patch using its corner points
already mapped to the transform matrix in SkDraw.

https://codereview.chromium.org/424663006/diff/1/src/core/SkCanvas.cpp
File src/core/SkCanvas.cpp (right):

https://codereview.chromium.org/424663006/diff/1/src/core/SkCanvas.cpp#newcod...
src/core/SkCanvas.cpp:2279: LOOPER_BEGIN(paint, SkDrawFilter::kPath_Type, NULL)
On 2014/07/28 20:56:26, bsalomon wrote:
> Should we add a new SkDrawFilter::kPatch? Not really sure what this enum is
used
> for, though.

Mike said that for now I could just leave it as a path, but I'll ask him how to
add it when he comes back.

[dandov, 2014-07-31 13:18:40]

Fixed some issues I found while working with the mesh gradients.

[jvanverth1, 2014-07-31 15:52:51]

LGTM, but I think bsalomon needs to approve.

[bsalomon, 2014-07-31 17:39:06]

https://codereview.chromium.org/424663006/diff/40001/gm/patch.cpp
File gm/patch.cpp (right):

https://codereview.chromium.org/424663006/diff/40001/gm/patch.cpp#newcode58
gm/patch.cpp:58: canvas->scale(1.3f, 1.f);
Might need some more test cases here with different matrix types.

https://codereview.chromium.org/424663006/diff/40001/src/core/SkDevice.cpp
File src/core/SkDevice.cpp (right):

https://codereview.chromium.org/424663006/diff/40001/src/core/SkDevice.cpp#ne...
src/core/SkDevice.cpp:89: int partitionSize = 25;
static const int kPartitionSize?

https://codereview.chromium.org/424663006/diff/40001/src/core/SkDevice.cpp#ne...
src/core/SkDevice.cpp:92: int lodX = SkMaxScalar(SkScalarAbs(corners[0].x() -
corners[1].x()),
Doesn't this need to be

SkScalarSqrt(SkMaxScalar(corners[0].distanceToSqd(corners[1]),
corners[3].distanceToSqd(corners[2])));

?

It doesn't seem right to look at just the x coords after you've transformed the
patch into the dst space.

https://codereview.chromium.org/424663006/diff/40001/src/core/SkPatch.cpp
File src/core/SkPatch.cpp (right):

https://codereview.chromium.org/424663006/diff/40001/src/core/SkPatch.cpp#new...
src/core/SkPatch.cpp:148: lodX = lodY = 255;
lodX = SkMin(lodX, 255) and same for lodY?

[dandov, 2014-07-31 21:35:40]

Fixed the level of detail issue, added some extra transformations for the
patches in the GM and fixed some issues with colors that I noticed while working
on the mesh gradients

https://codereview.chromium.org/424663006/diff/40001/src/core/SkDevice.cpp
File src/core/SkDevice.cpp (right):

https://codereview.chromium.org/424663006/diff/40001/src/core/SkDevice.cpp#ne...
src/core/SkDevice.cpp:89: int partitionSize = 25;
On 2014/07/31 17:39:06, bsalomon wrote:
> static const int kPartitionSize?

Done.

https://codereview.chromium.org/424663006/diff/40001/src/core/SkDevice.cpp#ne...
src/core/SkDevice.cpp:92: int lodX = SkMaxScalar(SkScalarAbs(corners[0].x() -
corners[1].x()),
On 2014/07/31 17:39:06, bsalomon wrote:
> Doesn't this need to be
> 
> SkScalarSqrt(SkMaxScalar(corners[0].distanceToSqd(corners[1]),
> corners[3].distanceToSqd(corners[2])));
> 
> ?
> 
> It doesn't seem right to look at just the x coords after you've transformed
the
> patch into the dst space.

Completed using an approximate arc length based on the distances between the
points already mapped to the 
transformation matrix of the canvas.

https://codereview.chromium.org/424663006/diff/40001/src/core/SkPatch.cpp
File src/core/SkPatch.cpp (right):

https://codereview.chromium.org/424663006/diff/40001/src/core/SkPatch.cpp#new...
src/core/SkPatch.cpp:148: lodX = lodY = 255;
On 2014/07/31 17:39:06, bsalomon wrote:
> lodX = SkMin(lodX, 255) and same for lodY?

Done.

[egdaniel, 2014-08-01 13:28:52]

https://codereview.chromium.org/424663006/diff/60001/src/core/SkDevice.cpp
File src/core/SkDevice.cpp (right):

https://codereview.chromium.org/424663006/diff/60001/src/core/SkDevice.cpp#ne...
src/core/SkDevice.cpp:88: int topLength =
SkSqrt32(mapPts[SkPatch::kTopP0_CubicCtrlPts]
This may be a good place for a function that calcs the length given 4 control
points.

Also since this is already used for an approximation of arc length, what are
other people's thoughts on calculating the total square dist between all points
and doing one sqrt at end when getting lodX/Y? Drops us from 12->2 sqrts but
less accurate? Then again maybe its best to keep the over-estimation (where as
my suggestion would under estimate) since not having enough subdivides would
probably be worse that being slower. Thoughts?

[bsalomon, 2014-08-01 13:30:28]

https://codereview.chromium.org/424663006/diff/60001/gm/patch.cpp
File gm/patch.cpp (right):

https://codereview.chromium.org/424663006/diff/60001/gm/patch.cpp#newcode19
gm/patch.cpp:19: void drawControlPoints(SkCanvas* canvas, SkPatch& patch,
SkPaint& paint) {
static draw_control_points

https://codereview.chromium.org/424663006/diff/60001/gm/patch.cpp#newcode62
gm/patch.cpp:62: void drawRandomPatch(SkPoint points[12], SkColor colors[4],
SkCanvas* canvas, SkPaint& paint,
static void draw_random_patch

https://codereview.chromium.org/424663006/diff/60001/include/core/SkPatch.h
File include/core/SkPatch.h (right):

https://codereview.chromium.org/424663006/diff/60001/include/core/SkPatch.h#n...
include/core/SkPatch.h:81: static const int kPartitionSize = 10;
Does this really belong in the public interface of this class? It seems like
this value is a property of SkBaseDevice::drawPatch and not fundamental to
SkPatch itself.

https://codereview.chromium.org/424663006/diff/60001/src/core/SkDevice.cpp
File src/core/SkDevice.cpp (right):

https://codereview.chromium.org/424663006/diff/60001/src/core/SkDevice.cpp#ne...
src/core/SkDevice.cpp:89: .distanceToSqd(mapPts[SkPatch::kTopP1_CubicCtrlPts]))
+
I'd just give up on trying to reduce sqrts and call distanceTo(). Summing and
then sqrt'ing seems like an unnecessary shortcut.

https://codereview.chromium.org/424663006/diff/60001/src/core/SkPatch.cpp
File src/core/SkPatch.cpp (right):

https://codereview.chromium.org/424663006/diff/60001/src/core/SkPatch.cpp#new...
src/core/SkPatch.cpp:145: for (int i = 0; i < 4; i++) {
nice catch

[dandov, 2014-08-01 15:11:14]

Added SkPatchUtils class to handle some of the operations related to a SkPatch,
I realized I needed this while working in the mesh gradients which will use it
too.

https://codereview.chromium.org/424663006/diff/60001/gm/patch.cpp
File gm/patch.cpp (right):

https://codereview.chromium.org/424663006/diff/60001/gm/patch.cpp#newcode19
gm/patch.cpp:19: void drawControlPoints(SkCanvas* canvas, SkPatch& patch,
SkPaint& paint) {
On 2014/08/01 13:30:27, bsalomon wrote:
> static draw_control_points

Done.

https://codereview.chromium.org/424663006/diff/60001/gm/patch.cpp#newcode62
gm/patch.cpp:62: void drawRandomPatch(SkPoint points[12], SkColor colors[4],
SkCanvas* canvas, SkPaint& paint,
On 2014/08/01 13:30:27, bsalomon wrote:
> static void draw_random_patch

Done.

https://codereview.chromium.org/424663006/diff/60001/include/core/SkPatch.h
File include/core/SkPatch.h (right):

https://codereview.chromium.org/424663006/diff/60001/include/core/SkPatch.h#n...
include/core/SkPatch.h:81: static const int kPartitionSize = 10;
On 2014/08/01 13:30:27, bsalomon wrote:
> Does this really belong in the public interface of this class? It seems like
> this value is a property of SkBaseDevice::drawPatch and not fundamental to
> SkPatch itself.

moved it to SkPatchUtils

https://codereview.chromium.org/424663006/diff/60001/src/core/SkDevice.cpp
File src/core/SkDevice.cpp (right):

https://codereview.chromium.org/424663006/diff/60001/src/core/SkDevice.cpp#ne...
src/core/SkDevice.cpp:88: int topLength =
SkSqrt32(mapPts[SkPatch::kTopP0_CubicCtrlPts]
On 2014/08/01 13:28:52, egdaniel wrote:
> This may be a good place for a function that calcs the length given 4 control
> points.
> 
> Also since this is already used for an approximation of arc length, what are
> other people's thoughts on calculating the total square dist between all
points
> and doing one sqrt at end when getting lodX/Y? Drops us from 12->2 sqrts but
> less accurate? Then again maybe its best to keep the over-estimation (where as
> my suggestion would under estimate) since not having enough subdivides would
> probably be worse that being slower. Thoughts?

Created a function in SkGeometry that calculates an approximate arc length using
the control points of a bezier curve. 
I also tried to use the distanceToSqd only but it overestimated a lot and I had
to adjust the partition size kind of arbitrarily, so I will try to come up with
a way to get the level of detail using the squared distances.

[egdaniel, 2014-08-01 15:41:41]

https://codereview.chromium.org/424663006/diff/80001/gm/patch.cpp
File gm/patch.cpp (right):

https://codereview.chromium.org/424663006/diff/80001/gm/patch.cpp#newcode19
gm/patch.cpp:19: static void drawControlPoints(SkCanvas* canvas, SkPatch& patch,
SkPaint& paint) {
missed the "_" change in function names. When we have local static functions we
name_them_like_this

https://codereview.chromium.org/424663006/diff/80001/gm/patch.cpp#newcode62
gm/patch.cpp:62: static void drawRandomPatch(SkPoint points[12], SkColor
colors[4], SkCanvas* canvas, SkPaint& paint,
same

[dandov, 2014-08-01 19:10:51]

https://codereview.chromium.org/424663006/diff/80001/gm/patch.cpp
File gm/patch.cpp (right):

https://codereview.chromium.org/424663006/diff/80001/gm/patch.cpp#newcode19
gm/patch.cpp:19: static void drawControlPoints(SkCanvas* canvas, SkPatch& patch,
SkPaint& paint) {
On 2014/08/01 15:41:40, egdaniel wrote:
> missed the "_" change in function names. When we have local static functions
we
> name_them_like_this

Done.

https://codereview.chromium.org/424663006/diff/80001/gm/patch.cpp#newcode62
gm/patch.cpp:62: static void drawRandomPatch(SkPoint points[12], SkColor
colors[4], SkCanvas* canvas, SkPaint& paint,
On 2014/08/01 15:41:40, egdaniel wrote:
> same

Done.

[bsalomon, 2014-08-01 21:02:28]

https://codereview.chromium.org/424663006/diff/100001/include/utils/SkPatchUt...
File include/utils/SkPatchUtils.h (right):

https://codereview.chromium.org/424663006/diff/100001/include/utils/SkPatchUt...
include/utils/SkPatchUtils.h:17: static const int kPartitionSize = 30;
I'd move this into the cpp. It's not really part of the  interface.

Also, I think this file should probably be in src/utils. We try to use include/
for files that we would want a Skia client to #include.

https://codereview.chromium.org/424663006/diff/100001/src/core/SkGeometry.h
File src/core/SkGeometry.h (right):

https://codereview.chromium.org/424663006/diff/100001/src/core/SkGeometry.h#n...
src/core/SkGeometry.h:194: SkScalar approx_arc_length(SkPoint* points, int
count);
I don't think this is general/accurate enough to be part of SkGeometry.h. I'd
just move it to src/utils/SkPatchUtils.cpp.

[dandov, 2014-08-01 21:25:01]

https://codereview.chromium.org/424663006/diff/100001/include/utils/SkPatchUt...
File include/utils/SkPatchUtils.h (right):

https://codereview.chromium.org/424663006/diff/100001/include/utils/SkPatchUt...
include/utils/SkPatchUtils.h:17: static const int kPartitionSize = 30;
On 2014/08/01 21:02:28, bsalomon wrote:
> I'd move this into the cpp. It's not really part of the  interface.
> 
> Also, I think this file should probably be in src/utils. We try to use
include/
> for files that we would want a Skia client to #include.

Done.

https://codereview.chromium.org/424663006/diff/100001/src/core/SkGeometry.h
File src/core/SkGeometry.h (right):

https://codereview.chromium.org/424663006/diff/100001/src/core/SkGeometry.h#n...
src/core/SkGeometry.h:194: SkScalar approx_arc_length(SkPoint* points, int
count);
On 2014/08/01 21:02:28, bsalomon wrote:
> I don't think this is general/accurate enough to be part of SkGeometry.h. I'd
> just move it to src/utils/SkPatchUtils.cpp.

Done.

[bsalomon, 2014-08-04 13:22:56]

https://codereview.chromium.org/424663006/diff/120001/src/utils/SkPatchUtils.h
File src/utils/SkPatchUtils.h (right):

https://codereview.chromium.org/424663006/diff/120001/src/utils/SkPatchUtils....
src/utils/SkPatchUtils.h:20: static SkIPoint GetLevelOfDetail(const SkPatch&
patch, const SkMatrix* matrix);
*Maybe* SkISize is more appropriate here, but not a strong suggestion.

[bsalomon, 2014-08-04 13:23:08]

On 2014/08/04 13:22:56, bsalomon wrote:
> https://codereview.chromium.org/424663006/diff/120001/src/utils/SkPatchUtils.h
> File src/utils/SkPatchUtils.h (right):
> 
>
https://codereview.chromium.org/424663006/diff/120001/src/utils/SkPatchUtils....
> src/utils/SkPatchUtils.h:20: static SkIPoint GetLevelOfDetail(const SkPatch&
> patch, const SkMatrix* matrix);
> *Maybe* SkISize is more appropriate here, but not a strong suggestion.

Also mean to say lgtm.

[dandov, 2014-08-04 14:52:45]

The CQ bit was checked by dandov@google.com

[commit-bot: I haz the power, 2014-08-04 14:53:03]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/dandov@google.com/424663006/140001

[commit-bot: I haz the power, 2014-08-04 15:06:53]

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-08-04 15:14:03]

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

[commit-bot: I haz the power, 2014-08-04 15:14:03]

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...)

[dandov, 2014-08-04 15:15:53]

The CQ bit was checked by dandov@google.com

[commit-bot: I haz the power, 2014-08-04 15:16:03]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/dandov@google.com/424663006/60002

[commit-bot: I haz the power, 2014-08-04 15:29:33]

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...)

[dandov, 2014-08-04 15:35:37]

The CQ bit was checked by dandov@google.com

[commit-bot: I haz the power, 2014-08-04 15:36:15]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/dandov@google.com/424663006/170001

[commit-bot: I haz the power, 2014-08-04 15:49:27]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  Test-Ubuntu13.10-GCE-NoGPU-x86_64-Debug-Trybot on tryserver.skia
(http://108.170.220.120:10117/builders/Test-Ubuntu13.10-GCE-NoGPU-x86_64-Debug...)

[commit-bot: I haz the power, 2014-08-04 16:02:48]

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

[commit-bot: I haz the power, 2014-08-04 16:02:50]

Try jobs failed on following builders:
  Test-Ubuntu13.10-GCE-NoGPU-x86_64-Debug-Trybot on tryserver.skia
(http://108.170.220.120:10117/builders/Test-Ubuntu13.10-GCE-NoGPU-x86_64-Debug...)

[dandov, 2014-08-04 17:01:04]

The CQ bit was checked by dandov@google.com

[commit-bot: I haz the power, 2014-08-04 17:01:26]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/dandov@google.com/424663006/170001

[commit-bot: I haz the power, 2014-08-04 17:02:04]

Change committed as ecfff21bde1f0ca3c36533eded325066b5f2d42d