add replay entry point to SkPictureRecorder for Android

add replay entry point to SkPictureRecorder for Android

This CL adds an Android-only entry point to address the Java Picture(Picture) and serialize use cases. Note that (in its current form) it doesn't preserve the old API's handling of unbalanced saves/saveLayers (this CL always balances them).

Committed: http://code.google.com/p/skia/source/detail?r=14911

[robertphillips, 2014-04-28 17:35:21]

https://codereview.chromium.org/252873005/diff/1/tests/PictureTest.cpp
File tests/PictureTest.cpp (right):

https://codereview.chromium.org/252873005/diff/1/tests/PictureTest.cpp#newcod...
tests/PictureTest.cpp:837: 
This is where the save/saveLayer balancing occurs in the new world.

https://codereview.chromium.org/252873005/diff/1/tests/PictureTest.cpp#newcod...
tests/PictureTest.cpp:854: 
The extra save and restore comes from the Copy process.

[robertphillips, 2014-04-30 20:02:15]

PTAL - this patch seems viable given that Android seems amenable to altering the
side-effects associated with Picture(Picture) and Picture::serialize

[scroggo, 2014-04-30 20:05:43]

lgtm

https://codereview.chromium.org/252873005/diff/1/src/core/SkPictureRecorder.cpp
File src/core/SkPictureRecorder.cpp (right):

https://codereview.chromium.org/252873005/diff/1/src/core/SkPictureRecorder.c...
src/core/SkPictureRecorder.cpp:8: #include "SkPicturePlayback.h"
Isn't this only for SK_BUILD_FOR_ANDROID?

[mtklein, 2014-04-30 20:20:11]

https://codereview.chromium.org/252873005/diff/1/include/core/SkPictureRecord...
File include/core/SkPictureRecorder.h (right):

https://codereview.chromium.org/252873005/diff/1/include/core/SkPictureRecord...
include/core/SkPictureRecorder.h:102: #ifdef SK_BUILD_FOR_ANDROID
Is this necessary to build, or just a guard to prevent future use?  Privatizing
and friending in AndroidPicture seems pretty good enough to me, but will get us
better coverage in tests, builds, etc.

https://codereview.chromium.org/252873005/diff/1/include/core/SkPictureRecord...
include/core/SkPictureRecorder.h:108: void replay(SkCanvas* canvas);
Maybe work "partial" into the name?  replay() sounds like the normal thing you
might do on a picture (like draw), but we want this one to seem niche right?

void drawPartialRecording()?

https://codereview.chromium.org/252873005/diff/1/tests/PictureTest.cpp
File tests/PictureTest.cpp (right):

https://codereview.chromium.org/252873005/diff/1/tests/PictureTest.cpp#newcod...
tests/PictureTest.cpp:783: explicit SaveCountingCanvas(int width, int height)
I guess it's fine, but you don't need explicit here.

https://codereview.chromium.org/252873005/diff/1/tests/PictureTest.cpp#newcod...
tests/PictureTest.cpp:790: virtual SaveLayerStrategy willSaveLayer(const SkRect*
bounds, const SkPaint* paint,
If you'd like to save space, virtual is not needed on these methods.

https://codereview.chromium.org/252873005/diff/1/tests/PictureTest.cpp#newcod...
tests/PictureTest.cpp:806: unsigned int getSaveCount() const { return
fSaveCount; }
I think we tend to just write "unsigned"

https://codereview.chromium.org/252873005/diff/1/tests/PictureTest.cpp#newcod...
tests/PictureTest.cpp:831: static SkPicture* Copy(SkPictureRecorder* recorder) {
Can you add a note that the class is here so it can be a friend of SkPicture?

https://codereview.chromium.org/252873005/diff/1/tests/PictureTest.cpp#newcod...
tests/PictureTest.cpp:831: static SkPicture* Copy(SkPictureRecorder* recorder) {
Is Android also going to be using this for copying?  If so, we might just
present SkPicture* copyPartial() instead of replay().  We can always do the same
thing under the covers, but might give us more wiggle room?

https://codereview.chromium.org/252873005/diff/1/tests/PictureTest.cpp#newcod...
tests/PictureTest.cpp:843: static void test_replay(skiatest::Reporter* reporter)
{
DEF_TEST(PictureRecorder_replay, reporter) {  ... } ?

https://codereview.chromium.org/252873005/diff/1/tests/PictureTest.cpp#newcod...
tests/PictureTest.cpp:854: 
On 2014/04/28 17:35:21, robertphillips wrote:
> The extra save and restore comes from the Copy process.

Tack this on as a comment here?

https://codereview.chromium.org/252873005/diff/1/tests/PictureTest.cpp#newcod...
tests/PictureTest.cpp:867: // (partially) check leakage of draw ops
Might as well split this into a separate DEF_TEST too.

[robertphillips, 2014-05-27 14:10:07]

Finally worked my way back around to this. PTAL.

https://codereview.chromium.org/252873005/diff/1/include/core/SkPictureRecord...
File include/core/SkPictureRecorder.h (right):

https://codereview.chromium.org/252873005/diff/1/include/core/SkPictureRecord...
include/core/SkPictureRecorder.h:102: #ifdef SK_BUILD_FOR_ANDROID
This will build without the Android guard but having it here makes it explicit
that it is Android only.

https://codereview.chromium.org/252873005/diff/1/tests/PictureTest.cpp
File tests/PictureTest.cpp (right):

https://codereview.chromium.org/252873005/diff/1/tests/PictureTest.cpp#newcod...
tests/PictureTest.cpp:783: explicit SaveCountingCanvas(int width, int height)
On 2014/04/30 20:20:11, mtklein wrote:
> I guess it's fine, but you don't need explicit here.

Done.

https://codereview.chromium.org/252873005/diff/1/tests/PictureTest.cpp#newcod...
tests/PictureTest.cpp:790: virtual SaveLayerStrategy willSaveLayer(const SkRect*
bounds, const SkPaint* paint,
We seem to pull through the virtual keyword whenever the base class has it as
virtual.

https://codereview.chromium.org/252873005/diff/1/tests/PictureTest.cpp#newcod...
tests/PictureTest.cpp:831: static SkPicture* Copy(SkPictureRecorder* recorder) {
On 2014/04/30 20:20:11, mtklein wrote:
> Can you add a note that the class is here so it can be a friend of SkPicture?

Done.

https://codereview.chromium.org/252873005/diff/1/tests/PictureTest.cpp#newcod...
tests/PictureTest.cpp:831: static SkPicture* Copy(SkPictureRecorder* recorder) {
I proposed a "snapshot" version of this in
https://codereview.chromium.org/253443003/. This CL evolved out of the
discussion of that approach. IIRC the thinking is that the replay-style
interface is more standoffish and separable.

https://codereview.chromium.org/252873005/diff/1/tests/PictureTest.cpp#newcod...
tests/PictureTest.cpp:843: static void test_replay(skiatest::Reporter* reporter)
{
On 2014/04/30 20:20:11, mtklein wrote:
> DEF_TEST(PictureRecorder_replay, reporter) {  ... } ?

Done.

https://codereview.chromium.org/252873005/diff/1/tests/PictureTest.cpp#newcod...
tests/PictureTest.cpp:854: 
On 2014/04/30 20:20:11, mtklein wrote:
> On 2014/04/28 17:35:21, robertphillips wrote:
> > The extra save and restore comes from the Copy process.
> 
> Tack this on as a comment here?

Done.

https://codereview.chromium.org/252873005/diff/1/tests/PictureTest.cpp#newcod...
tests/PictureTest.cpp:867: // (partially) check leakage of draw ops
I would prefer to keep these together since they are basically two ways of
testing the same thing. That additional operations added to the src picture
don't migrate to the copy.

[scroggo, 2014-05-27 14:50:03]

lgtm

https://codereview.chromium.org/252873005/diff/40001/tests/PictureTest.cpp
File tests/PictureTest.cpp (right):

https://codereview.chromium.org/252873005/diff/40001/tests/PictureTest.cpp#ne...
tests/PictureTest.cpp:1114: for (int i = 0; i < 4; ++i) {
It looks like this is slightly different from set_canvas_to_save_count_4. Is
there a reason for that?

https://codereview.chromium.org/252873005/diff/40001/tests/PictureTest.cpp#ne...
tests/PictureTest.cpp:1121: // Copying a mid-recording picture could result in
unbalanced saves/restores
Instead of "could result in", don't we really mean "must not result in"?

https://codereview.chromium.org/252873005/diff/40001/tests/PictureTest.cpp#ne...
tests/PictureTest.cpp:1126: set_canvas_to_save_count_4(&testCanvas);
Is this in case the prior assert failed?

[robertphillips, 2014-05-27 17:23:38]

https://codereview.chromium.org/252873005/diff/40001/tests/PictureTest.cpp
File tests/PictureTest.cpp (right):

https://codereview.chromium.org/252873005/diff/40001/tests/PictureTest.cpp#ne...
tests/PictureTest.cpp:1114: for (int i = 0; i < 4; ++i) {
In this instance the restoreToCount in set_canvas_to_save_count_4 would be a bit
misleading.

https://codereview.chromium.org/252873005/diff/40001/tests/PictureTest.cpp#ne...
tests/PictureTest.cpp:1121: // Copying a mid-recording picture could result in
unbalanced saves/restores
On 2014/05/27 14:50:03, scroggo wrote:
> Instead of "could result in", don't we really mean "must not result in"?

Done.

https://codereview.chromium.org/252873005/diff/40001/tests/PictureTest.cpp#ne...
tests/PictureTest.cpp:1126: set_canvas_to_save_count_4(&testCanvas);
Yes. I extended this test case to check the "fake" endRecording paths and it was
set up to always restore to a known state between sub-tests.

[mtklein, 2014-05-27 17:26:03]

lgtm

[reed1, 2014-05-27 17:34:36]

lgtm

[robertphillips, 2014-05-27 23:36:04]

The CQ bit was checked by robertphillips@google.com

[commit-bot: I haz the power, 2014-05-27 23:36:21]

CQ is trying da patch. Follow status at

https://skia-tree-status.appspot.com/cq/robertphillips@google.com/252873005/8...

[commit-bot: I haz the power, 2014-05-27 23:41:47]

Change committed as 14911