Implementation for cc::Picture::IsCheapInRect().

Implementation for cc::Picture::IsCheapInRect().

Minimal implementation for cc::Picture::IsCheapInRect() with new
classes skia::AnalysisCanvas and skia::AnalysisDevice.
Supercedes http://codereview.chromium.org/12184010/.

Based on GatherPixelRefs.

Not a good implementation yet, but it does seem to count draw calls
and is right more often than a stopped clock. I'm not happy with
clipping/culling; something is going very wrong there.

TBR=nduca,junov,tomhudson
CC=skyostil
BUG=173426
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=181369

[nduca, 2013-02-06 08:42:25]

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

https://codereview.chromium.org/12213018/diff/1/cc/picture.cc#newcode119
cc/picture.cc:119: TRACE_EVENT0("cc", "Picture::IsCheapInRect");
is your git branch mucked up? this looks like its based on your previous patch?

https://codereview.chromium.org/12213018/diff/1/cc/picture.cc#newcode127
cc/picture.cc:127: canvas.clipRect(gfx::RectToSkRect(layer_rect));
this line looks a little different than the call in ::Raster, any reason why?
namely ,we use layer_rect here but content_rect up above

[Tom Hudson, 2013-02-06 10:58:24]

On 2013/02/06 08:42:25, nduca wrote:
> https://codereview.chromium.org/12213018/diff/1/cc/picture.cc
> File cc/picture.cc (right):
> 
> https://codereview.chromium.org/12213018/diff/1/cc/picture.cc#newcode119
> cc/picture.cc:119: TRACE_EVENT0("cc", "Picture::IsCheapInRect");
> is your git branch mucked up? this looks like its based on your previous
patch?

Yep. I'll create a clean branch today and copy the files over. Which means yet
another codereview, right?

https://codereview.chromium.org/12213018/diff/1/cc/picture.cc#newcode127
> cc/picture.cc:127: canvas.clipRect(gfx::RectToSkRect(layer_rect));
> this line looks a little different than the call in ::Raster, any reason why?
> namely ,we use layer_rect here but content_rect up above

Bad API design? The function already has access to layer_rect_, but the
parameter was named layer_rect; I think it should be named content_rect, and
will make that change in a future revision.

[Tom Hudson, 2013-02-06 13:53:19]

I found my bug: what would need to be passed down from Render to IsCheapInRect
is the size (& offset) of the *SkCanvas* passed in, after translation & scaling.
So my testing approach was the biggest flaw.

[Justin Novosad, 2013-02-06 14:08:03]

Looks like a good foundation for further ramifications.
lgtm with minor corrections.

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

https://codereview.chromium.org/12213018/diff/4001/skia/ext/analysis_canvas.c...
skia/ext/analysis_canvas.cc:158: return SkCanvas::clipRect(rect, op, doAA);
Should use a typedef to refer to parent class.

https://codereview.chromium.org/12213018/diff/4001/skia/ext/analysis_canvas.c...
skia/ext/analysis_canvas.cc:163: return SkCanvas::clipPath(path, op, doAA);
To prevent complex clip rasterization in SkRasterClip, this should be:
return SkCanvas::clipRect(path.getBounds(), op, doAA);

[Tom Hudson, 2013-02-06 17:36:00]

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

https://codereview.chromium.org/12213018/diff/4001/skia/ext/analysis_canvas.c...
skia/ext/analysis_canvas.cc:158: return SkCanvas::clipRect(rect, op, doAA);
On 2013/02/06 14:08:03, junov wrote:
> Should use a typedef to refer to parent class.

I'd avoided it as an unChromey Skiaism, but
Done.

https://codereview.chromium.org/12213018/diff/4001/skia/ext/analysis_canvas.c...
skia/ext/analysis_canvas.cc:163: return SkCanvas::clipPath(path, op, doAA);
On 2013/02/06 14:08:03, junov wrote:
> To prevent complex clip rasterization in SkRasterClip, this should be:
> return SkCanvas::clipRect(path.getBounds(), op, doAA);

Done.
(I assume we aren't worrying about inverse filled clips...)

[Sami, 2013-02-06 17:41:47]

lgtm with some minor nit-picking.

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

https://codereview.chromium.org/12213018/diff/10001/skia/ext/analysis_canvas....
skia/ext/analysis_canvas.cc:17: int gPictureCostThreshold = 1000;
This could be a const (kPictureCostThreshold), right?

https://codereview.chromium.org/12213018/diff/10001/skia/ext/analysis_canvas....
skia/ext/analysis_canvas.cc:55: //TRACE_EVENT0("skia", "Who was that masked
rect?");
Did you mean to leave this in?

https://codereview.chromium.org/12213018/diff/10001/skia/ext/analysis_canvas....
skia/ext/analysis_canvas.cc:74: //TRACE_EVENT0("skia", "Who was that masked
path?");
Ditto.

https://codereview.chromium.org/12213018/diff/10001/skia/ext/analysis_canvas....
skia/ext/analysis_canvas.cc:160: return
((AnalysisDevice*)getDevice())->getEstimatedCost();
static_cast<> maybe?

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

https://codereview.chromium.org/12213018/diff/10001/skia/ext/analysis_canvas....
skia/ext/analysis_canvas.h:27: //static AnalysisCanvas* Create(int width, int
height);
Left over from refactoring?

[Tom Hudson, 2013-02-06 17:53:36]

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

https://codereview.chromium.org/12213018/diff/10001/skia/ext/analysis_canvas....
skia/ext/analysis_canvas.cc:17: int gPictureCostThreshold = 1000;
On 2013/02/06 17:41:47, Sami wrote:
> This could be a const (kPictureCostThreshold), right?

Done.

https://codereview.chromium.org/12213018/diff/10001/skia/ext/analysis_canvas....
skia/ext/analysis_canvas.cc:55: //TRACE_EVENT0("skia", "Who was that masked
rect?");
On 2013/02/06 17:41:47, Sami wrote:
> Did you mean to leave this in?

Done.

https://codereview.chromium.org/12213018/diff/10001/skia/ext/analysis_canvas....
skia/ext/analysis_canvas.cc:74: //TRACE_EVENT0("skia", "Who was that masked
path?");
On 2013/02/06 17:41:47, Sami wrote:
> Ditto.

Done.

https://codereview.chromium.org/12213018/diff/10001/skia/ext/analysis_canvas....
skia/ext/analysis_canvas.cc:160: return
((AnalysisDevice*)getDevice())->getEstimatedCost();
On 2013/02/06 17:41:47, Sami wrote:
> static_cast<> maybe?

Done.

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

https://codereview.chromium.org/12213018/diff/10001/skia/ext/analysis_canvas....
skia/ext/analysis_canvas.h:27: //static AnalysisCanvas* Create(int width, int
height);
On 2013/02/06 17:41:47, Sami wrote:
> Left over from refactoring?

Done.

[commit-bot: I haz the power, 2013-02-06 17:54:21]

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

[commit-bot: I haz the power, 2013-02-07 17:08:31]

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

[Tom Hudson, 2013-02-07 20:17:23]

New version will show up in the morning.
(I don't know what's wrong with the CQ...)
I was wrong about image decode and about cache misses; those are both severe
problems on Linux, but not on N10. (I do see about 1 inline image decode per
second). N10 is far-and-away dominated by antialiased/mask-filtered paths, and
the problem seems to be an issue of fill rate, so we can use Justin's sample
code in Skia to measure # of pixels touched once we get the clips and transforms
correct.

[commit-bot: I haz the power, 2013-02-07 22:08:25]

Change committed as 181369