add SkPictureUtils::GatherPixelRefs()

add SkPictureUtils::GatherPixelRefs()
Committed: https://code.google.com/p/skia/source/detail?r=6615

[Leon, 2012-11-28 22:13:29]

https://codereview.appspot.com/6845106/diff/3001/include/utils/SkPictureUtils.h
File include/utils/SkPictureUtils.h (right):

https://codereview.appspot.com/6845106/diff/3001/include/utils/SkPictureUtils...
include/utils/SkPictureUtils.h:20: *  of SkPixelRefs that might intersect that
area. To keept the call fast,
keep*

https://codereview.appspot.com/6845106/diff/3001/tests/PictureTest.cpp
File tests/PictureTest.cpp (right):

https://codereview.appspot.com/6845106/diff/3001/tests/PictureTest.cpp#newcode83
tests/PictureTest.cpp:83: // Allocate result to be large enough to old subset,
and then draw the picture
old -> hold*

https://codereview.appspot.com/6845106/diff/3001/tests/PictureTest.cpp#newcod...
tests/PictureTest.cpp:223: *  that we actually can see (usually becuase of
conservative bounds
because*

https://codereview.appspot.com/6845106/diff/3001/tests/PictureTest.cpp#newcod...
tests/PictureTest.cpp:227: *  only mostly-correct, but the moment this test
should work.
at* the moment

https://codereview.appspot.com/6845106/diff/3001/tests/PictureTest.cpp#newcod...
tests/PictureTest.cpp:233: SkData* data = SkPictureUtils::GatherPixelRefs(pic,
r);
This SkData will be leaked.

https://codereview.appspot.com/6845106/diff/3001/tests/PictureTest.cpp#newcod...
tests/PictureTest.cpp:234: size_t dataSize = data ? data->size() : 0;
I assume you left off some more tests regarding data and dataSize?

[reed1, 2012-11-29 12:31:37]

https://codereview.appspot.com/6845106/diff/3001/include/utils/SkPictureUtils.h
File include/utils/SkPictureUtils.h (right):

https://codereview.appspot.com/6845106/diff/3001/include/utils/SkPictureUtils...
include/utils/SkPictureUtils.h:20: *  of SkPixelRefs that might intersect that
area. To keept the call fast,
On 2012/11/28 22:13:29, scroggo-work wrote:
> keep*

Done.

https://codereview.appspot.com/6845106/diff/3001/tests/PictureTest.cpp
File tests/PictureTest.cpp (right):

https://codereview.appspot.com/6845106/diff/3001/tests/PictureTest.cpp#newcode83
tests/PictureTest.cpp:83: // Allocate result to be large enough to old subset,
and then draw the picture
On 2012/11/28 22:13:29, scroggo-work wrote:
> old -> hold*

Done.

https://codereview.appspot.com/6845106/diff/3001/tests/PictureTest.cpp#newcod...
tests/PictureTest.cpp:223: *  that we actually can see (usually becuase of
conservative bounds
On 2012/11/28 22:13:29, scroggo-work wrote:
> because*

Done.

https://codereview.appspot.com/6845106/diff/3001/tests/PictureTest.cpp#newcod...
tests/PictureTest.cpp:227: *  only mostly-correct, but the moment this test
should work.
On 2012/11/28 22:13:29, scroggo-work wrote:
> at* the moment

Done.

https://codereview.appspot.com/6845106/diff/3001/tests/PictureTest.cpp#newcod...
tests/PictureTest.cpp:233: SkData* data = SkPictureUtils::GatherPixelRefs(pic,
r);
On 2012/11/28 22:13:29, scroggo-work wrote:
> This SkData will be leaked.

Done.

https://codereview.appspot.com/6845106/diff/3001/tests/PictureTest.cpp#newcod...
tests/PictureTest.cpp:234: size_t dataSize = data ? data->size() : 0;
On 2012/11/28 22:13:29, scroggo-work wrote:
> I assume you left off some more tests regarding data and dataSize?

Done.

[TomH, 2012-11-29 12:37:28]

If this is performance-sensitive code, can we get a bench?
(With the non-render flag set?)

[Leon, 2012-11-29 15:48:59]

On 2012/11/29 12:37:28, TomH wrote:
> If this is performance-sensitive code, can we get a bench?
> (With the non-render flag set?)

+1

Otherwise, lgtm

[reed1, 2012-11-29 20:18:41]

add bench, and fix the code *a lot* in response to profiling. Sped it up by
almost 10x for androidpolice.skp

"Mr. Hudson, you go right on quoting regulations."

http://libertyinlaw.wordpress.com/2008/09/04/sir-may-i-quote-general-order-12/

[Leon, 2012-11-29 20:33:51]

lgtm

https://codereview.appspot.com/6845106/diff/12001/tools/PictureRenderer.cpp
File tools/PictureRenderer.cpp (right):

https://codereview.appspot.com/6845106/diff/12001/tools/PictureRenderer.cpp#n...
tools/PictureRenderer.cpp:673: return NULL == path;    // we don't have anything
to write
Should return true on successful writing, whatever that means for this
renderer...