cc: Consolidate the analysis_canvas operations

cc: Consolidate the analysis_canvas operations

This patch combines predictions (color, transparency,
and cheapness) into one processing step. Also adds code
to use color and transparency information.

BUG=179548
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=186819

[vmpstr, 2013-02-23 00:53:51]

This is just rough draft of the design, the interesting parts that change are in
picture_pile_impl and picture. I'd appreciate some early feedback so I don't go
too far without shooting myself in the foot.

[Tom Hudson, 2013-02-25 17:16:03]

Good to see other people having a use for the class.

We'd talked in LON about supporting early-exit from the analysis if the picture
is too expensive; we can't obviously do that here. Time in AnalysisCanvas is
nontrivial (I'm pretty sure this is just from unpacking the picture), and if it
gets too long we can revisit the issue - perhaps by counting opcodes as we go
and aborting to a "worst-case everything" state if the opcode count is too
high...

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

https://codereview.chromium.org/12316084/diff/1/skia/ext/analysis_canvas.cc#n...
skia/ext/analysis_canvas.cc:23: // NULL turns into kSrcOver mode.
This comment is unclear, since NULL is nowhere in sight.

https://codereview.chromium.org/12316084/diff/1/skia/ext/analysis_canvas.cc#n...
skia/ext/analysis_canvas.cc:120: //   That is, we're not clipping to a portion
of this bitmap.
You mention "bitmap", but when we're on the border of Skia "canvas" might be a
better term, both in docs and in naming the variable, since it isn't necessarily
backed by a bitmap and we commonly speak of drawing bitmaps TO devices.

Another way to phrase the introduction would be to say that you're testing to
see if this drawRect() is equivalent to a clear()?

https://codereview.chromium.org/12316084/diff/1/skia/ext/analysis_canvas.cc#n...
skia/ext/analysis_canvas.cc:286:
(static_cast<AnalysisDevice*>(getDevice()))->setForceNotSolid(false);
This looks really conservative: if we ever save a non-solid layer, we assume
we're non-solid until we pop all the way back to the root?

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

https://codereview.chromium.org/12316084/diff/1/skia/ext/analysis_canvas.h#ne...
skia/ext/analysis_canvas.h:53: int saveDepth_;
Not clear from the name what this does.

[vmpstr, 2013-02-26 00:07:47]

On 2013/02/25 17:16:03, Tom Hudson wrote:
> Good to see other people having a use for the class.
> 
> We'd talked in LON about supporting early-exit from the analysis if the
picture
> is too expensive; we can't obviously do that here. Time in AnalysisCanvas is
> nontrivial (I'm pretty sure this is just from unpacking the picture), and if
it
> gets too long we can revisit the issue - perhaps by counting opcodes as we go
> and aborting to a "worst-case everything" state if the opcode count is too
> high...

That makes sense, although we'd probably have to tune what "too high" means in
this context

> 
> https://codereview.chromium.org/12316084/diff/1/skia/ext/analysis_canvas.cc
> File skia/ext/analysis_canvas.cc (right):
> 
>
https://codereview.chromium.org/12316084/diff/1/skia/ext/analysis_canvas.cc#n...
> skia/ext/analysis_canvas.cc:23: // NULL turns into kSrcOver mode.
> This comment is unclear, since NULL is nowhere in sight.

I've updated the comment.

> 
>
https://codereview.chromium.org/12316084/diff/1/skia/ext/analysis_canvas.cc#n...
> skia/ext/analysis_canvas.cc:120: //   That is, we're not clipping to a portion
> of this bitmap.
> You mention "bitmap", but when we're on the border of Skia "canvas" might be a
> better term, both in docs and in naming the variable, since it isn't
necessarily
> backed by a bitmap and we commonly speak of drawing bitmaps TO devices.
> 
> Another way to phrase the introduction would be to say that you're testing to
> see if this drawRect() is equivalent to a clear()?

Thanks for pointing it out, I'm kind of inventing terminology as I go along.
I've changed this.

> 
>
https://codereview.chromium.org/12316084/diff/1/skia/ext/analysis_canvas.cc#n...
> skia/ext/analysis_canvas.cc:286:
> (static_cast<AnalysisDevice*>(getDevice()))->setForceNotSolid(false);
> This looks really conservative: if we ever save a non-solid layer, we assume
> we're non-solid until we pop all the way back to the root?

That's true. This is not ideal, but at this point I'd rather get lower accuracy,
but
have 0 "totally wrong" cases. I think this can definitely be improved on later.

> 
> https://codereview.chromium.org/12316084/diff/1/skia/ext/analysis_canvas.h
> File skia/ext/analysis_canvas.h (right):
> 
>
https://codereview.chromium.org/12316084/diff/1/skia/ext/analysis_canvas.h#ne...
> skia/ext/analysis_canvas.h:53: int saveDepth_;
> Not clear from the name what this does.

I've changed it to "savedLayerStackSize_"

Some random stats thus far:
I ran this on about 700 pages (key desktop sites):
Total pictures analyzed: 428,238
Total actually transparent tiles: 763
Total actually solid color tiles: 23,621
Percentage of transparent tiles detected: 100%
Percentage of solid color tiles detected: 66%

and now the bad news: 2 tiles (out of 428,238) were actually not
solid color, but the predictor said that it's solid color.
I'm tracking this case down now...

[nduca, 2013-02-26 05:09:57]

@vmpstr, once you get Tom's signoff on this, take it to @reveman for final
review to land. I'm supportive of whatever y'all decide upon.

[nduca, 2013-02-26 05:10:19]

btw, is GatherPixelRefs implemented in a similar way as AnalysisCanvas?

[Tom Hudson, 2013-02-26 10:39:07]

On 2013/02/26 05:10:19, nduca wrote:
> btw, is GatherPixelRefs implemented in a similar way as AnalysisCanvas?

Yes, I mostly cloned the GatherPixelRefs implementation to write AnalysisCanvas.
So we might could unify the two and cut down our picture-traversal overhead.
Right now GPR is in the Skia repo instead of Chrome, but I think that's mostly a
question of where we want to maintain it.

+junov@

[vmpstr, 2013-02-26 18:30:55]

I fixed the 2 tiles issue, all other stats are roughly the same. ptal

[vmpstr, 2013-02-26 18:31:31]

+reveman

[reveman, 2013-02-26 23:34:40]

https://codereview.chromium.org/12316084/diff/8002/cc/tile_manager.cc
File cc/tile_manager.cc (right):

https://codereview.chromium.org/12316084/diff/8002/cc/tile_manager.cc#newcode962
cc/tile_manager.cc:962: picture_pile->IsCheapInRect(rect, contents_scale);
nit: 4 space indent here

https://codereview.chromium.org/12316084/diff/8002/cc/tile_manager.cc#newcode971
cc/tile_manager.cc:971: picture_pile->GetColorIfSolidInRect(rect,
contents_scale, &solid_color);
nit: 4 space indent

https://codereview.chromium.org/12316084/diff/8002/cc/tile_manager.cc#newcode973
cc/tile_manager.cc:973: picture_pile->IsTransparentInRect(rect, contents_scale);
nit: 4 space indent

https://codereview.chromium.org/12316084/diff/8002/cc/tile_manager.cc#newcode995
cc/tile_manager.cc:995: //static
nit: space between // and static

https://codereview.chromium.org/12316084/diff/8002/cc/tile_manager.cc#newcode...
cc/tile_manager.cc:1000: bool is_predicted_transparent) {
nit: lines should be <= 80 characters long

https://codereview.chromium.org/12316084/diff/8002/cc/tile_manager.cc#newcode...
cc/tile_manager.cc:1001: CHECK(pixel_count > 0);
use CHECK_GT

https://codereview.chromium.org/12316084/diff/8002/cc/tile_manager.cc#newcode...
cc/tile_manager.cc:1008: actual_bytes[0]);
is it safe to assume pixels are packed this way? you should probably use the
macros in SkColorPriv.h here.

https://codereview.chromium.org/12316084/diff/8002/cc/tile_manager.cc#newcode...
cc/tile_manager.cc:1009: for (int i = (pixel_count-1)*4; i >= 0; i -= 4) {
what if stride != width?

https://codereview.chromium.org/12316084/diff/8002/cc/tile_manager.cc#newcode...
cc/tile_manager.cc:1028: else if(!is_predicted_solid && is_actually_solid)
nit: space between if and (

https://codereview.chromium.org/12316084/diff/8002/cc/tile_manager.cc#newcode...
cc/tile_manager.cc:1041:
UMA_HISTOGRAM_BOOLEAN("Renderer4.ColorPredictor.PredictedTransparentIsActually",
is_transparent);
nit: lines should be <= 80 characters long

https://codereview.chromium.org/12316084/diff/8002/cc/tile_manager.cc#newcode...
cc/tile_manager.cc:1042:
UMA_HISTOGRAM_BOOLEAN("Renderer4.ColorPredictor.IsActuallyTransparent",
is_transparent);
nit: lines should be <= 80 characters long

https://codereview.chromium.org/12316084/diff/8002/cc/tile_manager.h
File cc/tile_manager.h (right):

https://codereview.chromium.org/12316084/diff/8002/cc/tile_manager.h#newcode107
cc/tile_manager.h:107: bool solid_color_benchmarking);
is there something we can do to cleanup the names of these arguments?
use_cheapess_estimator tells the manager to use both the cheapness prediction to
raster tiles and record the accuracy. solid_color_benchmarking sounds like
something completely different but is similar as it records the accuracy of the
solid color prediction.

Maybe we could unify the record results argument:
bool record_prediction_results,
bool use_cheapness_prediction,

and when we include the solid color optimization we just add:
bool use_solid_color_prediction

[vmpstr, 2013-02-27 01:19:36]

On 2013/02/26 23:34:40, David Reveman wrote:
> https://codereview.chromium.org/12316084/diff/8002/cc/tile_manager.cc
> File cc/tile_manager.cc (right):
> 
>
https://codereview.chromium.org/12316084/diff/8002/cc/tile_manager.cc#newcode962
> cc/tile_manager.cc:962: picture_pile->IsCheapInRect(rect, contents_scale);
> nit: 4 space indent here

Fixed. 

> 
>
https://codereview.chromium.org/12316084/diff/8002/cc/tile_manager.cc#newcode971
> cc/tile_manager.cc:971: picture_pile->GetColorIfSolidInRect(rect,
> contents_scale, &solid_color);
> nit: 4 space indent

Fixed.

> 
>
https://codereview.chromium.org/12316084/diff/8002/cc/tile_manager.cc#newcode973
> cc/tile_manager.cc:973: picture_pile->IsTransparentInRect(rect,
contents_scale);
> nit: 4 space indent

Fixed.

> 
>
https://codereview.chromium.org/12316084/diff/8002/cc/tile_manager.cc#newcode995
> cc/tile_manager.cc:995: //static
> nit: space between // and static

Fixed.

> 
>
https://codereview.chromium.org/12316084/diff/8002/cc/tile_manager.cc#newcode...
> cc/tile_manager.cc:1000: bool is_predicted_transparent) {
> nit: lines should be <= 80 characters long

Fixed.

> 
>
https://codereview.chromium.org/12316084/diff/8002/cc/tile_manager.cc#newcode...
> cc/tile_manager.cc:1001: CHECK(pixel_count > 0);
> use CHECK_GT

Fixed.

> 
>
https://codereview.chromium.org/12316084/diff/8002/cc/tile_manager.cc#newcode...
> cc/tile_manager.cc:1008: actual_bytes[0]);
> is it safe to assume pixels are packed this way? you should probably use the
> macros in SkColorPriv.h here.
> 
>
https://codereview.chromium.org/12316084/diff/8002/cc/tile_manager.cc#newcode...
> cc/tile_manager.cc:1009: for (int i = (pixel_count-1)*4; i >= 0; i -= 4) {
> what if stride != width?

I kind of changed this a bit to be more in line with what I found in unittests.
We don't have anything that iterates over pixels in the code, so hopefully this
is good enough for benchmarking.

> 
>
https://codereview.chromium.org/12316084/diff/8002/cc/tile_manager.cc#newcode...
> cc/tile_manager.cc:1028: else if(!is_predicted_solid && is_actually_solid)
> nit: space between if and (

Fixed.

> 
>
https://codereview.chromium.org/12316084/diff/8002/cc/tile_manager.cc#newcode...
> cc/tile_manager.cc:1041:
>
UMA_HISTOGRAM_BOOLEAN("Renderer4.ColorPredictor.PredictedTransparentIsActually",
> is_transparent);
> nit: lines should be <= 80 characters long

Fixed.

> 
>
https://codereview.chromium.org/12316084/diff/8002/cc/tile_manager.cc#newcode...
> cc/tile_manager.cc:1042:
> UMA_HISTOGRAM_BOOLEAN("Renderer4.ColorPredictor.IsActuallyTransparent",
> is_transparent);
> nit: lines should be <= 80 characters long

Fixed.

> 
> https://codereview.chromium.org/12316084/diff/8002/cc/tile_manager.h
> File cc/tile_manager.h (right):
> 
>
https://codereview.chromium.org/12316084/diff/8002/cc/tile_manager.h#newcode107
> cc/tile_manager.h:107: bool solid_color_benchmarking);
> is there something we can do to cleanup the names of these arguments?
> use_cheapess_estimator tells the manager to use both the cheapness prediction
to
> raster tiles and record the accuracy. solid_color_benchmarking sounds like
> something completely different but is similar as it records the accuracy of
the
> solid color prediction.
> 
> Maybe we could unify the record results argument:
> bool record_prediction_results,
> bool use_cheapness_prediction,
> 
> and when we include the solid color optimization we just add:
> bool use_solid_color_prediction

Yeah, I agree. I changed the flags a bit.

Also, I added the actual using of solid colors (behind a flag). Please take a
look at that as well.

[Tom Hudson, 2013-02-27 14:49:36]

I'm a bit sad that the AnalysisCanvas is being constructed in an outer class,
instead of down in the narrowest scope possible; I'd put some work in on the
original to try to hide the information better. I guess there's some small
reduction in total work from doing it this way.

https://codereview.chromium.org/12316084/diff/8001/cc/picture_pile_impl.cc
File cc/picture_pile_impl.cc (right):

https://codereview.chromium.org/12316084/diff/8001/cc/picture_pile_impl.cc#ne...
cc/picture_pile_impl.cc:252: analysis_.is_solid_color_ = false;
Does this mean that if there's more than one picture in the picture list which
intersects with the content_clip, we assume that we're false? That seems way too
conservative.

(And since I already spent a minute and a half trying to reason about this, if
I'm wrong, please consider adding a comment.)

https://codereview.chromium.org/12316084/diff/12023/cc/picture.h
File cc/picture.h (right):

https://codereview.chromium.org/12316084/diff/12023/cc/picture.h#newcode13
cc/picture.h:13: #include "skia/ext/analysis_canvas.h"
We only need a forward decl here, not a full #include.
(Or if we need to keep the full #include for style reasons, can we remove the
duplicate from the .cc?)

https://codereview.chromium.org/12316084/diff/12023/cc/picture.h#newcode46
cc/picture.h:46: void AnalyzeInRect(skia::AnalysisCanvas* canvas,
How many more out args before we just wrap them in a struct?

https://codereview.chromium.org/12316084/diff/12023/cc/picture_layer_impl.cc
File cc/picture_layer_impl.cc (right):

https://codereview.chromium.org/12316084/diff/12023/cc/picture_layer_impl.cc#...
cc/picture_layer_impl.cc:118: if (*iter && iter->is_ready_to_draw()) {
You've changed the condition here; is it just for clarity (+1, it is more
meaningful now), or was there a bug?

https://codereview.chromium.org/12316084/diff/12023/cc/picture_layer_impl.cc#...
cc/picture_layer_impl.cc:177: else if (*iter && iter->is_transparent()) {
nit: else on same line as } ?

https://codereview.chromium.org/12316084/diff/12023/cc/tile_manager.cc
File cc/tile_manager.cc (left):

https://codereview.chromium.org/12316084/diff/12023/cc/tile_manager.cc#oldcod...
cc/tile_manager.cc:797: tile->picture_pile()->IsCheapInRect(tile->content_rect_,
We've lost the guarantee that this is called here, but it looks like it's called
on the expected path in, and otherwise it defaults to false for safety.

https://codereview.chromium.org/12316084/diff/12023/skia/ext/analysis_canvas.cc
File skia/ext/analysis_canvas.cc (left):

https://codereview.chromium.org/12316084/diff/12023/skia/ext/analysis_canvas....
skia/ext/analysis_canvas.cc:30: 
Nit: Is this whitespace change dictated by the Chromium style guide, or
accidental?

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

https://codereview.chromium.org/12316084/diff/12023/skia/ext/analysis_canvas....
skia/ext/analysis_canvas.cc:216: const SkRect* srcOrNull, const SkRect& rect,
Nit: I think you've changed this parameter name from the Skia convention to the
CC convention? I'm constitutionally opposed to the loss of information; there
are two rects in play here.

https://codereview.chromium.org/12316084/diff/12023/skia/ext/analysis_canvas....
skia/ext/analysis_canvas.cc:228: // This draw is equivalent to clear(), if the
following conditions hold:
Both of these clauses are duplicated from drawRect(); can we move these to a
helper function?

(Or we could call drawRect() directly until the cost estimation function is
sophisticated enough to need them to diverge...)

https://codereview.chromium.org/12316084/diff/12023/skia/ext/analysis_canvas....
skia/ext/analysis_canvas.cc:367: // solid color.
Thanks for adding these explanations.

[vmpstr, 2013-02-27 18:27:29]

On 2013/02/27 14:49:36, Tom Hudson wrote:
> I'm a bit sad that the AnalysisCanvas is being constructed in an outer class,
> instead of down in the narrowest scope possible; I'd put some work in on the
> original to try to hide the information better. I guess there's some small
> reduction in total work from doing it this way.

You're right, there's a bit less work. I don't think the fact that we're
exposing
AnalysisCanvas to picture pile impl is too prohibitive in this case. 

> 
> https://codereview.chromium.org/12316084/diff/8001/cc/picture_pile_impl.cc
> File cc/picture_pile_impl.cc (right):
> 
>
https://codereview.chromium.org/12316084/diff/8001/cc/picture_pile_impl.cc#ne...
> cc/picture_pile_impl.cc:252: analysis_.is_solid_color_ = false;
> Does this mean that if there's more than one picture in the picture list which
> intersects with the content_clip, we assume that we're false? That seems way
too
> conservative.
> 
> (And since I already spent a minute and a half trying to reason about this, if
> I'm wrong, please consider adding a comment.)

Again, you're right :) I cleaned up the logic a bit. I made it very conservative
initially to make sure that I'm not getting any false positives for the solid
color. Now that I've tracked all of them down, we can start loosening the 
constraints a little bit. I do want to run the full 700 page test again to
make sure it doesn't regress.

> 
> https://codereview.chromium.org/12316084/diff/12023/cc/picture.h
> File cc/picture.h (right):
> 
> https://codereview.chromium.org/12316084/diff/12023/cc/picture.h#newcode13
> cc/picture.h:13: #include "skia/ext/analysis_canvas.h"
> We only need a forward decl here, not a full #include.
> (Or if we need to keep the full #include for style reasons, can we remove the
> duplicate from the .cc?)

Fixed (changed to a forward declaration).

> 
> https://codereview.chromium.org/12316084/diff/12023/cc/picture.h#newcode46
> cc/picture.h:46: void AnalyzeInRect(skia::AnalysisCanvas* canvas,
> How many more out args before we just wrap them in a struct?
> 

Just one more? :) I changed it to an output struct

> https://codereview.chromium.org/12316084/diff/12023/cc/picture_layer_impl.cc
> File cc/picture_layer_impl.cc (right):
> 
>
https://codereview.chromium.org/12316084/diff/12023/cc/picture_layer_impl.cc#...
> cc/picture_layer_impl.cc:118: if (*iter && iter->is_ready_to_draw()) {
> You've changed the condition here; is it just for clarity (+1, it is more
> meaningful now), or was there a bug?

This condition checks if we can draw a tile, or whether we should draw "tile
missing"
borders. With solid colors in place, the check for whether we can draw the tile
is more involved than simply checking if we have a resource ID. Hence the
change.

> 
>
https://codereview.chromium.org/12316084/diff/12023/cc/picture_layer_impl.cc#...
> cc/picture_layer_impl.cc:177: else if (*iter && iter->is_transparent()) {
> nit: else on same line as } ?
> 

Fixed.

> https://codereview.chromium.org/12316084/diff/12023/cc/tile_manager.cc
> File cc/tile_manager.cc (left):
> 
>
https://codereview.chromium.org/12316084/diff/12023/cc/tile_manager.cc#oldcod...
> cc/tile_manager.cc:797:
tile->picture_pile()->IsCheapInRect(tile->content_rect_,
> We've lost the guarantee that this is called here, but it looks like it's
called
> on the expected path in, and otherwise it defaults to false for safety.

I think the guarantee is still there, it's just not as localized. I wanted to
put
all of the prediction calls next to each other to guarantee that we utilize the
AnalyzeInRect caching.

> 
>
https://codereview.chromium.org/12316084/diff/12023/skia/ext/analysis_canvas.cc
> File skia/ext/analysis_canvas.cc (left):
> 
>
https://codereview.chromium.org/12316084/diff/12023/skia/ext/analysis_canvas....
> skia/ext/analysis_canvas.cc:30: 
> Nit: Is this whitespace change dictated by the Chromium style guide, or
> accidental?

That was accidental. I'm not sure what the Chromium style says on empty function
spacing...
I've added the line back in.

> 
>
https://codereview.chromium.org/12316084/diff/12023/skia/ext/analysis_canvas.cc
> File skia/ext/analysis_canvas.cc (right):
> 
>
https://codereview.chromium.org/12316084/diff/12023/skia/ext/analysis_canvas....
> skia/ext/analysis_canvas.cc:216: const SkRect* srcOrNull, const SkRect& rect,
> Nit: I think you've changed this parameter name from the Skia convention to
the
> CC convention? I'm constitutionally opposed to the loss of information; there
> are two rects in play here.

Agreed, I changed it back.

> 
>
https://codereview.chromium.org/12316084/diff/12023/skia/ext/analysis_canvas....
> skia/ext/analysis_canvas.cc:228: // This draw is equivalent to clear(), if the
> following conditions hold:
> Both of these clauses are duplicated from drawRect(); can we move these to a
> helper function?
> 
> (Or we could call drawRect() directly until the cost estimation function is
> sophisticated enough to need them to diverge...)

Good point. I replaced this with a drawRect call to detect transparency

> 
>
https://codereview.chromium.org/12316084/diff/12023/skia/ext/analysis_canvas....
> skia/ext/analysis_canvas.cc:367: // solid color.
> Thanks for adding these explanations.

Np! I'm all for readable code.

ptal

[reveman, 2013-02-28 18:54:55]

https://codereview.chromium.org/12316084/diff/4005/cc/picture_layer_impl.cc
File cc/picture_layer_impl.cc (right):

https://codereview.chromium.org/12316084/diff/4005/cc/picture_layer_impl.cc#n...
cc/picture_layer_impl.cc:165: if (*iter && iter->is_solid_color()) {
I think it might be cleaner to move all this into a "if (*iter)" scope rather
than checking (*iter) multiple times. and you can merge it with the "if (*iter)
resource =..." above.

https://codereview.chromium.org/12316084/diff/4005/cc/picture_layer_impl.cc#n...
cc/picture_layer_impl.cc:176: } else if (*iter && iter->is_transparent()) {
is the "else if" needed? I think it'd be cleaner without if not needed.

https://codereview.chromium.org/12316084/diff/4005/cc/picture_pile_impl.h
File cc/picture_pile_impl.h (right):

https://codereview.chromium.org/12316084/diff/4005/cc/picture_pile_impl.h#new...
cc/picture_pile_impl.h:77: } analysis_;
is caching of last analysis just so we can call GetColorIfSolidInRect and
IsCheapInRect after each other without another call to AnalyzeInRect or is there
more to it?

If that's it, then lets instead make AnalyzeInRect public and have it return the
result in a "Picture::Analysis*". ManageTileState is a more appropriated place
to store this result. Caching the result and making it seem like
GetColorIfSolidInRect and such are cheap is misleading.

https://codereview.chromium.org/12316084/diff/4005/cc/tile.h
File cc/tile.h (right):

https://codereview.chromium.org/12316084/diff/4005/cc/tile.h#newcode64
cc/tile.h:64: bool is_ready_to_draw() const {
Lower case function names are usually only used for simple inlined accessors and
mutators. I think this should be renamed to IsReadyToDraw and impl moved to .cc
as you need to call GetResourceId().

[Tom Hudson, 2013-02-28 19:25:27]

I'm not certain about the (*iter) proposal, but I actively dislike the cache
refactor.

https://codereview.chromium.org/12316084/diff/4005/cc/picture_pile_impl.h
File cc/picture_pile_impl.h (right):

https://codereview.chromium.org/12316084/diff/4005/cc/picture_pile_impl.h#new...
cc/picture_pile_impl.h:77: } analysis_;
On 2013/02/28 18:54:55, David Reveman wrote:
> is caching of last analysis just so we can call GetColorIfSolidInRect and
> IsCheapInRect after each other without another call to AnalyzeInRect or is
there
> more to it?

This is really expensive. We really ought to unify it with GatherPixelRefs(),
although that's on a different thread (compositor instead of worker).

> If that's it, then lets instead make AnalyzeInRect public and have it return
the
> result in a "Picture::Analysis*". ManageTileState is a more appropriated place
> to store this result. Caching the result and making it seem like
> GetColorIfSolidInRect and such are cheap is misleading.

I don't really understand this part of the design. Why do we have
ManagedTileState? Why not store the information about the tile/picture pile on
the objects themselves, instead of in a separate object that needs to be kept in
sync?

The comment in the header file for ManagedTileState is pretty tautologically
empty, and doesn't tell me anything about the reasoning behind the class.

Wouldn't moving storage of the data into a different struct mean we couldn't
encapsulate the whole cache management / staleness issue inside PicturePileImpl,
and have to be much more explicit about it?

[reveman, 2013-02-28 19:53:48]

https://codereview.chromium.org/12316084/diff/4005/cc/picture_pile_impl.h
File cc/picture_pile_impl.h (right):

https://codereview.chromium.org/12316084/diff/4005/cc/picture_pile_impl.h#new...
cc/picture_pile_impl.h:77: } analysis_;
On 2013/02/28 19:25:27, Tom Hudson wrote:
> On 2013/02/28 18:54:55, David Reveman wrote:
> > is caching of last analysis just so we can call GetColorIfSolidInRect and
> > IsCheapInRect after each other without another call to AnalyzeInRect or is
> there
> > more to it?
> 
> This is really expensive. We really ought to unify it with GatherPixelRefs(),
> although that's on a different thread (compositor instead of worker).
> 
> > If that's it, then lets instead make AnalyzeInRect public and have it return
> the
> > result in a "Picture::Analysis*". ManageTileState is a more appropriated
place
> > to store this result. Caching the result and making it seem like
> > GetColorIfSolidInRect and such are cheap is misleading.
> 
> I don't really understand this part of the design. Why do we have
> ManagedTileState? Why not store the information about the tile/picture pile on
> the objects themselves, instead of in a separate object that needs to be kept
in
> sync?

Picture pile knows nothing about tiles. ManagedTileState needs a bit of cleanup
but it's part of the Tile and generally just the separation of the tile state
that is managed by the tile manager. Moving the analysis result to
ManagedTileState means moving it to the tile.
> 
> The comment in the header file for ManagedTileState is pretty tautologically
> empty, and doesn't tell me anything about the reasoning behind the class.
> 
> Wouldn't moving storage of the data into a different struct mean we couldn't
> encapsulate the whole cache management / staleness issue inside
PicturePileImpl,
> and have to be much more explicit about it?

Yes, we have to be more explicit about it which I think is good. Caching the
result at the picture pile level provides no benefits, it's just confusing.

[vmpstr, 2013-02-28 20:37:09]

> Picture pile knows nothing about tiles. ManagedTileState needs a bit of
cleanup
> but it's part of the Tile and generally just the separation of the tile state
> that is managed by the tile manager. Moving the analysis result to
> ManagedTileState means moving it to the tile.
> > 
> > The comment in the header file for ManagedTileState is pretty tautologically
> > empty, and doesn't tell me anything about the reasoning behind the class.
> > 
> > Wouldn't moving storage of the data into a different struct mean we couldn't
> > encapsulate the whole cache management / staleness issue inside
> PicturePileImpl,
> > and have to be much more explicit about it?
> 
> Yes, we have to be more explicit about it which I think is good. Caching the
> result at the picture pile level provides no benefits, it's just confusing.

I've moved AnalyzeInRect to be public and store the result in managed state. I
agree that this cleans up the code a bit and makes it explicit when the work is
being done, as opposed to just reading off a value, which is a good thing. 

There's another CL https://codereview.chromium.org/12353003/ where I'm trying to
get some stuff out of the managed tile state, which seems to be just a
collection of useful, but somewhat unrelated concepts.

Please give this one another look.

[reveman, 2013-02-28 21:23:10]

https://codereview.chromium.org/12316084/diff/20002/cc/picture.h
File cc/picture.h (right):

https://codereview.chromium.org/12316084/diff/20002/cc/picture.h#newcode50
cc/picture.h:50: bool is_solid_color_;
no "_" suffix for public struct members.

https://codereview.chromium.org/12316084/diff/20002/cc/picture_pile_impl.h
File cc/picture_pile_impl.h (right):

https://codereview.chromium.org/12316084/diff/20002/cc/picture_pile_impl.h#ne...
cc/picture_pile_impl.h:23: class Analysis {
can we move this or just use the Picture::Analysis struct? I'd prefer to just
have one struct.

https://codereview.chromium.org/12316084/diff/20002/cc/picture_pile_impl.h#ne...
cc/picture_pile_impl.h:50: bool is_analyzed_;
why do you need this flag? can't you just initialize is_solid_color_,
is_transparent_... properly to get the same result?

https://codereview.chromium.org/12316084/diff/20002/cc/tile_manager.cc
File cc/tile_manager.cc (right):

https://codereview.chromium.org/12316084/diff/20002/cc/tile_manager.cc#newcod...
cc/tile_manager.cc:384:
tile->picture_pile()->AnalyzeInRect(tile->content_rect(),
I'm worried that this is too expensive to do for each live tile.
AssignGpuMemoryToTiles() is probably going to change soon in a way that makes it
really easy to do this for an ideal set of tiles in there. But for now to avoid
a significant perf regression from this patch I think it might be better to do
this in DispatchMoreTasks even though that's a bit awkward.

[reveman, 2013-02-28 21:54:28]

https://codereview.chromium.org/12316084/diff/20002/cc/tile_manager.cc
File cc/tile_manager.cc (right):

https://codereview.chromium.org/12316084/diff/20002/cc/tile_manager.cc#newcod...
cc/tile_manager.cc:384:
tile->picture_pile()->AnalyzeInRect(tile->content_rect(),
I changed my mind after talking about this. If it's not a huge perf regression
lets keep it like this but make sure you don't analyze a tile twice. Delaying
this until dispatch is just too awkward.

[vmpstr, 2013-02-28 22:30:20]

On 2013/02/28 21:54:28, David Reveman wrote:
> https://codereview.chromium.org/12316084/diff/20002/cc/tile_manager.cc
> File cc/tile_manager.cc (right):
> 
>
https://codereview.chromium.org/12316084/diff/20002/cc/tile_manager.cc#newcod...
> cc/tile_manager.cc:384:
> tile->picture_pile()->AnalyzeInRect(tile->content_rect(),
> I changed my mind after talking about this. If it's not a huge perf regression
> lets keep it like this but make sure you don't analyze a tile twice. Delaying
> this until dispatch is just too awkward.

The changes that we discussed are in... I've added a check to make sure we don't
analyze the same tile twice. I'm going to run a bunch of tests on this to see
what is the performance impact.

[reveman, 2013-02-28 23:17:52]

ok, very close :)

https://codereview.chromium.org/12316084/diff/24002/cc/picture_pile_impl.h
File cc/picture_pile_impl.h (right):

https://codereview.chromium.org/12316084/diff/24002/cc/picture_pile_impl.h#ne...
cc/picture_pile_impl.h:22: 
nit: no need for this blank line

https://codereview.chromium.org/12316084/diff/24002/cc/tile_manager.cc
File cc/tile_manager.cc (left):

https://codereview.chromium.org/12316084/diff/24002/cc/tile_manager.cc#oldcod...
cc/tile_manager.cc:945: 
nit: no need to remove this line.

https://codereview.chromium.org/12316084/diff/24002/cc/tile_manager.cc
File cc/tile_manager.cc (right):

https://codereview.chromium.org/12316084/diff/24002/cc/tile_manager.cc#newcod...
cc/tile_manager.cc:379: tile->managed_state().resource_is_being_initialized) {
lets keep this a bit cleaner by just adding the AnalyzeInRect code to the
original if case scope. there's no point in avoiding to analyze when "resource
|| resource_is_being_initialized" is true as mts.picture_pile_analyzed will be
set in that case.

I would also invert the check "if (!tile->priority(ACTIVE_TREE).is_live &&
!tile->priority(PENDING_TREE).is_live...) continue;" to reduce the nesting

[vmpstr, 2013-02-28 23:55:28]

Addressed all points.

I did some tracing while pinching in and out on cnn.com (using a debug version):

PicturePileImpl::AnalyzeInRect 1508.378 ms 7403 occurrences (that's ~0.204ms /
analysis)
Picture::Raster 16210.45 ms 5923 occurrences (that's ~2.737ms per raster)

So the cost of analysis seems to be somewhere between 7-8% of the cost of
raster..
As you pinch in, there tends to be more solid colors, but overall there's also
about 7-8% of solid color tiles...

The problem is that the analysis time is taken up front for a chunk of tiles
(about 30ms at a time) before the raster kicks off

I'm not really sure what conclusion to draw from it, but hopefully once we can
narrow down the ideal set of tiles, we can move the analysis deeper in and if
possible interleave analysis and raster.

For now this lives behind a flag, so it shouldn't regress anything

[Tom Hudson, 2013-03-01 10:20:02]

On 2013/02/28 21:23:10, David Reveman wrote:
>
https://codereview.chromium.org/12316084/diff/20002/cc/picture_pile_impl.h#ne...
> cc/picture_pile_impl.h:50: bool is_analyzed_;
> why do you need this flag? can't you just initialize is_solid_color_,
> is_transparent_... properly to get the same result?

I don't understand removing this flag when later you're concerned about the
performance cost and the necessity of algorithmically guaranteeing that we don't
analyze the same tile twice.

Nor do I understand how we could set up the output values to replace it. They're
boolean, and we'd need a tristate to capture 'uninitialized', and then we'd have
to explicitly check the tristate in the caller, complicating the logic.

[Tom Hudson, 2013-03-01 10:21:24]

On 2013/02/28 23:55:28, vmpstr wrote:
> The problem is that the analysis time is taken up front for a chunk of tiles
> (about 30ms at a time) before the raster kicks off

It sounds like we've gone down a bad path in design, then, because the original
implementation didn't do all the work up front, instead analyzing each tile as
it was considered for dispatching. What do we gain to make up for the latency
hit?

[vmpstr, 2013-03-01 17:28:30]

On 2013/03/01 10:21:24, Tom Hudson wrote:
> On 2013/02/28 23:55:28, vmpstr wrote:
> > The problem is that the analysis time is taken up front for a chunk of tiles
> > (about 30ms at a time) before the raster kicks off
> 
> It sounds like we've gone down a bad path in design, then, because the
original
> implementation didn't do all the work up front, instead analyzing each tile as
> it was considered for dispatching. What do we gain to make up for the latency
> hit?

I agree that paying up front isn't acceptable, since if the amount of tiles is
large we basically don't do any raster work, instead just analyzing tiles.

I've moved the analysis code to be in the dispatch tasks loop. It kicks off
right before gatherpixelsrefs (and both gatherpixelrefs and raster are skipped
if it's solid color or transparent). This means the cost is not paid up front,
but interleaved with each gatherpixelsrefs/raster task. This comes at a "cost"
of assigning memory to tiles that might never need it, which isn't very clean
but isn't harmful either. If the same (analyzed and solid) tile is registered
again later on, it is skipped from being enqueued in the raster queue.

I've also cleaned up the analysis in picture pile/picture. I went down a wrong
path a bit, without fully realizing that each picture was being analyzed within
the same canvas, so I didn't need any complicated logic to figure out solid
or transparent values.

The timings look promising (on release version now...):

Each analysis in ~28us as compared to ~29us for gatherpixelrefs. They are
basically equivalent in speed now. For comparison raster is ~424us.

[reveman, 2013-03-01 18:33:29]

Awesome work! This looks very good and the assigngpumemory part will be cleanup
as soon as I finish my worker pool re-factor.

Merging gatherpixelrefs and the analyze code will be a nice follow up.

Adding Sami for him to review this.

[vmpstr, 2013-03-05 18:07:13]

ping :)

[Sami, 2013-03-05 20:18:31]

Apologies for the delay. I think overall this looks great. Just some questions
about the compositor integration.

https://codereview.chromium.org/12316084/diff/15025/cc/picture_pile_impl.cc
File cc/picture_pile_impl.cc (right):

https://codereview.chromium.org/12316084/diff/15025/cc/picture_pile_impl.cc#n...
cc/picture_pile_impl.cc:213: bool cheap = true;
Nit: is_cheap for consistency.

https://codereview.chromium.org/12316084/diff/15025/cc/picture_pile_impl.cc#n...
cc/picture_pile_impl.cc:223: PictureList& pic_list= map_iter->second;
Nit: space missing before =.

https://codereview.chromium.org/12316084/diff/15025/cc/picture_pile_impl.cc#n...
cc/picture_pile_impl.cc:235: // Do the analysis on the canvas.
Nit: doesn't seem like a very useful comment.

https://codereview.chromium.org/12316084/diff/15025/cc/picture_pile_impl.h
File cc/picture_pile_impl.h (right):

https://codereview.chromium.org/12316084/diff/15025/cc/picture_pile_impl.h#ne...
cc/picture_pile_impl.h:56: bool is_transparent;
Could we call this something like is_empty instead? I'm thinking that we could
check for full opaqueness and have something like a is_fully_opaque flag here
and that could get confusing.

https://codereview.chromium.org/12316084/diff/15025/cc/switches.h
File cc/switches.h (right):

https://codereview.chromium.org/12316084/diff/15025/cc/switches.h#newcode25
cc/switches.h:25: CC_EXPORT extern const char kEnablePredictionBenchmarking[];
Do we need another switch for this or could we reuse --enable-gpu-benchmarking?
It's already allowed to use more time to gather the statistics so I don't think
analysing the tile bitmaps would be too much overhead.

https://codereview.chromium.org/12316084/diff/15025/cc/tile_manager.cc
File cc/tile_manager.cc (right):

https://codereview.chromium.org/12316084/diff/15025/cc/tile_manager.cc#newcod...
cc/tile_manager.cc:173: state->Set("distance_to_visible_in_pixels",
MathUtil::asValueSafely(distance_to_visible_in_pixels).release());
Please also record the analysis result here.

https://codereview.chromium.org/12316084/diff/15025/cc/tile_manager.cc#newcod...
cc/tile_manager.cc:465: for (size_t i = 0; i < live_or_allocated_tiles_.size();
i++) {
Do we need to skip solid/empty tiles here?

https://codereview.chromium.org/12316084/diff/15025/cc/tile_manager.cc#newcod...
cc/tile_manager.cc:606: ManagedTileState& managed_state = tile->managed_state();
The rest of the file calls this managed_tile_state, so please make this
consistent.

https://codereview.chromium.org/12316084/diff/15025/cc/tile_manager.cc#newcod...
cc/tile_manager.cc:680: Tile* tile = *it;
This looks unused.

https://codereview.chromium.org/12316084/diff/15025/cc/tile_manager.cc#newcod...
cc/tile_manager.cc:681: ManagedTileState& managed_state =
(*it)->managed_state();
Is the idea that tiles with image decoding tasks aren't likely to be solid/fully
transparent? If so, might be worth adding a comment below.

https://codereview.chromium.org/12316084/diff/15025/cc/tile_manager.cc#newcod...
cc/tile_manager.cc:697: ManagedTileState& managed_state = tile->managed_state();
This function is getting long -- mind the analysis to a helper method --
something like CanSkipTileRastering?

https://codereview.chromium.org/12316084/diff/15025/cc/tile_manager.cc#newcod...
cc/tile_manager.cc:836: managed_state.picture_pile_analysis.is_cheap_to_raster;
Nit: indent by 4.

https://codereview.chromium.org/12316084/diff/15025/cc/tile_manager.cc#newcod...
cc/tile_manager.cc:1048: }
Add if (!is_actually_solid || !is_transparent) break; to save time.

[vmpstr, 2013-03-05 21:48:44]

On 2013/03/05 20:18:31, Sami wrote:
> Apologies for the delay. I think overall this looks great. Just some questions
> about the compositor integration.
> 
> https://codereview.chromium.org/12316084/diff/15025/cc/picture_pile_impl.cc
> File cc/picture_pile_impl.cc (right):
> 
>
https://codereview.chromium.org/12316084/diff/15025/cc/picture_pile_impl.cc#n...
> cc/picture_pile_impl.cc:213: bool cheap = true;
> Nit: is_cheap for consistency.

These are actually unused, so I removed them.

> 
>
https://codereview.chromium.org/12316084/diff/15025/cc/picture_pile_impl.cc#n...
> cc/picture_pile_impl.cc:223: PictureList& pic_list= map_iter->second;
> Nit: space missing before =.

Fixed. 

> 
>
https://codereview.chromium.org/12316084/diff/15025/cc/picture_pile_impl.cc#n...
> cc/picture_pile_impl.cc:235: // Do the analysis on the canvas.
> Nit: doesn't seem like a very useful comment.

Fixed. 

> 
> https://codereview.chromium.org/12316084/diff/15025/cc/picture_pile_impl.h
> File cc/picture_pile_impl.h (right):
> 
>
https://codereview.chromium.org/12316084/diff/15025/cc/picture_pile_impl.h#ne...
> cc/picture_pile_impl.h:56: bool is_transparent;
> Could we call this something like is_empty instead? I'm thinking that we could
> check for full opaqueness and have something like a is_fully_opaque flag here
> and that could get confusing.

I kind of prefer transparent... is empty implies to me that picture had no
recording
or something like that. I do see what you mean, but "not transparent" doesn't
imply
not translucent, so adding is_opaque should be fine. :) I kept the naming for
now,
but if you feel strongly about this I will change it.

> 
> https://codereview.chromium.org/12316084/diff/15025/cc/switches.h
> File cc/switches.h (right):
> 
> https://codereview.chromium.org/12316084/diff/15025/cc/switches.h#newcode25
> cc/switches.h:25: CC_EXPORT extern const char kEnablePredictionBenchmarking[];
> Do we need another switch for this or could we reuse
--enable-gpu-benchmarking?
> It's already allowed to use more time to gather the statistics so I don't
think
> analysing the tile bitmaps would be too much overhead.

Enable gpu benchmarking is used in more spots to time certain events. Tile
manager
(Raster in particular) only knows whether that flag is set implicitly (if stats
is not a NULL pointer). I would prefer to have an explicit flag here, since
it does do an extra analysis on the picture pile, and then iterate over the
resulting bitmap, which would be a bad if accidentally turned on.
Again, if you insist otherwise, I can change it...

> 
> https://codereview.chromium.org/12316084/diff/15025/cc/tile_manager.cc
> File cc/tile_manager.cc (right):
> 
>
https://codereview.chromium.org/12316084/diff/15025/cc/tile_manager.cc#newcod...
> cc/tile_manager.cc:173: state->Set("distance_to_visible_in_pixels",
> MathUtil::asValueSafely(distance_to_visible_in_pixels).release());
> Please also record the analysis result here.

Added.

> 
>
https://codereview.chromium.org/12316084/diff/15025/cc/tile_manager.cc#newcod...
> cc/tile_manager.cc:465: for (size_t i = 0; i <
live_or_allocated_tiles_.size();
> i++) {
> Do we need to skip solid/empty tiles here?

Good catch, I've added a "continue"

> 
>
https://codereview.chromium.org/12316084/diff/15025/cc/tile_manager.cc#newcod...
> cc/tile_manager.cc:606: ManagedTileState& managed_state =
tile->managed_state();
> The rest of the file calls this managed_tile_state, so please make this
> consistent.

Changed.

> 
>
https://codereview.chromium.org/12316084/diff/15025/cc/tile_manager.cc#newcod...
> cc/tile_manager.cc:680: Tile* tile = *it;
> This looks unused.

Removed.

> 
>
https://codereview.chromium.org/12316084/diff/15025/cc/tile_manager.cc#newcod...
> cc/tile_manager.cc:681: ManagedTileState& managed_state =
> (*it)->managed_state();
> Is the idea that tiles with image decoding tasks aren't likely to be
solid/fully
> transparent? If so, might be worth adding a comment below.

I've added a blurb about solid/transparent tiles in each comment. After we
detect
that a tile is solid or transparent, then we don't GatherPixelRefs or call
DecodeImage* on the tile, so it would never be in this loop.

>
https://codereview.chromium.org/12316084/diff/15025/cc/tile_manager.cc#newcod...
> cc/tile_manager.cc:697: ManagedTileState& managed_state =
tile->managed_state();
> This function is getting long -- mind the analysis to a helper method --
> something like CanSkipTileRastering?

I've split the analyze code into a separate function, see if it reads better
now.

> 
>
https://codereview.chromium.org/12316084/diff/15025/cc/tile_manager.cc#newcod...
> cc/tile_manager.cc:836:
managed_state.picture_pile_analysis.is_cheap_to_raster;
> Nit: indent by 4.
> 

Fixed.

>
https://codereview.chromium.org/12316084/diff/15025/cc/tile_manager.cc#newcod...
> cc/tile_manager.cc:1048: }
> Add if (!is_actually_solid || !is_transparent) break; to save time.

Fixed.


ptal.

[Tom Hudson, 2013-03-05 22:05:24]

On 2013/03/05 21:48:44, vmpstr wrote:
> On 2013/03/05 20:18:31, Sami wrote:

> > 
> > https://codereview.chromium.org/12316084/diff/15025/cc/switches.h
> > File cc/switches.h (right):
> > 
> > https://codereview.chromium.org/12316084/diff/15025/cc/switches.h#newcode25
> > cc/switches.h:25: CC_EXPORT extern const char
kEnablePredictionBenchmarking[];
> > Do we need another switch for this or could we reuse
> --enable-gpu-benchmarking?
> > It's already allowed to use more time to gather the statistics so I don't
> think
> > analysing the tile bitmaps would be too much overhead.
> 
> Enable gpu benchmarking is used in more spots to time certain events. Tile
> manager
> (Raster in particular) only knows whether that flag is set implicitly (if
stats
> is not a NULL pointer). I would prefer to have an explicit flag here, since
> it does do an extra analysis on the picture pile, and then iterate over the
> resulting bitmap, which would be a bad if accidentally turned on.
> Again, if you insist otherwise, I can change it...
> 

I think the separate switch makes sense.

[Justin Novosad, 2013-03-05 22:14:02]

I reviewed parts of the CL that are related to skia.  Found important bugs in
AnalysisCanvas, and a few missed opprtunities.  With all the added complexity in
AnalysisCanvas, I think it desperately needs to be unit tested.

https://codereview.chromium.org/12316084/diff/15025/cc/picture_pile_impl.cc
File cc/picture_pile_impl.cc (right):

https://codereview.chromium.org/12316084/diff/15025/cc/picture_pile_impl.cc#n...
cc/picture_pile_impl.cc:209: canvas.clipRect(gfx::RectToSkRect(content_rect));
I think this call to clip rect is redundant. It is basically clipping to the
bounds of the canvas.

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

https://codereview.chromium.org/12316084/diff/15025/skia/ext/analysis_canvas....
skia/ext/analysis_canvas.cc:20: static bool isSolidColorPaint(const SkPaint&
paint) {
I think isPaintOpaque (SkPaintPriv.cpp in skia) could inspire you to handle more
cases.  Look how it handles xfer modes.

https://codereview.chromium.org/12316084/diff/15025/skia/ext/analysis_canvas....
skia/ext/analysis_canvas.cc:48: draw.fMatrix->mapRect(&deviceRect, drawnRect);
The way this is implemented, you will sometimes get false positives. To be
conservative you should return false if draw.fMAtrix->rectStaysRect() returns
false.  If the matrix applies a rotation or perspective transform, mapRect will
give an axis-aligned rectangle that contains the transformed rect.  Right now,
there is no method to compute an axis-aligned rect that is contained by the
transformed rect, which is what you would need in order to be safe.

https://codereview.chromium.org/12316084/diff/15025/skia/ext/analysis_canvas....
skia/ext/analysis_canvas.cc:58: return draw.fRC->isRect() &&
This is not safe.  AnalysisCanvas converts clipPath calls to clipRect calls,
using the path bounds.  This approach is conservative for testing for
intersections, but it is unsafe for 'contains' tests.  The purpose of this hack
is to prevent SkRasterClip from rasterizing paths, which would be a huge
computational burden. I have a skia CL on the back burner that refactors this
optimization in a way that would solve your problem:
https://codereview.appspot.com/7301054/  Once this patch lands, you will be able
to call the contains method on the canvas's SkClipStack, which will provide the
added benefit of being able to test against certain types of path clips (ovals,
rounded rects, etc.)  Until then, I am not sure how you should handle the
clipPath case here. Perhaps AnalysisCanvas could maintain a stack of counters of
clipPath calls.  Something like: on save, you would push stack.last; on restore,
you would pop; on clipPath, you would increment stack.last

https://codereview.chromium.org/12316084/diff/15025/skia/ext/analysis_canvas....
skia/ext/analysis_canvas.cc:122: void AnalysisDevice::drawPaint(const SkDraw&,
const SkPaint& paint) {
This method could do more. it fills the entire clip, so it would be easy to
determine whether it covers the entire canvas.

https://codereview.chromium.org/12316084/diff/15025/skia/ext/analysis_canvas....
skia/ext/analysis_canvas.cc:187: void AnalysisDevice::drawOval(const SkDraw&,
const SkRect& oval,
FWIW: It is trivial to compute whether an oval contains the canvas rect.

https://codereview.chromium.org/12316084/diff/15025/skia/ext/analysis_canvas....
skia/ext/analysis_canvas.cc:369: if (--savedLayerStackSize_ == 0) {
This is overly conservative.  You could store the index of the first stack level
that that called setForceNotSolid with true, and set it back to false when that
stack level is popped, rather than requiring all layers to be popped off.  Same
thing for setForceNotTransparent.

[vmpstr, 2013-03-05 23:32:26]

On 2013/03/05 22:14:02, junov wrote:
> I reviewed parts of the CL that are related to skia.  Found important bugs in
> AnalysisCanvas, and a few missed opprtunities.  With all the added complexity
in
> AnalysisCanvas, I think it desperately needs to be unit tested.
> 

I agree with that, I will be writing unit tests to verify this

> https://codereview.chromium.org/12316084/diff/15025/cc/picture_pile_impl.cc
> File cc/picture_pile_impl.cc (right):
> 
>
https://codereview.chromium.org/12316084/diff/15025/cc/picture_pile_impl.cc#n...
> cc/picture_pile_impl.cc:209: canvas.clipRect(gfx::RectToSkRect(content_rect));
> I think this call to clip rect is redundant. It is basically clipping to the
> bounds of the canvas.
> 
>
https://codereview.chromium.org/12316084/diff/15025/skia/ext/analysis_canvas.cc
> File skia/ext/analysis_canvas.cc (right):
> 
>
https://codereview.chromium.org/12316084/diff/15025/skia/ext/analysis_canvas....
> skia/ext/analysis_canvas.cc:20: static bool isSolidColorPaint(const SkPaint&
> paint) {
> I think isPaintOpaque (SkPaintPriv.cpp in skia) could inspire you to handle
more
> cases.  Look how it handles xfer modes.

I think there are a few more modes that I can handle here, but I would like to
get the unit tests in first, so that I can be somewhat confident that I don't
break whatever currently works.

> 
>
https://codereview.chromium.org/12316084/diff/15025/skia/ext/analysis_canvas....
> skia/ext/analysis_canvas.cc:48: draw.fMatrix->mapRect(&deviceRect, drawnRect);
> The way this is implemented, you will sometimes get false positives. To be
> conservative you should return false if draw.fMAtrix->rectStaysRect() returns
> false.  If the matrix applies a rotation or perspective transform, mapRect
will
> give an axis-aligned rectangle that contains the transformed rect.  Right now,
> there is no method to compute an axis-aligned rect that is contained by the
> transformed rect, which is what you would need in order to be safe.
> 

I've fixed this, thanks.

>
https://codereview.chromium.org/12316084/diff/15025/skia/ext/analysis_canvas....
> skia/ext/analysis_canvas.cc:58: return draw.fRC->isRect() &&
> This is not safe.  AnalysisCanvas converts clipPath calls to clipRect calls,
> using the path bounds.  This approach is conservative for testing for
> intersections, but it is unsafe for 'contains' tests.  The purpose of this
hack
> is to prevent SkRasterClip from rasterizing paths, which would be a huge
> computational burden. I have a skia CL on the back burner that refactors this
> optimization in a way that would solve your problem:
> https://codereview.appspot.com/7301054/  Once this patch lands, you will be
able
> to call the contains method on the canvas's SkClipStack, which will provide
the
> added benefit of being able to test against certain types of path clips
(ovals,
> rounded rects, etc.)  Until then, I am not sure how you should handle the
> clipPath case here. Perhaps AnalysisCanvas could maintain a stack of counters
of
> clipPath calls.  Something like: on save, you would push stack.last; on
restore,
> you would pop; on clipPath, you would increment stack.last
> 

Interesting... I completely missed the clipPath optimization there. I added the
same type of "force not solid" and "force not transparent" calls here. Do you
think this is sufficient? I think this would eliminate false positives on
pictures that have clipPath and clipRRect calls.

>
https://codereview.chromium.org/12316084/diff/15025/skia/ext/analysis_canvas....
> skia/ext/analysis_canvas.cc:122: void AnalysisDevice::drawPaint(const SkDraw&,
> const SkPaint& paint) {
> This method could do more. it fills the entire clip, so it would be easy to
> determine whether it covers the entire canvas.
> 
>
https://codereview.chromium.org/12316084/diff/15025/skia/ext/analysis_canvas....
> skia/ext/analysis_canvas.cc:187: void AnalysisDevice::drawOval(const SkDraw&,
> const SkRect& oval,
> FWIW: It is trivial to compute whether an oval contains the canvas rect.
> 

I'll do the unittests first, since I don't want to introduce false positives :)

>
https://codereview.chromium.org/12316084/diff/15025/skia/ext/analysis_canvas....
> skia/ext/analysis_canvas.cc:369: if (--savedLayerStackSize_ == 0) {
> This is overly conservative.  You could store the index of the first stack
level
> that that called setForceNotSolid with true, and set it back to false when
that
> stack level is popped, rather than requiring all layers to be popped off. 
Same
> thing for setForceNotTransparent.

I've changed this to be less conservative.

[vmpstr, 2013-03-06 00:50:23]

I've added some very basic unittests.

[Sami, 2013-03-06 11:14:49]

Thanks for adding the test. I'm fine with keeping the separate flag for
benchmarking. I think the cc/ changes look fine -- just one question below on
how the benchmarking mode should work.

https://codereview.chromium.org/12316084/diff/47001/cc/picture.h
File cc/picture.h (right):

https://codereview.chromium.org/12316084/diff/47001/cc/picture.h#newcode20
cc/picture.h:20: class AnalysisCanvas;
Nit: no indent for namespaces.

https://codereview.chromium.org/12316084/diff/47001/cc/tile.h
File cc/tile.h (right):

https://codereview.chromium.org/12316084/diff/47001/cc/tile.h#newcode74
cc/tile.h:74: SkColor get_solid_color() const {
Nit: drop the "get_".

https://codereview.chromium.org/12316084/diff/47001/cc/tile_manager.cc
File cc/tile_manager.cc (right):

https://codereview.chromium.org/12316084/diff/47001/cc/tile_manager.cc#newcod...
cc/tile_manager.cc:709: if (tile->is_solid_color() || tile->is_transparent()) {
If we're doing prediction benchmarking, then we'd actually need to submit the
raster tasks to see whether the prediction was correct. Otherwise
RecordSolidColorPredictorResults will never be called for predicted solid or
transparent tiles, right?

I guess we could have AnalyzeTile also check the prediction in benchmarking
mode, but we'd also need to make sure it gets called for all tiles.

https://codereview.chromium.org/12316084/diff/47001/cc/tile_manager.cc#newcod...
cc/tile_manager.cc:733: &managed_tile_state.picture_pile_analysis);
Nit: line too long.

[Justin Novosad, 2013-03-06 14:23:27]

On 2013/03/05 23:32:26, vmpstr wrote:

> I think there are a few more modes that I can handle here, but I would like to
> get the unit tests in first, so that I can be somewhat confident that I don't
> break whatever currently works.

Good Idea, this CL is already big and complex enough as it is.


> Interesting... I completely missed the clipPath optimization there. I added
the
> same type of "force not solid" and "force not transparent" calls here. Do you
> think this is sufficient? I think this would eliminate false positives on
> pictures that have clipPath and clipRRect calls.

I agree with that conservative approach.  I think the opportunities that are
missed by doing it this way are extremely rare cases anyways.

[Justin Novosad, 2013-03-06 14:59:48]

Unit tests for AnalysisCanvas look good, but I have a few improvements to
suggest. In addition to the in-line comments: I think you should add test
coverage for clipPath/clipRRect, and another test that verifies that calling
restore will lift the transparent/solidColor overrides at the right stack level.

In a separate CL (Tom, Sami?): add unit tests that verify all the basic cases
for EXPECT_FALSE(canvas.isCheap());

https://codereview.chromium.org/12316084/diff/47001/skia/ext/analysis_canvas_...
File skia/ext/analysis_canvas_unittest.cc (right):

https://codereview.chromium.org/12316084/diff/47001/skia/ext/analysis_canvas_...
skia/ext/analysis_canvas_unittest.cc:71:
EXPECT_FALSE(canvas.getColorIfSolid(&outputColor));
Since you intend to come back to this later, you should file a bug, and insert a
TODO comment stating that this test should actually be EXPECT_TRUE

https://codereview.chromium.org/12316084/diff/47001/skia/ext/analysis_canvas_...
skia/ext/analysis_canvas_unittest.cc:83:
canvas.drawPoints(SkCanvas::kLines_PointMode, 4, points, paint);
This is not a very effective test because you are not testing in isolation from
the previous test.  How do I know that drawPoints affects the state in any way
(maybe the state is just staying unchanged)? To verify that these draws actually
affect the analysis, it would make sense to draw them on top of a solid color
and/or a transparent color fill in order to verify that state transitions work.
For example:
transparentFill(canvas);
canvas.drawPoints(...);
EXPECT_FALSE(canvas.isTransparent());
EXPECT_TRUE(canvas.isCheap());
solidColorFill(canvas);
canvas.drawPoints(...);
EXPECT_FALSE(canvas.getColorIfSolid(&out));

https://codereview.chromium.org/12316084/diff/47001/skia/ext/analysis_canvas_...
skia/ext/analysis_canvas_unittest.cc:159:
paint.setXfermodeMode(SkXfermode::kClear_Mode);
Should also have a similar test that creates a transparent canvas by using
SkXfermode::kSrc_Mode

https://codereview.chromium.org/12316084/diff/47001/skia/ext/analysis_canvas_...
skia/ext/analysis_canvas_unittest.cc:183: canvas.rotate(50);
Glad to see this covered.

[Justin Novosad, 2013-03-06 15:24:42]

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

https://codereview.chromium.org/12316084/diff/47001/skia/ext/analysis_canvas....
skia/ext/analysis_canvas.cc:64: return draw.fRC->isRect() &&
You can remove test on draw.fRC->isRect() since we know it always returns true
due to the complex clip hack.

https://codereview.chromium.org/12316084/diff/47001/skia/ext/analysis_canvas....
skia/ext/analysis_canvas.cc:335: if (forceNotSolidStackLevel_ == 0) {
There is a bug here due to '0' being ambiguous (0 could mean that we called
clipPath at stack level 0). Instead you could test that forceNotSolid is
currently set to false.
You should have a unit test verifies this.  Something like:
clipPath
saveLayer
clipPath
restore
fill canvas with solid color
EXPECT_FALSE(canvas.getColorIfSolid());

I think the current implementation would fail this test.

https://codereview.chromium.org/12316084/diff/47001/skia/ext/analysis_canvas....
skia/ext/analysis_canvas.cc:339: if (forceNotTransparentStackLevel_ == 0) {
Bug

https://codereview.chromium.org/12316084/diff/47001/skia/ext/analysis_canvas....
skia/ext/analysis_canvas.cc:352: if (forceNotSolidStackLevel_ == 0) {
Bug

https://codereview.chromium.org/12316084/diff/47001/skia/ext/analysis_canvas....
skia/ext/analysis_canvas.cc:356: if (forceNotTransparentStackLevel_ == 0) {
Bug

https://codereview.chromium.org/12316084/diff/47001/skia/ext/analysis_canvas....
skia/ext/analysis_canvas.cc:410: --savedStackSize_;
Need to gracefully handle the case of a restore that does not have a matching
save, or at least assert.

[vmpstr, 2013-03-06 17:52:56]

> cc/tile_manager.cc:709: if (tile->is_solid_color() || tile->is_transparent())
{
> If we're doing prediction benchmarking, then we'd actually need to submit the
> raster tasks to see whether the prediction was correct. Otherwise
> RecordSolidColorPredictorResults will never be called for predicted solid or
> transparent tiles, right?
> 
> I guess we could have AnalyzeTile also check the prediction in benchmarking
> mode, but we'd also need to make sure it gets called for all tiles.

Yes you're correct that in order to benchmark we need to rasterize the tile.
I've talked to reveman about this before, and we kind of agree that it would be
ok to say "if you're benchmarking the color prediction, then you can't actually
be _using_ the color prediction". Are you ok with this type of semantics?


> skia/ext/analysis_canvas.cc:335: if (forceNotSolidStackLevel_ == 0) {
> There is a bug here due to '0' being ambiguous (0 could mean that we called
> clipPath at stack level 0). Instead you could test that forceNotSolid is
> currently set to false.
> You should have a unit test verifies this.  Something like:
> clipPath
> saveLayer
> clipPath
> restore
> fill canvas with solid color
> EXPECT_FALSE(canvas.getColorIfSolid());
> 
> I think the current implementation would fail this test.

Agreed. I've added a test that fails it, then fixed the implementation :)

PTAL.

[Justin Novosad, 2013-03-06 18:10:30]

lgtm for skia/ext

[Justin Novosad, 2013-03-06 18:15:39]

On 2013/03/06 18:10:30, junov wrote:
> lgtm for skia/ext

Readability Nit: the literal -1 in analysis_canvas.cc could be a constant
(kNoLayer?)

[reveman, 2013-03-07 00:11:28]

lgtm for cc

https://codereview.chromium.org/12316084/diff/40002/cc/picture.h
File cc/picture.h (right):

https://codereview.chromium.org/12316084/diff/40002/cc/picture.h#newcode21
cc/picture.h:21: } // namespace skia
nit: "// namespace xxx" comments are usually not used for forward declarations
as it's pretty easy to see what the matching "{" is

https://codereview.chromium.org/12316084/diff/40002/cc/tile_manager.cc
File cc/tile_manager.cc (right):

https://codereview.chromium.org/12316084/diff/40002/cc/tile_manager.cc#newcod...
cc/tile_manager.cc:1018: reinterpret_cast<SkColor*>(bitmap.getPixels()),
I assume this is always safe with SkBitmap::kARGB_8888_Config but please add a
DCHECK_EQ(rowBytes(), width() * bytesPerPixel()) here as a sanity check.

[vmpstr, 2013-03-07 01:04:51]

+piman for content/ owner's review

Please take a look.

[piman, 2013-03-07 01:24:21]

On 2013/03/07 01:04:51, vmpstr wrote:
> +piman for content/ owner's review
> 
> Please take a look.

LGTM for content/ , I did not look at other parts.

[Sami, 2013-03-07 14:35:18]

On 2013/03/06 17:52:56, vmpstr wrote:
> Yes you're correct that in order to benchmark we need to rasterize the tile.
> I've talked to reveman about this before, and we kind of agree that it would
be
> ok to say "if you're benchmarking the color prediction, then you can't
actually
> be _using_ the color prediction". Are you ok with this type of semantics?

That makes perfect sense. Just have to keep this in mind when benchmarking I
guess :)

[commit-bot: I haz the power, 2013-03-07 17:51:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vmpstr@chromium.org/12316084/69001

[commit-bot: I haz the power, 2013-03-07 18:06:25]

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.

[commit-bot: I haz the power, 2013-03-07 18:09:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vmpstr@chromium.org/12316084/78002

[commit-bot: I haz the power, 2013-03-07 19:42:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vmpstr@chromium.org/12316084/85001

[commit-bot: I haz the power, 2013-03-07 23:52:29]

Change committed as 186819