Add GPU support for axis-aligned ovals and enable all stroked circles

Add GPU support for axis-aligned ovals:
- Add drawOval base function to SkDevice, and override in SkGpuDevice
- Move isSimilarityTransform to SkMatrix (renamed to mapsCircleToCircle to match mapsRectToRect)
- Since both SkGpuDevice::drawOval() and GrContext::drawPath() can try to draw ovals, added GrContext::canDrawOval() 
- Hooked in axis-aligned oval fill shader

[reed1, 2013-01-17 15:18:27]

https://codereview.appspot.com/7137050/diff/2001/include/core/SkDevice.h
File include/core/SkDevice.h (right):

https://codereview.appspot.com/7137050/diff/2001/include/core/SkDevice.h#newc...
include/core/SkDevice.h:228: const SkPaint& paint);
Perhaps we need to document which of these entry points are implemented in terms
of other virtuals (e.g. drawOval calls drawPath), and which *must* be overridden
by the subclass... Not strictly part of the CL, but it does make me think about
it.

https://codereview.appspot.com/7137050/diff/2001/src/core/SkMatrix.cpp
File src/core/SkMatrix.cpp (right):

https://codereview.appspot.com/7137050/diff/2001/src/core/SkMatrix.cpp#newcod...
src/core/SkMatrix.cpp:173: bool SkMatrix::circleStaysCircle(SkScalar tol) const
{
if (this->getType() <= kTranslate)

https://codereview.appspot.com/7137050/diff/2001/src/core/SkMatrix.cpp#newcod...
src/core/SkMatrix.cpp:180: 
Given that most of our matrices will be kScale, but not kAffine, is it worth it
to notice that and do fewer tests?

https://codereview.appspot.com/7137050/diff/2001/src/core/SkMatrix.cpp#newcod...
src/core/SkMatrix.cpp:185: 
- Perhaps a comment explaining why this test.
- If it is to say that degenerate matrices return false, perhaps that should be
in the public dox
- nit: we like to place the literate on the left...

e.g.
    if (0 == mx && ...)

[robertphillips, 2013-01-17 15:25:43]

LGTM + nits & questions

https://codereview.appspot.com/7137050/diff/2001/include/gpu/GrContext.h
File include/gpu/GrContext.h (right):

https://codereview.appspot.com/7137050/diff/2001/include/gpu/GrContext.h#newc...
include/gpu/GrContext.h:454: *
rename 'rect' to 'oval'?

https://codereview.appspot.com/7137050/diff/2001/src/core/SkCanvas.cpp
File src/core/SkCanvas.cpp (right):

https://codereview.appspot.com/7137050/diff/2001/src/core/SkCanvas.cpp#newcod...
src/core/SkCanvas.cpp:1544: 
is kRect_Type correct here?

https://codereview.appspot.com/7137050/diff/2001/src/core/SkDevice.cpp
File src/core/SkDevice.cpp (right):

https://codereview.appspot.com/7137050/diff/2001/src/core/SkDevice.cpp#newcod...
src/core/SkDevice.cpp:330: void SkDevice::drawOval(const SkDraw& draw, const
SkRect& oval,
spacing?

https://codereview.appspot.com/7137050/diff/2001/src/gpu/GrContext.cpp
File src/gpu/GrContext.cpp (right):

https://codereview.appspot.com/7137050/diff/2001/src/gpu/GrContext.cpp#newcod...
src/gpu/GrContext.cpp:950: #ifdef SK_DEBUG
add brackets here so the variables don't leak out of the debug block?

https://codereview.appspot.com/7137050/diff/2001/src/gpu/GrContext.cpp#newcod...
src/gpu/GrContext.cpp:952: bool isStroke = (style == SkStrokeRec::kStroke_Style
|| style == SkStrokeRec::kHairline_Style);
isAxisAlignedEllipse -> isFilledAxisAlignedEllipse?

https://codereview.appspot.com/7137050/diff/2001/src/gpu/GrContext.cpp#newcod...
src/gpu/GrContext.cpp:1002: if (strokeWidth == 0) {
SK_ScalarHalf

https://codereview.appspot.com/7137050/diff/2001/src/gpu/GrContext.cpp#newcod...
src/gpu/GrContext.cpp:1043: 
Not sure if we still care about this: SkScalarDiv

https://codereview.appspot.com/7137050/diff/2001/src/gpu/GrContext.cpp#newcod...
src/gpu/GrContext.cpp:1060: // will be captured.
Not yours but SkFloatToScalar(0.5f) -> SK_ScalarHalf

https://codereview.appspot.com/7137050/diff/2001/src/gpu/GrContext.cpp#newcod...
src/gpu/GrContext.cpp:1074: bool GrContext::canDrawOval(const GrRect& rect, bool
isStroke, bool isAntiAlias) const {
We can't draw aliased ovals?

https://codereview.appspot.com/7137050/diff/2001/src/gpu/SkGpuDevice.cpp
File src/gpu/SkGpuDevice.cpp (right):

https://codereview.appspot.com/7137050/diff/2001/src/gpu/SkGpuDevice.cpp#newc...
src/gpu/SkGpuDevice.cpp:693: // check with context to see if it can render this
directly
Do we need to check for hairlines here?

[bsalomon, 2013-01-17 16:31:00]

Do we have a good GM that exercises ovals in interesting ways
-hairline stroke
-fill
-regular stroke
-different matrix types
-clipped
-with a non-solid fill (e.g. gradient)

https://codereview.appspot.com/7137050/diff/2001/bench/PathBench.cpp
File bench/PathBench.cpp (right):

https://codereview.appspot.com/7137050/diff/2001/bench/PathBench.cpp#newcode696
bench/PathBench.cpp:696: paint.setStyle(SkPaint::kStroke_Style);
Is this and the stroke-width below the only diff between circles and circles2?
Maybe they should be the same class with a pool param to stroke or not. Either
way we should have the word "stroke" in the name methinks.

https://codereview.appspot.com/7137050/diff/2001/include/core/SkMatrix.h
File include/core/SkMatrix.h (right):

https://codereview.appspot.com/7137050/diff/2001/include/core/SkMatrix.h#newc...
include/core/SkMatrix.h:91: bool circleStaysCircle(SkScalar tol =
SK_ScalarNearlyZero) const;
isSimilarity()?

isSimilarTransform()?

I prefer the former but am open to other suggestions.

https://codereview.appspot.com/7137050/diff/2001/include/gpu/GrContext.h
File include/gpu/GrContext.h (right):

https://codereview.appspot.com/7137050/diff/2001/include/gpu/GrContext.h#newc...
include/gpu/GrContext.h:459: bool canDrawOval(const GrRect& rect,
must this be public?

I'd prefer to hide it from the client of this class. Perhaps we take the
existing drawOval and make it a private called internalDrawOval. Then drawOval
becomes:

if (this->canDrawOval()) {
  this->internalDrawOval()
} else {
  this->internalDrawPath();
}

drawPath could directly call internalDrawOval() when the path is an oval and
canDrawOval() is true..

https://codereview.appspot.com/7137050/diff/2001/src/gpu/SkGpuDevice.cpp
File src/gpu/SkGpuDevice.cpp (right):

https://codereview.appspot.com/7137050/diff/2001/src/gpu/SkGpuDevice.cpp#newc...
src/gpu/SkGpuDevice.cpp:694: if (!usePath && !fContext->canDrawOval(oval,
paint.getStyle() == SkPaint::kStroke_Style, paint.isAntiAlias())) {
nit, wrap at 100

https://codereview.appspot.com/7137050/diff/2001/src/gpu/gl/GrGLProgram.cpp
File src/gpu/gl/GrGLProgram.cpp (right):

https://codereview.appspot.com/7137050/diff/2001/src/gpu/gl/GrGLProgram.cpp#n...
src/gpu/gl/GrGLProgram.cpp:284: builder->fFSCode.appendf("\tedgeAlpha =
smoothstep(d - 0.5, d + 0.5, %s.z);\n", fsName);
Not your doing, but it bugs me that circle code uses smoothstep. Not that it is
wrong, but that we are inconsistent in the ramp we use for edge-aa. When we move
these to effects I'd like us to able to experiment with different ramps
(including LUTs) but in a way that applies to all edge-aa shaders.

[bsalomon, 2013-01-17 19:55:42]

I hand over the keys to my L_G_T_M to Robert since I'll be out of the office
until Tuesday.

[JimVV, 2013-01-17 20:53:44]

Updated with requested changes. 

I don't think we have a single GM that covers all the modes Brian mentioned, but
circles does test a lot of them, and so does convexpaths.

And as justification for this, here are the bench results from Windows. Linux
results are comparable. I didn't find any that were consistently worse. I did
change the path_*_*_oval benches so they actually draw an ellipse, not a circle.

                     ovals_1 GPU             5.20       2.96      +2.24   +43.1%
          path_fill_big_oval GPU             5.01       2.84      +2.17   +43.3%
                     ovals_3 GPU             3.05       0.93      +2.12   +69.5%
        path_fill_small_oval GPU             8.07       1.72      +6.35   +78.7%
      path_stroke_big_circle GPU            53.16       6.34     +46.82   +88.1%
              circles_stroke GPU           150.97      15.65    +135.32   +89.6%
    path_stroke_small_circle GPU            42.14       2.12     +40.02   +95.0%

https://codereview.appspot.com/7137050/diff/2001/bench/PathBench.cpp
File bench/PathBench.cpp (right):

https://codereview.appspot.com/7137050/diff/2001/bench/PathBench.cpp#newcode696
bench/PathBench.cpp:696: paint.setStyle(SkPaint::kStroke_Style);
On 2013/01/17 16:31:00, bsalomon wrote:
> Is this and the stroke-width below the only diff between circles and circles2?
> Maybe they should be the same class with a pool param to stroke or not. Either
> way we should have the word "stroke" in the name methinks.

Done.

https://codereview.appspot.com/7137050/diff/2001/include/core/SkDevice.h
File include/core/SkDevice.h (right):

https://codereview.appspot.com/7137050/diff/2001/include/core/SkDevice.h#newc...
include/core/SkDevice.h:228: const SkPaint& paint);
On 2013/01/17 15:18:28, reed1 wrote:
> Perhaps we need to document which of these entry points are implemented in
terms
> of other virtuals (e.g. drawOval calls drawPath), and which *must* be
overridden
> by the subclass... Not strictly part of the CL, but it does make me think
about
> it.

I'm not sure where this documentation would go -- a comment here doesn't seem to
be the right place. None of them need to be overridden, and the choice to use
drawPath to render ovals in some cases and drawOval for others lies within the
subclasses. I could note it as a possibility, but I'm not sure that would be
helpful.

https://codereview.appspot.com/7137050/diff/2001/include/core/SkMatrix.h
File include/core/SkMatrix.h (right):

https://codereview.appspot.com/7137050/diff/2001/include/core/SkMatrix.h#newc...
include/core/SkMatrix.h:91: bool circleStaysCircle(SkScalar tol =
SK_ScalarNearlyZero) const;
On 2013/01/17 16:31:00, bsalomon wrote:
> isSimilarity()?
> 
> isSimilarTransform()?
> 
> I prefer the former but am open to other suggestions.

Done.

https://codereview.appspot.com/7137050/diff/2001/include/gpu/GrContext.h
File include/gpu/GrContext.h (right):

https://codereview.appspot.com/7137050/diff/2001/include/gpu/GrContext.h#newc...
include/gpu/GrContext.h:454: *
On 2013/01/17 15:25:43, robertphillips wrote:
> rename 'rect' to 'oval'?

Done.

https://codereview.appspot.com/7137050/diff/2001/include/gpu/GrContext.h#newc...
include/gpu/GrContext.h:459: bool canDrawOval(const GrRect& rect,
On 2013/01/17 16:31:00, bsalomon wrote:
> must this be public?
> 
> I'd prefer to hide it from the client of this class. Perhaps we take the
> existing drawOval and make it a private called internalDrawOval. Then drawOval
> becomes:
> 
> if (this->canDrawOval()) {
>   this->internalDrawOval()
> } else {
>   this->internalDrawPath();
> }
> 
> drawPath could directly call internalDrawOval() when the path is an oval and
> canDrawOval() is true..
> 

Done.

https://codereview.appspot.com/7137050/diff/2001/src/core/SkCanvas.cpp
File src/core/SkCanvas.cpp (right):

https://codereview.appspot.com/7137050/diff/2001/src/core/SkCanvas.cpp#newcod...
src/core/SkCanvas.cpp:1544: 
On 2013/01/17 15:25:43, robertphillips wrote:
> is kRect_Type correct here?

Added kOval_Type to SkDrawFilter.

https://codereview.appspot.com/7137050/diff/2001/src/core/SkDevice.cpp
File src/core/SkDevice.cpp (right):

https://codereview.appspot.com/7137050/diff/2001/src/core/SkDevice.cpp#newcod...
src/core/SkDevice.cpp:330: void SkDevice::drawOval(const SkDraw& draw, const
SkRect& oval,
On 2013/01/17 15:25:43, robertphillips wrote:
> spacing?

Done.

https://codereview.appspot.com/7137050/diff/2001/src/core/SkMatrix.cpp
File src/core/SkMatrix.cpp (right):

https://codereview.appspot.com/7137050/diff/2001/src/core/SkMatrix.cpp#newcod...
src/core/SkMatrix.cpp:173: bool SkMatrix::circleStaysCircle(SkScalar tol) const
{
On 2013/01/17 15:18:28, reed1 wrote:
> if (this->getType() <= kTranslate)

Done.

https://codereview.appspot.com/7137050/diff/2001/src/core/SkMatrix.cpp#newcod...
src/core/SkMatrix.cpp:180: 
On 2013/01/17 15:18:28, reed1 wrote:
> Given that most of our matrices will be kScale, but not kAffine, is it worth
it
> to notice that and do fewer tests?

Done.

https://codereview.appspot.com/7137050/diff/2001/src/core/SkMatrix.cpp#newcod...
src/core/SkMatrix.cpp:185: 
On 2013/01/17 15:18:28, reed1 wrote:
> - Perhaps a comment explaining why this test.
> - If it is to say that degenerate matrices return false, perhaps that should
be
> in the public dox
> - nit: we like to place the literate on the left...
> 
> e.g.
>     if (0 == mx && ...)

Done.

https://codereview.appspot.com/7137050/diff/2001/src/gpu/GrContext.cpp
File src/gpu/GrContext.cpp (right):

https://codereview.appspot.com/7137050/diff/2001/src/gpu/GrContext.cpp#newcod...
src/gpu/GrContext.cpp:950: #ifdef SK_DEBUG
On 2013/01/17 15:25:43, robertphillips wrote:
> add brackets here so the variables don't leak out of the debug block?

Done.

https://codereview.appspot.com/7137050/diff/2001/src/gpu/GrContext.cpp#newcod...
src/gpu/GrContext.cpp:952: bool isStroke = (style == SkStrokeRec::kStroke_Style
|| style == SkStrokeRec::kHairline_Style);
On 2013/01/17 15:25:43, robertphillips wrote:
> isAxisAlignedEllipse -> isFilledAxisAlignedEllipse?

Done.

https://codereview.appspot.com/7137050/diff/2001/src/gpu/GrContext.cpp#newcod...
src/gpu/GrContext.cpp:1002: if (strokeWidth == 0) {
On 2013/01/17 15:25:43, robertphillips wrote:
> SK_ScalarHalf

Done.

https://codereview.appspot.com/7137050/diff/2001/src/gpu/GrContext.cpp#newcod...
src/gpu/GrContext.cpp:1043: 
On 2013/01/17 15:25:43, robertphillips wrote:
> Not sure if we still care about this: SkScalarDiv

Done.

https://codereview.appspot.com/7137050/diff/2001/src/gpu/GrContext.cpp#newcod...
src/gpu/GrContext.cpp:1060: // will be captured.
On 2013/01/17 15:25:43, robertphillips wrote:
> Not yours but SkFloatToScalar(0.5f) -> SK_ScalarHalf

Done.

https://codereview.appspot.com/7137050/diff/2001/src/gpu/GrContext.cpp#newcod...
src/gpu/GrContext.cpp:1074: bool GrContext::canDrawOval(const GrRect& rect, bool
isStroke, bool isAntiAlias) const {
On 2013/01/17 15:25:43, robertphillips wrote:
> We can't draw aliased ovals?

Nope, not yet.

https://codereview.appspot.com/7137050/diff/2001/src/gpu/SkGpuDevice.cpp
File src/gpu/SkGpuDevice.cpp (right):

https://codereview.appspot.com/7137050/diff/2001/src/gpu/SkGpuDevice.cpp#newc...
src/gpu/SkGpuDevice.cpp:693: // check with context to see if it can render this
directly
On 2013/01/17 15:25:43, robertphillips wrote:
> Do we need to check for hairlines here?

It's not necessary, it gets checked in GrContext.

https://codereview.appspot.com/7137050/diff/2001/src/gpu/SkGpuDevice.cpp#newc...
src/gpu/SkGpuDevice.cpp:694: if (!usePath && !fContext->canDrawOval(oval,
paint.getStyle() == SkPaint::kStroke_Style, paint.isAntiAlias())) {
On 2013/01/17 16:31:00, bsalomon wrote:
> nit, wrap at 100

This block's been removed (handled in GrContext now).

https://codereview.appspot.com/7137050/diff/2001/src/gpu/gl/GrGLProgram.cpp
File src/gpu/gl/GrGLProgram.cpp (right):

https://codereview.appspot.com/7137050/diff/2001/src/gpu/gl/GrGLProgram.cpp#n...
src/gpu/gl/GrGLProgram.cpp:284: builder->fFSCode.appendf("\tedgeAlpha =
smoothstep(d - 0.5, d + 0.5, %s.z);\n", fsName);
On 2013/01/17 16:31:00, bsalomon wrote:
> Not your doing, but it bugs me that circle code uses smoothstep. Not that it
is
> wrong, but that we are inconsistent in the ramp we use for edge-aa. When we
move
> these to effects I'd like us to able to experiment with different ramps
> (including LUTs) but in a way that applies to all edge-aa shaders.

I'm not keen on it either, as it's probably slower than clamp and I'm not sure
you'd notice the difference across one pixel. I'll note it for later as
something to look at.

[reed1, 2013-01-17 21:21:13]

https://codereview.appspot.com/7137050/diff/17001/src/core/SkDevice.cpp
File src/core/SkDevice.cpp (right):

https://codereview.appspot.com/7137050/diff/17001/src/core/SkDevice.cpp#newco...
src/core/SkDevice.cpp:334: // call the non-virtual version
pass the last two arguments to drawPath: (..., NULL, true), since the path is
mutable.

[JimVV, 2013-01-17 22:06:14]

On 2013/01/17 21:21:13, reed1 wrote:
> https://codereview.appspot.com/7137050/diff/17001/src/core/SkDevice.cpp
> File src/core/SkDevice.cpp (right):
> 
>
https://codereview.appspot.com/7137050/diff/17001/src/core/SkDevice.cpp#newco...
> src/core/SkDevice.cpp:334: // call the non-virtual version
> pass the last two arguments to drawPath: (..., NULL, true), since the path is
> mutable.

Done.

Also fixed up the test_matrix_is_similarity() unit test to use the SkMatrix
function, and fixed a bug in SkMatrix::isSimilarity() exposed by the test.

[reed1, 2013-01-18 18:22:36]

lgtm w/ random style thoughts (not required)

https://codereview.appspot.com/7137050/diff/18001/src/core/SkCanvas.cpp
File src/core/SkCanvas.cpp (right):

https://codereview.appspot.com/7137050/diff/18001/src/core/SkCanvas.cpp#newco...
src/core/SkCanvas.cpp:1544: 
Do you want kOval_Type here?

https://codereview.appspot.com/7137050/diff/18001/src/core/SkMatrix.cpp
File src/core/SkMatrix.cpp (right):

https://codereview.appspot.com/7137050/diff/18001/src/core/SkMatrix.cpp#newco...
src/core/SkMatrix.cpp:178: }
hasPerspective() is fine, though (mask & kPerspective_Mask) might be more in
parallel w/ the rest of this code (and minutely faster). Just a style
suggestion, not a request.

https://codereview.appspot.com/7137050/diff/18001/src/core/SkMatrix.cpp#newco...
src/core/SkMatrix.cpp:187: return !SkScalarNearlyZero(mx)
Just a thought. Is this code clearer than

nearlyequal(SkScalarAbs(mx), SkScalarAbs(my))
?