cc: Merge GatherPixelRefs and AnalyzeInRect

cc: Merge GatherPixelRefs and AnalyzeInRect

Both GatherPixelRefs and AnalyzeInRect use the same loop,
and a similar canvas to do their work. It makes sense to
merge the two so part of the work is not replicated.


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

[vmpstr, 2013-03-04 23:06:26]

There's a bit of overlap with the following CL:
https://codereview.chromium.org/12316084/

I've included some parts of that CL in here so that the code at least compiles.
This should however, only go in after 12316084.

The time of AnalyzeAndGatherPixelRefs seems to be about 25-50% more than
GatherPixelRefs. I'm going to run more timings to be sure though

[vmpstr, 2013-03-08 21:20:13]

PTAL.

On average, the cost of doing both analysis and gathering in one call is about
50-60% more than just doing the gather using SkPictureUtils.

[Tom Hudson, 2013-03-08 21:48:54]

Justin might need to take a look?

[Justin Novosad, 2013-03-11 15:26:11]

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

https://codereview.chromium.org/12388095/diff/4001/skia/ext/analysis_canvas.c...
skia/ext/analysis_canvas.cc:22: const char labelLazyDecoded[] = "lazy";
This string constant should only be defined in one place.

https://codereview.chromium.org/12388095/diff/4001/skia/ext/analysis_canvas.c...
skia/ext/analysis_canvas.cc:123: std::pair<std::set<uint32_t>::iterator, bool>
result =
"result" is a bad name for a variable that is not a return value;

https://codereview.chromium.org/12388095/diff/4001/skia/ext/analysis_canvas.c...
skia/ext/analysis_canvas.cc:134: void AnalysisDevice::addBitmap(const SkBitmap&
bitmap) {
Needs unit testing. Test should verify:
a) lazy vs non-lazy pixel refs
b) duplicate pixel refs only counted once.
c) pixel refs packaged in bitmap shaders (addBitmapFromPaint)

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

https://codereview.chromium.org/12388095/diff/4001/skia/ext/analysis_canvas.h...
skia/ext/analysis_canvas.h:141: std::set<uint32_t> existingPixelRefIDs_;
for performance, this should be an std::unordered_set

[vmpstr, 2013-03-11 19:51:29]

On 2013/03/11 15:26:11, junov wrote:
> https://codereview.chromium.org/12388095/diff/4001/skia/ext/analysis_canvas.cc
> File skia/ext/analysis_canvas.cc (right):
> 
>
https://codereview.chromium.org/12388095/diff/4001/skia/ext/analysis_canvas.c...
> skia/ext/analysis_canvas.cc:22: const char labelLazyDecoded[] = "lazy";
> This string constant should only be defined in one place.

Although I agree in general, in this case I'm hoping that this will be the only
reference to it in the future. The one that exists in picture is there in case
the analysis canvas is disabled.

However, there is a crbug open to enable this functionality by default. If that
goes well, the picture.cc string should go away.

> 
>
https://codereview.chromium.org/12388095/diff/4001/skia/ext/analysis_canvas.c...
> skia/ext/analysis_canvas.cc:123: std::pair<std::set<uint32_t>::iterator, bool>
> result =
> "result" is a bad name for a variable that is not a return value;

Changed this to "insertionResult"

> 
>
https://codereview.chromium.org/12388095/diff/4001/skia/ext/analysis_canvas.c...
> skia/ext/analysis_canvas.cc:134: void AnalysisDevice::addBitmap(const
SkBitmap&
> bitmap) {
> Needs unit testing. Test should verify:
> a) lazy vs non-lazy pixel refs
> b) duplicate pixel refs only counted once.
> c) pixel refs packaged in bitmap shaders (addBitmapFromPaint)

Added unittests for these cases

> 
> https://codereview.chromium.org/12388095/diff/4001/skia/ext/analysis_canvas.h
> File skia/ext/analysis_canvas.h (right):
> 
>
https://codereview.chromium.org/12388095/diff/4001/skia/ext/analysis_canvas.h...
> skia/ext/analysis_canvas.h:141: std::set<uint32_t> existingPixelRefIDs_;
> for performance, this should be an std::unordered_set

I changed this to base::hash_set. unordered_set is C++11, which I don't think
we're using yet?


PTAL.

[jamesr, 2013-03-11 19:54:28]

On 2013/03/11 19:51:29, vmpstr wrote:
https://codereview.chromium.org/12388095/diff/4001/skia/ext/analysis_canvas.h...
> > skia/ext/analysis_canvas.h:141: std::set<uint32_t> existingPixelRefIDs_;
> > for performance, this should be an std::unordered_set
> 
> I changed this to base::hash_set. unordered_set is C++11, which I don't think
> we're using yet?

You are correct - in Chromium, use the types from base/hash_tables.h

[vmpstr, 2013-03-12 23:21:07]

ping :)

[reveman, 2013-03-15 01:51:11]

junov, can you have a look at this asap? with color estimator turn on by default
this becomes an important perf improvement. thanks!

[reveman, 2013-03-15 22:20:32]

+tom, +sami

[Tom Hudson, 2013-03-18 11:52:39]

https://codereview.chromium.org/12388095/diff/11001/cc/tile_manager.cc
File cc/tile_manager.cc (right):

https://codereview.chromium.org/12388095/diff/11001/cc/tile_manager.cc#newcod...
cc/tile_manager.cc:755: managed_tile_state.pending_pixel_refs.swap(
Is swap really the right way to combine these? All the lines above are using &=,
as if they're accumulating.

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

https://codereview.chromium.org/12388095/diff/11001/skia/ext/analysis_canvas....
skia/ext/analysis_canvas.cc:179: addBitmapFromPaint(paint);
Hmm, idiomatically this isn't how I expected drawPaint to be used, but it looks
like RenderWidget::PaintRect() will hit this.

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

https://codereview.chromium.org/12388095/diff/11001/skia/ext/analysis_canvas....
skia/ext/analysis_canvas.h:36: void
consumeLazyPixelRefs(std::list<LazyPixelRef*>& pixelRefs);
This name is really unclear to me; "consume" doesn't describe what's happening
from the caller's point of view, but a detail of internal implementation, right?

[Sami, 2013-03-18 12:14:33]

I think this is a great idea to reduce the amount of duplicate work. Just some
style niggles.

https://codereview.chromium.org/12388095/diff/11001/cc/picture_pile_impl.h
File cc/picture_pile_impl.h (right):

https://codereview.chromium.org/12388095/diff/11001/cc/picture_pile_impl.h#ne...
cc/picture_pile_impl.h:59: std::list<skia::LazyPixelRef*> lazy_pixel_refs;
Maybe use a LazyPixelRefList typedef to make the code a little easier to read. A
lot of cc/ does that.

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

https://codereview.chromium.org/12388095/diff/11001/skia/ext/analysis_canvas....
skia/ext/analysis_canvas.cc:22: const char labelLazyDecoded[] = "lazy";
const char kLabelLazyDecoded[] = ...

https://codereview.chromium.org/12388095/diff/11001/skia/ext/analysis_canvas....
skia/ext/analysis_canvas.cc:132: !strncmp(pixelRef->getURI(), labelLazyDecoded,
4)) {
Use a named constant instead of 4.

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

https://codereview.chromium.org/12388095/diff/11001/skia/ext/analysis_canvas....
skia/ext/analysis_canvas.h:36: void
consumeLazyPixelRefs(std::list<LazyPixelRef*>& pixelRefs);
Use a pointer instead of reference (all reference parameters must be const).

https://codereview.chromium.org/12388095/diff/11001/skia/ext/analysis_canvas....
skia/ext/analysis_canvas.h:142: base::hash_set<uint32_t> existingPixelRefIDs_;
This could be a typedef too.

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

https://codereview.chromium.org/12388095/diff/11001/skia/ext/analysis_canvas_...
skia/ext/analysis_canvas_unittest.cc:47: SkShader::BitmapType
asABitmap(SkBitmap* bitmap, SkMatrix*, TileMode xy[2]) const {
Nit: line length.

https://codereview.chromium.org/12388095/diff/11001/skia/ext/analysis_canvas_...
skia/ext/analysis_canvas_unittest.cc:399: EXPECT_EQ(pixelRefs.front(),
static_cast<LazyPixelRef*>(&firstLazyPixelRef));
Nit: line length.

[Justin Novosad, 2013-03-18 15:00:44]

Apologies for the delayed response (gmail filter blunder on my side)
LGTM for skia/ with corrections suggested by Tom and Sami.
Great tests!

[vmpstr, 2013-03-18 17:24:51]

Thanks for the reviews! I fixed most issues mentioned.

On 2013/03/18 11:52:39, Tom Hudson wrote:
> https://codereview.chromium.org/12388095/diff/11001/cc/tile_manager.cc
> File cc/tile_manager.cc (right):
> 
>
https://codereview.chromium.org/12388095/diff/11001/cc/tile_manager.cc#newcod...
> cc/tile_manager.cc:755: managed_tile_state.pending_pixel_refs.swap(
> Is swap really the right way to combine these? All the lines above are using
&=,
> as if they're accumulating.

&= are meant to be "filters" in that context... That is,
if we judge a tile to be X but --use-X isn't set, then
we clear the X flag (if that makes any sense).

That code path should execute only once per tile,
and I swap here purely for efficiency... It could be
a simple copy as well.

>
https://codereview.chromium.org/12388095/diff/11001/skia/ext/analysis_canvas....
> skia/ext/analysis_canvas.h:36: void
> consumeLazyPixelRefs(std::list<LazyPixelRef*>& pixelRefs);
> This name is really unclear to me; "consume" doesn't describe what's happening
> from the caller's point of view, but a detail of internal implementation,
right?

I wanted this to be indicative of the fact that once you get the items,
it clears the items stored in the canvas... If I called this "getLazy.."
then it would imply that two gets in a row would retrieve the same data,
which is not the case. I'm more than happy to change the name to
something more appropriate, if you have a suggestion :)

[Sami, 2013-03-18 17:33:19]

Thanks for the changes, lgtm.

https://codereview.chromium.org/12388095/diff/24001/cc/resources/picture_pile...
File cc/resources/picture_pile_impl.h (right):

https://codereview.chromium.org/12388095/diff/24001/cc/resources/picture_pile...
cc/resources/picture_pile_impl.h:52: std::list<skia::LazyPixelRef*>
lazy_pixel_refs;
Can we also use the typedef from AnalysisCanvas here?

[reveman, 2013-03-18 19:11:47]

lgtm

[commit-bot: I haz the power, 2013-03-18 19:24:03]

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

[commit-bot: I haz the power, 2013-03-18 20:27:00]

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

[commit-bot: I haz the power, 2013-03-18 23:37:40]

Change committed as 188879