Add ability to ninepatch blurred rounded rectangle

Add ability to ninepatch blurred rounded rectangle

Speed up drawing large blurry round rectangles by converting them to
nine patches.

SkDraw:
Add drawRRect.

SkBitmapDevice:
Call SkDraw::drawRRect instead of converting SkRRect to an SkPath.

SkMaskFilter/SkBlurMaskFilter:
Create a nine patch of a blurred round rect and draw it instead of
drawing the entire thing.

SkPDFDevice:
Override drawRRect to perform the old behavior in
SkBitmapDevice::drawRect.

Depends on https://codereview.chromium.org/52703003

Tests are in https://codereview.chromium.org/52793005

BUG=https://b.corp.google.com/issue?id=11174385
R=reed@google.com, robertphillips@google.com

Committed: https://code.google.com/p/skia/source/detail?r=12198

[scroggo, 2013-10-30 22:36:19]

On the SKP giving us problems, I see the following times, where old is without
this change, new is with this change, and diff and diffP are old - new, so a
positive number in the diff columns means a speedup:

         bench conf    time        old        new       diff    diffP 
picture571.skp GPU     c      44994.47   45470.08    -475.61    -1.1% 
picture571.skp GPU     g      45866.75   45529.90    +336.85    +0.7% 
picture571.skp NULLGPU c       1318.15    1042.85    +275.30   +20.9% 
picture571.skp 565     c      13233.54    2432.56  +10800.98   +81.6% 
picture571.skp 8888    c      19891.38    3073.82  +16817.56   +84.5% 

There was a speedup except in the case of the GPU, which I think is just
variance.

[scroggo, 2013-10-30 22:51:31]

https://codereview.chromium.org/48623006/diff/40001/src/effects/SkBlurMaskFil...
File src/effects/SkBlurMaskFilter.cpp (right):

https://codereview.chromium.org/48623006/diff/40001/src/effects/SkBlurMaskFil...
src/effects/SkBlurMaskFilter.cpp:183: SkCanvas* canvas = SkNEW_ARGS(SkCanvas,
(bitmap));
In trying to share code, I've added an allocation! I'll try to figure out the
right way to share code without the extra allocation...

[robertphillips, 2013-10-31 00:24:58]

lgtm. Are there enough tests/gms?

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

https://codereview.chromium.org/48623006/diff/40001/src/core/SkDraw.cpp#newco...
src/core/SkDraw.cpp:1042: if (paint.getPathEffect()
will this fit on the prior line?

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

https://codereview.chromium.org/48623006/diff/40001/src/core/SkMaskFilter.cpp...
src/core/SkMaskFilter.cpp:210: SkBlitter* blitter, SkPaint::Style style) const {
Could you expand on what makes this case sneaky?

https://codereview.chromium.org/48623006/diff/40001/src/effects/SkBlurMaskFil...
File src/effects/SkBlurMaskFilter.cpp (right):

https://codereview.chromium.org/48623006/diff/40001/src/effects/SkBlurMaskFil...
src/effects/SkBlurMaskFilter.cpp:183: SkCanvas* canvas = SkNEW_ARGS(SkCanvas,
(bitmap));
Can you just set up the mask then return true or false and let the caller create
the canvas (and do the translate)?

https://codereview.chromium.org/48623006/diff/40001/src/effects/SkBlurMaskFil...
src/effects/SkBlurMaskFilter.cpp:241: case SkRRect::kRect_Type:
I think we should assert here (that the RRect isn't a rect). It seems like that
should be caught upstream.

https://codereview.chromium.org/48623006/diff/40001/src/effects/SkBlurMaskFil...
src/effects/SkBlurMaskFilter.cpp:289: 
I think there should be a +3 in here and maybe make it a constant?

https://codereview.chromium.org/48623006/diff/40001/src/effects/SkBlurMaskFil...
src/effects/SkBlurMaskFilter.cpp:297: 
Here too

https://codereview.chromium.org/48623006/diff/40001/src/effects/SkBlurMaskFil...
src/effects/SkBlurMaskFilter.cpp:303: // Add some extra space to make sure we
have a stretch row/col
Are the x & y values correct? Can they be 0 & 0?

https://codereview.chromium.org/48623006/diff/40001/src/effects/SkBlurMaskFil...
src/effects/SkBlurMaskFilter.cpp:308: SkRRect smallRR;
Maybe change setRectRadii to setAllRadii?

[robertphillips, 2013-10-31 13:30:30]

https://codereview.chromium.org/48623006/diff/40001/src/effects/SkBlurMaskFil...
File src/effects/SkBlurMaskFilter.cpp (right):

https://codereview.chromium.org/48623006/diff/40001/src/effects/SkBlurMaskFil...
src/effects/SkBlurMaskFilter.cpp:277: 
Does this behave correctly if the RR is 100x100 with 1x1 corners and the blur is
10 pixels wide?

[scroggo, 2013-11-01 21:45:46]

New patch fixes the case robert pointed out in patch set 1. PTAL.

> Are there enough tests/gms?

I am updating https://codereview.chromium.org/52793005 to have more tests.

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

https://codereview.chromium.org/48623006/diff/40001/src/core/SkDraw.cpp#newco...
src/core/SkDraw.cpp:1042: if (paint.getPathEffect()
On 2013/10/31 00:24:58, robertphillips wrote:
> will this fit on the prior line?

It is 81 chars, so it falls within our hard limit of 100 chars, but is just
above our soft "fit in with surrounding code" limit of 80 chars.

Either way is fine with me.

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

https://codereview.chromium.org/48623006/diff/40001/src/core/SkMaskFilter.cpp...
src/core/SkMaskFilter.cpp:210: SkBlitter* blitter, SkPaint::Style style) const {
On 2013/10/31 00:24:58, robertphillips wrote:
> Could you expand on what makes this case sneaky?

Done.

https://codereview.chromium.org/48623006/diff/40001/src/effects/SkBlurMaskFil...
File src/effects/SkBlurMaskFilter.cpp (right):

https://codereview.chromium.org/48623006/diff/40001/src/effects/SkBlurMaskFil...
src/effects/SkBlurMaskFilter.cpp:183: SkCanvas* canvas = SkNEW_ARGS(SkCanvas,
(bitmap));
On 2013/10/31 00:24:58, robertphillips wrote:
> Can you just set up the mask then return true or false and let the caller
create
> the canvas (and do the translate)?

I could, but that means I'm still duplicating code. How about a macro?

https://codereview.chromium.org/48623006/diff/40001/src/effects/SkBlurMaskFil...
src/effects/SkBlurMaskFilter.cpp:241: case SkRRect::kRect_Type:
On 2013/10/31 00:24:58, robertphillips wrote:
> I think we should assert here (that the RRect isn't a rect). It seems like
that
> should be caught upstream.

Done.

https://codereview.chromium.org/48623006/diff/40001/src/effects/SkBlurMaskFil...
src/effects/SkBlurMaskFilter.cpp:277: 
On 2013/10/31 13:30:31, robertphillips wrote:
> Does this behave correctly if the RR is 100x100 with 1x1 corners and the blur
is
> 10 pixels wide?

No. Latest patch fixes this. Thank you for the suggested test case.

https://codereview.chromium.org/48623006/diff/40001/src/effects/SkBlurMaskFil...
src/effects/SkBlurMaskFilter.cpp:289: 
On 2013/10/31 00:24:58, robertphillips wrote:
> I think there should be a +3 in here and maybe make it a constant?

Done.

https://codereview.chromium.org/48623006/diff/40001/src/effects/SkBlurMaskFil...
src/effects/SkBlurMaskFilter.cpp:297: 
On 2013/10/31 00:24:58, robertphillips wrote:
> Here too

Done.

https://codereview.chromium.org/48623006/diff/40001/src/effects/SkBlurMaskFil...
src/effects/SkBlurMaskFilter.cpp:303: // Add some extra space to make sure we
have a stretch row/col
On 2013/10/31 00:24:58, robertphillips wrote:
> Are the x & y values correct? Can they be 0 & 0?

The x and y values are irrelevant. I have switched to calling MakeWH, so they
are 0, 0.

https://codereview.chromium.org/48623006/diff/40001/src/effects/SkBlurMaskFil...
src/effects/SkBlurMaskFilter.cpp:308: SkRRect smallRR;
On 2013/10/31 00:24:58, robertphillips wrote:
> Maybe change setRectRadii to setAllRadii?

I think the current name makes sense. It sets both the rect and the radii.

[reed1, 2013-11-04 17:46:51]

what would happen if we didn't make the changes to PDFDevice?

https://codereview.chromium.org/48623006/diff/150001/include/core/SkDraw.h
File include/core/SkDraw.h (right):

https://codereview.chromium.org/48623006/diff/150001/include/core/SkDraw.h#ne...
include/core/SkDraw.h:27: struct SkRRect;
class

https://codereview.chromium.org/48623006/diff/150001/include/core/SkRRect.h
File include/core/SkRRect.h (right):

https://codereview.chromium.org/48623006/diff/150001/include/core/SkRRect.h#n...
include/core/SkRRect.h:179: const SkVector* getAllRadii() const { return fRadii;
}
// returns all 4 radii vectors, in Corner order.

Are we sure we want to promise that we will always store them contiguously in
this order?

https://codereview.chromium.org/48623006/diff/150001/src/effects/SkBlurMaskFi...
File src/effects/SkBlurMaskFilter.cpp (right):

https://codereview.chromium.org/48623006/diff/150001/src/effects/SkBlurMaskFi...
src/effects/SkBlurMaskFilter.cpp:163: // Common code used by both
draw_rrect_into_mask and drawRectsIntoMask.
errrrrrr, a giant macro with an embedded return? Did a conventional helper
function not work?

e.g.

static bool prepare_mask(const SkRect& bounds, SkMask* mask) {
    ...
}

https://codereview.chromium.org/48623006/diff/150001/src/effects/SkBlurMaskFi...
src/effects/SkBlurMaskFilter.cpp:276: const SkVector& UL =
rrect.radii(SkRRect::kUpperLeft_Corner);
we're not calling the new allRadii API here...? (which is fine with me, since
I'm not convinced we should add it, but I am curious why we aren't).

[scroggo, 2013-11-04 19:52:53]

https://codereview.chromium.org/48623006/diff/150001/include/core/SkDraw.h
File include/core/SkDraw.h (right):

https://codereview.chromium.org/48623006/diff/150001/include/core/SkDraw.h#ne...
include/core/SkDraw.h:27: struct SkRRect;
On 2013/11/04 17:46:52, reed1 wrote:
> class 

Done.

https://codereview.chromium.org/48623006/diff/150001/include/core/SkRRect.h
File include/core/SkRRect.h (right):

https://codereview.chromium.org/48623006/diff/150001/include/core/SkRRect.h#n...
include/core/SkRRect.h:179: const SkVector* getAllRadii() const { return fRadii;
}
On 2013/11/04 17:46:52, reed1 wrote:
> // returns all 4 radii vectors, in Corner order.
> 
> Are we sure we want to promise that we will always store them contiguously in
> this order?

It seems logical to me that this would return them in the same order as
setRectRadii returns them. That said, it is only a convenience, so I'm happy to
remove.

https://codereview.chromium.org/48623006/diff/150001/src/effects/SkBlurMaskFi...
File src/effects/SkBlurMaskFilter.cpp (right):

https://codereview.chromium.org/48623006/diff/150001/src/effects/SkBlurMaskFi...
src/effects/SkBlurMaskFilter.cpp:163: // Common code used by both
draw_rrect_into_mask and drawRectsIntoMask.
On 2013/11/04 17:46:52, reed1 wrote:
> errrrrrr, a giant macro with an embedded return? Did a conventional helper
> function not work?
> 
> e.g.
> 
> static bool prepare_mask(const SkRect& bounds, SkMask* mask) {
>     ...
> }
> 

Ideally, I'd like to share common code for creating the SkCanvas (and
translating it to the right position). Since SkCanvas does not provide a way to
change its device, I originally allocated a new SkCanvas, which allows that, at
the cost of the extra allocation. The latest patch just accepts some code
duplication.

https://codereview.chromium.org/48623006/diff/150001/src/effects/SkBlurMaskFi...
src/effects/SkBlurMaskFilter.cpp:276: const SkVector& UL =
rrect.radii(SkRRect::kUpperLeft_Corner);
On 2013/11/04 17:46:52, reed1 wrote:
> we're not calling the new allRadii API here...? (which is fine with me, since
> I'm not convinced we should add it, but I am curious why we aren't).

Really getAllRadii() is just a convenience for my call to setRectRadii(), where
I want to set to exactly the same values. Here I want to know about each set of
radii individually, so getAllRadii() provides no benefit.

https://codereview.chromium.org/48623006/diff/150001/src/pdf/SkPDFDevice.cpp
File src/pdf/SkPDFDevice.cpp (right):

https://codereview.chromium.org/48623006/diff/150001/src/pdf/SkPDFDevice.cpp#...
src/pdf/SkPDFDevice.cpp:939: this->drawPath(draw, path, paint, NULL, true);
On 2013/11/04 17:46:51, reed1 wrote:
> what would happen if we didn't make the changes to PDFDevice?

PDF would not draw a blurred round rect. The mask filter happily reports that it
drew successfully, but we never end up going down into the drawPath code, which
emits the PDF commands.

This code just does what SkBitmapDevice::drawRRect used to do, so the PDF output
has not changed.

[reed1, 2013-11-04 20:03:48]

On 2013/11/04 19:52:53, scroggo wrote:
> https://codereview.chromium.org/48623006/diff/150001/include/core/SkDraw.h
> File include/core/SkDraw.h (right):
> 
>
https://codereview.chromium.org/48623006/diff/150001/include/core/SkDraw.h#ne...
> include/core/SkDraw.h:27: struct SkRRect;
> On 2013/11/04 17:46:52, reed1 wrote:
> > class 
> 
> Done.
> 
> https://codereview.chromium.org/48623006/diff/150001/include/core/SkRRect.h
> File include/core/SkRRect.h (right):
> 
>
https://codereview.chromium.org/48623006/diff/150001/include/core/SkRRect.h#n...
> include/core/SkRRect.h:179: const SkVector* getAllRadii() const { return
fRadii;
> }
> On 2013/11/04 17:46:52, reed1 wrote:
> > // returns all 4 radii vectors, in Corner order.
> > 
> > Are we sure we want to promise that we will always store them contiguously
in
> > this order?
> 
> It seems logical to me that this would return them in the same order as
> setRectRadii returns them. That said, it is only a convenience, so I'm happy
to
> remove.
> 
>
https://codereview.chromium.org/48623006/diff/150001/src/effects/SkBlurMaskFi...
> File src/effects/SkBlurMaskFilter.cpp (right):
> 
>
https://codereview.chromium.org/48623006/diff/150001/src/effects/SkBlurMaskFi...
> src/effects/SkBlurMaskFilter.cpp:163: // Common code used by both
> draw_rrect_into_mask and drawRectsIntoMask.
> On 2013/11/04 17:46:52, reed1 wrote:
> > errrrrrr, a giant macro with an embedded return? Did a conventional helper
> > function not work?
> > 
> > e.g.
> > 
> > static bool prepare_mask(const SkRect& bounds, SkMask* mask) {
> >     ...
> > }
> > 
> 
> Ideally, I'd like to share common code for creating the SkCanvas (and
> translating it to the right position). Since SkCanvas does not provide a way
to
> change its device, I originally allocated a new SkCanvas, which allows that,
at
> the cost of the extra allocation. The latest patch just accepts some code
> duplication.
> 
>
https://codereview.chromium.org/48623006/diff/150001/src/effects/SkBlurMaskFi...
> src/effects/SkBlurMaskFilter.cpp:276: const SkVector& UL =
> rrect.radii(SkRRect::kUpperLeft_Corner);
> On 2013/11/04 17:46:52, reed1 wrote:
> > we're not calling the new allRadii API here...? (which is fine with me,
since
> > I'm not convinced we should add it, but I am curious why we aren't).
> 
> Really getAllRadii() is just a convenience for my call to setRectRadii(),
where
> I want to set to exactly the same values. Here I want to know about each set
of
> radii individually, so getAllRadii() provides no benefit.
> 
> https://codereview.chromium.org/48623006/diff/150001/src/pdf/SkPDFDevice.cpp
> File src/pdf/SkPDFDevice.cpp (right):
> 
>
https://codereview.chromium.org/48623006/diff/150001/src/pdf/SkPDFDevice.cpp#...
> src/pdf/SkPDFDevice.cpp:939: this->drawPath(draw, path, paint, NULL, true);
> On 2013/11/04 17:46:51, reed1 wrote:
> > what would happen if we didn't make the changes to PDFDevice?
> 
> PDF would not draw a blurred round rect. The mask filter happily reports that
it
> drew successfully, but we never end up going down into the drawPath code,
which
> emits the PDF commands.
> 
> This code just does what SkBitmapDevice::drawRRect used to do, so the PDF
output
> has not changed.

Seems like perhaps that is what SkDeviceBase could/should do ... just turn
around and call drawPath. Maybe for another CL...

[reed1, 2013-11-04 20:05:05]

lgtm

[scroggo, 2013-11-05 16:33:58]

On 2013/11/04 20:03:48, reed1 wrote:
> On 2013/11/04 19:52:53, scroggo wrote:
> > https://codereview.chromium.org/48623006/diff/150001/include/core/SkDraw.h
> > File include/core/SkDraw.h (right):
> > 
> >
>
https://codereview.chromium.org/48623006/diff/150001/include/core/SkDraw.h#ne...
> > include/core/SkDraw.h:27: struct SkRRect;
> > On 2013/11/04 17:46:52, reed1 wrote:
> > > class 
> > 
> > Done.
> > 
> > https://codereview.chromium.org/48623006/diff/150001/include/core/SkRRect.h
> > File include/core/SkRRect.h (right):
> > 
> >
>
https://codereview.chromium.org/48623006/diff/150001/include/core/SkRRect.h#n...
> > include/core/SkRRect.h:179: const SkVector* getAllRadii() const { return
> fRadii;
> > }
> > On 2013/11/04 17:46:52, reed1 wrote:
> > > // returns all 4 radii vectors, in Corner order.
> > > 
> > > Are we sure we want to promise that we will always store them contiguously
> in
> > > this order?
> > 
> > It seems logical to me that this would return them in the same order as
> > setRectRadii returns them. That said, it is only a convenience, so I'm happy
> to
> > remove.
> > 
> >
>
https://codereview.chromium.org/48623006/diff/150001/src/effects/SkBlurMaskFi...
> > File src/effects/SkBlurMaskFilter.cpp (right):
> > 
> >
>
https://codereview.chromium.org/48623006/diff/150001/src/effects/SkBlurMaskFi...
> > src/effects/SkBlurMaskFilter.cpp:163: // Common code used by both
> > draw_rrect_into_mask and drawRectsIntoMask.
> > On 2013/11/04 17:46:52, reed1 wrote:
> > > errrrrrr, a giant macro with an embedded return? Did a conventional helper
> > > function not work?
> > > 
> > > e.g.
> > > 
> > > static bool prepare_mask(const SkRect& bounds, SkMask* mask) {
> > >     ...
> > > }
> > > 
> > 
> > Ideally, I'd like to share common code for creating the SkCanvas (and
> > translating it to the right position). Since SkCanvas does not provide a way
> to
> > change its device, I originally allocated a new SkCanvas, which allows that,
> at
> > the cost of the extra allocation. The latest patch just accepts some code
> > duplication.
> > 
> >
>
https://codereview.chromium.org/48623006/diff/150001/src/effects/SkBlurMaskFi...
> > src/effects/SkBlurMaskFilter.cpp:276: const SkVector& UL =
> > rrect.radii(SkRRect::kUpperLeft_Corner);
> > On 2013/11/04 17:46:52, reed1 wrote:
> > > we're not calling the new allRadii API here...? (which is fine with me,
> since
> > > I'm not convinced we should add it, but I am curious why we aren't).
> > 
> > Really getAllRadii() is just a convenience for my call to setRectRadii(),
> where
> > I want to set to exactly the same values. Here I want to know about each set
> of
> > radii individually, so getAllRadii() provides no benefit.
> > 
> > https://codereview.chromium.org/48623006/diff/150001/src/pdf/SkPDFDevice.cpp
> > File src/pdf/SkPDFDevice.cpp (right):
> > 
> >
>
https://codereview.chromium.org/48623006/diff/150001/src/pdf/SkPDFDevice.cpp#...
> > src/pdf/SkPDFDevice.cpp:939: this->drawPath(draw, path, paint, NULL, true);
> > On 2013/11/04 17:46:51, reed1 wrote:
> > > what would happen if we didn't make the changes to PDFDevice?
> > 
> > PDF would not draw a blurred round rect. The mask filter happily reports
that
> it
> > drew successfully, but we never end up going down into the drawPath code,
> which
> > emits the PDF commands.
> > 
> > This code just does what SkBitmapDevice::drawRRect used to do, so the PDF
> output
> > has not changed.
> 
> Seems like perhaps that is what SkDeviceBase could/should do ... just turn
> around and call drawPath. Maybe for another CL...

That is probably true, but SkPDFDevice currently inherits from SkBitmapDevice,
which
overrides it. If it's possible to make it inherit from SkBaseDevice (and maybe
it is),
then we could make that change.

[robertphillips, 2013-11-05 17:26:22]

lgtm + nit

https://codereview.chromium.org/48623006/diff/350001/src/effects/SkBlurMaskFi...
File src/effects/SkBlurMaskFilter.cpp (right):

https://codereview.chromium.org/48623006/diff/350001/src/effects/SkBlurMaskFi...
src/effects/SkBlurMaskFilter.cpp:202: 
draw_rects_into_mask?

[scroggo, 2013-11-05 21:15:06]

https://codereview.chromium.org/48623006/diff/350001/src/effects/SkBlurMaskFi...
File src/effects/SkBlurMaskFilter.cpp (right):

https://codereview.chromium.org/48623006/diff/350001/src/effects/SkBlurMaskFi...
src/effects/SkBlurMaskFilter.cpp:202: 
On 2013/11/05 17:26:22, robertphillips wrote:
> draw_rects_into_mask?

Done.

[scroggo, 2013-11-08 15:01:25]

On 2013/11/05 21:15:06, scroggo wrote:
>
https://codereview.chromium.org/48623006/diff/350001/src/effects/SkBlurMaskFi...
> File src/effects/SkBlurMaskFilter.cpp (right):
> 
>
https://codereview.chromium.org/48623006/diff/350001/src/effects/SkBlurMaskFi...
> src/effects/SkBlurMaskFilter.cpp:202: 
> On 2013/11/05 17:26:22, robertphillips wrote:
> > draw_rects_into_mask?
> 
> Done.

Robert, how can I make sure this doesn't need a rebaseline in Chrome? Should I
just put it behind a flag just in case?

[scroggo, 2013-11-08 18:03:00]

Committed patchset #5 manually as r12198 (presubmit successful).