skia::AnalysisCanvas: implementation for IsCheapInRect().

skia::AnalysisCanvas: implementation for IsCheapInRect().

Minimal implementation for cc::Picture::IsCheapInRect(),
without bringing in SkBBoxHierarchy speedup possibilities.  As we find
effects & relative impacts we can drop them in the overrides in
analysis_canvas.cc.

Based on the approach Android Browser used to check SkPictures for
optimizations (Is this a single color fill?  Does it contain text?).

This prototype version only counts draw calls. It ignores the clip rect.
It does not correctly handle any of the three high costs associated
with theverge.com: image decodes (need to expand LazyPixelRef AIP?),
glyph cache misses (need to expand Skia API?), and antialiased filled
paths (need to recreate some side-effects buried in Skia).

I think we can also hide all the SkDevice boilerplate inside analysis_canvas
if we write a factory function; that will happen in a follow-on patch.

TBR=nduca,junov,tomhudson
CC=skyostil
BUG=173426

[nduca, 2013-02-04 10:34:32]

Even this lgtm [with the tweak below] as a way to start hacking on using this
call to tweak scheduling. If you can get the analysis tweaked to catch theverge
cases, I'd be happy.

https://codereview.chromium.org/12184010/diff/1/cc/picture.cc
File cc/picture.cc (right):

https://codereview.chromium.org/12184010/diff/1/cc/picture.cc#newcode123
cc/picture.cc:123: if (canvas.getEstimatedCost() <= pictureCostThreshold) {
love this. convert this to bool isCheap() and push the threshold to analyscanvas
maybe? That way we dont have to instantiate a cc picture in order to unit test
it.

[Sami, 2013-02-04 10:57:54]

https://codereview.chromium.org/12184010/diff/1/cc/picture.cc
File cc/picture.cc (right):

https://codereview.chromium.org/12184010/diff/1/cc/picture.cc#newcode112
cc/picture.cc:112: bool Picture::IsCheapInRect(const gfx::Rect& layer_rect) {
Should we make this a const method?

[Justin Novosad, 2013-02-04 14:28:50]

I am pretty sure this is the correct general approach, we'll just have to add
weights.

[Justin Novosad, 2013-02-04 20:23:55]

https://codereview.chromium.org/12184010/diff/6001/cc/picture.cc
File cc/picture.cc (right):

https://codereview.chromium.org/12184010/diff/6001/cc/picture.cc#newcode107
cc/picture.cc:107: emptyBitmap.setConfig(SkBitmap::kARGB_8888_Config,
picture_->width(),
Config should be SkBitmap::kNo_Config

[Justin Novosad, 2013-02-04 22:03:31]

General comments: I thing AnalysisCanvas should wrap an "analysis device",
derived from SkDevice. Most of the cost estimation logic should be handled there
: there would be fewer overloads to manage, and the draw loopers would be
unrolled at that point, which will simplify things once we start factoring-in
the SkPaint state in the analysis, which will be necessary.

Some of the comments are more involved than others. In the interest of moving
forward, I would be in favor of only fixing the simple comments and addressing
the complex things in follow-up CLs. Use your judgement. lgtm

https://codereview.chromium.org/12184010/diff/6001/skia/ext/analysis_canvas.cc
File skia/ext/analysis_canvas.cc (right):

https://codereview.chromium.org/12184010/diff/6001/skia/ext/analysis_canvas.c...
skia/ext/analysis_canvas.cc:26: int AnalysisCanvas::save(SkCanvas::SaveFlags) {
Some forward thinking here... In a future iteration we will likely want to
compute bounding boxes in order to estimate the number of pixels affected by a
draw command (e.g. large vs. small rects do not take the same time to rasterize)
To get that right, we will have to track matrix and clip state. Therefore, state
modifying method should flow through to the base class.

https://codereview.chromium.org/12184010/diff/6001/skia/ext/analysis_canvas.c...
skia/ext/analysis_canvas.cc:32: ++estimatedCost_;
In the case of save layer we want to flow-through but without allocating a
layer.  
See how SkPictureRecord does this using save + clipRectBounds.

https://codereview.chromium.org/12184010/diff/6001/skia/ext/analysis_canvas.c...
skia/ext/analysis_canvas.cc:36: ++estimatedCost_;
flow-though

https://codereview.chromium.org/12184010/diff/6001/skia/ext/analysis_canvas.c...
skia/ext/analysis_canvas.cc:40: ++estimatedCost_;
You want this to flow-through to base class, force doAA to false for speed.

https://codereview.chromium.org/12184010/diff/6001/skia/ext/analysis_canvas.c...
skia/ext/analysis_canvas.cc:44: ++estimatedCost_;
We need this to flow-through to base class, but we only really need to track the
bounding box of the clip. You can just redirect to clipRect, with rrect.rect(). 
This will prevent the SkCanvas from triggering complex computations in the
SkRasterClip

https://codereview.chromium.org/12184010/diff/6001/skia/ext/analysis_canvas.c...
skia/ext/analysis_canvas.cc:48: ++estimatedCost_;
Same here, redirect to clipRect with path.bounds, or with the canvas's current
clipRect if path has an inverted fill type.

https://codereview.chromium.org/12184010/diff/6001/skia/ext/analysis_canvas.c...
skia/ext/analysis_canvas.cc:97: if (doFill && paint.getMaskFilter()) {
The Mask filter should result in the same perf penalty regardless of fill style.
It basically applies a blur to a rasterized mask that is the size of the bbox. 
I would get rid of this special-case code until we have facts (numbers) to
support it.

https://codereview.chromium.org/12184010/diff/6001/skia/ext/analysis_canvas.c...
skia/ext/analysis_canvas.cc:99: estimatedCost_ += 100;
Cost of mask filter does not depend on fill vs. stroke, and is not specific to
paths. In all cases the filter ends up blurring a temp buffer the size of the
object's bbox. I think we should get rid of this special-case code until we have
empirical results to back it up.

https://codereview.chromium.org/12184010/diff/6001/skia/ext/analysis_canvas.c...
skia/ext/analysis_canvas.cc:110: ++estimatedCost_;
If you gathered stats in an SkDevice-derived class instead (AnalysisDevice?), 
you would have fewer drawBitmap variants to overload.

https://codereview.chromium.org/12184010/diff/6001/skia/ext/analysis_canvas.c...
skia/ext/analysis_canvas.cc:133: void AnalysisCanvas::drawText(const void*,
size_t, SkScalar, SkScalar,
If you gathered stats in an SkDevice-derived class instead, 
you would have fewer drawText variants to overload.

[Justin Novosad, 2013-02-04 22:08:04]

https://codereview.chromium.org/12184010/diff/6001/skia/ext/analysis_canvas.h
File skia/ext/analysis_canvas.h (right):

https://codereview.chromium.org/12184010/diff/6001/skia/ext/analysis_canvas.h...
skia/ext/analysis_canvas.h:26: virtual int save(SkCanvas::SaveFlags =
kMatrixClip_SaveFlag);
SK_OVERRIDE on all the virtuals please!

[Tom Hudson, 2013-02-05 10:23:24]

https://codereview.chromium.org/12184010/diff/6001/cc/picture.cc
File cc/picture.cc (right):

https://codereview.chromium.org/12184010/diff/6001/cc/picture.cc#newcode107
cc/picture.cc:107: emptyBitmap.setConfig(SkBitmap::kARGB_8888_Config,
picture_->width(),
On 2013/02/04 20:23:55, junov wrote:
> Config should be SkBitmap::kNo_Config

Done.

I'd cribbed this from GatherPixelRefDevice, which is also using kARGB_8888.

https://codereview.chromium.org/12184010/diff/6001/skia/ext/analysis_canvas.cc
File skia/ext/analysis_canvas.cc (right):

https://codereview.chromium.org/12184010/diff/6001/skia/ext/analysis_canvas.c...
skia/ext/analysis_canvas.cc:97: if (doFill && paint.getMaskFilter()) {
On 2013/02/04 22:03:31, junov wrote:
> The Mask filter should result in the same perf penalty regardless of fill
style.
> It basically applies a blur to a rasterized mask that is the size of the bbox.

> I would get rid of this special-case code until we have facts (numbers) to
> support it.

You're correct, but right now I have evidence of slowdown from filled masked
paths. I'm not seeing any other path type or mask context causing slowdown
(presumably because of limited dataset), so I was trying to be fact-driven.

Since this is buggy anyway (apparently the mask filter doesn't exist at analysis
time, it's showing up as a side-effect of some other draw?) leaving as-is and
addressing in a followon patch.

https://codereview.chromium.org/12184010/diff/6001/skia/ext/analysis_canvas.h
File skia/ext/analysis_canvas.h (right):

https://codereview.chromium.org/12184010/diff/6001/skia/ext/analysis_canvas.h...
skia/ext/analysis_canvas.h:26: virtual int save(SkCanvas::SaveFlags =
kMatrixClip_SaveFlag);
On 2013/02/04 22:08:04, junov wrote:
> SK_OVERRIDE on all the virtuals please!

Done.

However, as much as I love SK_OVERRIDE, this is Chrome code, not Skia code,
right?

(And although next patch should be of a SkDevice class, so they'll all get
moved.)

[nduca, 2013-02-05 10:39:30]

Chrome has OVERRIDE

[commit-bot: I haz the power, 2013-02-05 10:43:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tomhudson@chromium.org/12184010/12001

[commit-bot: I haz the power, 2013-02-05 10:59:49]

Sorry for I got bad news for ya.
Compile failed with a clobber build on mac.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac&number...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[Tom Hudson, 2013-02-05 11:53:50]

On 2013/02/05 10:39:30, nduca wrote:
> Chrome has OVERRIDE

...which reveals that one of my function signatures was wrong.

[Justin Novosad, 2013-02-05 12:23:58]

On 2013/02/05 11:53:50, Tom Hudson wrote:
> ...which reveals that one of my function signatures was wrong.

Aha!

> Since this is buggy anyway (apparently the mask filter doesn't exist at
analysis
> time, it's showing up as a side-effect of some other draw?) leaving as-is and
> addressing in a followon patch.

This is exactly why we should use an SkDevice-based approach. When we are
applying shadows, we use a draw looper that alters the SkPaint by appending a
mask filter during the shadow pass. The mask filter is encapsulated by the draw
looper, which makes it hard to get to from SkCanvas land, but easy to get to for
a device.

[Tom Hudson, 2013-02-05 17:01:59]

It's my intent to close this CL and replace it with
https://codereview.chromium.org/12213018, which is SkDevice-based, but I think
that's not going to apply cleanly (thanks to git branching structures).

PTAL there.

https://codereview.chromium.org/12184010/diff/1/cc/picture.cc
File cc/picture.cc (right):

https://codereview.chromium.org/12184010/diff/1/cc/picture.cc#newcode112
cc/picture.cc:112: bool Picture::IsCheapInRect(const gfx::Rect& layer_rect) {
On 2013/02/04 10:57:54, Sami wrote:
> Should we make this a const method?

Done.

I also had to add contents_scale to the list of args so that the AnalysisCanvas
would line up with the real canvas in Raster().

https://codereview.chromium.org/12184010/diff/1/cc/picture.cc#newcode123
cc/picture.cc:123: if (canvas.getEstimatedCost() <= pictureCostThreshold) {
On 2013/02/04 10:34:33, nduca wrote:
> love this. convert this to bool isCheap() and push the threshold to
analyscanvas
> maybe? That way we dont have to instantiate a cc picture in order to unit test
> it.

Done.

https://codereview.chromium.org/12184010/diff/6001/skia/ext/analysis_canvas.cc
File skia/ext/analysis_canvas.cc (right):

https://codereview.chromium.org/12184010/diff/6001/skia/ext/analysis_canvas.c...
skia/ext/analysis_canvas.cc:32: ++estimatedCost_;
On 2013/02/04 22:03:31, junov wrote:
> In the case of save layer we want to flow-through but without allocating a
> layer.  
> See how SkPictureRecord does this using save + clipRectBounds.

Done.

I'm not sure about the clipRectBounds, though, because if I'm reading
PicturePileImpl correctly we're dealing with a clip that is (rect - bunch of
rects), and so just taking its bounds is way too conservative and overestimates
cost.

https://codereview.chromium.org/12184010/diff/6001/skia/ext/analysis_canvas.c...
skia/ext/analysis_canvas.cc:40: ++estimatedCost_;
On 2013/02/04 22:03:31, junov wrote:
> You want this to flow-through to base class, force doAA to false for speed.

Done.

I'm not sure about forcing AA to false, though, because we need to know if
something is going to be AA to detect expensive mask filters?

https://codereview.chromium.org/12184010/diff/6001/skia/ext/analysis_canvas.c...
skia/ext/analysis_canvas.cc:97: if (doFill && paint.getMaskFilter()) {
On 2013/02/04 22:03:31, junov wrote:
> The Mask filter should result in the same perf penalty regardless of fill
style.
> It basically applies a blur to a rasterized mask that is the size of the bbox.

> I would get rid of this special-case code until we have facts (numbers) to
> support it.

Done.

https://codereview.chromium.org/12184010/diff/6001/skia/ext/analysis_canvas.c...
skia/ext/analysis_canvas.cc:110: ++estimatedCost_;
On 2013/02/04 22:03:31, junov wrote:
> If you gathered stats in an SkDevice-derived class instead (AnalysisDevice?), 
> you would have fewer drawBitmap variants to overload.

Done.

https://codereview.chromium.org/12184010/diff/6001/skia/ext/analysis_canvas.c...
skia/ext/analysis_canvas.cc:133: void AnalysisCanvas::drawText(const void*,
size_t, SkScalar, SkScalar,
On 2013/02/04 22:03:31, junov wrote:
> If you gathered stats in an SkDevice-derived class instead, 
> you would have fewer drawText variants to overload.

Done.