draw circle paths directly via GPU

This CL tries to implement the feature that detects whether a path is a circle and then renders it in a specific shader. Thus it would save us from drawing 8 quads and then we have the performance boost.

With the benchmark below:
http://themaninblue.com/experiment/AnimationBenchmark/canvas/

I've seen the FPS goes from 60FPS to 100FPS with the introduction of this CL.

This implementation takes a top-down approach, i.e. the client of skia has to call 'addCircle' directly. Then the state of whether it's a circle is kept along in path structure. I tried my best to keep the implementation and simple and change the core parts as little as possible. Therefore in this implementation, I cache several fields in SkPath to track whether it's a circle or not. The complex part comes with the transformation of a path, it has to check whether it's still a circle after transformation.

At a glance, I don't like the way I add so much helper functions in SkMatrix and touch almost every function in SkPath. So I've always come up with another bottom-up approach. It guesses whether it's a circle or not. Please take a look at this CL: http://codereview.appspot.com/5698072/

Please help me decide which approach is better so that I'm not going too far away on the wrong path.
Also please answer some questions below if it's OK:

1. in which circumstances, can the view matrix of draw state have perspective? Is it possible in canvas 2d? I'm not making another specific circle path renderer because I'm suspecting it could have perspectives and thus this special shader can't be used at all.

2. For now, we use 8 quads to approximate one circle. With this rendering technique, I'm reverting it back to  a perfectly sized circle. Therefore, for circles with large diameter, the differences between software rendering and this GPU rendering is obvious, some regions along the circle edges don't match any more. Is it acceptable.

3. In this implementation, I'm using smoothstep() in fragment shader to do the anti-aliasing. From my experiment, it's OK, but I'd like to know what's the policy behind rendering quality? How good should we get?

BUG=
TEST=improve the performance of http://themaninblue.com/experiment/AnimationBenchmark/canvas/ and http://ie.microsoft.com/testdrive/Performance/Fireworks/Default.html a lot.

[guanqun, 2012-02-27 07:30:28]

Please see Description for more info and help me decide which approach is
better. this one or that one (http://codereview.appspot.com/5698072/) ? Thanks!

[bsalomon, 2012-02-27 15:12:34]

On 2012/02/27 07:30:28, guanqun wrote:
> Please see Description for more info and help me decide which approach is
> better. this one or that one (http://codereview.appspot.com/5698072/) ?
Thanks!

Thanks for the patch and for testing it on those sites. This is something we
have been planning to do for a while (and also for other common shapes).

A few high level points:

When possible we'd prefer to expose an API at the canvas level to draw a shape
rather than funneling it through path. It may not be easy to change some callers
(e.g. canvas1 in WebKit) which are already passing around paths, however. I
think this requires more investigation before we commit to a particular
implementation.

It isn't correct to draw multiple circles that intersect as separate primitives
without making some assumptions about the drawing state (xfermode).

Assuming we can't rewrite the caller to call a drawCircle API, it feels like
we're adding quite a few fields to SkPath to support a particular optimization.
If we restrict ourselves to a single circle it is probably possible to flag the
path with a single bit and reconstruct the circle parameters from the bounds of
the path.

I'd prefer to not further complicate the already somewhat tricky convex path
renderer and instead use a separate subclass (which would have a higher priority
than the convex path renderer).

I believe we would have to inflate the radius so that the AA is applied on both
sides of the ideal circle (i.e. partially shade a pixel that the circle
intersects but where the pixel center is not covered.)

[bsalomon, 2012-02-27 15:27:54]

>1. in which circumstances, can the view matrix of draw state have perspective?
>Is it possible in canvas 2d? I'm not making another specific circle path
>renderer because I'm suspecting it could have perspectives and thus this
special
>shader can't be used at all.

canvas2D does not have perspective. I don't know of anything other than WebGL
and css 3d that support perspective in WebKit, though maybe there is. The convex
path renderer today calls SkPath::transform with the view matrix, which rather
than producing rationals when there are perspective just subdivides. Actually, I
plan to take out this call and for now make the convex path renderer reject when
the matrix has perspective. Transform makes an unnecessary copy of the path.
Support for perspective can be added later when it becomes important.

>2. For now, we use 8 quads to approximate one circle. With this rendering
>technique, I'm reverting it back to  a perfectly sized circle. Therefore, for
>circles with large diameter, the differences between software rendering and
this
>GPU rendering is obvious, some regions along the circle edges don't match any
>more. Is it acceptable.

I think that is OK. The sw code path could also directly render circles. It'd be
an improvement.

>3. In this implementation, I'm using smoothstep() in fragment shader to do the
>anti-aliasing. From my experiment, it's OK, but I'd like to know what's the
>policy behind rendering quality? How good should we get?

In general the cheapest way to look good enough. Or cut corners until someone
complains. :) For consistency we'd probably use a linear ramp as the other
edgeAA techniques do. In the future we could explore using a different / wider
ramp but for all paths.

[guanqun, 2012-02-28 01:47:30]

On 2012/02/27 15:12:34, bsalomon wrote:
> On 2012/02/27 07:30:28, guanqun wrote:
> > Please see Description for more info and help me decide which approach is
> > better. this one or that one (http://codereview.appspot.com/5698072/) ?
> Thanks!
> 
> Thanks for the patch and for testing it on those sites. This is something we
> have been planning to do for a while (and also for other common shapes).

I'd like to take this task if you're too busy with other stuff. :)

> When possible we'd prefer to expose an API at the canvas level to draw a shape
> rather than funneling it through path. It may not be easy to change some
callers
> (e.g. canvas1 in WebKit) which are already passing around paths, however. I
> think this requires more investigation before we commit to a particular
> implementation.

What's the benefit of using the canvas level API such as drawCircle? As I see
it, in current implementation, it creates a path and then draws it. What're the
differences with using paths directly?

> 
> It isn't correct to draw multiple circles that intersect as separate
primitives
> without making some assumptions about the drawing state (xfermode).

Ah, yes. You're right. What I was supposed to think these circles don't overlap,
thus we can have only one single glDrawElement call for lots of circles.
However, in my alternative approach, I resort to only one contour in the path.
So this issue will not exist.

> 
> Assuming we can't rewrite the caller to call a drawCircle API, it feels like
> we're adding quite a few fields to SkPath to support a particular
optimization.

I have the same feeling here...

> If we restrict ourselves to a single circle it is probably possible to flag
the
> path with a single bit and reconstruct the circle parameters from the bounds
of
> the path.

This current implementation basically uses this idea, but the complex part in
the code is to determine whether after transformation, it's still a matrix.

> 
> I'd prefer to not further complicate the already somewhat tricky convex path
> renderer and instead use a separate subclass (which would have a higher
priority
> than the convex path renderer).

OK. If another path renderer is used, this new path renderer has to reject when
the view matrix has skews or perspectives.

> 
> I believe we would have to inflate the radius so that the AA is applied on
both
> sides of the ideal circle (i.e. partially shade a pixel that the circle
> intersects but where the pixel center is not covered.)

[reed1, 2012-02-28 13:18:49]

On 2012/02/28 01:47:30, guanqun wrote:
> On 2012/02/27 15:12:34, bsalomon wrote:
> > On 2012/02/27 07:30:28, guanqun wrote:
> > > Please see Description for more info and help me decide which approach is
> > > better. this one or that one (http://codereview.appspot.com/5698072/) ?
> > Thanks!
> > 
> > Thanks for the patch and for testing it on those sites. This is something we
> > have been planning to do for a while (and also for other common shapes).
> 
> I'd like to take this task if you're too busy with other stuff. :)
> 
> > When possible we'd prefer to expose an API at the canvas level to draw a
shape
> > rather than funneling it through path. It may not be easy to change some
> callers
> > (e.g. canvas1 in WebKit) which are already passing around paths, however. I
> > think this requires more investigation before we commit to a particular
> > implementation.
> 
> What's the benefit of using the canvas level API such as drawCircle? As I see
> it, in current implementation, it creates a path and then draws it. What're
the
> differences with using paths directly?

The benefit would be if we can keep circles/ovals/roundrects as those directly,
all the way down to the GPU, without ever having to create a SkPath, since the
GPU can implement these just from their high-level parameters (e.g.
center+radius, bounding rect, bounding_rect+radii). This deeper tunneling will
also make these much cheaper to record into pictures, since today we convert
them to SkPath and make a deep copy of that (requiring several more calls to
new).

> 
> > 
> > It isn't correct to draw multiple circles that intersect as separate
> primitives
> > without making some assumptions about the drawing state (xfermode).
> 
> Ah, yes. You're right. What I was supposed to think these circles don't
overlap,
> thus we can have only one single glDrawElement call for lots of circles.
> However, in my alternative approach, I resort to only one contour in the path.
> So this issue will not exist.
> 
> > 
> > Assuming we can't rewrite the caller to call a drawCircle API, it feels like
> > we're adding quite a few fields to SkPath to support a particular
> optimization.
> 
> I have the same feeling here...
> 
> > If we restrict ourselves to a single circle it is probably possible to flag
> the
> > path with a single bit and reconstruct the circle parameters from the bounds
> of
> > the path.
> 
> This current implementation basically uses this idea, but the complex part in
> the code is to determine whether after transformation, it's still a matrix.
> 
> > 
> > I'd prefer to not further complicate the already somewhat tricky convex path
> > renderer and instead use a separate subclass (which would have a higher
> priority
> > than the convex path renderer).
> 
> OK. If another path renderer is used, this new path renderer has to reject
when
> the view matrix has skews or perspectives.
> 
> > 
> > I believe we would have to inflate the radius so that the AA is applied on
> both
> > sides of the ideal circle (i.e. partially shade a pixel that the circle
> > intersects but where the pixel center is not covered.)

[guanqun, 2012-02-29 07:12:33]

On 2012/02/28 13:18:49, reed1 wrote:
> The benefit would be if we can keep circles/ovals/roundrects as those
directly,
> all the way down to the GPU, without ever having to create a SkPath, since the
> GPU can implement these just from their high-level parameters (e.g.
> center+radius, bounding rect, bounding_rect+radii). This deeper tunneling will
> also make these much cheaper to record into pictures, since today we convert
> them to SkPath and make a deep copy of that (requiring several more calls to
> new).

Hi Mike,

Thanks for your clarification. But still one thing unclear: if we're caching
these high level parameters, then how do we manipulate correctly with canvas
transform (e.g. skew, rotate) and how to track the bounds etc? I'm afraid that's
what we should figure out, right? Let me better understand the implementation of
SkCanvas first...

Thanks!

[guanqun, 2012-03-12 08:55:26]

Hi all,

I've tried to use SkCanvas interface to implement this feature, please take a
look whether it's what you want, and see if some interface needs to be polished.
NOTE: I haven't added GrContext::drawCircle which is the main part yet. I want
to get your approval of the API design first before I convert my original
implementation to this version.

Thanks!

[reed1, 2012-03-12 12:44:48]

Thank you.

We have been thinking about a deeper change in this area for a while, and have
already begun to make related changes as high up as in the SVG code in webkit.
To avoid duplicate, and possibly conflicting work, I suggest you park this CL,
and wait to see the outcome of our larger change. That change is also aimed at
speeding up circles, ovals, roundrects, etc.

[guanqun, 2012-03-12 14:36:34]

Thanks for your info, I'll close this issue.

[guanqun, 2012-03-29 06:41:17]

Hi Brian,

Here's the issue of my circle drawing patch. The latest patch is modelled after
the way drawRect does. You can skim through to see whether the overall
architecture is correct or not (the shader part is not added yet).

As you said, let's try to move this forward. If anything doesn't fit into your
overall goal, you can raise these issue up and I'll fix them. :)

Thanks!

[bsalomon, 2012-04-03 14:28:12]

On 2012/03/29 06:41:17, guanqun wrote:
> Hi Brian,
> 
> Here's the issue of my circle drawing patch. The latest patch is modelled
after
> the way drawRect does. You can skim through to see whether the overall
> architecture is correct or not (the shader part is not added yet).
> 
> As you said, let's try to move this forward. If anything doesn't fit into your
> overall goal, you can raise these issue up and I'll fix them. :)
> 
> Thanks!

I think this is generally in the right direction. However, Mike told me that
right now is not a good time to add new virtuals at the SkCanvas level because
of things in flux around the SkCanvas API and picture serialization.

So here is what I propose: We take what you've done for GrAACirclePathRenderer
(and supporting changes to GrDrawState and the shader generator) in patchset 2
and turn it into GrContext::drawEllipse(). It takes a rect and a strokeWidth. A
negative strokeWidth means fill. The reason for making it be drawEllipse rather
than circle and including a strokeWidth is so in the future we can also directly
draw any ellipse without having to change the function signatures.

We then go back to the circle detection you had in patchset 1 on this CL (bool
fIsCircle member on path). We make it a somewhat smaller change by using the
bounds to reconstruct the circle parameters (center and radius) rather than
storing them directly. I think we will want this even if when we get to the
point of having a virtual on SkCanvas because some clients like HTML5 canvas may
be hard to modify to call the virtual.

We move most of the guts of GrContext::drawPath into a private function
GrContext::internalDrawPath. GrContext::drawPath checks if the SkPath is a
circle and if so calls drawEllipse. Otherwise, it calls internalDrawPath.
drawEllipse can check whether the parameters are compatible with the shader code
you've written (rotation/translation/uniform-scaling matrix, circle rather than
ellipse, and fill rather than stroke) and if so draws it directly. Otherwise, it
calls internalDrawPath.

Does this seem like a reasonable path forward?

[guanqun, 2012-04-04 13:43:34]

Hi Brian,

Thanks for your detailed comments, I get the idea of how I should implement this
feature. I'll finish it ASAP. Some of my comments are inline:

On 2012/04/03 14:28:12, bsalomon wrote:
> I think this is generally in the right direction. However, Mike told me that
> right now is not a good time to add new virtuals at the SkCanvas level because
> of things in flux around the SkCanvas API and picture serialization.

It's good to know the current status.

> 
> So here is what I propose: We take what you've done for GrAACirclePathRenderer
> (and supporting changes to GrDrawState and the shader generator) in patchset 2
> and turn it into GrContext::drawEllipse(). It takes a rect and a strokeWidth.
A
> negative strokeWidth means fill. The reason for making it be drawEllipse
rather
> than circle and including a strokeWidth is so in the future we can also
directly
> draw any ellipse without having to change the function signatures.

nitpicked: as we don't want to change the function names often, should we name
it "drawOval" instead of "drawEllipse"? The word "Oval" is already used in many
places.

> 
> We then go back to the circle detection you had in patchset 1 on this CL (bool
> fIsCircle member on path). We make it a somewhat smaller change by using the
> bounds to reconstruct the circle parameters (center and radius) rather than

In patchset 2, I have an attempted implementation to check whether a path is a
circle without adding any new members on path. But that makes our checking a
little heavy, so adding only one new member (fIsCircle) seems a good trade-off.

> storing them directly. I think we will want this even if when we get to the
> point of having a virtual on SkCanvas because some clients like HTML5 canvas
may
> be hard to modify to call the virtual.

I'v also took a look into webkit, and got the idea that its canvas
implementation is heavily based on its 'path' abstraction. So you're right, it's
hard to modify it to call our SkCanvas interface.

> 
> We move most of the guts of GrContext::drawPath into a private function
> GrContext::internalDrawPath. GrContext::drawPath checks if the SkPath is a
> circle and if so calls drawEllipse. Otherwise, it calls internalDrawPath.
> drawEllipse can check whether the parameters are compatible with the shader
code
> you've written (rotation/translation/uniform-scaling matrix, circle rather
than
> ellipse, and fill rather than stroke) and if so draws it directly. Otherwise,
it
> calls internalDrawPath.
> 
> Does this seem like a reasonable path forward?

Your explanation is a good gudiance for me. :) I'll start completing this
feature.

[bsalomon, 2012-04-04 13:48:54]

On 2012/04/04 13:43:34, guanqun wrote:
> Hi Brian,
> 
> Thanks for your detailed comments, I get the idea of how I should implement
this
> feature. I'll finish it ASAP. Some of my comments are inline:
> 
> On 2012/04/03 14:28:12, bsalomon wrote:
> > I think this is generally in the right direction. However, Mike told me that
> > right now is not a good time to add new virtuals at the SkCanvas level
because
> > of things in flux around the SkCanvas API and picture serialization.
> 
> It's good to know the current status.
> 
> > 
> > So here is what I propose: We take what you've done for
GrAACirclePathRenderer
> > (and supporting changes to GrDrawState and the shader generator) in patchset
2
> > and turn it into GrContext::drawEllipse(). It takes a rect and a
strokeWidth.
> A
> > negative strokeWidth means fill. The reason for making it be drawEllipse
> rather
> > than circle and including a strokeWidth is so in the future we can also
> directly
> > draw any ellipse without having to change the function signatures.
> 
> nitpicked: as we don't want to change the function names often, should we name
> it "drawOval" instead of "drawEllipse"? The word "Oval" is already used in
many
> places.

I didn't realize that we used the name "oval". Let's use that then for
consistency.
 
> 
> > 
> > We then go back to the circle detection you had in patchset 1 on this CL
(bool
> > fIsCircle member on path). We make it a somewhat smaller change by using the
> > bounds to reconstruct the circle parameters (center and radius) rather than
> 
> In patchset 2, I have an attempted implementation to check whether a path is a
> circle without adding any new members on path. But that makes our checking a
> little heavy, so adding only one new member (fIsCircle) seems a good
trade-off.
> 
> > storing them directly. I think we will want this even if when we get to the
> > point of having a virtual on SkCanvas because some clients like HTML5 canvas
> may
> > be hard to modify to call the virtual.
> 
> I'v also took a look into webkit, and got the idea that its canvas
> implementation is heavily based on its 'path' abstraction. So you're right,
it's
> hard to modify it to call our SkCanvas interface.
> 
> > 
> > We move most of the guts of GrContext::drawPath into a private function
> > GrContext::internalDrawPath. GrContext::drawPath checks if the SkPath is a
> > circle and if so calls drawEllipse. Otherwise, it calls internalDrawPath.
> > drawEllipse can check whether the parameters are compatible with the shader
> code
> > you've written (rotation/translation/uniform-scaling matrix, circle rather
> than
> > ellipse, and fill rather than stroke) and if so draws it directly.
Otherwise,
> it
> > calls internalDrawPath.
> > 
> > Does this seem like a reasonable path forward?
> 
> Your explanation is a good gudiance for me. :) I'll start completing this
> feature.

Great!

[guanqun, 2012-04-09 12:39:28]

Hi Brian,

I'm done with this feature and FYI there are some differences between your
advices and the actual implementation here.

Right now, I'm forking the code path in SkGpuDevice::drawPath(), the reason here
is that if I fork the code path in GrContext::drawPath(), then we have no
information about the stroke width, the stroked part is directly handled in
SkGpuDevice::drawPath() to generate the stroke path.

Therefore, I'm checking all the conditions in SkGpuDevice::drawOval() to see if
we can handle it correctly in GrContext::drawOval(). If it's impossible to do
it, we fallback to SkGpuDevice::drawPath().

I want to make sure my understanding is correct here. In
SkGpuDevice::drawOval(), I'm checking draw.fMatrix to get the CTM. When the code
begins to execute GrContext::drawOval(), the view matrix of draw state should be
the same with draw.fMatrix, right? Though the view matrix can be changed later.
(As I see it, it gets set in SkGpuDevice::gainFocus()).

Another problem: should we specialize SkPath::addOval() to call
SkPath::addCircle() when the width and height of rect is the same? Or we'd
better let the caller use the explicit SkPath::addCircle() interface or
SkCanvas::drawCircle() interface?

Rough tests show that this patch has a lot performance boost, I'll run more
tests and publish detailed numbers tomorrow.

http://codereview.appspot.com/5696086/diff/17018/include/core/SkMatrix.h
File include/core/SkMatrix.h (right):

http://codereview.appspot.com/5696086/diff/17018/include/core/SkMatrix.h#newc...
include/core/SkMatrix.h:91: bool hasNoSkewAndPerspective() const;
Is this name good enough?

http://codereview.appspot.com/5696086/diff/17018/src/core/SkPath.cpp
File src/core/SkPath.cpp (right):

http://codereview.appspot.com/5696086/diff/17018/src/core/SkPath.cpp#newcode1150
src/core/SkPath.cpp:1150: fIsCircle = false;
Right now, all the append related path operations would invalidate the
fIsCircle. It could be enhance by checking e.g. whether src is empty or not, but
for simplicity, I'm making it false.

http://codereview.appspot.com/5696086/diff/17018/src/gpu/GrContext.cpp
File src/gpu/GrContext.cpp (right):

http://codereview.appspot.com/5696086/diff/17018/src/gpu/GrContext.cpp#newcod...
src/gpu/GrContext.cpp:1447: SkPaint::Style style, SkScalar strokeWidth) {
I'm passing the style parameter because we need to differentiate StrokeAndFill
style and Stroke style.

Using negative stroke width isn't enough for this scenario.

[bsalomon, 2012-04-09 21:17:58]

http://codereview.appspot.com/5696086/diff/17018/include/core/SkMatrix.h
File include/core/SkMatrix.h (right):

http://codereview.appspot.com/5696086/diff/17018/include/core/SkMatrix.h#newc...
include/core/SkMatrix.h:91: bool hasNoSkewAndPerspective() const;
On 2012/04/09 12:39:28, guanqun wrote:
> Is this name good enough?

I'm having trouble coming up with a much better name. We want to allow
translation, rotation, mirror and bar non-uniform scale/skew and perspective.

How about "isUniformScaling()"?

http://codereview.appspot.com/5696086/diff/17018/src/core/SkMatrix.cpp
File src/core/SkMatrix.cpp (right):

http://codereview.appspot.com/5696086/diff/17018/src/core/SkMatrix.cpp#newcod...
src/core/SkMatrix.cpp:1146: vec[0].set(SkFloatToScalar(1.0), 0);
Would this be better:

vec[0].set(this->get(kMScaleX), this->get(kMSkewX));
vec[1].set(this->get(kMSkewY), this->get(kMScaleY));

return SkScalarNearlyZero(vec[0].dot(vec[1])) &&
       SkNearlyEqual(vec[0].lengthSqd(), vec[1].lengthSqd());

Seems like it would save some operations.

http://codereview.appspot.com/5696086/diff/17018/src/core/SkPath.cpp
File src/core/SkPath.cpp (right):

http://codereview.appspot.com/5696086/diff/17018/src/core/SkPath.cpp#newcode1150
src/core/SkPath.cpp:1150: fIsCircle = false;
On 2012/04/09 12:39:28, guanqun wrote:
> Right now, all the append related path operations would invalidate the
> fIsCircle. It could be enhance by checking e.g. whether src is empty or not,
but
> for simplicity, I'm making it false.

Let's add comments to that effect. This one seems like it could just be
fIsCircle = src.fIsCircle, right?

http://codereview.appspot.com/5696086/diff/17018/src/gpu/GrContext.cpp
File src/gpu/GrContext.cpp (right):

http://codereview.appspot.com/5696086/diff/17018/src/gpu/GrContext.cpp#newcod...
src/gpu/GrContext.cpp:1447: SkPaint::Style style, SkScalar strokeWidth) {
On 2012/04/09 12:39:28, guanqun wrote:
> I'm passing the style parameter because we need to differentiate StrokeAndFill
> style and Stroke style.
> 
> Using negative stroke width isn't enough for this scenario.

SkPaint::Style isn't currently used below SkGpuDevice. I'm not completely
opposed to using it but it makes the GrContext interface somewhat inconsistent.
Is a stroked and filled oval the same as filling an oval with the rect is
inflated by half the stroke width?

http://codereview.appspot.com/5696086/diff/17018/src/gpu/GrContext.cpp#newcod...
src/gpu/GrContext.cpp:1449: GrAssert(rect.width() == rect.height());
Rather than assert, can we fall back to drawPath?

http://codereview.appspot.com/5696086/diff/17018/src/gpu/GrContext.cpp#newcod...
src/gpu/GrContext.cpp:1457: if (rt == NULL) {
NULL == rt, we like to keep the rvalue on the left in == comparisons to prevent
accidental assignment.

http://codereview.appspot.com/5696086/diff/17018/src/gpu/GrContext.cpp#newcod...
src/gpu/GrContext.cpp:1481: strokeWidth = 0.5;
I think we should stick to 1.0 We're thicker for other types of hairlines
already. If we decide to match SW then I think it should be a global decision
for all hairlines. (IMO the apparent thickness of hairlines is not invariant to
rotation).

http://codereview.appspot.com/5696086/diff/17018/src/gpu/GrDrawState.h
File src/gpu/GrDrawState.h (right):

http://codereview.appspot.com/5696086/diff/17018/src/gpu/GrDrawState.h#newcod...
src/gpu/GrDrawState.h:554: kCircle_EdgeType,
Let's put a comment about what the params are to this edge type.

[reed1, 2012-04-09 21:28:45]

Comments on SkPath.[h,cpp]

1. Can we track ovals instead of circles?
2. I think empty is not a circle/oval
3. Lets use SkBool8 instead of bool
4. isOval() should take an optional SkRect*, just as isRect() does

[reed1, 2012-04-09 21:33:39]

Comments on SkMatrix:

1. This method is a bit tricky, since it involves tolerances to detect
rotations.
2. I notice it doesn't need any private access in its impl.
3. Is it called in lots of places? If not, can we just move it into the caller
as a local static function?

[guanqun, 2012-04-10 02:30:14]

2. I think empty is not a circle/oval
3. Lets use SkBool8 instead of bool

OK. I'll add a check for empty.

1. Can we track ovals instead of circles?
4. isOval() should take an optional SkRect*, just as isRect() does

If we're about to track ovals, how about the oval is rotated? Is it still an
oval? And if we return the bounding rect of the oval with a rotated angle, the
result could be incorrect.

1. This method is a bit tricky, since it involves tolerances to detect
rotations.
2. I notice it doesn't need any private access in its impl.
3. Is it called in lots of places? If not, can we just move it into the caller
as a local static function?

This method in SkMatrix is only used in two places, SkPath and SkGpuDevice. As
it's called in two different files, if we're reluctant to add a new interface in
SkMatrix. I can make it a utility function in SkMatrix.h header file.

http://codereview.appspot.com/5696086/diff/17018/include/core/SkMatrix.h
File include/core/SkMatrix.h (right):

http://codereview.appspot.com/5696086/diff/17018/include/core/SkMatrix.h#newc...
include/core/SkMatrix.h:91: bool hasNoSkewAndPerspective() const;
> How about "isUniformScaling()"?

I once thought of the name to be 'circleStaysCircle' to mimic the function
'rectStaysRect'. :D

http://codereview.appspot.com/5696086/diff/17018/src/core/SkMatrix.cpp
File src/core/SkMatrix.cpp (right):

http://codereview.appspot.com/5696086/diff/17018/src/core/SkMatrix.cpp#newcod...
src/core/SkMatrix.cpp:1146: vec[0].set(SkFloatToScalar(1.0), 0);
On 2012/04/09 21:17:58, bsalomon wrote:
> Would this be better:
> 
> vec[0].set(this->get(kMScaleX), this->get(kMSkewX));
> vec[1].set(this->get(kMSkewY), this->get(kMScaleY));
> 
> return SkScalarNearlyZero(vec[0].dot(vec[1])) &&
>        SkNearlyEqual(vec[0].lengthSqd(), vec[1].lengthSqd());
> 
> Seems like it would save some operations.

Thanks for your suggestion, I'll take that.

http://codereview.appspot.com/5696086/diff/17018/src/core/SkPath.cpp
File src/core/SkPath.cpp (right):

http://codereview.appspot.com/5696086/diff/17018/src/core/SkPath.cpp#newcode1150
src/core/SkPath.cpp:1150: fIsCircle = false;
> Let's add comments to that effect. This one seems like it could just be
> fIsCircle = src.fIsCircle, right?

I'll add a comment somewhere. For this function 'reverseAddPath()', no. Strictly
speaking, it should be [in pseudo code]: fIsCircle = fIsCircle && ((src is
consisted of all kMove_Verb) || (src is empty))

I don't bother to do this check because reverseAddPath is not a widely used
operation.

http://codereview.appspot.com/5696086/diff/17018/src/gpu/GrContext.cpp
File src/gpu/GrContext.cpp (right):

http://codereview.appspot.com/5696086/diff/17018/src/gpu/GrContext.cpp#newcod...
src/gpu/GrContext.cpp:1447: SkPaint::Style style, SkScalar strokeWidth) {
> SkPaint::Style isn't currently used below SkGpuDevice. I'm not completely
> opposed to using it but it makes the GrContext interface somewhat
inconsistent.

I think there are two options to accommodate this issue:
1) we remove this SkPaint::Style parameter, but add another bool to specify
whether it's StrokeAndFill or simply Stroke. and strokeWidth can be negative if
it's a Fill.
2) We add the Style information into GrPaint.

> Is a stroked and filled oval the same as filling an oval with the rect is
> inflated by half the stroke width?

Yes. No matter what fill type it is, winding or even odd.

But a stroked oval is not the same as a stroked and filled oval when the rect is
inflated by half the stroke width.

That's the reason we should differentiate it here.

http://codereview.appspot.com/5696086/diff/17018/src/gpu/GrContext.cpp#newcod...
src/gpu/GrContext.cpp:1449: GrAssert(rect.width() == rect.height());
On 2012/04/09 21:17:58, bsalomon wrote:
> Rather than assert, can we fall back to drawPath?

As right now, GrContext::drawOval() is only called from SkGpuDevice::drawOval(),
so this assertion won't trigger in real life... Hmm, may be I can move it into
SkGpuDevice::drawOval() to make sure that the bounding rect of the path must be
equal with its height and width. If it's not, fall back to drawPath() at
SkGpuDevice level.

Falling back to drawPath() at GrContext level is not available for stroked path.
If we can pass down the whole SkPaint structure in GrContext::drawOval(), then
we can generate a new path with "paint.getFillPath()" and call
GrContext::drawPath() with the new stroked path. That said, I think it's a
violation of correct abstraction...

http://codereview.appspot.com/5696086/diff/17018/src/gpu/GrContext.cpp#newcod...
src/gpu/GrContext.cpp:1457: if (rt == NULL) {
On 2012/04/09 21:17:58, bsalomon wrote:
> NULL == rt, we like to keep the rvalue on the left in == comparisons to
prevent
> accidental assignment.

Done.

http://codereview.appspot.com/5696086/diff/17018/src/gpu/GrContext.cpp#newcod...
src/gpu/GrContext.cpp:1481: strokeWidth = 0.5;
On 2012/04/09 21:17:58, bsalomon wrote:
> I think we should stick to 1.0 We're thicker for other types of hairlines
> already. If we decide to match SW then I think it should be a global decision
> for all hairlines. (IMO the apparent thickness of hairlines is not invariant
to
> rotation).

Done.

http://codereview.appspot.com/5696086/diff/17018/src/gpu/GrDrawState.h
File src/gpu/GrDrawState.h (right):

http://codereview.appspot.com/5696086/diff/17018/src/gpu/GrDrawState.h#newcod...
src/gpu/GrDrawState.h:554: kCircle_EdgeType,
On 2012/04/09 21:17:58, bsalomon wrote:
> Let's put a comment about what the params are to this edge type.

Done.

[guanqun, 2012-04-10 07:16:27]

With some testing, I found the method used right now (track whether it's a
circle and reconstruct its center point and radius) is incorrect:

I add a circle (radius is 10) to a path and rotate the path from degree 0 to
degree 359. For each degree, I get the bounding rect of the rotated path:

rotate: 0 degree. height/width: 20.000000
rotate: 1 degree. height/width: 20.141535
rotate: 2 degree. height/width: 20.276934
rotate: 3 degree. height/width: 20.406155
rotate: 4 degree. height/width: 20.529163
rotate: 5 degree. height/width: 20.645916
rotate: 6 degree. height/width: 20.756380
rotate: 7 degree. height/width: 20.860523
rotate: 8 degree. height/width: 20.958309
rotate: 9 degree. height/width: 21.049713
rotate: 10 degree. height/width: 21.134705
rotate: 11 degree. height/width: 21.213257
rotate: 12 degree. height/width: 21.285349
rotate: 13 degree. height/width: 21.350958
rotate: 14 degree. height/width: 21.410061
rotate: 15 degree. height/width: 21.462643
rotate: 16 degree. height/width: 21.508688
rotate: 17 degree. height/width: 21.548182
rotate: 18 degree. height/width: 21.581110
rotate: 19 degree. height/width: 21.607466
rotate: 20 degree. height/width: 21.627239

As you can see from the output, the bounding rect returned is a little bigger
when it's rotated.

However, this issue can be fixed by interpolating the quad points in the path.

[reed1, 2012-04-10 15:51:54]

Comments on SkPath.

1. For now, can we decide that if we see a non-scale/translate matrix, we just
forget that it was an oval? rotation and skew seems to be very rare in the
browser.

2. If we take #1, then rectStaysRect is all we'll need from matrix.

[bsalomon, 2012-04-10 23:55:02]

On 2012/04/10 15:51:54, reed1 wrote:
> Comments on SkPath.
> 
> 1. For now, can we decide that if we see a non-scale/translate matrix, we just
> forget that it was an oval? rotation and skew seems to be very rare in the
> browser.

It's probably true that circles / ovals are usually drawn without rotation. We
can always revisit and perhaps come up with a more extensible way of
storing/updating a higher level description of a SkPath (like a subclassed
SkPathShape object hanging off SkPath).

> 
> 2. If we take #1, then rectStaysRect is all we'll need from matrix.

I'm not sure #2 is totally correct. We would only need rectStaysRect in within
SkPath for path transformations. But GrContext will want to examine the view
matrix and know that a circle stays a circle.

Guanqun, I think the word we are looking for is "similarity". Why don't we call
your matrix function isSimilarity or isSimilarityTransformation? Also, it
probably makes sense for the caller to provide the tolerance rather than always
using SK_ScalarNearlyZero (or whatever the constant is called). That way its
obvious from the call site that the determination has some fuzz factor.

The CL has gotten pretty big. Let's separate out these parts and get them in:

SkPath detection and the corresponding test
SkMatrix change and test

[guanqun, 2012-04-12 06:58:35]

On 2012/04/10 23:55:02, bsalomon wrote:
> On 2012/04/10 15:51:54, reed1 wrote:
> > Comments on SkPath.
> > 
> > 1. For now, can we decide that if we see a non-scale/translate matrix, we
just
> > forget that it was an oval? rotation and skew seems to be very rare in the
> > browser.
> 
> It's probably true that circles / ovals are usually drawn without rotation. We
> can always revisit and perhaps come up with a more extensible way of
> storing/updating a higher level description of a SkPath (like a subclassed
> SkPathShape object hanging off SkPath).

I'll take Mike's approach. Therefore we don't track rotated circles. I have a
local patch to track the rotated circle and I have to interpolate the quad
points in SkPath to get the correct bounding rect. And finally the code becomes
tricky and messy. So for simple implementation, I don't track rotated circles.

[guanqun, 2012-04-12 07:38:33]

I've separated this big CL into several smaller ones, the sequence should be:

1. http://codereview.appspot.com/5999050/ add isSimilarityTransform() and some
tests

2. http://codereview.appspot.com/6012047/ track oval path

3. and this one.

4. http://codereview.appspot.com/6005047/ gm for circles.

[guanqun, 2012-04-24 02:36:19]

Hi,

I've rebased this CL to the latest revision. And please review! Are there any
other issues that should be tackled besides isSimilarityTransform() ?

Thanks!

[bsalomon, 2012-04-24 14:19:52]

The thrust of my comments is that GrContext::drawOval should feel like a real
API to (theoretical) callers other than SkGpuDevice. It should have a clean
interface and just do what it says, draw an oval, without the caller having to
know whether internally it is drawn as a path or not.

http://codereview.appspot.com/5696086/diff/40001/include/gpu/GrContext.h
File include/gpu/GrContext.h (right):

http://codereview.appspot.com/5696086/diff/40001/include/gpu/GrContext.h#newc...
include/gpu/GrContext.h:448: * @param strokeWidth   specify the stroke width,
I don't understand why we need both doFill and strokeWidth. If a caller wants a
larger filled oval can't they just send down a larger rect? Can we not make
negative strokeWidth mean fill?

I think the comment about larger ovals and strokeWidth > 0 isn't really
necessary. It should be clear that stroking an oval will hit pixels outside the
rect that specifies the oval.

http://codereview.appspot.com/5696086/diff/40001/src/gpu/SkGpuDevice.cpp
File src/gpu/SkGpuDevice.cpp (right):

http://codereview.appspot.com/5696086/diff/40001/src/gpu/SkGpuDevice.cpp#newc...
src/gpu/SkGpuDevice.cpp:1023: // TODO: it's easy to add another shader for
aliased circles.
It seems to me like we are coupling SkGpuDevice to the limitations of
GrContext's drawOval method. I think GrContext's drawOval should always work for
any oval (AA or not, skew matrix or not, ..) and it should fallback to drawPath
when necessary. I don't like the idea of putting the burden of checking all
these conditions on the caller of drawOval

[guanqun, 2012-04-25 16:05:56]

Hi Brian,

I've come up two approaches to decouple SkGpuDevice::drawOval() and
GrContext::drawOval() and tried to make GrContext::drawOval() handle the drawing
properly (e.g. it can fallback to drawPath).

PatchSet 16:
    Nothing is added in SkGpuDevice. But in GrContext class's drawPath method,
we goto GrContext::drawOval() if the path is an oval. This approach gives us
less changes in the current code. But sadly it can't handle the stroke, because
when the code is in GrContext::drawPath(), its path parameter is already
modified, it's not the original path anymore.

PathSet 17:
    It adds some code in SkGpuDevice::drawPath() to check whether we should call
GrContext::drawPath() or GrContext::drawOval(). This is not that elegant as
PatchSet 16, but it can handle stroke. The parameter of "strokeWidth" is added
into GrContext::drawOval(). I'm not sure whether this approach would fit into
your decoupling design.

The two pathsets have passed the compiling, the details haven't been polished
yet. I'd like to get the feedback first before I go to the details.

Thanks!

http://codereview.appspot.com/5696086/diff/40001/include/gpu/GrContext.h
File include/gpu/GrContext.h (right):

http://codereview.appspot.com/5696086/diff/40001/include/gpu/GrContext.h#newc...
include/gpu/GrContext.h:448: * @param strokeWidth   specify the stroke width,
On 2012/04/24 14:19:52, bsalomon wrote:
> I don't understand why we need both doFill and strokeWidth. If a caller wants
a
> larger filled oval can't they just send down a larger rect? Can we not make
> negative strokeWidth mean fill?

It should be "Can we make negative strokeWidth mean fill?", right?

I'm using it for differentiate stroke style and strokeandfill style.

http://codereview.appspot.com/5696086/diff/40001/src/gpu/SkGpuDevice.cpp
File src/gpu/SkGpuDevice.cpp (right):

http://codereview.appspot.com/5696086/diff/40001/src/gpu/SkGpuDevice.cpp#newc...
src/gpu/SkGpuDevice.cpp:1023: // TODO: it's easy to add another shader for
aliased circles.
On 2012/04/24 14:19:52, bsalomon wrote:
> It seems to me like we are coupling SkGpuDevice to the limitations of
> GrContext's drawOval method. I think GrContext's drawOval should always work
for
> any oval (AA or not, skew matrix or not, ..) and it should fallback to
drawPath
> when necessary. I don't like the idea of putting the burden of checking all
> these conditions on the caller of drawOval

I agree SkGpuDevice::drawOval() relates too much to GrContext::drawOval(), I've
come up two approaches, please help decide which is better.

[bsalomon, 2012-04-25 17:30:23]

On 2012/04/25 16:05:56, guanqun wrote:
> Hi Brian,
> 
> I've come up two approaches to decouple SkGpuDevice::drawOval() and
> GrContext::drawOval() and tried to make GrContext::drawOval() handle the
drawing
> properly (e.g. it can fallback to drawPath).
> 
> PatchSet 16:
>     Nothing is added in SkGpuDevice. But in GrContext class's drawPath method,
> we goto GrContext::drawOval() if the path is an oval. This approach gives us
> less changes in the current code. But sadly it can't handle the stroke,
because
> when the code is in GrContext::drawPath(), its path parameter is already
> modified, it's not the original path anymore.

Patchset 16 is the direction what I was thinking. We will want to move the
decision about how to stroke down to GrContext rather than SkGpuDevice anyway
for other reasons. So let's still make the GrContext::drawOval API support
stroking even if the current caller has no way to call it. We'll make it
possible for drawPath to pass along the strokeWidth later.

I don't think we need a fill rule param to drawOval.  Let's let drawPath
determine if the fill is inverted to not call drawOval. It makes drawOval more
like the other existing special-case draws (drawRect, drawBitmap, etc) that
don't support inverse fills.


> 
> PathSet 17:
>     It adds some code in SkGpuDevice::drawPath() to check whether we should
call
> GrContext::drawPath() or GrContext::drawOval(). This is not that elegant as
> PatchSet 16, but it can handle stroke. The parameter of "strokeWidth" is added
> into GrContext::drawOval(). I'm not sure whether this approach would fit into
> your decoupling design.
> 
> The two pathsets have passed the compiling, the details haven't been polished
> yet. I'd like to get the feedback first before I go to the details.
> 
> Thanks!
> 
> http://codereview.appspot.com/5696086/diff/40001/include/gpu/GrContext.h
> File include/gpu/GrContext.h (right):
> 
>
http://codereview.appspot.com/5696086/diff/40001/include/gpu/GrContext.h#newc...
> include/gpu/GrContext.h:448: * @param strokeWidth   specify the stroke width,
> On 2012/04/24 14:19:52, bsalomon wrote:
> > I don't understand why we need both doFill and strokeWidth. If a caller
wants
> a
> > larger filled oval can't they just send down a larger rect? Can we not make
> > negative strokeWidth mean fill?
> 
> It should be "Can we make negative strokeWidth mean fill?", right?
> 

yes

> I'm using it for differentiate stroke style and strokeandfill style.

What I meant to suggest is this:

strokeWidth > 0  means stroke the oval
strokeWidth == 0 means hairline-stroke the oval
strokeWidth < 0 means fill the oval

If the caller (furture stroking-aware version of drawPath) wants to fill and
stroke they inflate the rect and call with strokeWidth = -1.


> 
> http://codereview.appspot.com/5696086/diff/40001/src/gpu/SkGpuDevice.cpp
> File src/gpu/SkGpuDevice.cpp (right):
> 
>
http://codereview.appspot.com/5696086/diff/40001/src/gpu/SkGpuDevice.cpp#newc...
> src/gpu/SkGpuDevice.cpp:1023: // TODO: it's easy to add another shader for
> aliased circles.
> On 2012/04/24 14:19:52, bsalomon wrote:
> > It seems to me like we are coupling SkGpuDevice to the limitations of
> > GrContext's drawOval method. I think GrContext's drawOval should always work
> for
> > any oval (AA or not, skew matrix or not, ..) and it should fallback to
> drawPath
> > when necessary. I don't like the idea of putting the burden of checking all
> > these conditions on the caller of drawOval
> 
> I agree SkGpuDevice::drawOval() relates too much to GrContext::drawOval(),
I've
> come up two approaches, please help decide which is better.

[guanqun, 2012-04-26 13:13:23]

Updated patch is available, circles in GM can be rendered properly via GPU.

However, as you mentioned, this implementation can't handle stroke right now, we
need to add this feature later...

Please review.

[bsalomon, 2012-04-27 13:06:30]

On 2012/04/26 13:13:23, guanqun wrote:
> Updated patch is available, circles in GM can be rendered properly via GPU.
> 
> However, as you mentioned, this implementation can't handle stroke right now,
we
> need to add this feature later...
> 
> Please review.

LGTM, committed as r3772. I made a few very minor edits (comments and changed
GrAssert(!" to GrCrash(")