Implement SkRecord::willPlaybackBitmaps, cache in SkPicture

Linear-time implementation of willPlaybackBitmaps(), computed & cached
on construction in SkPicture. Unit test.

Template trickery thanks to mtklein@.

BUG=skia:2702
R=mtklein,reed

Committed: https://skia.googlesource.com/skia/+/d930511ee73bf0ddc8bdf570ec23a17dd3e19709

[tomhudson, 2014-06-30 19:55:48]

If I'm reading skia/skia_library.gypi correctly, we now include the
third_party/skia gypis and so no longer have to make dependent Chrome changes
when adding files.

[tomhudson, 2014-06-30 22:04:08]

+Enne, David to compare to the previous CL.

This seems pretty trivial to write a cached version of, either in SkPlayback or
cc::Picture. We can land this if the compositor really still cares about
bitmaps.

[tomhudson, 2014-07-01 18:52:23]

Mike; PTAL - I think this is ready to land. I now cache the value when
SkPlayback is constructed.

[mtklein, 2014-07-01 18:58:02]

https://codereview.chromium.org/366443002/diff/60001/include/record/SkRecordi...
File include/record/SkRecording.h (right):

https://codereview.chromium.org/366443002/diff/60001/include/record/SkRecordi...
include/record/SkRecording.h:47: bool willPlaybackBitmaps() const { return
fWillPlaybackBitmaps; }
This file and all its classes will be deleted.

I think you want to implement this in SkPicture.cpp, and cache it there if you
cache it?

https://codereview.chromium.org/366443002/diff/60001/tests/RecordTest.cpp
File tests/RecordTest.cpp (right):

https://codereview.chromium.org/366443002/diff/60001/tests/RecordTest.cpp#new...
tests/RecordTest.cpp:83:
SkNEW_PLACEMENT_ARGS(record.append<SkRecords::DrawRect>(), SkRecords::DrawRect,
(paint, rect));
I find a macro like this helps keep things straight:

#define APPEND(record, type, ...) SkNEW_PLACEMENT_ARGS(record.append<type>(),
type, (__VA_ARGS__))

YMMV.

[mtklein, 2014-07-01 19:02:24]

https://codereview.chromium.org/366443002/diff/60001/include/record/SkRecordi...
File include/record/SkRecording.h (right):

https://codereview.chromium.org/366443002/diff/60001/include/record/SkRecordi...
include/record/SkRecording.h:47: bool willPlaybackBitmaps() const { return
fWillPlaybackBitmaps; }
On 2014/07/01 18:58:01, mtklein wrote:
> This file and all its classes will be deleted.
> 
> I think you want to implement this in SkPicture.cpp, and cache it there if you
> cache it?

Alternatively, we can do it proactively in SkPictureRecorder::endRecording() if
you'd like to avoid issues of caching, thread safety, etc.  Sure seems cheap.

[tomhudson, 2014-07-02 15:25:42]

Now we need Mike Classic for API review.

https://codereview.chromium.org/366443002/diff/60001/include/record/SkRecordi...
File include/record/SkRecording.h (right):

https://codereview.chromium.org/366443002/diff/60001/include/record/SkRecordi...
include/record/SkRecording.h:47: bool willPlaybackBitmaps() const { return
fWillPlaybackBitmaps; }
On 2014/07/01 19:02:23, mtklein wrote:
> On 2014/07/01 18:58:01, mtklein wrote:
> > This file and all its classes will be deleted.
> > 
> > I think you want to implement this in SkPicture.cpp, and cache it there if
you
> > cache it?
> 
> Alternatively, we can do it proactively in SkPictureRecorder::endRecording()
if
> you'd like to avoid issues of caching, thread safety, etc.  Sure seems cheap.

Done.

https://codereview.chromium.org/366443002/diff/60001/tests/RecordTest.cpp
File tests/RecordTest.cpp (right):

https://codereview.chromium.org/366443002/diff/60001/tests/RecordTest.cpp#new...
tests/RecordTest.cpp:83:
SkNEW_PLACEMENT_ARGS(record.append<SkRecords::DrawRect>(), SkRecords::DrawRect,
(paint, rect));
On 2014/07/01 18:58:01, mtklein wrote:
> I find a macro like this helps keep things straight:
> 
> #define APPEND(record, type, ...) SkNEW_PLACEMENT_ARGS(record.append<type>(),
> type, (__VA_ARGS__))
> 
> YMMV.

MMV - feels like a break-even in this instance.
Done.

[mtklein, 2014-07-02 15:53:08]

lgtm

https://codereview.chromium.org/366443002/diff/140001/include/core/SkPicture.h
File include/core/SkPicture.h (right):

https://codereview.chromium.org/366443002/diff/140001/include/core/SkPicture....
include/core/SkPicture.h:315: bool fRecordWillPlayBackBitmaps;  // cached
I think the // cached here can go?  Makes it seem like we're going to record it
the first time we call willPlayBackBitmaps() instead of in the constructor.

I'd be all for a // TODO: const, though.  I think we can constify a bunch of
this stuff once clone's gone.

https://codereview.chromium.org/366443002/diff/140001/src/core/SkPicture.cpp
File src/core/SkPicture.cpp (right):

https://codereview.chromium.org/366443002/diff/140001/src/core/SkPicture.cpp#...
src/core/SkPicture.cpp:574: , fRecordWillPlayBackBitmaps(record ?
SkRecordWillPlaybackBitmaps(*record) : false) {
You can assume record != NULL here.  If you'd like, add an SkASSERT?

[tomhudson, 2014-07-02 19:21:34]

Mr Reed to the API Review theatre, paging Mr Reed.

https://codereview.chromium.org/366443002/diff/140001/include/core/SkPicture.h
File include/core/SkPicture.h (right):

https://codereview.chromium.org/366443002/diff/140001/include/core/SkPicture....
include/core/SkPicture.h:315: bool fRecordWillPlayBackBitmaps;  // cached
On 2014/07/02 15:53:08, mtklein wrote:
> I think the // cached here can go?  Makes it seem like we're going to record
it
> the first time we call willPlayBackBitmaps() instead of in the constructor.
> 
> I'd be all for a // TODO: const, though.  I think we can constify a bunch of
> this stuff once clone's gone.

Done.

https://codereview.chromium.org/366443002/diff/140001/src/core/SkPicture.cpp
File src/core/SkPicture.cpp (right):

https://codereview.chromium.org/366443002/diff/140001/src/core/SkPicture.cpp#...
src/core/SkPicture.cpp:574: , fRecordWillPlayBackBitmaps(record ?
SkRecordWillPlaybackBitmaps(*record) : false) {
On 2014/07/02 15:53:08, mtklein wrote:
> You can assume record != NULL here.  If you'd like, add an SkASSERT?

Since this is in the initializer, by the time we hit the assert we'd already
have tried to dereference NULL?

[mtklein, 2014-07-02 19:23:03]

https://codereview.chromium.org/366443002/diff/140001/src/core/SkPicture.cpp
File src/core/SkPicture.cpp (right):

https://codereview.chromium.org/366443002/diff/140001/src/core/SkPicture.cpp#...
src/core/SkPicture.cpp:574: , fRecordWillPlayBackBitmaps(record ?
SkRecordWillPlaybackBitmaps(*record) : false) {
On 2014/07/02 19:21:34, tomhudson wrote:
> On 2014/07/02 15:53:08, mtklein wrote:
> > You can assume record != NULL here.  If you'd like, add an SkASSERT?
> 
> Since this is in the initializer, by the time we hit the assert we'd already
> have tried to dereference NULL?

Yeah, so doesn't make much of a difference if we guard record or not.  I'd
rather not so it doesn't look optional.

[reed1, 2014-07-02 19:56:54]

api lgtm

[tomhudson, 2014-07-05 20:09:44]

The CQ bit was checked by tomhudson@google.com

[commit-bot: I haz the power, 2014-07-05 20:10:11]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/tomhudson@chromium.org/366443002/180001

[commit-bot: I haz the power, 2014-07-05 20:10:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-07-05 20:10:14]

Failed to apply patch for gyp/core.gypi:
While running git apply --index -p1;
  error: patch failed: gyp/core.gypi:181
  error: gyp/core.gypi: patch does not apply

Patch:       gyp/core.gypi
Index: gyp/core.gypi
diff --git a/gyp/core.gypi b/gyp/core.gypi
index
b4784bba59004c76ca36161728ff85798fb09c53..6de5bec81fd91cf7907a885007f681fe68499ba1
100644
--- a/gyp/core.gypi
+++ b/gyp/core.gypi
@@ -151,6 +151,8 @@
         '<(skia_src_path)/core/SkRasterClip.cpp',
         '<(skia_src_path)/core/SkRasterizer.cpp',
         '<(skia_src_path)/core/SkReadBuffer.cpp',
+        '<(skia_src_path)/core/SkRecordAnalysis.cpp',
+        '<(skia_src_path)/core/SkRecordAnalysis.h',
         '<(skia_src_path)/core/SkRecordDraw.cpp',
         '<(skia_src_path)/core/SkRecordOpts.cpp',
         '<(skia_src_path)/core/SkRecorder.cpp',
@@ -181,6 +183,7 @@
         '<(skia_src_path)/core/SkSpriteBlitter.h',
         '<(skia_src_path)/core/SkSpriteBlitterTemplate.h',
         '<(skia_src_path)/core/SkStream.cpp',
+        '<(skia_src_path)/core/SkStreamPriv.h',
         '<(skia_src_path)/core/SkString.cpp',
         '<(skia_src_path)/core/SkStringUtils.cpp',
         '<(skia_src_path)/core/SkStroke.h',

[tomhudson, 2014-07-05 20:25:06]

The CQ bit was checked by tomhudson@google.com

[commit-bot: I haz the power, 2014-07-05 20:25:11]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/tomhudson@chromium.org/366443002/200001

[commit-bot: I haz the power, 2014-07-05 20:37:57]

Change committed as d930511ee73bf0ddc8bdf570ec23a17dd3e19709